[webkit-reviews] review granted: [Bug 239853] ARIA reflection for FrozenArray<Element> attributes : [Attachment 459048] Patch

[bugzilla-daemon at webkit.org]

Chris Dumez <cdumez at apple.com> has granted Manuel Rego Casasnovas
<rego at igalia.com>'s request for review:
Bug 239853: ARIA reflection for FrozenArray<Element> attributes
https://bugs.webkit.org/show_bug.cgi?id=239853

Attachment 459048: Patch

https://bugs.webkit.org/attachment.cgi?id=459048&action=review




--- Comment #4 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 459048
  --> https://bugs.webkit.org/attachment.cgi?id=459048
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=459048&action=review

r=me with comments

> Source/WebCore/dom/Element.cpp:2038
> +

Unnecessary blank line.

> Source/WebCore/dom/Element.cpp:2045
> +	   if (it != map->end()) {

With modern C++, I like this syntax personally:
if (auto it = map->find(attributeName); it != map->end()) {

> Source/WebCore/dom/Element.cpp:2046
> +	       for (auto element : it->value) {

auto& to avoid some refcounting churn.

> Source/WebCore/dom/Element.cpp:2061
> +    auto ids = SpaceSplitString(getAttribute(attr),
SpaceSplitString::ShouldFoldCase::No);

SpaceSplitString ids(getAttribute(attr), SpaceSplitString::ShouldFoldCase::No);

Looks better IMO.

> Source/WebCore/dom/Element.cpp:2062
> +    for (unsigned i = 0; i < ids.size(); i++) {

++i

> Source/WebCore/dom/Element.cpp:2069
> +void Element::setElementsArrayAttribute(const QualifiedName& attributeName,
std::optional<Vector<RefPtr<Element>>> elements)

Should likely be a `std::optional<Vector<RefPtr<Element>>>&&`. Should work
given it comes from bindings. If it doesn't, then plan B would be
`std::optional<Vector<RefPtr<Element>>>` but not passing by value.

> Source/WebCore/dom/Element.cpp:2081
> +    Vector<WeakPtr<Element>> newElements;

newElements.reserveInitialCapacity(elements->size());

> Source/WebCore/dom/Element.cpp:2083
> +    for (auto element : elements.value()) {

auto&

> Source/WebCore/dom/Element.cpp:2085
> +	       newElements.append(element);

uncheckedAppend

> Source/WebCore/dom/Element.cpp:2089
> +	   newElements.append(element);

uncheckedAppend

Seems we could avoid some code duplication by doing:
```
newElements.uncheckedAppend(element);
if (value.isEmpty() && newElements.size() > 1)
    continue;
```