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.
Created attachment 166834 [details] WIP
Created attachment 166848 [details] WIP
Comment on attachment 166848 [details] WIP Attachment 166848 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/14105021
Comment on attachment 166848 [details] WIP Attachment 166848 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14134539
Comment on attachment 166848 [details] WIP Attachment 166848 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/14118842
Comment on attachment 166848 [details] WIP Attachment 166848 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/14124800
Comment on attachment 166848 [details] WIP Attachment 166848 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/14152061
This is interesting. Are you planning to break this up into several patches to land?
Created attachment 167057 [details] Patch
Comment on attachment 167057 [details] Patch Attachment 167057 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14134916
Comment on attachment 167057 [details] Patch Attachment 167057 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/14173086
Comment on attachment 167057 [details] Patch Attachment 167057 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/14137917
Comment on attachment 167057 [details] Patch Attachment 167057 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/14136935
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?
Dimitri, thanks for your feedback. I'll create sub-bug to split existing class out from StyleResolver.cpp/h
(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.
Created attachment 167292 [details] Patch
Comment on attachment 167292 [details] Patch Attachment 167292 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14171669
Comment on attachment 167292 [details] Patch Attachment 167292 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/14169685
Created attachment 167296 [details] I'm back
Comment on attachment 167296 [details] I'm back Attachment 167296 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14168685
Comment on attachment 167296 [details] I'm back Attachment 167296 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/14175469
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
Comment on attachment 167296 [details] I'm back Attachment 167296 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/14173537
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 :)
(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.
Created attachment 167667 [details] Patch for landing
Comment on attachment 167667 [details] Patch for landing cq- for now to see EWS result.
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
Created attachment 167680 [details] Patch for landing
Comment on attachment 167680 [details] Patch for landing Clearing flags on attachment: 167680 Committed r130732: <http://trac.webkit.org/changeset/130732>
All reviewed patches have been landed. Closing bug.