[Webkit-unassigned] [Bug 202520] [CSS Shadow Parts] Support 'exportparts' attribute

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=202520

Ryosuke Niwa <rniwa at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #380146|review?                     |review+
              Flags|                            |

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

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20191003/25b950c2/attachment.html>