Bug 239853 – ARIA reflection for FrozenArray<Element> attributes

• Status: RESOLVED
• Resolution: FIXED
• Component: Bindings

[Manuel Rego Casasnovas]

This is related to bug #196843 and it'll cover reflection of attributes that refer to a list of Elements.

This would be the 2nd step after bug #239852 gets fixed.

[Radar WebKit Bug Importer]

<rdar://problem/92797836>

[Manuel Rego Casasnovas]

Created attachment 459048 [details]
Patch

[Manuel Rego Casasnovas]

Patch is ready for review, please take a look. Thanks!

[Chris Dumez]

Comment on attachment 459048 [details]
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;
```

[Ryosuke Niwa]

Comment on attachment 459048 [details]
Patch

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

>> Source/WebCore/dom/Element.cpp:2046
>> +            for (auto element : it->value) {
> 
> auto& to avoid some refcounting churn.

We can just use compactMap here.

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

Ditto

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

Ditto

[Ryosuke Niwa]

Comment on attachment 459048 [details]
Patch

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

>> Source/WebCore/dom/Element.cpp:2046
>> +            for (auto element : it->value) {
> 
> auto& to avoid some refcounting churn.

We can just use compactMap here.

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

Ditto

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

Ditto

[Chris Dumez]

Comment on attachment 459048 [details]
Patch

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

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

I agree compactMap() is the way to go for the 2 cases above. For this one, it is less obvious to me given that the loop that other things that populating the vector (building a string).
Sure, we could do that in the compactMap() lambda but it might be a bit confusing, no?

[Ryosuke Niwa]

Comment on attachment 459048 [details]
Patch

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

>>>>> Source/WebCore/dom/Element.cpp:2083
>>>>> +    for (auto element : elements.value()) {
>>>> 
>>>> auto&
>>> 
>>> Ditto
>> 
>> Ditto
> 
> I agree compactMap() is the way to go for the 2 cases above. For this one, it is less obvious to me given that the loop that other things that populating the vector (building a string).
> Sure, we could do that in the compactMap() lambda but it might be a bit confusing, no?

Oh I see. I was thinking that we can have a compactMap to create the new element vector and iterate over the new element vector but I guess it's optional so we can't do that.

[Manuel Rego Casasnovas]

Created attachment 459219 [details]
Patch

[Manuel Rego Casasnovas]

Comment on attachment 459048 [details]
Patch

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

Thanks for the reviews, other than the compactMap() thing I have applied all the suggested changes.

I'd appreciate some advice regarding compactMap() and returning a Vector<RefPtr<>>.

>> Source/WebCore/dom/Element.cpp:2038
>> +
> 
> Unnecessary blank line.

Removed.

>> 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()) {

Done.

>>>> Source/WebCore/dom/Element.cpp:2046
>>>> +            for (auto element : it->value) {
>>> 
>>> auto& to avoid some refcounting churn.
>> 
>> We can just use compactMap here.
> 
> We can just use compactMap here.

> auto& to avoid some refcounting churn.

Done.


> We can just use compactMap here.

I've tried using compactMap like this:
            return compactMap(it->value, [&](auto& element) -> RefPtr<Element> {
                if (element && isDescendantOrShadowDescendantOf(element->rootNode()))
                    return element.get();
                return nullptr;
            });

However that creates a Vector<Ref<Element>> instead of a Vector<RefPtr<Element>> like I need to return here.

Not sure if there's a way to create a Vector<RefPtr<Element>> with compactMap.

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

Done.

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

> ++i

Done.

>> 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.

Changed it to use "std::optional<Vector<RefPtr<Element>>>&&".

>> Source/WebCore/dom/Element.cpp:2081
>> +    Vector<WeakPtr<Element>> newElements;
> 
> newElements.reserveInitialCapacity(elements->size());

Done.

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

Ok.

>> 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;
> ```

Good idea, done.

[Chris Dumez]

(In reply to Manuel Rego Casasnovas from comment #10)
> Comment on attachment 459048 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=459048&action=review
> 
> Thanks for the reviews, other than the compactMap() thing I have applied all
> the suggested changes.
> 
> I'd appreciate some advice regarding compactMap() and returning a
> Vector<RefPtr<>>.
> 
> >> Source/WebCore/dom/Element.cpp:2038
> >> +
> > 
> > Unnecessary blank line.
> 
> Removed.
> 
> >> 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()) {
> 
> Done.
> 
> >>>> Source/WebCore/dom/Element.cpp:2046
> >>>> +            for (auto element : it->value) {
> >>> 
> >>> auto& to avoid some refcounting churn.
> >> 
> >> We can just use compactMap here.
> > 
> > We can just use compactMap here.
> 
> > auto& to avoid some refcounting churn.
> 
> Done.
> 
> 
> > We can just use compactMap here.
> 
> I've tried using compactMap like this:
>             return compactMap(it->value, [&](auto& element) ->
> RefPtr<Element> {
>                 if (element &&
> isDescendantOrShadowDescendantOf(element->rootNode()))
>                     return element.get();
>                 return nullptr;
>             });
> 
> However that creates a Vector<Ref<Element>> instead of a
> Vector<RefPtr<Element>> like I need to return here.
> 
> Not sure if there's a way to create a Vector<RefPtr<Element>> with
> compactMap.

I don't feel like it is a really big deal to be using compactMap() here. However, I *think* you would be able to do what you want by returning a std::optional<RefPtr<Element>>, and have your lambda return std::nullopt instead of nullptr when you don't want this item in the output vector. It is a little awkward though.

[Manuel Rego Casasnovas]

Created attachment 459237 [details]
Patch

[Manuel Rego Casasnovas]

Comment on attachment 459048 [details]
Patch

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

Thanks for the help on compactMap(), I didn't manage to use it in one place.
I hope this is fine to land anyway.

>>>> Source/WebCore/dom/Element.cpp:2038
>>>> +
>>> 
>>> Unnecessary blank line.
>> 
>> Removed.
> 
> I don't feel like it is a really big deal to be using compactMap() here. However, I *think* you would be able to do what you want by returning a std::optional<RefPtr<Element>>, and have your lambda return std::nullopt instead of nullptr when you don't want this item in the output vector. It is a little awkward though.

Yeah that worked. Thank you very much, I've now used compactMap() in the first loop.

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

I haven't used compactMap() here because I didn't find a way to use it with SpaceSplitString.

[Manuel Rego Casasnovas]

Created attachment 459238 [details]
Patch

[EWS]

Committed r294141 (250511@main): <https://commits.webkit.org/250511@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 459238 [details].