Bug 88606

Summary: [Shadow DOM] Needs @host rule for ShadowDOM styling
Product: WebKit Reporter: Hajime Morrita <morrita>
Component: CSSAssignee: Takashi Sakamoto <tasak>
Status: RESOLVED FIXED    
Severity: Normal CC: allan.jensen, cmarcelo, d-r, macpherson, menard, shinyak, syoichi, tasak, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 87805, 97184, 100203, 100246, 100287, 100455    
Bug Blocks: 97282    
Attachments:
Description Flags
WIP
none
WIP
none
WIP
none
WIP
none
WIP
none
WIP
none
WIP
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
WIP
none
Patch for landing none

Description Hajime Morrita 2012-06-07 19:09:30 PDT
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.
Comment 1 Takashi Sakamoto 2012-07-30 04:16:35 PDT
Created attachment 155250 [details]
WIP
Comment 2 Build Bot 2012-07-30 04:47:09 PDT
Comment on attachment 155250 [details]
WIP

Attachment 155250 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13389318
Comment 3 Build Bot 2012-07-30 05:42:32 PDT
Comment on attachment 155250 [details]
WIP

Attachment 155250 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13381765
Comment 4 Takashi Sakamoto 2012-07-30 22:19:38 PDT
Created attachment 155438 [details]
WIP
Comment 5 Build Bot 2012-07-31 01:33:57 PDT
Comment on attachment 155438 [details]
WIP

Attachment 155438 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13400343
Comment 6 Build Bot 2012-07-31 02:34:07 PDT
Comment on attachment 155438 [details]
WIP

Attachment 155438 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13396580
Comment 7 Build Bot 2012-07-31 02:37:06 PDT
Comment on attachment 155438 [details]
WIP

