Bug 88606 – [Shadow DOM] Needs @host rule for ShadowDOM styling

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Hajime Morrita]

http://dvcs.w3.org/hg/webcomponents/raw-file/tip/spec/shadow/index.html#selecting-nodes-distributed-to-insertion-points
> The :host pseudoclass represents the shadow host of a shadow DOM subtree.
> If the contextual reference element set is empty or includes elements outside of a shadow DOM subtree, :host matches northing.

[Takashi Sakamoto]

Created attachment 155250 [details]
WIP

[Build Bot]

Comment on attachment 155250 [details]
WIP

Attachment 155250 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13389318

[Build Bot]

Comment on attachment 155250 [details]
WIP

Attachment 155250 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13381765

[Takashi Sakamoto]

Created attachment 155438 [details]
WIP

[Build Bot]

Comment on attachment 155438 [details]
WIP

Attachment 155438 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13400343

[Build Bot]

Comment on attachment 155438 [details]
WIP

Attachment 155438 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13396580

[Build Bot]

Comment on attachment 155438 [details]
WIP

Attachment 155438 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13393566

[Takashi Sakamoto]

Created attachment 155477 [details]
WIP

[Dimitri Glazkov (Google)]

Comment on attachment 155477 [details]
WIP

View in context: https://bugs.webkit.org/attachment.cgi?id=155477&action=review

There are several patches in here, to be landed separately. It's fine to work on this as one patch for now, however.

ProTip: don't fill out the ChangeLog until you're ready to start landing. Otherwise, you'll end up rewriting it many times :)

> Source/WebCore/html/HTMLStyleElement.cpp:155
> +    if (hasHostRule())
> +        if (ShadowRoot* sr = shadowRoot())
> +            if (Element* host = sr->host()) {
> +                host->registerScopedHTMLStyleChild();
> +                isHostRuleRegistered = true;
> +            }

Wild to see this here. Why can't we do this when we're adding rules in StyleResolver, like all other machinery in CSS does?

> Source/WebCore/html/HTMLStyleElement.cpp:185
> +        if (m_scopedStyleRegistrationState == RegisteredInShadowRootWithHostRule || m_scopedStyleRegistrationState == RegisteredAsScopedWithHostRule)
> +            if (ShadowRoot* sr = shadowRoot())
> +                if (Element* host = sr->host())
> +                    host->unregisterScopedHTMLStyleChild();

You won't need any of these when you move @host accounting in styleresolver.

> Source/WebCore/html/HTMLStyleElement.cpp:200
> +bool HTMLStyleElement::hasHostRule() const
> +{
> +    if (CSSStyleSheet* styleSheet = const_cast<HTMLStyleElement*>(this)->sheet())
> +        return styleSheet->contents()->hasHostRule();
> +    return false;
> +}

Or these.

> Source/WebCore/html/shadow/HTMLShadowElement.h:56
> +    bool m_registeredWithShadowRoot;

It seems we're building an extra set of machinery for something that already exists. We already _know_ which shadow trees are rendered or not by the time we've created the composed tree, which happens before style resolution. Please ask morrita@/hayato@ for guidance here.

[Takashi Sakamoto]

Created attachment 155752 [details]
WIP

[Takashi Sakamoto]

Created attachment 155754 [details]
WIP

[Takashi Sakamoto]

Created attachment 155769 [details]
WIP

[Takashi Sakamoto]

Thank you for reviewing my WIP patch.

