Bug 98244 - [Refactoring] Scoped Style related code should have its own class.
Summary: [Refactoring] Scoped Style related code should have its own class.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Hajime Morrita
URL:
Keywords:
Depends on: 98469
Blocks: 95664
  Show dependency treegraph
 
Reported: 2012-10-03 02:25 PDT by Hajime Morrita
Modified: 2012-10-09 01:00 PDT (History)
9 users (show)

See Also:


Attachments
WIP (81.49 KB, patch)
2012-10-03 02:35 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
WIP (84.33 KB, patch)
2012-10-03 03:45 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
Patch (97.67 KB, patch)
2012-10-04 02:10 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
Patch (26.69 KB, patch)
2012-10-05 02:49 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
I'm back (34.13 KB, patch)
2012-10-05 03:21 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
Patch for landing (34.18 KB, patch)
2012-10-08 18:41 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
Patch for landing (34.19 KB, patch)
2012-10-08 20:54 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hajime Morrita 2012-10-03 02:25:10 PDT
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.
Comment 1 Hajime Morrita 2012-10-03 02:35:36 PDT
Created attachment 166834 [details]
WIP
Comment 2 Hajime Morrita 2012-10-03 03:45:37 PDT
Created attachment 166848 [details]
WIP
Comment 3 Early Warning System Bot 2012-10-03 03:57:51 PDT
Comment on attachment 166848 [details]
WIP

Attachment 166848 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14105021
Comment 4 Build Bot 2012-10-03 04:00:29 PDT
Comment on attachment 166848 [details]
WIP

Attachment 166848 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14134539
Comment 5 Build Bot 2012-10-03 04:12:57 PDT
Comment on attachment 166848 [details]
WIP

Attachment 166848 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14118842
Comment 6 Early Warning System Bot 2012-10-03 04:15:53 PDT
Comment on attachment 166848 [details]
WIP

Attachment 166848 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14124800
Comment 7 Gyuyoung Kim 2012-10-03 04:17:04 PDT
Comment on attachment 166848 [details]
WIP

Attachment 166848 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14152061
Comment 8 Dimitri Glazkov (Google) 2012-10-03 09:28:41 PDT
This is interesting. Are you planning to break this up into several patches to land?
Comment 9 Hajime Morrita 2012-10-04 02:10:23 PDT
Created attachment 167057 [details]
Patch
Comment 10 Build Bot 2012-10-04 02:15:44 PDT
Comment on attachment 167057 [details]
Patch

Attachment 167057 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14134916
Comment 11 Early Warning System Bot 2012-10-04 02:22:20 PDT
Comment on attachment 167057 [details]
Patch

Attachment 167057 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14173086
Comment 12 Early Warning System Bot 2012-10-04 02:23:52 PDT
Comment on attachment 167057 [details]
Patch

Attachment 167057 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14137917
Comment 13 Build Bot 2012-10-04 02:41:15 PDT
Comment on attachment 167057 [details]
Patch

Attachment 167057 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14136935
Comment 14 Dimitri Glazkov (Google) 2012-10-04 10:35:51 PDT
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?
Comment 15 Hajime Morrita 2012-10-04 17:34:18 PDT
Dimitri, thanks for your feedback.
I'll create sub-bug to split existing class out from StyleResolver.cpp/h
Comment 16 Hajime Morrita 2012-10-04 17:36:12 PDT
(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.
Comment 17 Hajime Morrita 2012-10-05 02:49:52 PDT
Created attachment 167292 [details]
Patch
Comment 18 Build Bot 2012-10-05 02:55:55 PDT
Comment on attachment 167292 [details]
Patch

Attachment 167292 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14171669
Comment 19 Gyuyoung Kim 2012-10-05 03:09:08 PDT
Comment on attachment 167292 [details]
Patch

Attachment 167292 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14169685
Comment 20 Hajime Morrita 2012-10-05 03:21:56 PDT
Created attachment 167296 [details]
I'm back
Comment 21 Build Bot 2012-10-05 03:31:11 PDT
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 22 Early Warning System Bot 2012-10-05 03:51:03 PDT
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 23 Early Warning System Bot 2012-10-05 04:23:54 PDT
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 24 Build Bot 2012-10-05 05:19:17 PDT
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 25 Dimitri Glazkov (Google) 2012-10-05 09:09:49 PDT
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 :)
Comment 26 Hajime Morrita 2012-10-08 18:06:42 PDT
(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.
Comment 27 Hajime Morrita 2012-10-08 18:41:00 PDT
Created attachment 167667 [details]
Patch for landing
Comment 28 Hajime Morrita 2012-10-08 18:42:45 PDT
Comment on attachment 167667 [details]
Patch for landing

cq- for now to see EWS result.
Comment 29 WebKit Review Bot 2012-10-08 20:01:09 PDT
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
Comment 30 Hajime Morrita 2012-10-08 20:54:27 PDT
Created attachment 167680 [details]
Patch for landing
Comment 31 WebKit Review Bot 2012-10-09 01:00:50 PDT
Comment on attachment 167680 [details]
Patch for landing

Clearing flags on attachment: 167680

Committed r130732: <http://trac.webkit.org/changeset/130732>
Comment 32 WebKit Review Bot 2012-10-09 01:00:57 PDT
All reviewed patches have been landed.  Closing bug.