Bug 98244 – [Refactoring] Scoped Style related code should have its own class.

• Component: CSS
• Status: RESOLVED FIXED

[Hajime Morrita]

This is preparation for Bug 95664. 
By extracting scoped stylesheet feature to separate code path,
we could see how it affects the performance for both scoped and non-scoped style cases.
It would clarify the impact of making scoping machinery available for Shadow DOM.

[Hajime Morrita]

Created attachment 166834 [details]
WIP

[Hajime Morrita]

Created attachment 166848 [details]
WIP

[Early Warning System Bot]

Comment on attachment 166848 [details]
WIP

Attachment 166848 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14105021

[Build Bot]

Comment on attachment 166848 [details]
WIP

Attachment 166848 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14134539

[Build Bot]

Comment on attachment 166848 [details]
WIP

Attachment 166848 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14118842

[Early Warning System Bot]

Comment on attachment 166848 [details]
WIP

Attachment 166848 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14124800

[Gyuyoung Kim]

Comment on attachment 166848 [details]
WIP

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

[Dimitri Glazkov (Google)]

This is interesting. Are you planning to break this up into several patches to land?

[Hajime Morrita]

Created attachment 167057 [details]
Patch

[Build Bot]

Comment on attachment 167057 [details]
Patch

Attachment 167057 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14134916

[Early Warning System Bot]

Comment on attachment 167057 [details]
Patch

Attachment 167057 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14173086

[Early Warning System Bot]

Comment on attachment 167057 [details]
Patch

Attachment 167057 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14137917

[Build Bot]

Comment on attachment 167057 [details]
Patch

Attachment 167057 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14136935

[Dimitri Glazkov (Google)]

Comment on attachment 167057 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=167057&action=review

I really like splitting out scope-related things into its own class, and anything else that makes StyleResolver a smaller seems like a win.


Having said that, the patch is too large and needs to be split up to allow for meaningful review. Maybe we could start with moving RuleData/RuleSet into their own files?

Also, is there a performance impact?

> Source/WebCore/css/StyleResolver.cpp:401
> +bool StyleResolver::usesSiblingRules() const
> +{
> +    return !m_features->siblingRules.isEmpty();
> +}
> +
> +bool StyleResolver::usesFirstLineRules() const
> +{
> +    return m_features->usesFirstLineRules;
> +}
> +
> +bool StyleResolver::usesBeforeAfterRules() const
> +{
> +    return m_features->usesBeforeAfterRules;
> +}

Probably should go at the bottom of StyleResolver.h with inline prefix? Or has this convention changed?

> Source/WebCore/css/StyleResolverRules.h:26
> +#ifndef StyleResolverRules_h

Can we name it to match at least one class name in the header? This way, it will be easier to find by file name. How about RuleSet, since it's the majority of the code?

[Hajime Morrita]

Dimitri, thanks for your feedback.
I'll create sub-bug to split existing class out from StyleResolver.cpp/h

[Hajime Morrita]

(In reply to comment #14)
> (From update of attachment 167057 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=167057&action=review
> 
> I really like splitting out scope-related things into its own class, and anything else that makes StyleResolver a smaller seems like a win.
> 
> 
> Having said that, the patch is too large and needs to be split up to allow for meaningful review. Maybe we could start with moving RuleData/RuleSet into their own files?
> 
> Also, is there a performance impact?
> 
> > Source/WebCore/css/StyleResolver.cpp:401
> > +bool StyleResolver::usesSiblingRules() const
> > +{
> > +    return !m_features->siblingRules.isEmpty();
> > +}
> > +
> > +bool StyleResolver::usesFirstLineRules() const
> > +{
> > +    return m_features->usesFirstLineRules;
> > +}
> > +
> > +bool StyleResolver::usesBeforeAfterRules() const
> > +{
> > +    return m_features->usesBeforeAfterRules;
> > +}
> 
> Probably should go at the bottom of StyleResolver.h with inline prefix? Or has this convention changed?
> 
It looks I went too far here. Will keep existing code and just fucus renaming for now.

[Hajime Morrita]

Created attachment 167292 [details]
Patch

[Build Bot]

Comment on attachment 167292 [details]
Patch

Attachment 167292 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14171669

[Gyuyoung Kim]

Comment on attachment 167292 [details]
Patch

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

[Hajime Morrita]

Created attachment 167296 [details]
I'm back

[Build Bot]

Comment on attachment 167296 [details]
I'm back

Attachment 167296 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14168685

[Early Warning System Bot]

Comment on attachment 167296 [details]
I'm back

Attachment 167296 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14175469

[Early Warning System Bot]

Comment on attachment 167296 [details]
I'm back

Attachment 167296 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14180417

[Build Bot]

Comment on attachment 167296 [details]
I'm back

Attachment 167296 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14173537

[Dimitri Glazkov (Google)]

Comment on attachment 167296 [details]
I'm back

View in context: https://bugs.webkit.org/attachment.cgi?id=167296&action=review

This is great. Please make the bubbles happy before landing.

> Source/WebCore/css/StyleResolver.h:489
> +    OwnPtr<StyleScopeResolver> m_scoper;

Who u callin scoper? http://www.urbandictionary.com/define.php?term=Scoper

Maybe m_scopeResolver would be better :)

[Hajime Morrita]

(In reply to comment #25)
> (From update of attachment 167296 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=167296&action=review
> 
> This is great. Please make the bubbles happy before landing.
> 
> > Source/WebCore/css/StyleResolver.h:489
> > +    OwnPtr<StyleScopeResolver> m_scoper;
> 
> Who u callin scoper? http://www.urbandictionary.com/define.php?term=Scoper
> 
> Maybe m_scopeResolver would be better :)
Right. I was grabbed by the short-name-tempation...

Thanks for your review Dimitri, I'll fix it and the build breaks then land this.

[Hajime Morrita]

Created attachment 167667 [details]
Patch for landing

[Hajime Morrita]

Comment on attachment 167667 [details]
Patch for landing

cq- for now to see EWS result.

[WebKit Review Bot]

Comment on attachment 167667 [details]
Patch for landing

Rejecting attachment 167667 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
Kit/chromium/third_party/yasm/source/patched-yasm --revision 154708 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
51>At revision 154708.

________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'

________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
Updating webkit projects from gyp files...

Full output: http://queues.webkit.org/results/14221508

[Hajime Morrita]

Created attachment 167680 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 167680 [details]
Patch for landing

Clearing flags on attachment: 167680

Committed r130732: <http://trac.webkit.org/changeset/130732>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.