From c224644bed6672b9d6442d5179413c05871a56f3 Mon Sep 17 00:00:00 2001 From: Sam Atkins Date: Thu, 10 Apr 2025 16:04:34 +0100 Subject: [PATCH] LibWeb/CSS: Fetch ImageStyleValue images closer to spec We now don't absolutize the URL during parsing, keeping it as a CSS::URL object which we then pass to the "fetch an external image for a stylesheet" algorithm. Our version of this algorithm is a bit ad-hoc, in order to make use of SharedResourceRequest. To try and reduce duplication, I've pulled all of fetch_a_style_resource() into a static function, apart from the "actually do the fetch" step. --- Libraries/LibWeb/CSS/Fetch.cpp | 66 ++++++++++++++-- Libraries/LibWeb/CSS/Fetch.h | 4 + Libraries/LibWeb/CSS/Parser/ValueParsing.cpp | 9 +-- .../CSS/StyleValues/ImageStyleValue.cpp | 77 ++++++++++--------- .../LibWeb/CSS/StyleValues/ImageStyleValue.h | 21 ++--- 5 files changed, 120 insertions(+), 57 deletions(-) diff --git a/Libraries/LibWeb/CSS/Fetch.cpp b/Libraries/LibWeb/CSS/Fetch.cpp index f8ca5e11faa..ebceef14a20 100644 --- a/Libraries/LibWeb/CSS/Fetch.cpp +++ b/Libraries/LibWeb/CSS/Fetch.cpp @@ -9,11 +9,12 @@ #include #include #include +#include namespace Web::CSS { // https://drafts.csswg.org/css-values-4/#fetch-a-style-resource -void fetch_a_style_resource(StyleResourceURL const& url_value, StyleSheetOrDocument sheet_or_document, Fetch::Infrastructure::Request::Destination destination, CorsMode cors_mode, Fetch::Infrastructure::FetchAlgorithms::ProcessResponseConsumeBodyFunction process_response) +static GC::Ptr fetch_a_style_resource_impl(StyleResourceURL const& url_value, StyleSheetOrDocument sheet_or_document, Fetch::Infrastructure::Request::Destination destination, CorsMode cors_mode) { // AD-HOC: Not every caller has a CSSStyleSheet, so allow passing a Document in instead for URL completion. // Spec issue: https://github.com/w3c/csswg-drafts/issues/12065 @@ -39,7 +40,7 @@ void fetch_a_style_resource(StyleResourceURL const& url_value, StyleSheetOrDocum [](CSS::URL const& url) { return url.url(); }); auto parsed_url = ::URL::Parser::basic_parse(url_string, base); if (!parsed_url.has_value()) - return; + return {}; // 4. Let req be a new request whose url is parsedUrl, whose destination is destination, mode is corsMode, // origin is environmentSettings’s origin, credentials mode is "same-origin", use-url-credentials flag is set, @@ -70,10 +71,65 @@ void fetch_a_style_resource(StyleResourceURL const& url_value, StyleSheetOrDocum request->set_initiator_type(Fetch::Infrastructure::Request::InitiatorType::CSS); // 8. Fetch req, with processresponseconsumebody set to processResponse. - Fetch::Infrastructure::FetchAlgorithms::Input fetch_algorithms_input {}; - fetch_algorithms_input.process_response_consume_body = move(process_response); + // NB: Implemented by caller. + return request; +} - (void)Fetch::Fetching::fetch(environment_settings.realm(), request, Fetch::Infrastructure::FetchAlgorithms::create(vm, move(fetch_algorithms_input))); +// https://drafts.csswg.org/css-values-4/#fetch-a-style-resource +void fetch_a_style_resource(StyleResourceURL const& url_value, StyleSheetOrDocument sheet_or_document, Fetch::Infrastructure::Request::Destination destination, CorsMode cors_mode, Fetch::Infrastructure::FetchAlgorithms::ProcessResponseConsumeBodyFunction process_response) +{ + if (auto request = fetch_a_style_resource_impl(url_value, sheet_or_document, destination, cors_mode)) { + auto& environment_settings = HTML::relevant_settings_object(sheet_or_document.visit([](auto& it) -> JS::Object& { return it; })); + auto& vm = environment_settings.vm(); + + Fetch::Infrastructure::FetchAlgorithms::Input fetch_algorithms_input {}; + fetch_algorithms_input.process_response_consume_body = move(process_response); + + (void)Fetch::Fetching::fetch(environment_settings.realm(), *request, Fetch::Infrastructure::FetchAlgorithms::create(vm, move(fetch_algorithms_input))); + } +} + +// https://drafts.csswg.org/css-images-4/#fetch-an-external-image-for-a-stylesheet +GC::Ptr fetch_an_external_image_for_a_stylesheet(StyleResourceURL const& url_value, StyleSheetOrDocument sheet_or_document) +{ + // To fetch an external image for a stylesheet, given a url and CSSStyleSheet sheet, fetch a style resource + // given url, with stylesheet CSSStyleSheet, destination "image", CORS mode "no-cors", and processResponse being + // the following steps given response res and null, failure or a byte stream byteStream: If byteStream is a byte + // stream, load the image from the byte stream. + + // NB: We can't directly call fetch_a_style_resource() because we want to make use of SharedResourceRequest to + // deduplicate image requests. + + if (auto request = fetch_a_style_resource_impl(url_value, sheet_or_document, Fetch::Infrastructure::Request::Destination::Image, CorsMode::NoCors)) { + + auto document = sheet_or_document.visit( + [&](GC::Ref const& sheet) -> GC::Ref { return *sheet->owning_document(); }, + [](GC::Ref const& document) -> GC::Ref { return document; }); + auto& realm = document->realm(); + + auto shared_resource_request = HTML::SharedResourceRequest::get_or_create(realm, document->page(), request->url()); + shared_resource_request->add_callbacks( + [document, weak_document = document->make_weak_ptr()] { + if (!weak_document) + return; + + if (auto navigable = document->navigable()) { + // Once the image has loaded, we need to re-resolve CSS properties that depend on the image's dimensions. + document->set_needs_to_resolve_paint_only_properties(); + + // FIXME: Do less than a full repaint if possible? + document->set_needs_display(); + } + }, + nullptr); + + if (shared_resource_request->needs_fetching()) + shared_resource_request->fetch_resource(realm, *request); + + return shared_resource_request; + } + + return nullptr; } } diff --git a/Libraries/LibWeb/CSS/Fetch.h b/Libraries/LibWeb/CSS/Fetch.h index 4e0c648b5ea..3e6b3035510 100644 --- a/Libraries/LibWeb/CSS/Fetch.h +++ b/Libraries/LibWeb/CSS/Fetch.h @@ -10,6 +10,7 @@ #include #include #include +#include namespace Web::CSS { @@ -26,4 +27,7 @@ using StyleSheetOrDocument = Variant, GC::Ref fetch_an_external_image_for_a_stylesheet(StyleResourceURL const&, StyleSheetOrDocument); + } diff --git a/Libraries/LibWeb/CSS/Parser/ValueParsing.cpp b/Libraries/LibWeb/CSS/Parser/ValueParsing.cpp index 16ce3677fd1..924305b848d 100644 --- a/Libraries/LibWeb/CSS/Parser/ValueParsing.cpp +++ b/Libraries/LibWeb/CSS/Parser/ValueParsing.cpp @@ -2013,13 +2013,10 @@ RefPtr Parser::parse_image_value(TokenStream`. if (!url->url().starts_with('#')) { - // FIXME: Stop completing the URL here - auto completed_url = complete_url(url->url()); - if (completed_url.has_value()) { - tokens.discard_a_mark(); - return ImageStyleValue::create(completed_url.release_value()); - } + tokens.discard_a_mark(); + return ImageStyleValue::create(url.release_value()); } tokens.restore_a_mark(); return nullptr; diff --git a/Libraries/LibWeb/CSS/StyleValues/ImageStyleValue.cpp b/Libraries/LibWeb/CSS/StyleValues/ImageStyleValue.cpp index e3aefdb2e78..920f97d7967 100644 --- a/Libraries/LibWeb/CSS/StyleValues/ImageStyleValue.cpp +++ b/Libraries/LibWeb/CSS/StyleValues/ImageStyleValue.cpp @@ -1,18 +1,17 @@ /* * Copyright (c) 2018-2023, Andreas Kling * Copyright (c) 2021, Tobias Christiansen - * Copyright (c) 2021-2023, Sam Atkins + * Copyright (c) 2021-2025, Sam Atkins * Copyright (c) 2022-2023, MacDue * * SPDX-License-Identifier: BSD-2-Clause */ -#include "ImageStyleValue.h" #include -#include +#include +#include #include #include -#include #include #include #include @@ -21,7 +20,17 @@ namespace Web::CSS { -ImageStyleValue::ImageStyleValue(::URL::URL const& url) +ValueComparingNonnullRefPtr ImageStyleValue::create(URL const& url) +{ + return adopt_ref(*new (nothrow) ImageStyleValue(url)); +} + +ValueComparingNonnullRefPtr ImageStyleValue::create(::URL::URL const& url) +{ + return adopt_ref(*new (nothrow) ImageStyleValue(URL { url.to_string() })); +} + +ImageStyleValue::ImageStyleValue(URL const& url) : AbstractImageStyleValue(Type::Image) , m_url(url) { @@ -35,6 +44,7 @@ void ImageStyleValue::visit_edges(JS::Cell::Visitor& visitor) const // FIXME: visit_edges in non-GC allocated classes is confusing pattern. // Consider making CSSStyleValue to be GC allocated instead. visitor.visit(m_resource_request); + visitor.visit(m_style_sheet); visitor.visit(m_timer); } @@ -44,37 +54,26 @@ void ImageStyleValue::load_any_resources(DOM::Document& document) return; m_document = &document; - m_resource_request = HTML::SharedResourceRequest::get_or_create(document.realm(), document.page(), m_url); - m_resource_request->add_callbacks( - [this, weak_this = make_weak_ptr()] { - if (!weak_this) - return; + if (m_style_sheet) { + m_resource_request = fetch_an_external_image_for_a_stylesheet(m_url, { *m_style_sheet }); + } else { + m_resource_request = fetch_an_external_image_for_a_stylesheet(m_url, { document }); + } + if (m_resource_request) { + m_resource_request->add_callbacks( + [this, weak_this = make_weak_ptr()] { + if (!weak_this || !m_document) + return; - if (!m_document) - return; - - if (auto navigable = m_document->navigable()) { - // Once the image has loaded, we need to re-resolve CSS properties that depend on the image's dimensions. - m_document->set_needs_to_resolve_paint_only_properties(); - - // FIXME: Do less than a full repaint if possible? - m_document->set_needs_display(); - } - - auto image_data = m_resource_request->image_data(); - if (image_data->is_animated() && image_data->frame_count() > 1) { - m_timer = Platform::Timer::create(m_document->heap()); - m_timer->set_interval(image_data->frame_duration(0)); - m_timer->on_timeout = GC::create_function(m_document->heap(), [this] { animate(); }); - m_timer->start(); - } - }, - nullptr); - - if (m_resource_request->needs_fetching()) { - auto request = HTML::create_potential_CORS_request(document.vm(), m_url, Fetch::Infrastructure::Request::Destination::Image, HTML::CORSSettingAttribute::NoCORS); - request->set_client(&document.relevant_settings_object()); - m_resource_request->fetch_resource(document.realm(), request); + auto image_data = m_resource_request->image_data(); + if (image_data->is_animated() && image_data->frame_count() > 1) { + m_timer = Platform::Timer::create(m_document->heap()); + m_timer->set_interval(image_data->frame_duration(0)); + m_timer->on_timeout = GC::create_function(m_document->heap(), [this] { animate(); }); + m_timer->start(); + } + }, + nullptr); } } @@ -116,7 +115,7 @@ Gfx::ImmutableBitmap const* ImageStyleValue::bitmap(size_t frame_index, Gfx::Int String ImageStyleValue::to_string(SerializationMode) const { - return serialize_a_url(m_url.to_string()); + return m_url.to_string(); } bool ImageStyleValue::equals(CSSStyleValue const& other) const @@ -177,4 +176,10 @@ Optional ImageStyleValue::color_if_single_pixel_bitmap() const return {}; } +void ImageStyleValue::set_style_sheet(GC::Ptr style_sheet) +{ + Base::set_style_sheet(style_sheet); + m_style_sheet = style_sheet; +} + } diff --git a/Libraries/LibWeb/CSS/StyleValues/ImageStyleValue.h b/Libraries/LibWeb/CSS/StyleValues/ImageStyleValue.h index 16b1e6fba7e..b57495f659a 100644 --- a/Libraries/LibWeb/CSS/StyleValues/ImageStyleValue.h +++ b/Libraries/LibWeb/CSS/StyleValues/ImageStyleValue.h @@ -1,7 +1,7 @@ /* * Copyright (c) 2018-2020, Andreas Kling * Copyright (c) 2021, Tobias Christiansen - * Copyright (c) 2021-2023, Sam Atkins + * Copyright (c) 2021-2025, Sam Atkins * Copyright (c) 2022-2023, MacDue * * SPDX-License-Identifier: BSD-2-Clause @@ -9,11 +9,10 @@ #pragma once -#include #include -#include #include #include +#include #include namespace Web::CSS { @@ -25,10 +24,9 @@ class ImageStyleValue final using Base = AbstractImageStyleValue; public: - static ValueComparingNonnullRefPtr create(::URL::URL const& url) - { - return adopt_ref(*new (nothrow) ImageStyleValue(url)); - } + static ValueComparingNonnullRefPtr create(URL const&); + static ValueComparingNonnullRefPtr create(::URL::URL const&); + virtual ~ImageStyleValue() override; virtual void visit_edges(JS::Cell::Visitor& visitor) const override; @@ -53,14 +51,17 @@ public: GC::Ptr image_data() const; private: - ImageStyleValue(::URL::URL const&); + ImageStyleValue(URL const&); - GC::Ptr m_resource_request; + virtual void set_style_sheet(GC::Ptr) override; void animate(); Gfx::ImmutableBitmap const* bitmap(size_t frame_index, Gfx::IntSize = {}) const; - ::URL::URL m_url; + GC::Ptr m_resource_request; + GC::Ptr m_style_sheet; + + URL m_url; WeakPtr m_document; size_t m_current_frame_index { 0 };