From 285bca16330e7b4d2ebc37905565454f1f3288a0 Mon Sep 17 00:00:00 2001 From: Shannon Booth Date: Tue, 2 Jan 2024 23:49:13 +1300 Subject: [PATCH] LibWeb: Use Optional const& in Element and NamedNodeMap This is enabled with the newly added IDL generator support for FlyStrings. --- Userland/Libraries/LibWeb/DOM/Element.cpp | 23 +++------ Userland/Libraries/LibWeb/DOM/Element.h | 6 +-- Userland/Libraries/LibWeb/DOM/Element.idl | 4 +- .../Libraries/LibWeb/DOM/NamedNodeMap.cpp | 47 ++++--------------- Userland/Libraries/LibWeb/DOM/NamedNodeMap.h | 10 ++-- .../Libraries/LibWeb/DOM/NamedNodeMap.idl | 8 ++-- 6 files changed, 28 insertions(+), 70 deletions(-) diff --git a/Userland/Libraries/LibWeb/DOM/Element.cpp b/Userland/Libraries/LibWeb/DOM/Element.cpp index 4eb0398688..230a93616a 100644 --- a/Userland/Libraries/LibWeb/DOM/Element.cpp +++ b/Userland/Libraries/LibWeb/DOM/Element.cpp @@ -224,15 +224,10 @@ WebIDL::ExceptionOr validate_and_extract(JS::Realm& realm, Option } // https://dom.spec.whatwg.org/#dom-element-setattributens -WebIDL::ExceptionOr Element::set_attribute_ns(Optional const& namespace_, FlyString const& qualified_name, FlyString const& value) +WebIDL::ExceptionOr Element::set_attribute_ns(Optional const& namespace_, FlyString const& qualified_name, FlyString const& value) { - // FIXME: This conversion is ugly - Optional namespace_to_use; - if (namespace_.has_value()) - namespace_to_use = namespace_.value(); - // 1. Let namespace, prefix, and localName be the result of passing namespace and qualifiedName to validate and extract. - auto extracted_qualified_name = TRY(validate_and_extract(realm(), namespace_to_use, qualified_name)); + auto extracted_qualified_name = TRY(validate_and_extract(realm(), namespace_, qualified_name)); // 2. Set an attribute value for this using localName, value, and also prefix and namespace. set_attribute_value(extracted_qualified_name.local_name(), value.to_deprecated_fly_string(), extracted_qualified_name.prefix(), extracted_qualified_name.namespace_()); @@ -296,18 +291,14 @@ bool Element::has_attribute(FlyString const& name) const } // https://dom.spec.whatwg.org/#dom-element-hasattributens -bool Element::has_attribute_ns(Optional const& namespace_, FlyString const& name) const +bool Element::has_attribute_ns(Optional const& namespace_, FlyString const& name) const { - StringView namespace_view; - if (namespace_.has_value()) - namespace_view = namespace_->bytes_as_string_view(); - // 1. If namespace is the empty string, then set it to null. - if (namespace_view.is_empty()) - namespace_view = {}; - // 2. Return true if this has an attribute whose namespace is namespace and local name is localName; otherwise false. - return m_attributes->get_attribute_ns(namespace_view, name) != nullptr; + if (namespace_ == FlyString {}) + return m_attributes->get_attribute_ns(OptionalNone {}, name) != nullptr; + + return m_attributes->get_attribute_ns(namespace_, name) != nullptr; } // https://dom.spec.whatwg.org/#dom-element-toggleattribute diff --git a/Userland/Libraries/LibWeb/DOM/Element.h b/Userland/Libraries/LibWeb/DOM/Element.h index 1ef877d875..3292054fe1 100644 --- a/Userland/Libraries/LibWeb/DOM/Element.h +++ b/Userland/Libraries/LibWeb/DOM/Element.h @@ -92,8 +92,7 @@ public: Optional const& namespace_uri() const { return m_qualified_name.namespace_(); } bool has_attribute(FlyString const& name) const; - // FIXME: This should be taking a 'Optional const&' - bool has_attribute_ns(Optional const& namespace_, FlyString const& name) const; + bool has_attribute_ns(Optional const& namespace_, FlyString const& name) const; bool has_attributes() const; ByteString deprecated_attribute(FlyString const& name) const { return deprecated_get_attribute(name); } @@ -105,8 +104,7 @@ public: WebIDL::ExceptionOr set_attribute(FlyString const& name, String const& value); - // FIXME: This should be taking an Optional - WebIDL::ExceptionOr set_attribute_ns(Optional const& namespace_, FlyString const& qualified_name, FlyString const& value); + WebIDL::ExceptionOr set_attribute_ns(Optional const& namespace_, FlyString const& qualified_name, FlyString const& value); void set_attribute_value(FlyString const& local_name, ByteString const& value, Optional const& prefix = {}, Optional const& namespace_ = {}); WebIDL::ExceptionOr> set_attribute_node(Attr&); WebIDL::ExceptionOr> set_attribute_node_ns(Attr&); diff --git a/Userland/Libraries/LibWeb/DOM/Element.idl b/Userland/Libraries/LibWeb/DOM/Element.idl index 182c94395d..297023feb2 100644 --- a/Userland/Libraries/LibWeb/DOM/Element.idl +++ b/Userland/Libraries/LibWeb/DOM/Element.idl @@ -30,14 +30,14 @@ interface Element : Node { DOMString? getAttribute(DOMString qualifiedName); [CEReactions] undefined setAttribute(DOMString qualifiedName, DOMString value); - [CEReactions] undefined setAttributeNS(DOMString? namespace , DOMString qualifiedName , DOMString value); + [CEReactions] undefined setAttributeNS([FlyString] DOMString? namespace , [FlyString] DOMString qualifiedName , DOMString value); [CEReactions] Attr? setAttributeNode(Attr attr); [CEReactions] Attr? setAttributeNodeNS(Attr attr); [CEReactions] undefined removeAttribute(DOMString qualifiedName); [CEReactions] boolean toggleAttribute(DOMString qualifiedName, optional boolean force); boolean hasAttribute(DOMString qualifiedName); - boolean hasAttributeNS(DOMString? namespace, DOMString localName); + boolean hasAttributeNS([FlyString] DOMString? namespace, [FlyString] DOMString localName); boolean hasAttributes(); [SameObject] readonly attribute NamedNodeMap attributes; sequence getAttributeNames(); diff --git a/Userland/Libraries/LibWeb/DOM/NamedNodeMap.cpp b/Userland/Libraries/LibWeb/DOM/NamedNodeMap.cpp index e4b65350bf..4c457ddb39 100644 --- a/Userland/Libraries/LibWeb/DOM/NamedNodeMap.cpp +++ b/Userland/Libraries/LibWeb/DOM/NamedNodeMap.cpp @@ -91,14 +91,9 @@ Attr const* NamedNodeMap::get_named_item(FlyString const& qualified_name) const } // https://dom.spec.whatwg.org/#dom-namednodemap-getnameditemns -Attr const* NamedNodeMap::get_named_item_ns(Optional const& namespace_, FlyString const& local_name) const +Attr const* NamedNodeMap::get_named_item_ns(Optional const& namespace_, FlyString const& local_name) const { - // FIXME: This conversion is quite ugly. - StringView namespace_view; - if (namespace_.has_value()) - namespace_view = namespace_->bytes_as_string_view(); - - return get_attribute_ns(namespace_view, local_name); + return get_attribute_ns(namespace_, local_name); } // https://dom.spec.whatwg.org/#dom-namednodemap-setnameditem @@ -128,15 +123,10 @@ WebIDL::ExceptionOr NamedNodeMap::remove_named_item(FlyString const } // https://dom.spec.whatwg.org/#dom-namednodemap-removenameditemns -WebIDL::ExceptionOr NamedNodeMap::remove_named_item_ns(Optional const& namespace_, FlyString const& local_name) +WebIDL::ExceptionOr NamedNodeMap::remove_named_item_ns(Optional const& namespace_, FlyString const& local_name) { - // FIXME: This conversion is quite ugly. - StringView namespace_view; - if (namespace_.has_value()) - namespace_view = namespace_->bytes_as_string_view(); - // 1. Let attr be the result of removing an attribute given namespace, localName, and element. - auto const* attribute = remove_attribute_ns(namespace_view, local_name); + auto const* attribute = remove_attribute_ns(namespace_, local_name); // 2. If attr is null, then throw a "NotFoundError" DOMException. if (!attribute) @@ -179,17 +169,6 @@ Attr const* NamedNodeMap::get_attribute(FlyString const& qualified_name, size_t* return nullptr; } -// https://dom.spec.whatwg.org/#concept-element-attributes-get-by-namespace -Attr const* NamedNodeMap::get_attribute_ns(Optional const& namespace_, FlyString const& local_name, size_t* item_index) const -{ - // FIXME: We shouldn't need to do any conversion when looking up in the node map. - StringView namespace_view; - if (namespace_.has_value()) - namespace_view = namespace_->bytes_as_string_view(); - - return get_attribute_ns(namespace_view, local_name, item_index); -} - // https://dom.spec.whatwg.org/#concept-element-attributes-get-by-namespace Attr* NamedNodeMap::get_attribute_ns(Optional const& namespace_, FlyString const& local_name, size_t* item_index) { @@ -197,25 +176,19 @@ Attr* NamedNodeMap::get_attribute_ns(Optional const& namespace_, FlyS } // https://dom.spec.whatwg.org/#concept-element-attributes-get-by-namespace -Attr* NamedNodeMap::get_attribute_ns(StringView namespace_, FlyString const& local_name, size_t* item_index) -{ - return const_cast(const_cast(this)->get_attribute_ns(namespace_, local_name, item_index)); -} - -// https://dom.spec.whatwg.org/#concept-element-attributes-get-by-namespace -Attr const* NamedNodeMap::get_attribute_ns(StringView namespace_, FlyString const& local_name, size_t* item_index) const +Attr const* NamedNodeMap::get_attribute_ns(Optional const& namespace_, FlyString const& local_name, size_t* item_index) const { if (item_index) *item_index = 0; // 1. If namespace is the empty string, then set it to null. - if (namespace_.is_empty()) - namespace_ = {}; + Optional normalized_namespace; + if (namespace_.has_value() && namespace_->is_empty()) + normalized_namespace = namespace_; // 2. Return the attribute in element’s attribute list whose namespace is namespace and local name is localName, if any; otherwise null. for (auto const& attribute : m_attributes) { - // FIXME: This is quite awkard. We should probably be taking an Optional for namespace here. - if ((!attribute->namespace_uri().has_value() == namespace_.is_null() || attribute->namespace_uri() == namespace_) && attribute->local_name() == local_name) + if (attribute->namespace_uri() == normalized_namespace && attribute->local_name() == local_name) return attribute.ptr(); if (item_index) ++(*item_index); @@ -320,7 +293,7 @@ Attr const* NamedNodeMap::remove_attribute(FlyString const& qualified_name) } // https://dom.spec.whatwg.org/#concept-element-attributes-remove-by-namespace -Attr const* NamedNodeMap::remove_attribute_ns(StringView namespace_, FlyString const& local_name) +Attr const* NamedNodeMap::remove_attribute_ns(Optional const& namespace_, FlyString const& local_name) { size_t item_index = 0; diff --git a/Userland/Libraries/LibWeb/DOM/NamedNodeMap.h b/Userland/Libraries/LibWeb/DOM/NamedNodeMap.h index b3ff85af49..b006725e97 100644 --- a/Userland/Libraries/LibWeb/DOM/NamedNodeMap.h +++ b/Userland/Libraries/LibWeb/DOM/NamedNodeMap.h @@ -9,7 +9,6 @@ #pragma once -#include #include #include #include @@ -36,18 +35,15 @@ public: // Methods defined by the spec for JavaScript: Attr const* item(u32 index) const; Attr const* get_named_item(FlyString const& qualified_name) const; - Attr const* get_named_item_ns(Optional const& namespace_, FlyString const& local_name) const; + Attr const* get_named_item_ns(Optional const& namespace_, FlyString const& local_name) const; WebIDL::ExceptionOr> set_named_item(Attr& attribute); WebIDL::ExceptionOr> set_named_item_ns(Attr& attribute); WebIDL::ExceptionOr remove_named_item(FlyString const& qualified_name); - WebIDL::ExceptionOr remove_named_item_ns(Optional const& namespace_, FlyString const& local_name); + WebIDL::ExceptionOr remove_named_item_ns(Optional const& namespace_, FlyString const& local_name); // Methods defined by the spec for internal use: - // FIXME: These should all be taking 'Optional const&' for namespace instead of a StringView Attr* get_attribute(FlyString const& qualified_name, size_t* item_index = nullptr); - Attr* get_attribute_ns(StringView namespace_, FlyString const& local_name, size_t* item_index = nullptr); Attr const* get_attribute(FlyString const& qualified_name, size_t* item_index = nullptr) const; - Attr const* get_attribute_ns(StringView namespace_, FlyString const& local_name, size_t* item_index = nullptr) const; WebIDL::ExceptionOr> set_attribute(Attr& attribute); void replace_attribute(Attr& old_attribute, Attr& new_attribute, size_t old_attribute_index); void append_attribute(Attr& attribute); @@ -56,7 +52,7 @@ public: Attr const* get_attribute_ns(Optional const& namespace_, FlyString const& local_name, size_t* item_index = nullptr) const; Attr const* remove_attribute(FlyString const& qualified_name); - Attr const* remove_attribute_ns(StringView namespace_, FlyString const& local_name); + Attr const* remove_attribute_ns(Optional const& namespace_, FlyString const& local_name); private: explicit NamedNodeMap(Element&); diff --git a/Userland/Libraries/LibWeb/DOM/NamedNodeMap.idl b/Userland/Libraries/LibWeb/DOM/NamedNodeMap.idl index 77d50f41d1..6a8b6d03f5 100644 --- a/Userland/Libraries/LibWeb/DOM/NamedNodeMap.idl +++ b/Userland/Libraries/LibWeb/DOM/NamedNodeMap.idl @@ -6,12 +6,12 @@ interface NamedNodeMap { readonly attribute unsigned long length; getter Attr? item(unsigned long index); - getter Attr? getNamedItem(DOMString qualifiedName); - Attr? getNamedItemNS(DOMString? namespace, DOMString localName); + getter Attr? getNamedItem([FlyString] DOMString qualifiedName); + Attr? getNamedItemNS([FlyString] DOMString? namespace, [FlyString] DOMString localName); [CEReactions] Attr? setNamedItem(Attr attr); [CEReactions] Attr? setNamedItemNS(Attr attr); - [CEReactions] Attr removeNamedItem(DOMString qualifiedName); - [CEReactions] Attr removeNamedItemNS(DOMString? namespace, DOMString localName); + [CEReactions] Attr removeNamedItem([FlyString] DOMString qualifiedName); + [CEReactions] Attr removeNamedItemNS([FlyString] DOMString? namespace, [FlyString] DOMString localName); };