[webkit-reviews] review granted: [Bug 202520] [CSS Shadow Parts] Support 'exportparts' attribute : [Attachment 380146] patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has granted Antti Koivisto <koivisto at iki.fi>'s
request for review:
Bug 202520: [CSS Shadow Parts] Support 'exportparts' attribute
https://bugs.webkit.org/show_bug.cgi?id=202520

Attachment 380146: patch

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




--- Comment #3 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 380146
  --> https://bugs.webkit.org/attachment.cgi?id=380146
patch

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

> Source/WebCore/css/ElementRuleCollector.cpp:273
> +	   SetForScope<const Element*>
partMatchingScope(m_shadowHostInPartRuleScope, &shadowHost);

Can we use RefPtr?

> Source/WebCore/css/ElementRuleCollector.cpp:282
> +    if (!containingShadowRoot.partMappings().isEmpty()) {

Why not exit early instead of nesting?

> Source/WebCore/css/ElementRuleCollector.h:103
> +    const Element* m_shadowHostInPartRuleScope { nullptr };

Can we use RefPtr instead?

> Source/WebCore/css/SelectorChecker.cpp:1154
> +	       auto mapPartName = [&](AtomString partName) {

mapPartName isn't quite descriptive.
How about partNameInRuleScope or translatePartNameToRuleScope?

> Source/WebCore/css/SelectorChecker.cpp:1155
> +		   for (auto* shadowRoot = element.containingShadowRoot();
shadowRoot; shadowRoot = shadowRoot->host()->containingShadowRoot()) {

Use RefPtr?

> Source/WebCore/css/SelectorChecker.cpp:1166
> +	       Vector<AtomString, 3> mappedElementPartNames;

Why not 4??

> Source/WebCore/css/SelectorChecker.h:87
> +	   const Element* shadowHostInPartRuleScope { nullptr };

Can we use RefPtr instead?

> Source/WebCore/dom/Element.cpp:1746
> +	   else if (name  == HTMLNames::exportpartsAttr) {

Nit: Two spaces.

> Source/WebCore/dom/Element.cpp:1747
> +	       if (auto* shadowRoot = this->shadowRoot())

Use makeRefPtr?

> Source/WebCore/dom/ShadowRoot.cpp:259
> +    auto parsePartMapping = [&](StringView mappingString) {

I would have preferred this as a regular static function which returns an
optional pair of AtomicString or a optional struct.

> Source/WebCore/dom/ShadowRoot.cpp:262
> +	   auto skipWhitespace = [&] (auto position) {

We should really add StringView::skipUntil & StringView::skipWhile... not that
I think we should do it in this patch.

> Source/WebCore/dom/ShadowRoot.cpp:278
> +	   

Nit: whitespace.

> Source/WebCore/dom/ShadowRoot.cpp:288
> +	       mappings.add(firstPart, firstPart);
> +	       return;

That way, this would read: return { firstPart, firstPart };

> Source/WebCore/dom/ShadowRoot.cpp:313
> +    while (begin < end) {
> +	   size_t mappingEnd = begin;
> +	   while (mappingEnd < end && mappingsListString[mappingEnd] != ',')
> +	       ++mappingEnd;

Can't we just use StringView::split and iterate over SplitResult?

> Source/WebCore/dom/ShadowRoot.h:123
> +    mutable std::unique_ptr<HashMap<AtomString, AtomString>> m_partMappings;

Maybe we can use Optional instead?
It doesn't seem like an extra pointer size increase to ShadowRoot itself isn't
a big deal.