[webkit-reviews] review denied: [Bug 89061] applyAuthorStyles makes rules declared in all enclosing shadow dom subtrees applicable. : [Attachment 147506] 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 89061: applyAuthorStyles makes rules declared in all enclosing shadow dom
subtrees applicable.
https://bugs.webkit.org/show_bug.cgi?id=89061

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

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


> Source/WebCore/css/StyleResolver.cpp:945
> +	   for (const ContainerNode* scope = m_element; scopedIndex >
authorRuleIndex && scope; scope = scope->parentOrHostNode()) {

in this context, authorRuleIndex means first shadow DOM scope. It sounds like
moving this code to a static non-class function would serve well to better
explain this.

Why are we going up using parentOrHostNode?

> Source/WebCore/css/StyleResolver.cpp:948
> +	       for (; scopedIndex > authorRuleIndex; --scopedIndex)
> +		   if (m_scopeStack[scopedIndex - 1].m_scope != scope)
> +		       break;

Can you explain what this code is doing? It seems to me that just going up the
scope stack should be enough?

> LayoutTests/fast/css/style-scoped/style-scoped-apply-author-styles.html:32
> +    document.body.offsetLeft;

I am worried that you keep using this to trigger style recalc. Shouldn't
setting applyAuthorStyles trigger the recalc?