[Webkit-unassigned] [Bug 88606] [Shadow DOM] Needs @host rule for ShadowDOM styling

[bugzilla-daemon at webkit.org]

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





--- Comment #41 from Takashi Sakamoto <tasak at google.com>  2012-10-17 23:38:27 PST ---
(From update of attachment 169112)
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?

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