Attachment 155438 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13393566
Comment 8 Takashi Sakamoto 2012-07-31 02:45:21 PDT
Created attachment 155477 [details]
WIP
Comment 9 Dimitri Glazkov (Google) 2012-07-31 11:29:09 PDT
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.
Comment 10 Takashi Sakamoto 2012-08-01 01:41:50 PDT
Created attachment 155752 [details]
WIP
Comment 11 Takashi Sakamoto 2012-08-01 01:52:58 PDT
Created attachment 155754 [details]
WIP
Comment 12 Takashi Sakamoto 2012-08-01 03:11:30 PDT
Created attachment 155769 [details]
WIP
Comment 13 Takashi Sakamoto 2012-08-01 03:21:36 PDT
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
Comment 14 Takashi Sakamoto 2012-08-07 03:19:15 PDT
Created attachment 156902 [details]
WIP
Comment 15 Takashi Sakamoto 2012-08-20 23:44:41 PDT
Created attachment 159626 [details]
Patch
Comment 16 Dimitri Glazkov (Google) 2012-08-22 15:06:44 PDT
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?
Comment 17 Takashi Sakamoto 2012-08-28 03:29:41 PDT
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?
Comment 18 Takashi Sakamoto 2012-08-29 00:16:47 PDT
(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.
Comment 19 Takashi Sakamoto 2012-09-02 23:08:45 PDT
Created attachment 161866 [details]
Patch
Comment 20 Takashi Sakamoto 2012-09-18 00:34:22 PDT
Created attachment 164506 [details]
Patch
Comment 21 Dimitri Glazkov (Google) 2012-09-19 10:21:35 PDT
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.
Comment 22 Takashi Sakamoto 2012-09-20 21:54:30 PDT
Created attachment 165044 [details]
Patch
Comment 23 Takashi Sakamoto 2012-09-20 21:59:47 PDT
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.
Comment 24 Takashi Sakamoto 2012-10-03 00:45:47 PDT
Created attachment 166821 [details]
Patch
Comment 25 Hajime Morrita 2012-10-03 03:51:38 PDT
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.
Comment 26 Takashi Sakamoto 2012-10-09 02:25:33 PDT
Created attachment 167716 [details]
Patch
Comment 27 Takashi Sakamoto 2012-10-09 02:27:24 PDT
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.
Comment 28 Takashi Sakamoto 2012-10-10 01:01:36 PDT
Created attachment 167949 [details]
Patch
Comment 29 Gyuyoung Kim 2012-10-10 01:06:33 PDT
Comment on attachment 167949 [details]
Patch

Attachment 167949 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14258024
Comment 30 Early Warning System Bot 2012-10-10 01:08:12 PDT
Comment on attachment 167949 [details]
Patch

Attachment 167949 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14253116
Comment 31 Build Bot 2012-10-10 01:08:47 PDT
Comment on attachment 167949 [details]
Patch

Attachment 167949 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14257035
Comment 32 Early Warning System Bot 2012-10-10 01:18:10 PDT
Comment on attachment 167949 [details]
Patch

Attachment 167949 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14257036
Comment 33 Build Bot 2012-10-10 01:57:53 PDT
Comment on attachment 167949 [details]
Patch

Attachment 167949 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14253130
Comment 34 Takashi Sakamoto 2012-10-11 00:50:25 PDT
Created attachment 168155 [details]
Patch
Comment 35 Hajime Morrita 2012-10-12 00:38:24 PDT
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.
Comment 36 Takashi Sakamoto 2012-10-12 06:37:19 PDT
Created attachment 168412 [details]
Patch
Comment 37 Takashi Sakamoto 2012-10-12 06:41:04 PDT
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.
Comment 38 Takashi Sakamoto 2012-10-17 00:56:25 PDT
Created attachment 169112 [details]
Patch
Comment 39 Hajime Morrita 2012-10-17 19:16:43 PDT
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.
Comment 40 Takashi Sakamoto 2012-10-17 23:36:33 PDT
Created attachment 169352 [details]
Patch
Comment 41 Takashi Sakamoto 2012-10-17 23:38:27 PDT
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?
Comment 42 Build Bot 2012-10-17 23:42:27 PDT
Comment on attachment 169352 [details]
Patch

Attachment 169352 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14400708
Comment 43 Early Warning System Bot 2012-10-17 23:43:31 PDT
Comment on attachment 169352 [details]
Patch

Attachment 169352 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14392927
Comment 44 Early Warning System Bot 2012-10-17 23:43:33 PDT
Comment on attachment 169352 [details]
Patch

Attachment 169352 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14400710
Comment 45 Hajime Morrita 2012-10-17 23:53:01 PDT
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.
Comment 46 Takashi Sakamoto 2012-10-18 20:41:13 PDT
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.
Comment 47 Takashi Sakamoto 2012-10-19 03:48:01 PDT
Created attachment 169594 [details]
Patch
Comment 48 Takashi Sakamoto 2012-10-19 04:08:13 PDT
(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
Comment 49 Hajime Morrita 2012-10-21 18:06:48 PDT
> >> 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.
Comment 50 Hajime Morrita 2012-10-21 18:06:49 PDT
> >> 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.
Comment 51 Hajime Morrita 2012-10-21 18:12:23 PDT
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.
Comment 52 Hajime Morrita 2012-10-21 18:46:42 PDT
Comment on attachment 169594 [details]
Patch

oops.
Comment 53 Takashi Sakamoto 2012-10-23 04:49:18 PDT
Created attachment 170124 [details]
Patch
Comment 54 Takashi Sakamoto 2012-10-23 04:50:44 PDT
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.
Comment 55 Hajime Morrita 2012-10-23 17:59:14 PDT
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/
Comment 56 Takashi Sakamoto 2012-10-23 19:09:35 PDT
Created attachment 170292 [details]
Patch
Comment 57 Takashi Sakamoto 2012-10-23 19:10:34 PDT
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.
Comment 58 WebKit Review Bot 2012-10-23 21:37:36 PDT
Comment on attachment 170292 [details]
Patch

Clearing flags on attachment: 170292

Committed r132303: <http://trac.webkit.org/changeset/132303>
Comment 59 WebKit Review Bot 2012-10-23 21:37:45 PDT
All reviewed patches have been landed.  Closing bug.
Comment 60 Dominik Röttsches (drott) 2012-10-24 07:08:29 PDT
This is causing failures on EFL, pls. cmp bug 100246.
Comment 61 WebKit Review Bot 2012-10-24 14:17:49 PDT
Re-opened since this is blocked by bug 100287
Comment 62 Takashi Sakamoto 2012-10-24 23:10:35 PDT
Created attachment 170555 [details]
WIP
Comment 63 Takashi Sakamoto 2012-10-24 23:16:44 PDT
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
Comment 64 Takashi Sakamoto 2012-10-25 23:50:58 PDT
Created attachment 170831 [details]
Patch for landing
Comment 65 Takashi Sakamoto 2012-10-25 23:52:54 PDT
(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
Comment 66 Hajime Morrita 2012-10-26 04:01:30 PDT
Comment on attachment 170831 [details]
Patch for landing

Let's try again.
Comment 67 WebKit Review Bot 2012-10-26 04:33:39 PDT
Comment on attachment 170831 [details]
Patch for landing

Clearing flags on attachment: 170831

Committed r132618: <http://trac.webkit.org/changeset/132618>
Comment 68 WebKit Review Bot 2012-10-26 04:33:47 PDT
All reviewed patches have been landed.  Closing bug.
Comment 69 Antti Koivisto 2012-11-14 06:32:16 PST
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).
Comment 70 Hajime Morrita 2012-11-14 19:52:06 PST
> 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.