<style scoped>: refactor StyleScopeResolver

• Component: CSS
• Status: NEW ---

[Takashi Sakamoto]

Class StyleScopeResolver should have a scoped tree to find matched scoped author rules. This would make it easier to understand the code for style scoped.

[Takashi Sakamoto]

Created attachment 172500 [details]
WIP

[Hajime Morrita]

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.

[Peter Beverloo (cr-android ews)]

Comment on attachment 172500 [details]
WIP

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

[kov's GTK+ EWS bot]

Comment on attachment 172500 [details]
WIP

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

[EFL EWS Bot]

Comment on attachment 172500 [details]
WIP

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

[WebKit Review Bot]

Comment on attachment 172500 [details]
WIP

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

[Takashi Sakamoto]

Created attachment 172719 [details]
WIP

[EFL EWS Bot]

Comment on attachment 172719 [details]
WIP

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

[WebKit Review Bot]

Comment on attachment 172719 [details]
WIP

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

[Takashi Sakamoto]

Created attachment 172722 [details]
WIP

[Takashi Sakamoto]

Created attachment 173538 [details]
Patch

[Hajime Morrita]

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?

[Dimitri Glazkov (Google)]

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.

[Ahmad Saleem]

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!