Bug 239853

<rdar://problem/92797836>

Created attachment 459048 [details] Patch

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

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

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

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

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?

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.

Created attachment 459219 [details] Patch

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.

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

Created attachment 459237 [details] Patch

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.

Created attachment 459238 [details] Patch

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