Bug 101312

Created attachment 172500 [details] WIP

Comment on attachment 172500 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=172500&action=review > Source/WebCore/css/StyleScopeResolver.h:94 > + }; Let's move this out from the class. Nested classes tend to limit flexibility to moving code around. Also, make this a class instead of a struct.

Comment on attachment 172500 [details] WIP Attachment 172500 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/14683353

Comment on attachment 172500 [details] WIP Attachment 172500 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/14722936

Comment on attachment 172500 [details] WIP Attachment 172500 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/14665481

Comment on attachment 172500 [details] WIP Attachment 172500 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14726877

Created attachment 172719 [details] WIP

Comment on attachment 172719 [details] WIP Attachment 172719 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/14761208

Comment on attachment 172719 [details] WIP Attachment 172719 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14763081

Created attachment 172722 [details] WIP

Created attachment 173538 [details] Patch

Comment on attachment 173538 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=173538&action=review > Source/WebCore/css/StyleResolver.cpp:751 > + Vector<std::pair<const ContainerNode*, RuleSet*> > matchedRules; Let's specify some initial vector capacity for easy perf win. Also, I think we can hold this vector as a ScopedRuleData member. Is that right? And do you think is it a good approach? > Source/WebCore/css/StyleScopeResolver.cpp:88 > +ScopedRuleData* StyleScopeResolver::addScopedRuleData(const ContainerNode* scope, bool needRuleSet, bool& isNewEntry) - The name sounds misleading. This doesn't always create nor add ScopedRuleData. It lazily creates an instance for them instead. - Is it possible to make the tree setup as a part of this instead of let caller do it. the isNewEntry out parameter is something sad to see. > Source/WebCore/css/StyleScopeResolver.cpp:100 > +void StyleScopeResolver::setupScopedRuleDataTree(ScopedRuleData* target) Can this be loop instead of recursion? > Source/WebCore/css/StyleScopeResolver.cpp:107 > + // prepared. How about to have "already prepared" flag on each ScopedRuleData that indicates the parent is correctly set? We've spent too much complexity just to avoid extra lookups of their parents, which better be avoided if possible. > Source/WebCore/css/StyleScopeResolver.cpp:145 > + if (scope->isShadowRoot()) { It seems this can be part of scopedRuleDataFor() because every ShadowRoot is going to have ScopedRuleData eventually and The logic is overlapping to setupScopedRuleDataTree() in certain way. > Source/WebCore/css/StyleScopeResolver.cpp:187 > +inline ScopedRuleData* StyleScopeResolver::parentRuleData(ScopedRuleData* ruleData) Why not a method of ScopedRuleData? > Source/WebCore/css/StyleScopeResolver.h:118 > + ScopedRuleData* m_stackHead; It is confusing to see having both m_stackHead and m_stackParent, whose relationship is unclear. What is the stack here after all? Is there any better way to explain what is happening here?

Comment on attachment 173538 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=173538&action=review I think this is a great start in the right direction, but I worry there's still not enough clarity in data structures and their consumers. The way I see it is that there are: 1) The list of scope ancestors, whose integrity is maintained during cascading attach calls. This data structure persists throughout the entire cycle of resolving styles. This is one nicely encapsulated class. 2) The scope-aware style resolver, or the actor on the data structure mentioned in #1, which is instantiated at will (it is only relevant while computing style of one element), and which incorporates all the logic of crawling up #1 and collecting rules on it. Then, the StyleResolver only needs to hold on to the #1 and create #2 only when it needs to resolve scoped styles. WDYT? > Source/WebCore/ChangeLog:3 > + <style scoped>: refactor StyleScopeResolver The title of the bug is too generic to say anything useful. > Source/WebCore/css/StyleScopeResolver.cpp:138 > + m_stackHead = 0; Whoa. What happens with the ScopedRuleData? >> Source/WebCore/css/StyleScopeResolver.h:118 >> + ScopedRuleData* m_stackHead; > > It is confusing to see having both m_stackHead and m_stackParent, whose relationship is unclear. > What is the stack here after all? Is there any better way to explain what is happening here? Stacks usually have top and bottom, not head and tail. Also: is this really a stack? This is simply a chain of ancestor scopes.

If I am not mistaken <style scoped> was removed from Webkit and also from Web-Spec but came back in different way. Is this refactoring required? Thanks!