From f857c6a6e653ee0e966da67a5ac7330303ec4158 Mon Sep 17 00:00:00 2001 From: Shannon Booth Date: Sat, 22 Feb 2025 20:16:45 +1300 Subject: [PATCH] LibWeb/HTML: Make HTMLImageRequests currentURL a String This is the same type as what is spec'd. We cannot use a URL record for this member as the spec in some scenarios will set and compare the URL string to an invalid URL value, such as the empty string. With implicit string constructors for the URL class removed explicitly using URL::Parser::basic_parse makes the code look quite silly in those places. --- Libraries/LibWeb/HTML/HTMLImageElement.cpp | 44 +++++++++++----------- Libraries/LibWeb/HTML/HTMLImageElement.h | 2 +- Libraries/LibWeb/HTML/ImageRequest.cpp | 12 ++---- Libraries/LibWeb/HTML/ImageRequest.h | 6 +-- 4 files changed, 30 insertions(+), 34 deletions(-) diff --git a/Libraries/LibWeb/HTML/HTMLImageElement.cpp b/Libraries/LibWeb/HTML/HTMLImageElement.cpp index 7d5b3e3fe6a..3fbdb14651f 100644 --- a/Libraries/LibWeb/HTML/HTMLImageElement.cpp +++ b/Libraries/LibWeb/HTML/HTMLImageElement.cpp @@ -315,10 +315,7 @@ bool HTMLImageElement::complete() const String HTMLImageElement::current_src() const { // The currentSrc IDL attribute must return the img element's current request's current URL. - auto current_url = m_current_request->current_url(); - if (!current_url.is_valid()) - return {}; - return current_url.to_string(); + return m_current_request->current_url(); } // https://html.spec.whatwg.org/multipage/embedded-content.html#dom-img-decode @@ -522,14 +519,15 @@ ErrorOr HTMLImageElement::update_the_image_data(bool restart_animations, b // 1. Parse selected source, relative to the element's node document. // If that is not successful, then abort this inner set of steps. // Otherwise, let urlString be the resulting URL string. - auto url_string = document().parse_url(selected_source.value()); - if (!url_string.has_value()) + auto maybe_url = document().parse_url(selected_source.value()); + if (!maybe_url.has_value()) goto after_step_7; + auto url_string = maybe_url->serialize(); // 2. Let key be a tuple consisting of urlString, the img element's crossorigin attribute's mode, // and, if that mode is not No CORS, the node document's origin. ListOfAvailableImages::Key key; - key.url = url_string.value(); + key.url = maybe_url.value(); key.mode = m_cors_setting; key.origin = document().origin(); @@ -565,7 +563,7 @@ ErrorOr HTMLImageElement::update_the_image_data(bool restart_animations, b restart_the_animation(); // 2. Set current request's current URL to urlString. - m_current_request->set_current_url(realm(), *url_string); + m_current_request->set_current_url(realm(), url_string); // 3. If maybe omit events is not set or previousURL is not equal to urlString, then fire an event named load at the img element. if (!maybe_omit_events || previous_url != url_string) @@ -604,7 +602,7 @@ after_step_7: // 2. Queue an element task on the DOM manipulation task source given the img element and the following steps: queue_an_element_task(HTML::Task::Source::DOMManipulation, [this, maybe_omit_events, previous_url] { // 1. Change the current request's current URL to the empty string. - m_current_request->set_current_url(realm(), ""sv); + m_current_request->set_current_url(realm(), String {}); // 2. If all of the following conditions are true: // - the element has a src attribute or it uses srcset or picture; and @@ -621,9 +619,9 @@ after_step_7: } // 12. Parse selected source, relative to the element's node document, and let urlString be the resulting URL string. - auto url_string = document().parse_url(selected_source.value().url.to_byte_string()); + auto maybe_url = document().parse_url(selected_source.value().url.to_byte_string()); // If that is not successful, then: - if (!url_string.has_value()) { + if (!maybe_url.has_value()) { // 1. Abort the image request for the current request and the pending request. abort_the_image_request(realm(), m_current_request); abort_the_image_request(realm(), m_pending_request); @@ -647,6 +645,7 @@ after_step_7: // 5. Return. return; } + auto url_string = maybe_url->serialize(); // 13. If the pending request is not null and urlString is the same as the pending request's current URL, then return. if (m_pending_request && url_string == m_pending_request->current_url()) @@ -674,7 +673,7 @@ after_step_7: // 16. Set image request to a new image request whose current URL is urlString. auto image_request = ImageRequest::create(realm(), document().page()); - image_request->set_current_url(realm(), *url_string); + image_request->set_current_url(realm(), url_string); // 17. If current request's state is unavailable or broken, then set the current request to image request. // Otherwise, set the pending request to image request. @@ -691,7 +690,7 @@ after_step_7: if (delay_load_event) m_load_event_delayer.emplace(document()); - add_callbacks_to_image_request(*image_request, maybe_omit_events, *url_string, previous_url); + add_callbacks_to_image_request(*image_request, maybe_omit_events, *maybe_url, previous_url); // AD-HOC: If the image request is already available or fetching, no need to start another fetch. if (image_request->is_available() || image_request->is_fetching()) @@ -699,7 +698,7 @@ after_step_7: // 18. Let request be the result of creating a potential-CORS request given urlString, "image", // and the current state of the element's crossorigin content attribute. - auto request = create_potential_CORS_request(vm(), *url_string, Fetch::Infrastructure::Request::Destination::Image, m_cors_setting); + auto request = create_potential_CORS_request(vm(), *maybe_url, Fetch::Infrastructure::Request::Destination::Image, m_cors_setting); // 19. Set request's client to the element's node document's relevant settings object. request->set_client(&document().relevant_settings_object()); @@ -733,7 +732,7 @@ after_step_7: return {}; } -void HTMLImageElement::add_callbacks_to_image_request(GC::Ref image_request, bool maybe_omit_events, URL::URL const& url_string, URL::URL const& previous_url) +void HTMLImageElement::add_callbacks_to_image_request(GC::Ref image_request, bool maybe_omit_events, URL::URL const& url_string, String const& previous_url) { image_request->add_callbacks( [this, image_request, maybe_omit_events, url_string, previous_url]() { @@ -766,7 +765,7 @@ void HTMLImageElement::add_callbacks_to_image_request(GC::Ref imag document().set_needs_layout(); // 4. If maybe omit events is not set or previousURL is not equal to urlString, then fire an event named load at the img element. - if (!maybe_omit_events || previous_url != url_string) + if (!maybe_omit_events || previous_url != url_string.serialize()) dispatch_event(DOM::Event::create(realm(), HTML::EventNames::load)); if (image_data->is_animated() && image_data->frame_count() > 1) { @@ -795,7 +794,7 @@ void HTMLImageElement::add_callbacks_to_image_request(GC::Ref imag // and then, if maybe omit events is not set or previousURL is not equal to urlString, // queue an element task on the DOM manipulation task source given the img element // to fire an event named error at the img element. - if (!maybe_omit_events || previous_url != url_string) + if (!maybe_omit_events || previous_url != url_string.serialize()) dispatch_event(DOM::Event::create(realm(), HTML::EventNames::error)); m_load_event_delayer.clear(); @@ -848,9 +847,10 @@ void HTMLImageElement::react_to_changes_in_the_environment() // 6. ⌛ Parse selected source, relative to the element's node document, // and let urlString be the resulting URL string. If that is not successful, then return. - auto url_string = document().parse_url(selected_source.value()); - if (!url_string.has_value()) + auto maybe_url = document().parse_url(selected_source.value()); + if (!maybe_url.has_value()) return; + auto url_string = maybe_url->serialize(); // 7. ⌛ Let corsAttributeState be the state of the element's crossorigin content attribute. auto cors_attribute_state = m_cors_setting; @@ -863,14 +863,14 @@ void HTMLImageElement::react_to_changes_in_the_environment() // 10. ⌛ Let key be a tuple consisting of urlString, corsAttributeState, and, if corsAttributeState is not No CORS, origin. ListOfAvailableImages::Key key; - key.url = *url_string; + key.url = *maybe_url; key.mode = m_cors_setting; if (cors_attribute_state != CORSSettingAttribute::NoCORS) key.origin = document().origin(); // 11. ⌛ Let image request be a new image request whose current URL is urlString auto image_request = ImageRequest::create(realm(), document().page()); - image_request->set_current_url(realm(), *url_string); + image_request->set_current_url(realm(), url_string); // 12. ⌛ Let the element's pending request be image request. m_pending_request = image_request; @@ -918,7 +918,7 @@ void HTMLImageElement::react_to_changes_in_the_environment() // Otherwise: else { // 1. Let request be the result of creating a potential-CORS request given urlString, "image", and corsAttributeState. - auto request = create_potential_CORS_request(vm(), *url_string, Fetch::Infrastructure::Request::Destination::Image, m_cors_setting); + auto request = create_potential_CORS_request(vm(), *maybe_url, Fetch::Infrastructure::Request::Destination::Image, m_cors_setting); // 2. Set request's client to client, initiator to "imageset", and set request's synchronous flag. request->set_client(&client); diff --git a/Libraries/LibWeb/HTML/HTMLImageElement.h b/Libraries/LibWeb/HTML/HTMLImageElement.h index 380de2758d1..5105bcd6f94 100644 --- a/Libraries/LibWeb/HTML/HTMLImageElement.h +++ b/Libraries/LibWeb/HTML/HTMLImageElement.h @@ -134,7 +134,7 @@ private: void handle_successful_fetch(URL::URL const&, StringView mime_type, ImageRequest&, ByteBuffer, bool maybe_omit_events, URL::URL const& previous_url); void handle_failed_fetch(); - void add_callbacks_to_image_request(GC::Ref, bool maybe_omit_events, URL::URL const& url_string, URL::URL const& previous_url); + void add_callbacks_to_image_request(GC::Ref, bool maybe_omit_events, URL::URL const& url_string, String const& previous_url); void animate(); diff --git a/Libraries/LibWeb/HTML/ImageRequest.cpp b/Libraries/LibWeb/HTML/ImageRequest.cpp index 69222078f0e..c35df4e580a 100644 --- a/Libraries/LibWeb/HTML/ImageRequest.cpp +++ b/Libraries/LibWeb/HTML/ImageRequest.cpp @@ -6,6 +6,7 @@ #include #include +#include #include #include #include @@ -67,16 +68,11 @@ void ImageRequest::set_state(State state) m_state = state; } -URL::URL const& ImageRequest::current_url() const -{ - return m_current_url; -} - -void ImageRequest::set_current_url(JS::Realm& realm, URL::URL url) +void ImageRequest::set_current_url(JS::Realm& realm, String url) { m_current_url = move(url); - if (m_current_url.is_valid()) - m_shared_resource_request = SharedResourceRequest::get_or_create(realm, m_page, m_current_url); + if (auto url = URL::Parser::basic_parse(m_current_url); url.has_value()) + m_shared_resource_request = SharedResourceRequest::get_or_create(realm, m_page, url.release_value()); } // https://html.spec.whatwg.org/multipage/images.html#abort-the-image-request diff --git a/Libraries/LibWeb/HTML/ImageRequest.h b/Libraries/LibWeb/HTML/ImageRequest.h index 5566ce77c12..8de4f6fa509 100644 --- a/Libraries/LibWeb/HTML/ImageRequest.h +++ b/Libraries/LibWeb/HTML/ImageRequest.h @@ -39,8 +39,8 @@ public: State state() const; void set_state(State); - URL::URL const& current_url() const; - void set_current_url(JS::Realm&, URL::URL); + String const& current_url() const { return m_current_url; } + void set_current_url(JS::Realm&, String); [[nodiscard]] GC::Ptr image_data() const; void set_image_data(GC::Ptr); @@ -72,7 +72,7 @@ private: // https://html.spec.whatwg.org/multipage/images.html#img-req-url // An image request's current URL is initially the empty string. - URL::URL m_current_url; + String m_current_url; // https://html.spec.whatwg.org/multipage/images.html#img-req-data GC::Ptr m_image_data;