Bug 72462

Created attachment 172030 [details] Patch

*** Bug 93545 has been marked as a duplicate of this bug. ***

Comment on attachment 172030 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=172030&action=review > Source/WebCore/css/StyleResolver.cpp:469 > +#if ENABLE(STYLE_SCOPED) || ENABLE(SHADOW_DOM) We don't need these ifdefs considering that scope is always null when these flags are off. > Source/WebCore/css/StyleResolver.cpp:1661 > +#if ENABLE(STYLE_SCOPED) || ENABLE(SHADOW_DOM) Ditto. > Source/WebCore/css/StyleResolver.cpp:1663 > + if (!keyframesRule && (!e->isInShadowTree() || e->shadowRoot()->applyAuthorStyles())) { Can we return early for these cases? I guess extracting the keyframeRule lookup as a separate method can be a good simplification. > Source/WebCore/css/StyleScopeResolver.cpp:250 > +{ I hope this function to be further clearer. - Let's define helper function as needed. - Having a few extra traversal is acceptable for clarity. We can make it faster when it becomes really hot. > Source/WebCore/css/StyleScopeResolver.h:109 > + NamedScopedKeyframesRuleMap m_keyframesRuleMap; Can we make a pair of the ContainerNode pointer and AtomicStringImpl pointer as a key, instead of having nesting map?

Created attachment 172263 [details] Patch

Comment on attachment 172263 [details] Patch Attachment 172263 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/14719680

Comment on attachment 172263 [details] Patch Attachment 172263 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14725611

Comment on attachment 172263 [details] Patch Attachment 172263 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/14722641

Comment on attachment 172030 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=172030&action=review Thank you for reviewing. >> Source/WebCore/css/StyleResolver.cpp:469 >> +#if ENABLE(STYLE_SCOPED) || ENABLE(SHADOW_DOM) > > We don't need these ifdefs considering that scope is always null when these flags are off. Done. >> Source/WebCore/css/StyleResolver.cpp:1661 >> +#if ENABLE(STYLE_SCOPED) || ENABLE(SHADOW_DOM) > > Ditto. Done. >> Source/WebCore/css/StyleResolver.cpp:1663 >> + if (!keyframesRule && (!e->isInShadowTree() || e->shadowRoot()->applyAuthorStyles())) { > > Can we return early for these cases? > I guess extracting the keyframeRule lookup as a separate method can be a good simplification. I added a separate method to look up @keyframes rule. >> Source/WebCore/css/StyleScopeResolver.cpp:250 >> +{ > > I hope this function to be further clearer. > - Let's define helper function as needed. > - Having a few extra traversal is acceptable for clarity. We can make it faster when it becomes really hot. I see. Done. >> Source/WebCore/css/StyleScopeResolver.h:109 >> + NamedScopedKeyframesRuleMap m_keyframesRuleMap; > > Can we make a pair of the ContainerNode pointer and AtomicStringImpl pointer as a key, > instead of having nesting map? I'm afraid of performance issue. Firstly I was thinking of such a pair. However, we have to traverse a dom tree from the given node. This means, we always have to see all ancestor nodes of the given node to know whether we have any matched @keyframes rules or not. This is the reason why I used such nesting map.

Comment on attachment 172030 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=172030&action=review >>> Source/WebCore/css/StyleResolver.cpp:469 >>> +#if ENABLE(STYLE_SCOPED) || ENABLE(SHADOW_DOM) >> >> We don't need these ifdefs considering that scope is always null when these flags are off. > > Done. Sorry. I need this because of cq-. Since I use addKeyframeStyle here, we need an implementation of class StyleScopeResolver. But the implementation is enabled when ENABLE(STYLE_SCOPED) || ENABLE(SHADOW_DOM). And I changed the logic. Unlsess ENABLE(STYLE_SCOPED) || ENABLE(SHADOW_DOM), check whether scope is 0 or not. If scope is not 0, just return. This is the same behavior as the original code.

Comment on attachment 172263 [details] Patch Attachment 172263 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/14733401

