[Webkit-unassigned] [Bug 87805] [Shadow DOM] <style> inside Shadow subtree should be scoped inside the subtree.

[bugzilla-daemon at webkit.org]

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


Dimitri Glazkov (Google) <dglazkov at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #145728|review?                     |review-
               Flag|                            |




--- Comment #5 from Dimitri Glazkov (Google) <dglazkov at chromium.org>  2012-06-05 09:25:44 PST ---
(From update of attachment 145728)
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 "?".

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.