(In reply to comment #9)
> (From update of attachment 155477 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=155477&action=review
> 
> There are several patches in here, to be landed separately. It's fine to work on this as one patch for now, however.
> 
> ProTip: don't fill out the ChangeLog until you're ready to start landing. Otherwise, you'll end up rewriting it many times :)

I see. I recreated ChangeLog and removed all details about my patch.

> > Source/WebCore/html/HTMLStyleElement.cpp:155
> > +    if (hasHostRule())
> > +        if (ShadowRoot* sr = shadowRoot())
> > +            if (Element* host = sr->host()) {
> > +                host->registerScopedHTMLStyleChild();
> > +                isHostRuleRegistered = true;
> > +            }
> 
> Wild to see this here. Why can't we do this when we're adding rules in StyleResolver, like all other machinery in CSS does?
> 
> > Source/WebCore/html/HTMLStyleElement.cpp:185
> > +        if (m_scopedStyleRegistrationState == RegisteredInShadowRootWithHostRule || m_scopedStyleRegistrationState == RegisteredAsScopedWithHostRule)
> > +            if (ShadowRoot* sr = shadowRoot())
> > +                if (Element* host = sr->host())
> > +                    host->unregisterScopedHTMLStyleChild();
> 
> You won't need any of these when you move @host accounting in styleresolver.
> 
> > Source/WebCore/html/HTMLStyleElement.cpp:200
> > +bool HTMLStyleElement::hasHostRule() const
> > +{
> > +    if (CSSStyleSheet* styleSheet = const_cast<HTMLStyleElement*>(this)->sheet())
> > +        return styleSheet->contents()->hasHostRule();
> > +    return false;
> > +}
> 
> Or these.

I see. As I decided to create the own matchHostRules or collectMatchingRules, I don't need to use m_scopedAuthorStyles now.
I removed these codes and modified StyleResolver, i.e. adding m_atHostRules to class StyleResolver, not class RuleSet.

> 
> > Source/WebCore/html/shadow/HTMLShadowElement.h:56
> > +    bool m_registeredWithShadowRoot;
> 
> It seems we're building an extra set of machinery for something that already exists. We already _know_ which shadow trees are rendered or not by the time we've created the composed tree, which happens before style resolution. Please ask morrita@/hayato@ for guidance here.

I asked hayato-san and he said that it is possible to find insertion points when distributed nodes are given. However, when shadow roots are given, there are no direct way to find whether there are any <shadow> in the shadow DOM subtrees.
He said, m_nodeToInsertionPoint's values() are insertion points. So, by using the following way:
(1) obtaining treescope of each m_nodeToInsertionPoint value and
(2) comparing the treescope with the given shadow root.
we can find active insertion points.

Is it better to try hayato-san's way and to file a new bug: "improve performance of finding <shadow>"?

Best regards,
Takashi Sakamoto

[Takashi Sakamoto]

Created attachment 156902 [details]
WIP

[Takashi Sakamoto]

Created attachment 159626 [details]
Patch

[Dimitri Glazkov (Google)]

Comment on attachment 159626 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=159626&action=review

> Source/WebCore/css/StyleResolver.cpp:1046
> +    for (ShadowRoot* shadowRoot = shadow->youngestShadowRoot(); shadowRoot; shadowRoot = shadowRoot->olderShadowRoot()) {
> +        if (atHostRuleSetForScope(shadowRoot))
> +            return true;
> +        if (!shadowRoot->hasShadowInsertionPoint())
> +            break;
> +    }

Can we cache this value on ElementShadow?

> Source/WebCore/css/StyleResolver.cpp:1069
> +    Vector<RuleSet*, 16> matchedRules; 
> +    for (ShadowRoot* shadowRoot = shadow->youngestShadowRoot(); shadowRoot; shadowRoot = shadowRoot->olderShadowRoot()) { 
> +        if (RuleSet* ruleSet = atHostRuleSetForScope(shadowRoot))
> +            matchedRules.append(ruleSet);
> +        if (!shadowRoot->hasShadowInsertionPoint())
> +            break;
> +    }
> +    if (matchedRules.isEmpty())
> +        return;

Ah, this could be on ElementShadow, just as I suggested above. Basically, maintain a list of activeShadowRoots (the interesting bit is that you only need to store one value, the last visible shadow root from the stack). This should allow you to make the patch neatly splittable into the section where we first expose the ElementShadow::activeShadowRoots along with window.internals tests for it, and then a trivial patch of adding the support @host rules. WDYT?

[Takashi Sakamoto]

Comment on attachment 159626 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=159626&action=review

Thank you for reviewing.
I have not updated the patch yet, however I would like to confirm whether we should implement activeShadowRoots or not.

>> Source/WebCore/css/StyleResolver.cpp:1046
>> +    }
> 
> Can we cache this value on ElementShadow?

I see. We hav e to cache the value. I will implement.

>> Source/WebCore/css/StyleResolver.cpp:1069
>> +        return;
> 
> Ah, this could be on ElementShadow, just as I suggested above. Basically, maintain a list of activeShadowRoots (the interesting bit is that you only need to store one value, the last visible shadow root from the stack). This should allow you to make the patch neatly splittable into the section where we first expose the ElementShadow::activeShadowRoots along with window.internals tests for it, and then a trivial patch of adding the support @host rules. WDYT?

I think, we can add (or remove from)<style> which has some @host @-rules to any shadow dom subtree after adding the shadow dom subtree. So we have to update activeShadowRoots when childrenChanged in class ShadowRoot is invoked.
And I think, by using the activeShadowRoots, we can only remove "if (!shadowRoot->hasShadowInsertionPoint())". ( I guess, it will take almost the same time to traverse activeShadowRoots as just to traverse shadow roots from the youngest one.)
So I'm not sure whether we should implement activeShadowRoots. Do we need the list?

[Takashi Sakamoto]

(In reply to comment #17)
> (From update of attachment 159626 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=159626&action=review
> 
> Thank you for reviewing.
> I have not updated the patch yet, however I would like to confirm whether we should implement activeShadowRoots or not.
> 
> >> Source/WebCore/css/StyleResolver.cpp:1046
> >> +    }
> > 
> > Can we cache this value on ElementShadow?
> 
> I see. We hav e to cache the value. I will implement.
> 
> >> Source/WebCore/css/StyleResolver.cpp:1069
> >> +        return;
> > 
> > Ah, this could be on ElementShadow, just as I suggested above. Basically, maintain a list of activeShadowRoots (the interesting bit is that you only need to store one value, the last visible shadow root from the stack). This should allow you to make the patch neatly splittable into the section where we first expose the ElementShadow::activeShadowRoots along with window.internals tests for it, and then a trivial patch of adding the support @host rules. WDYT?
> 
> I think, we can add (or remove from)<style> which has some @host @-rules to any shadow dom subtree after adding the shadow dom subtree. So we have to update activeShadowRoots when childrenChanged in class ShadowRoot is invoked.
> And I think, by using the activeShadowRoots, we can only remove "if (!shadowRoot->hasShadowInsertionPoint())". ( I guess, it will take almost the same time to traverse activeShadowRoots as just to traverse shadow roots from the youngest one.)
> So I'm not sure whether we should implement activeShadowRoots. Do we need the list?

I'm sorry. Not <style>, I mean <shadow>. Since it is possible to add/remove <shadow>, probably we have to maintain an entire list of active shadow roots.

[Takashi Sakamoto]

Created attachment 161866 [details]
Patch

[Takashi Sakamoto]

Created attachment 164506 [details]
Patch

[Dimitri Glazkov (Google)]

Comment on attachment 164506 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=164506&action=review

Thank you for working on this!

> Source/WebCore/css/StyleResolver.cpp:1112
> +        transferMatchedRules(result);

This is wrong. We should somehow change the specificity of the @host at-rules, so that it all works as sort-and-match. Hmmm.. This might be a spec bug.

> Source/WebCore/css/StyleResolver.h:556
> +    RuleSet* atHostRuleSetForScope(const ShadowRoot*);
> +    bool matchesHostRules();

please group functions with functions and data with data.

> Source/WebCore/dom/ShadowRoot.h:94
> +    void registerShadowElement() { ++m_numberOfShadowElementChildren; }
> +    void unregisterShadowElement() { --m_numberOfShadowElementChildren; }
> +    bool hasShadowInsertionPoint() const { return m_numberOfShadowElementChildren > 0; }

This seems like a separate, smaller patch.

[Takashi Sakamoto]

Created attachment 165044 [details]
Patch

[Takashi Sakamoto]

Comment on attachment 164506 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=164506&action=review

Thank you for reviewing. I filed a new bug 97184, which blocks this bug 88606. After finishing bug 97184, I will request review again.

Best regards,
Takashi Sakamoto

>> Source/WebCore/css/StyleResolver.cpp:1112
>> +        transferMatchedRules(result);
> 
> This is wrong. We should somehow change the specificity of the @host at-rules, so that it all works as sort-and-match. Hmmm.. This might be a spec bug.

I see. I modified "RuleSet::addStyleRule" and "RuleSet::addRule" to modify RuleData's specificity when the given style rule comes from @host @-rules.

>> Source/WebCore/css/StyleResolver.h:556
>> +    bool matchesHostRules();
> 
> please group functions with functions and data with data.

Done.

>> Source/WebCore/dom/ShadowRoot.h:94
>> +    bool hasShadowInsertionPoint() const { return m_numberOfShadowElementChildren > 0; }
> 
> This seems like a separate, smaller patch.

I filed a new bug for this, i.e. https://bugs.webkit.org/show_bug.cgi?id=97184.
My patch still has these changes... to pass cq-bot check. I will remove these changes after a patch for bug 97184 is landed.

[Takashi Sakamoto]

Created attachment 166821 [details]
Patch

[Hajime Morrita]

Comment on attachment 166821 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=166821&action=review

> Source/WebCore/css/StyleResolver.cpp:271
> +    void addStyleRule(StyleRule*, bool hasDocumentSecurityOrigin, bool canUseFastCheckSelector, bool isInRegionRule = false, bool isHostRule = false);

Let's turn the multiple boolean into a enum as a bit mask. something like "true, false, true" on the call side is readability disaster.

[Takashi Sakamoto]

Created attachment 167716 [details]
Patch

[Takashi Sakamoto]

Comment on attachment 166821 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=166821&action=review

>> Source/WebCore/css/StyleResolver.cpp:271
>> +    void addStyleRule(StyleRule*, bool hasDocumentSecurityOrigin, bool canUseFastCheckSelector, bool isInRegionRule = false, bool isHostRule = false);
> 
> Let's turn the multiple boolean into a enum as a bit mask. something like "true, false, true" on the call side is readability disaster.

I see.
Done.

[Takashi Sakamoto]

Created attachment 167949 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 167949 [details]
Patch

Attachment 167949 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14258024

[Early Warning System Bot]

Comment on attachment 167949 [details]
Patch

Attachment 167949 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14253116

[Build Bot]

Comment on attachment 167949 [details]
Patch

Attachment 167949 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14257035

[Early Warning System Bot]

Comment on attachment 167949 [details]
Patch

Attachment 167949 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14257036

[Build Bot]

Comment on attachment 167949 [details]
Patch

Attachment 167949 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14253130

[Takashi Sakamoto]

Created attachment 168155 [details]
Patch

[Hajime Morrita]

Comment on attachment 168155 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=168155&action=review

> Source/WebCore/css/RuleSet.h:50
> +    RuleData(StyleRule*, unsigned selectorIndex, unsigned position, unsigned addRuleState);

Coud you use AddRuleState instead of unsigned?

> Source/WebCore/css/StyleResolver.cpp:438
> +void StyleResolver::addHostRule(StyleRuleHost* hostRule, bool hasDocumentSecurityOrigin, const ContainerNode* scope)

Can we move this implementation more to StyleScopeResolver?
Ideally all @host related code should be owned by StyleScopeResolver.
I know it's unfeasible for now. But I think we could be better here.

> Source/WebCore/css/StyleResolver.cpp:440
> +#if ENABLE(STYLE_SCOPED)

This isn't related STYLE_SCOPED. This should be part of SHAODW_ROOT.

> Source/WebCore/css/StyleResolver.cpp:781
> +#if ENABLE(STYLE_SCOPED)

Ditto.

> Source/WebCore/css/StyleResolver.cpp:790
> +#if ENABLE(STYLE_SCOPED)

Ditto.

[Takashi Sakamoto]

Created attachment 168412 [details]
Patch

[Takashi Sakamoto]

Comment on attachment 168155 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=168155&action=review

Thank you for reviewing.

>> Source/WebCore/css/RuleSet.h:50
>> +    RuleData(StyleRule*, unsigned selectorIndex, unsigned position, unsigned addRuleState);
> 
> Coud you use AddRuleState instead of unsigned?

I see. Done.

>> Source/WebCore/css/StyleResolver.cpp:438
>> +void StyleResolver::addHostRule(StyleRuleHost* hostRule, bool hasDocumentSecurityOrigin, const ContainerNode* scope)
> 
> Can we move this implementation more to StyleScopeResolver?
> Ideally all @host related code should be owned by StyleScopeResolver.
> I know it's unfeasible for now. But I think we could be better here.

I see. I moved.... 
So I created a new method to ensure m_scopeResolver, i.e. ensureScopeResolver to create (if needed) and obtain m_scopeResolver. Is this ok...?

>> Source/WebCore/css/StyleResolver.cpp:440
>> +#if ENABLE(STYLE_SCOPED)
> 
> This isn't related STYLE_SCOPED. This should be part of SHAODW_ROOT.

I see. I modified to use ENABLE(SHADOW_DOM).

>> Source/WebCore/css/StyleResolver.cpp:781
>> +#if ENABLE(STYLE_SCOPED)
> 
> Ditto.

Done.

>> Source/WebCore/css/StyleResolver.cpp:790
>> +#if ENABLE(STYLE_SCOPED)
> 
> Ditto.

Done.

[Takashi Sakamoto]

Created attachment 169112 [details]
Patch

[Hajime Morrita]

Comment on attachment 169112 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=169112&action=review

> Source/WebCore/css/RuleSet.cpp:360
> +            resolver->ensureScopeResolver()->addHostRule(static_cast<StyleRuleHost*>(rule), hasDocumentSecurityOrigin, scope);

If possible I'd keep StyleScopeResovler as a n implementation detail of StyleResolver.

> Source/WebCore/css/StyleResolver.cpp:795
> +    matchHostRules(result, includeEmptyRules);

Could you move this call int to matchScopedAuthorRules()? Then we can assume that we have m_scopedResolver

> Source/WebCore/css/StyleResolver.h:34
> +#include "StyleScopeResolver.h"

Can we eliminate this?

> Source/WebCore/css/StyleResolver.h:76
> +class ShadowRoot;

It looks we don't need this?

> Source/WebCore/css/StyleRule.cpp:393
> +StyleRuleHost::StyleRuleHost(Vector<RefPtr<StyleRuleBase> >& adoptRules)

it's fine to inline these in the header.

> Source/WebCore/css/StyleRule.cpp:398
> +StyleRuleHost::StyleRuleHost(const StyleRuleHost& o)

ditto.

> Source/WebCore/css/StyleScopeResolver.cpp:195
> +bool StyleScopeResolver::styleSharingCandidateMatchesHostRules(const Element* element)

How about to have a flag just for indicating existence of @host rules regardless that matches or not?
This hidden side effect is adding complexity which we might not need. 
We can assume that many host rule will match to the host in many cases. That's why the author put @host rule after all.

[Takashi Sakamoto]

Created attachment 169352 [details]
Patch

[Takashi Sakamoto]

Comment on attachment 169112 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=169112&action=review

Thank you for reviewing.
I updated the patch except styleSharingCandidateMatchesHostRules. I would like to confirm the comment about styleSharingCandidateMatchesHostRules.

Best regards,
Takashi Sakamoto

>> Source/WebCore/css/RuleSet.cpp:360
>> +            resolver->ensureScopeResolver()->addHostRule(static_cast<StyleRuleHost*>(rule), hasDocumentSecurityOrigin, scope);
> 
> If possible I'd keep StyleScopeResovler as a n implementation detail of StyleResolver.

I added a wrapper method to StyleScopeResolver::addHostRule to StyleResolver.
The wrapper method looks like just ... "{ ensureScopeResolver()->addHostRule(...);". Now ensureScopeResolver is a private method.

>> Source/WebCore/css/StyleResolver.cpp:795
>> +    matchHostRules(result, includeEmptyRules);
> 
> Could you move this call int to matchScopedAuthorRules()? Then we can assume that we have m_scopedResolver

Sure. Done.

>> Source/WebCore/css/StyleResolver.h:34
>> +#include "StyleScopeResolver.h"
> 
> Can we eliminate this?

I need StyleScopeResolver.h because addHostRule() and ensureScopeResolver() defined in StyleResolver.h need information about StyleScopeResolver. i.e.
adoptPtr(new StyleScopeResolver()) and ensureScopeResolver()->addHostRule(...).
Is it better to move these implementation into StyleResolver.cpp?

>> Source/WebCore/css/StyleRule.cpp:393
>> +StyleRuleHost::StyleRuleHost(Vector<RefPtr<StyleRuleBase> >& adoptRules)
> 
> it's fine to inline these in the header.

Done.

>> Source/WebCore/css/StyleRule.cpp:398
>> +StyleRuleHost::StyleRuleHost(const StyleRuleHost& o)
> 
> ditto.

Done.

>> Source/WebCore/css/StyleScopeResolver.cpp:195
>> +bool StyleScopeResolver::styleSharingCandidateMatchesHostRules(const Element* element)
> 
> How about to have a flag just for indicating existence of @host rules regardless that matches or not?
> This hidden side effect is adding complexity which we might not need. 
> We can assume that many host rule will match to the host in many cases. That's why the author put @host rule after all.

I would like to confirm this.
Currently, I added a flag to ElementShadow and the flag caches whether any @host @-rule is applied to shadow host or not.
So would you mean adding a flag to class Element or class RenderStyle and caching existence of @host @-rules instead of ElementShadow's flag?

[Build Bot]

Comment on attachment 169352 [details]
Patch

Attachment 169352 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14400708

[Early Warning System Bot]

Comment on attachment 169352 [details]
Patch

Attachment 169352 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14392927

[Early Warning System Bot]

Comment on attachment 169352 [details]
Patch

Attachment 169352 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14400710

[Hajime Morrita]

Comment on attachment 169112 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=169112&action=review

>>> Source/WebCore/css/StyleScopeResolver.cpp:195
>>> +bool StyleScopeResolver::styleSharingCandidateMatchesHostRules(const Element* element)
>> 
>> How about to have a flag just for indicating existence of @host rules regardless that matches or not?
>> This hidden side effect is adding complexity which we might not need. 
>> We can assume that many host rule will match to the host in many cases. That's why the author put @host rule after all.
> 
> I would like to confirm this.
> Currently, I added a flag to ElementShadow and the flag caches whether any @host @-rule is applied to shadow host or not.
> So would you mean adding a flag to class Element or class RenderStyle and caching existence of @host @-rules instead of ElementShadow's flag?

Having the flag on ElementShadow is fine. I though we could have a flag that just implies whether the styles in shadow have @host rule or not
since we can set the flag in StyleScopeResolver::addHostRule(). If it's not so straightforward, we could keep current approach.
I prefer a tri-state enum rather than two bool values with uninitialized one - uninitialized values better not be used if possible.

[Takashi Sakamoto]

I see. Thank you, Morita-san.

I agree with you. I will update addHostRule.

And I will also fix cq-.

Best regards,
Takashi Sakamoto


(In reply to comment #45)
> (From update of attachment 169112 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=169112&action=review
> 
> >>> Source/WebCore/css/StyleScopeResolver.cpp:195
> >>> +bool StyleScopeResolver::styleSharingCandidateMatchesHostRules(const Element* element)
> >> 
> >> How about to have a flag just for indicating existence of @host rules regardless that matches or not?
> >> This hidden side effect is adding complexity which we might not need. 
> >> We can assume that many host rule will match to the host in many cases. That's why the author put @host rule after all.
> > 
> > I would like to confirm this.
> > Currently, I added a flag to ElementShadow and the flag caches whether any @host @-rule is applied to shadow host or not.
> > So would you mean adding a flag to class Element or class RenderStyle and caching existence of @host @-rules instead of ElementShadow's flag?
> 
> Having the flag on ElementShadow is fine. I though we could have a flag that just implies whether the styles in shadow have @host rule or not
> since we can set the flag in StyleScopeResolver::addHostRule(). If it's not so straightforward, we could keep current approach.
> I prefer a tri-state enum rather than two bool values with uninitialized one - uninitialized values better not be used if possible.

[Takashi Sakamoto]

Created attachment 169594 [details]
Patch

[Takashi Sakamoto]

(In reply to comment #45)
> (From update of attachment 169112 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=169112&action=review
> 
> >>> Source/WebCore/css/StyleScopeResolver.cpp:195
> >>> +bool StyleScopeResolver::styleSharingCandidateMatchesHostRules(const Element* element)
> >> 
> >> How about to have a flag just for indicating existence of @host rules regardless that matches or not?
> >> This hidden side effect is adding complexity which we might not need. 
> >> We can assume that many host rule will match to the host in many cases. That's why the author put @host rule after all.
> > 
> > I would like to confirm this.
> > Currently, I added a flag to ElementShadow and the flag caches whether any @host @-rule is applied to shadow host or not.
> > So would you mean adding a flag to class Element or class RenderStyle and caching existence of @host @-rules instead of ElementShadow's flag?
> 
> Having the flag on ElementShadow is fine. I though we could have a flag that just implies whether the styles in shadow have @host rule or not
> since we can set the flag in StyleScopeResolver::addHostRule(). If it's not so straightforward, we could keep current approach.
> I prefer a tri-state enum rather than two bool values with uninitialized one - uninitialized values better not be used if possible.

I filed a new bug, https://bugs.webkit.org/show_bug.cgi?id=99827 to add some flag which has information about whether any @host @-rules is applied or not.

Best regards,
Takashi Sakamoto

[Hajime Morrita]

> >> Source/WebCore/css/StyleResolver.h:34
> >> +#include "StyleScopeResolver.h"
> > 
> > Can we eliminate this?
> 
> I need StyleScopeResolver.h because addHostRule() and ensureScopeResolver() defined in StyleResolver.h need information about StyleScopeResolver. i.e.
> adoptPtr(new StyleScopeResolver()) and ensureScopeResolver()->addHostRule(...).
> Is it better to move these implementation into StyleResolver.cpp?

I think so. addHostRule() is guarded by isHost() thus won't be hot.

[Hajime Morrita]

> >> Source/WebCore/css/StyleResolver.h:34
> >> +#include "StyleScopeResolver.h"
> > 
> > Can we eliminate this?
> 
> I need StyleScopeResolver.h because addHostRule() and ensureScopeResolver() defined in StyleResolver.h need information about StyleScopeResolver. i.e.
> adoptPtr(new StyleScopeResolver()) and ensureScopeResolver()->addHostRule(...).
> Is it better to move these implementation into StyleResolver.cpp?

I think so. addHostRule() is guarded by isHost() thus won't be hot.

[Hajime Morrita]

Comment on attachment 169594 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=169594&action=review

Almost looks good. Added some nits comment.

> Source/WebCore/css/CSSGrammar.y.in:907
> +before_host_rule:

We need to #ifdef these.

> Source/WebCore/css/CSSParser.cpp:9188
> +    case 'h':

Ditto.

> Source/WebCore/css/RuleSet.h:34
> +enum AddRuleState {

Nit: This isn't actually a state. Maybe flags?

> Source/WebCore/css/StyleResolver.h:178
> +    {

Could you assert RuntimeEnabledFeatures::shadowDOMEnabled() || styleScoepedEnabled() here to emphasize this is only enabled with these features being on?

> Source/WebCore/css/StyleScopeResolver.cpp:203
> +    // FIXME(99827): add a new flag to ElementShadow and cache whether any

Could you type the url instead of bug number?

> Source/WebCore/css/StyleScopeResolver.cpp:225
> +    // FIXME(99827): add a new flag to ElementShadow and cache whether any

Ditto.

[Hajime Morrita]

Comment on attachment 169594 [details]
Patch

oops.

[Takashi Sakamoto]

Created attachment 170124 [details]
Patch

[Takashi Sakamoto]

Comment on attachment 169594 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=169594&action=review

>> Source/WebCore/css/CSSGrammar.y.in:907
>> +before_host_rule:
> 
> We need to #ifdef these.

I see. Done.

>> Source/WebCore/css/CSSParser.cpp:9188
>> +    case 'h':
> 
> Ditto.

Done.

>> Source/WebCore/css/RuleSet.h:34
>> +enum AddRuleState {
> 
> Nit: This isn't actually a state. Maybe flags?

I see. Now AddRuleFlag.

>> Source/WebCore/css/StyleResolver.h:178
>> +    {
> 
> Could you assert RuntimeEnabledFeatures::shadowDOMEnabled() || styleScoepedEnabled() here to emphasize this is only enabled with these features being on?

Sure. And I also added #ifdef here.

>> Source/WebCore/css/StyleScopeResolver.cpp:203
>> +    // FIXME(99827): add a new flag to ElementShadow and cache whether any
> 
> Could you type the url instead of bug number?

I added the url.

>> Source/WebCore/css/StyleScopeResolver.cpp:225
>> +    // FIXME(99827): add a new flag to ElementShadow and cache whether any
> 
> Ditto.

Done.

[Hajime Morrita]

Comment on attachment 170124 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=170124&action=review

OK, let's land and see!

> Source/WebCore/css/RuleSet.h:34
> +enum AddRuleFlag {

s/Flag/Flags/

[Takashi Sakamoto]

Created attachment 170292 [details]
Patch

[Takashi Sakamoto]

Comment on attachment 170124 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=170124&action=review

>> Source/WebCore/css/RuleSet.h:34
>> +enum AddRuleFlag {
> 
> s/Flag/Flags/

Sure. Done.

[WebKit Review Bot]

Comment on attachment 170292 [details]
Patch

Clearing flags on attachment: 170292

Committed r132303: <http://trac.webkit.org/changeset/132303>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Dominik Röttsches (drott)]

This is causing failures on EFL, pls. cmp bug 100246.

[WebKit Review Bot]

Re-opened since this is blocked by bug 100287

[Takashi Sakamoto]

Created attachment 170555 [details]
WIP

[Takashi Sakamoto]

I fixed two shadow dom test failures reported by https://bugs.webkit.org/show_bug.cgi?id=100246 :

- fast/dom/shadow/shadowdom-dynamic-styling.html &
   fast/dom/shadow/style-scoped-not-enabled.html

The reason why these tests failed is that I used #if ENABLE(STYLE_SCOPED) in StyleResolver::appendAuthorStyleSheets. The code should be #if ENABLE(STYLE_SCOPED) || ENABLE(SHADOW_DOM). 
Because if only SHADOW_DOM is enabled, my old patch will register styles in shadow dom subtrees with document. This is a problem.

I'm now trying to find why my patch causes https://bugs.webkit.org/show_bug.cgi?id=100287 and crashes @host @-rules test.

Best regards,
Takashi Sakamoto

[Takashi Sakamoto]

Created attachment 170831 [details]
Patch for landing

[Takashi Sakamoto]

(In reply to comment #63)

> I'm now trying to find why my patch causes https://bugs.webkit.org/show_bug.cgi?id=100287 and crashes @host @-rules test.

I obtained the comment that my patch doesn't cause the crashing in many popular sites.

So after finishing the bug https://bugs.webkit.org/show_bug.cgi?id=100455, I would like to request cq?.

Best regards,
Takashi Sakamoto

[Hajime Morrita]

Comment on attachment 170831 [details]
Patch for landing

Let's try again.

[WebKit Review Bot]

Comment on attachment 170831 [details]
Patch for landing

Clearing flags on attachment: 170831

Committed r132618: <http://trac.webkit.org/changeset/132618>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Antti Koivisto]

Comment on attachment 170831 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=170831&action=review

> Source/WebCore/css/CSSPropertySourceData.h:98
> +        REGION_RULE,
> +        HOST_RULE

HOST_RULE should be behind #if ENABLE(SHADOW_DOM) here too (and as a result everywhere it is used).

[Hajime Morrita]

> HOST_RULE should be behind #if ENABLE(SHADOW_DOM) here too (and as a result everywhere it is used).

Thanks for the catch! Filed Bug 102321.