Created attachment 172266 [details] Patch

Comment on attachment 172030 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=172030&action=review >>>> Source/WebCore/css/StyleResolver.cpp:469 >>>> +#if ENABLE(STYLE_SCOPED) || ENABLE(SHADOW_DOM) >>> >>> We don't need these ifdefs considering that scope is always null when these flags are off. >> >> Done. > > Sorry. I need this because of cq-. Since I use addKeyframeStyle here, we need an implementation of class StyleScopeResolver. But the implementation is enabled when ENABLE(STYLE_SCOPED) || ENABLE(SHADOW_DOM). > And I changed the logic. Unlsess ENABLE(STYLE_SCOPED) || ENABLE(SHADOW_DOM), check whether scope is 0 or not. If scope is not 0, just return. This is the same behavior as the original code. We already have a fake implementation for dealing with these cases.

(In reply to comment #8) > I'm afraid of performance issue. Firstly I was thinking of such a pair. However, we have to traverse a dom tree from the given node. > This means, we always have to see all ancestor nodes of the given node to know whether we have any matched @keyframes rules or not. > This is the reason why I used such nesting map. Hmm. Something wrong, it seems... I wonder why we need different maps for each scoped styles and scoped key frames. It looks we should have a class which maintains a set of scoped style descriptions. What do you think?

(In reply to comment #13) > I wonder why we need different maps for each scoped styles and scoped key frames. > It looks we should have a class which maintains a set of scoped style descriptions. > What do you think? Would you mean adding a new hashmap from animation names to @keyframes rules into class RuleSet? However, in the case, we still need our own method to find a matched @keyframes rule, because we don't need to find all matched @keyframes rules. ("matched" means, @keyframes rules whose animation names are the same.) --- The @keyframes rule that is used by an animation will be the last one encountered in sorted rules order that matches the name of the animation specified by the ‘animation-name’ property. @keyframes rules do not cascade; therefore, an animation will never derive keyframes from more than one @keyframes rule. --- (c.f. http://dev.w3.org/csswg/css3-animations/#keyframes) So what I'm trying to do is to obtain the last one encountered in document order from the inner scope to the outer scope. If any @keyframes rule is found, quickly return the rule and apply the rule to some CSS animation. If we move such a hashmap into class RuleSet, we will always have to see all ancestor nodes for each animation names. For examples, document() | +--A | +--B | +--C | +--D If the given element is D, we have to check: (1) whether D has a @keyframes rule whose animation name matches the given name. If any @keyframes rule is found, return the rule. (2) C, ditto. (3) B, ditto. (4) A, ditto. (5) When we reach document(), we can know the animation name is not defined in the given scope. If we have a few number of animations / shadow roots / scoped styles, we can ignore.

Just some observations/intuitions, hopefully will aid you in finding the right approach: * scoped stylesheets and shadow tree style scoping feel like two different things. If we only look at the latter, it's a lot more encapsulated and manageable as a separate chink. With the former, we definitely have to treat style resolution across all trees as one thing. * the components (and thus their shadow trees) will prefer "apply-author-styles" set to false, thus making style resolution inside shadow trees completely independent of the enclosing trees. We should optimize for that. * with "apply-author-styles" false, the UA stylesheets are still resolved in all trees, but maybe they should be the exception, not the rule? * points above indicate that we could consider separate StyleResolvers for each tree, rather than one to rule them all. Indeed, each ShadowRoot has a stylesheet collection already. * chunking style resolution into separate StyleResolver per tree probably has performance benefits.

> > * points above indicate that we could consider separate StyleResolvers for each tree, rather than one to rule them all. Indeed, each ShadowRoot has a stylesheet collection already. > > * chunking style resolution into separate StyleResolver per tree probably has performance benefits. This sounds radical, but I agree that we need to go toward per-subtree style resolution. Tasak is working on Bug 101312, which is a smaller step to that direction.

Separate StyleResolvers for author shadow trees (but not ua ones) sounds like the correct way to go.

Scoped stylesheets were taken out.