LibWeb: Use Optional<FlyString> const& in Element and NamedNodeMap

This is enabled with the newly added IDL generator support for FlyStrings.
2025-07-26 08:57:34 +00:00 · 2024-01-02 23:49:13 +13:00 · 2024-01-02 23:49:13 +13:00 · 285bca1633
commit 285bca1633
parent 8ba3caf6ab
6 changed files with 28 additions and 70 deletions
--- a/Userland/Libraries/LibWeb/DOM/Element.cpp
+++ b/Userland/Libraries/LibWeb/DOM/Element.cpp
@ -224,15 +224,10 @@ WebIDL::ExceptionOr<QualifiedName> validate_and_extract(JS::Realm& realm, Option
 }

 // https://dom.spec.whatwg.org/#dom-element-setattributens
-WebIDL::ExceptionOr<void> Element::set_attribute_ns(Optional<String> const& namespace_, FlyString const& qualified_name, FlyString const& value)
+WebIDL::ExceptionOr<void> Element::set_attribute_ns(Optional<FlyString> const& namespace_, FlyString const& qualified_name, FlyString const& value)
 {
-    // FIXME: This conversion is ugly
-    Optional<FlyString> 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<String> const& namespace_, FlyString const& name) const
+bool Element::has_attribute_ns(Optional<FlyString> 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
--- a/Userland/Libraries/LibWeb/DOM/Element.h
+++ b/Userland/Libraries/LibWeb/DOM/Element.h
@ -92,8 +92,7 @@ public:
    Optional<FlyString> const& namespace_uri() const { return m_qualified_name.namespace_(); }

    bool has_attribute(FlyString const& name) const;
-    // FIXME: This should be taking a 'Optional<FlyString> const&'
-    bool has_attribute_ns(Optional<String> const& namespace_, FlyString const& name) const;
+    bool has_attribute_ns(Optional<FlyString> 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<void> set_attribute(FlyString const& name, String const& value);

-    // FIXME: This should be taking an Optional<FlyString>
-    WebIDL::ExceptionOr<void> set_attribute_ns(Optional<String> const& namespace_, FlyString const& qualified_name, FlyString const& value);
+    WebIDL::ExceptionOr<void> set_attribute_ns(Optional<FlyString> const& namespace_, FlyString const& qualified_name, FlyString const& value);
    void set_attribute_value(FlyString const& local_name, ByteString const& value, Optional<FlyString> const& prefix = {}, Optional<FlyString> const& namespace_ = {});
    WebIDL::ExceptionOr<JS::GCPtr<Attr>> set_attribute_node(Attr&);
    WebIDL::ExceptionOr<JS::GCPtr<Attr>> set_attribute_node_ns(Attr&);
--- 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<DOMString> getAttributeNames();
--- 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<String> const& namespace_, FlyString const& local_name) const
+Attr const* NamedNodeMap::get_named_item_ns(Optional<FlyString> 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<Attr const*> NamedNodeMap::remove_named_item(FlyString const
 }

 // https://dom.spec.whatwg.org/#dom-namednodemap-removenameditemns
-WebIDL::ExceptionOr<Attr const*> NamedNodeMap::remove_named_item_ns(Optional<String> const& namespace_, FlyString const& local_name)
+WebIDL::ExceptionOr<Attr const*> NamedNodeMap::remove_named_item_ns(Optional<FlyString> 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<FlyString> 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<FlyString> const& namespace_, FlyString const& local_name, size_t* item_index)
 {
@ -197,25 +176,19 @@ Attr* NamedNodeMap::get_attribute_ns(Optional<FlyString> 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<Attr*>(const_cast<NamedNodeMap const*>(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<FlyString> 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<FlyString> 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<FlyString> 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<FlyString> const& namespace_, FlyString const& local_name)
 {
    size_t item_index = 0;

--- a/Userland/Libraries/LibWeb/DOM/NamedNodeMap.h
+++ b/Userland/Libraries/LibWeb/DOM/NamedNodeMap.h
@ -9,7 +9,6 @@

 #pragma once

-#include <AK/StringView.h>
 #include <LibWeb/Bindings/LegacyPlatformObject.h>
 #include <LibWeb/Forward.h>
 #include <LibWeb/WebIDL/ExceptionOr.h>
@ -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<String> const& namespace_, FlyString const& local_name) const;
+    Attr const* get_named_item_ns(Optional<FlyString> const& namespace_, FlyString const& local_name) const;
    WebIDL::ExceptionOr<JS::GCPtr<Attr>> set_named_item(Attr& attribute);
    WebIDL::ExceptionOr<JS::GCPtr<Attr>> set_named_item_ns(Attr& attribute);
    WebIDL::ExceptionOr<Attr const*> remove_named_item(FlyString const& qualified_name);
-    WebIDL::ExceptionOr<Attr const*> remove_named_item_ns(Optional<String> const& namespace_, FlyString const& local_name);
+    WebIDL::ExceptionOr<Attr const*> remove_named_item_ns(Optional<FlyString> const& namespace_, FlyString const& local_name);

    // Methods defined by the spec for internal use:
-    // FIXME: These should all be taking 'Optional<FlyString> 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<JS::GCPtr<Attr>> 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<FlyString> 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<FlyString> const& namespace_, FlyString const& local_name);

 private:
    explicit NamedNodeMap(Element&);
--- 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);
 };