Bug 101312 - <style scoped>: refactor StyleScopeResolver
Summary: <style scoped>: refactor StyleScopeResolver
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Takashi Sakamoto
URL:
Keywords:
Depends on:
Blocks: 72462
  Show dependency treegraph
 
Reported: 2012-11-05 23:53 PST by Takashi Sakamoto
Modified: 2013-01-07 14:06 PST (History)
15 users (show)

See Also:


Attachments
WIP (16.03 KB, patch)
2012-11-06 00:04 PST, Takashi Sakamoto
no flags Details | Formatted Diff | Diff
WIP (16.00 KB, patch)
2012-11-06 22:05 PST, Takashi Sakamoto
no flags Details | Formatted Diff | Diff
WIP (16.79 KB, patch)
2012-11-06 22:34 PST, Takashi Sakamoto
no flags Details | Formatted Diff | Diff
Patch (17.79 KB, patch)
2012-11-11 19:03 PST, Takashi Sakamoto
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Takashi Sakamoto 2012-11-05 23:53:44 PST
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.
Comment 1 Takashi Sakamoto 2012-11-06 00:04:09 PST
Created attachment 172500 [details]
WIP
Comment 2 Hajime Morrita 2012-11-06 00:13:29 PST
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 3 Peter Beverloo (cr-android ews) 2012-11-06 00:28:11 PST
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 4 kov's GTK+ EWS bot 2012-11-06 01:28:09 PST
Comment on attachment 172500 [details]
WIP

Attachment 172500 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/14722936
Comment 5 EFL EWS Bot 2012-11-06 01:50:50 PST
Comment on attachment 172500 [details]
WIP

Attachment 172500 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14665481
Comment 6 WebKit Review Bot 2012-11-06 02:08:03 PST
Comment on attachment 172500 [details]
WIP

Attachment 172500 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14726877
Comment 7 Takashi Sakamoto 2012-11-06 22:05:55 PST
Created attachment 172719 [details]
WIP
Comment 8 EFL EWS Bot 2012-11-06 22:11:32 PST
Comment on attachment 172719 [details]
WIP

Attachment 172719 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14761208
Comment 9 WebKit Review Bot 2012-11-06 22:13:30 PST
Comment on attachment 172719 [details]
WIP

Attachment 172719 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14763081
Comment 10 Takashi Sakamoto 2012-11-06 22:34:49 PST
Created attachment 172722 [details]
WIP
Comment 11 Takashi Sakamoto 2012-11-11 19:03:33 PST
Created attachment 173538 [details]
Patch
Comment 12 Hajime Morrita 2012-11-11 20:59:54 PST
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 13 Dimitri Glazkov (Google) 2013-01-07 14:06:02 PST
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.