[webkit-reviews] review denied: [Bug 87805] [Shadow DOM] <style> inside Shadow subtree should be scoped inside the subtree. : [Attachment 145728] 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 145728: Patch
https://bugs.webkit.org/attachment.cgi?id=145728&action=review

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


Thank you for adding the tests! I am now reasonably confident that when we land
this, we have enough coverage to capture regressions in the intended behavior.

> Source/WebCore/ChangeLog:6
> +	   Modified any style in shadow subtree to be treated as a scoped style


"style" is not the right reference here. In WebKit (and web platform), the word
"style" by itself is ambiguous. In your particular case, it would be good to
make sure you are discussing the STYLE element, or HTMLStyleElement. Can you
change your references to "style" to either "STYLE element" or
"HTMLStyleElement"?

> Source/WebCore/ChangeLog:40
> +	   (WebCore::HTMLStyleElement::updateForStyleScoped):
> +	   Newly added. This method is for updating a style's scope when scoped

> +	   attribute is set to be true. Currently there are the following four
> +	   cases:
> +	   1, the style is "scoped" and is in document tree,
> +	   2, the style is "scoped" and is in shadow subtree,
> +	   3, the style is not "scoped" and is in document tree, and
> +	   4, the style is not "scoped" and is in shadow subtree.
> +	   If the style is not in document, just updating only m_scoped.
> +	   And this method treats the case 3 to 1 and 4 to 2. In both case,
> +	   the style must be registered with scoping node. However
> +	   in 4 to 2, the style has been already registered with shadow root.
> +	   So firstly the style must be unregistered with shadow root.
> +	   (WebCore::HTMLStyleElement::updateForStyleNotScoped):
> +	   Newly added. This method is for updating a style's scope when scoped

> +	   attribute is set to be null, i.e. false. The method treats
> +	   the case 1 to 3 and 2 to 4.

This is no longer relevant. Can you update the entry? I would recommend moving
the problem description (more general) out of the file modification
explanations (more specific).

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

Having both m_scoped and scoped() in the same class, and scoped() not
reflecting the value of m_scoped is bad. This code will be hard to understand.
It seems that Roland had a good name that matched what was happening and didn't
clash with scoped().

Also, when I suggested combining them, I didn't realize you'd just rip one of
them out like that. The m_isRegisteredWithScopingNode serves an important
purpose: it is almost always equal to scoped(), except for the times when the
attribute was set (or unset), but the underlying style-scoped machinery has not
yet been adjusted. In these cases, we can rely on m_isRegisteredWithScopingNode
to determine the actual state.

It seems to me that the correct solution is to keep
m_isRegisteredWithScopingNode and rely on isInShadowTree to detect cases 2 and
4 from your problem description.

> Source/WebCore/html/HTMLStyleElement.cpp:-62
> -    // During tear-down, willRemove isn't called, so
m_isRegisteredWithScopingNode may still be set here.
> -    // Therefore we can't ASSERT(!m_isRegisteredWithScopingNode).

This comment is ok to remove.

> Source/WebCore/html/HTMLStyleElement.cpp:89
> +void HTMLStyleElement::styleScopedUpdated(bool scoped)

I apologize for leading you to the wrong name, I now see it. This should be
scopedAttributeChanged. Since we are already on HTMLStyleElement, we don't need
"style" in the name, and mentioning "attribute" describes it precisely.

> Source/WebCore/html/HTMLStyleElement.cpp:100
> +	   // Note: As any style in a shadow tree is treated as "scoped",

"Note:" is superfluous.

> Source/WebCore/html/HTMLStyleElement.cpp:106
> +	   registerWithScopingNode(parentNode());

Can just early return here.

> Source/WebCore/html/HTMLStyleElement.cpp:-158
> -	   if (scoped() && !m_isRegisteredWithScopingNode)

Dropping this condition seems wrong. Are you sure your new condition is its
equivalent?

> Source/WebCore/html/HTMLStyleElement.cpp:172
> +	       ContainerNode* scope = m_scoped? parentNode() : shadowRoot();

need space between "m_scoped" and the "?".