[webkit-reviews] review denied: [Bug 87805] [Shadow DOM] <style> inside Shadow subtree should be scoped inside the subtree. : [Attachment 145543] Patch

[bugzilla-daemon at webkit.org]

Dimitri Glazkov (Google) <dglazkov at chromium.org> has denied Takashi Sakamoto
<tasak at google.com>'s request for review:
Bug 87805: [Shadow DOM] <style> inside Shadow subtree should be scoped inside
the subtree.
https://bugs.webkit.org/show_bug.cgi?id=87805

Attachment 145543: Patch
https://bugs.webkit.org/attachment.cgi?id=145543&action=review

------- Additional Comments from Dimitri Glazkov (Google)
<dglazkov at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=145543&action=review


This looks good overall, I think we need a bit more test coverage:
1) at least make sure that all of the state transitions are accounted for
2) look at nested shadow dom and multiple shadow roots per element.

> Source/WebCore/html/HTMLStyleElement.cpp:55
>      , m_isRegisteredWithScopingNode(false)
> +    , m_scoped(false)

Can we not combine these two? Is there ever a situation when you are scoped,
but have no scoping node?

> Source/WebCore/html/HTMLStyleElement.cpp:96
> +void HTMLStyleElement::updateForStyleScoped()

Would it work better if we name it as a notification to react and combine into
one method?: styleScopedUpdated(attribute.isNull());

>
LayoutTests/fast/css/style-scoped/style-scoped-change-scoped-in-shadow.html:28
> +    debug("E: " + document.defaultView.getComputedStyle(e,
null).getPropertyValue('color')); /* red */
> +    debug("E: " + document.defaultView.getComputedStyle(e,
null).getPropertyValue('border')); /* blue 1px border */
> +    debug("F: " + document.defaultView.getComputedStyle(f,
null).getPropertyValue('color')); /* black */
> +    debug("F: " + document.defaultView.getComputedStyle(e,
null).getPropertyValue('border')); /* blue 1px border */

this can be a function.

>
LayoutTests/fast/css/style-scoped/style-scoped-change-scoped-in-shadow.html:35
> +    document.body.offsetLeft;

Why do we need this here?

>
LayoutTests/fast/css/style-scoped/style-scoped-change-scoped-in-shadow.html:42
> +    document.body.offsetLeft;

.. or here?