Bug 109916

Summary: [Refactoring] Implement RuleCollector
Product: WebKit Reporter: Takashi Sakamoto <tasak>
Component: CSSAssignee: Takashi Sakamoto <tasak>
Status: RESOLVED FIXED    
Severity: Normal CC: allan.jensen, buildbot, dglazkov, esprehn+autocc, gyuyoung.kim, koivisto, macpherson, menard, ojan.autocc, ossy, rakuco, rego+ews, rniwa, webcomponents-bugzilla, WebkitBugTracker, webkit-ews, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 111707, 111966    
Bug Blocks: 89879    
Attachments:
Description Flags
WIP
none
WIP
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Takashi Sakamoto
Reported 2013-02-15 02:13:04 PST
Move collectingRules and matchRules code from StyleResolver to another class.
Attachments
WIP (80.41 KB, patch)
2013-02-15 02:19 PST, Takashi Sakamoto 		no flags
DetailsFormatted DiffDiff
WIP (96.98 KB, patch)
2013-02-21 00:52 PST, Takashi Sakamoto 		no flags
DetailsFormatted DiffDiff
Patch (97.02 KB, patch)
2013-02-21 23:21 PST, Takashi Sakamoto 		no flags
DetailsFormatted DiffDiff
Patch (97.05 KB, patch)
2013-02-21 23:35 PST, Takashi Sakamoto 		no flags
DetailsFormatted DiffDiff
Patch (1.06 MB, patch)
2013-03-07 01:15 PST, Takashi Sakamoto 		no flags
DetailsFormatted DiffDiff
Patch (1.06 MB, patch)
2013-03-07 01:40 PST, Takashi Sakamoto 		no flags
DetailsFormatted DiffDiff
Patch (1.05 MB, patch)
2013-03-08 00:57 PST, Takashi Sakamoto 		no flags
DetailsFormatted DiffDiff
Patch (1.05 MB, patch)
2013-03-08 02:21 PST, Takashi Sakamoto 		no flags
DetailsFormatted DiffDiff
Patch (1.05 MB, patch)
2013-03-10 21:07 PDT, Takashi Sakamoto 		no flags
DetailsFormatted DiffDiff
Show Obsolete (8) View All Add attachment proposed patch, testcase, etc.
Takashi Sakamoto
Comment 1 2013-02-15 02:19:35 PST
Created attachment 188518 [details] WIP
Takashi Sakamoto
Comment 2 2013-02-15 02:22:45 PST
Still working in progress. Because: - I have to fix a style error caused by the following: //ASSERT((propertyWhitelistType != PropertyWhitelistRegion) || m_state.regionForStyling()); - Also need to remove "StyleResolver*" from ElementRuleCollector.
Early Warning System Bot
Comment 3 2013-02-15 02:25:32 PST
Comment on attachment 188518 [details] WIP Attachment 188518 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/16590038
Early Warning System Bot
Comment 4 2013-02-15 02:26:38 PST
Comment on attachment 188518 [details] WIP Attachment 188518 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/16580772
EFL EWS Bot
Comment 5 2013-02-15 02:33:33 PST
Comment on attachment 188518 [details] WIP Attachment 188518 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/16598001
Build Bot
Comment 6 2013-02-15 02:47:35 PST
Comment on attachment 188518 [details] WIP Attachment 188518 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/16584122
Build Bot
Comment 7 2013-02-15 11:30:02 PST
Comment on attachment 188518 [details] WIP Attachment 188518 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/16586233
WebKit Review Bot
Comment 8 2013-02-15 12:48:12 PST
Attachment 188518 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/WebCore.gypi', u'Source/WebCore/css/ElementRuleCollector.cpp', u'Source/WebCore/css/ElementRuleCollector.h', u'Source/WebCore/css/PageRuleCollector.cpp', u'Source/WebCore/css/PageRuleCollector.h', u'Source/WebCore/css/StyleResolver.cpp', u'Source/WebCore/css/StyleResolver.h']" exit_code: 1 Source/WebCore/css/StyleResolver.cpp:1686: Should have a space between // and comment [whitespace/comments] [4] Total errors found: 1 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.
Build Bot
Comment 9 2013-02-17 00:46:10 PST
Comment on attachment 188518 [details] WIP Attachment 188518 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/16603535
Takashi Sakamoto
Comment 10 2013-02-21 00:52:37 PST
Created attachment 189469 [details] WIP
Dimitri Glazkov (Google)
Comment 11 2013-02-21 08:50:46 PST
Comment on attachment 189469 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=189469&action=review Neat. RuleCollector looks like a pretty straightforward builder. > Source/WebCore/css/ElementRuleCollector.h:93 > + StyleResolver::State& m_state; Sounds like this should be const, right? We're never affecting state from collector.
Takashi Sakamoto
Comment 12 2013-02-21 23:21:18 PST
Created attachment 189694 [details] Patch
Takashi Sakamoto
Comment 13 2013-02-21 23:35:59 PST
Created attachment 189698 [details] Patch
Takashi Sakamoto
Comment 14 2013-02-21 23:38:21 PST
Comment on attachment 189469 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=189469&action=review Thank you for reviewing. >> Source/WebCore/css/ElementRuleCollector.h:93 >> + StyleResolver::State& m_state; > > Sounds like this should be const, right? We're never affecting state from collector. Yes, it's right. Done.
Dimitri Glazkov (Google)
Comment 15 2013-02-22 09:10:06 PST
The EWSes are all purple..
Antti Koivisto
Comment 16 2013-02-22 10:11:30 PST
Comment on attachment 189698 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=189698&action=review Are you using svn copy? We don't want to lose the history. r=me if so. > Source/WebCore/GNUmakefile.list.am:2658 > + Source/WebCore/css/ElementRuleCollector.cpp \ > + Source/WebCore/css/ElementRuleCollector.h \ At some point StyleResolver and the related classes should be moved to a top level directory of their own. > Source/WebCore/css/ElementRuleCollector.h:42 > +class ElementRuleCollector { I don't much like these names (they imply something generic rather than an implementation detail of StyleResolver) though I don't have great suggestions. StyleResolverElementRuleCollector is rather long. Maybe we should move this stuff to its own namespace?
Takashi Sakamoto
Comment 17 2013-03-07 01:15:09 PST
Created attachment 191943 [details] Patch
Takashi Sakamoto
Comment 18 2013-03-07 01:40:53 PST
Created attachment 191948 [details] Patch
Takashi Sakamoto
Comment 19 2013-03-07 02:24:59 PST
Comment on attachment 189698 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=189698&action=review Thank you for reviewing. I recreated the patch by using svn copy and rebased. I also measured html5-full-render's performance. The result is: https://docs.google.com/spreadsheet/ccc?key=0AtdkWCxGaEc2dHVJdEtFZl9qVDd3VmoxcjRESjN6MXc&usp=sharing Since my patch cannot be applied correctly, I would like to try manual check-in. >> Source/WebCore/GNUmakefile.list.am:2658 >> + Source/WebCore/css/ElementRuleCollector.h \ > > At some point StyleResolver and the related classes should be moved to a top level directory of their own. I see. >> Source/WebCore/css/ElementRuleCollector.h:42 >> +class ElementRuleCollector { > > I don't much like these names (they imply something generic rather than an implementation detail of StyleResolver) though I don't have great suggestions. StyleResolverElementRuleCollector is rather long. Maybe we should move this stuff to its own namespace? I see. I would like to do this in another patch.
Takashi Sakamoto
Comment 20 2013-03-07 02:29:51 PST
> Since my patch cannot be applied correctly, I would like to try manual check-in. > I mean, svn-apply seems not to work correctly when too large diff is given.
Csaba Osztrogonác
Comment 21 2013-03-07 03:35:46 PST
It broke the Qt build: http://build.webkit.org/builders/Qt%20Linux%20Release/builds/58233 Could you fix it as soon is possible?
Csaba Osztrogonác
Comment 22 2013-03-07 03:40:47 PST
and the Apple Mac build: http://build.webkit.org/builders/Apple%20Lion%20Release%20%28Build%29/builds/10845
Takashi Sakamoto
Comment 23 2013-03-07 04:05:29 PST
(In reply to comment #22) > and the Apple Mac build: http://build.webkit.org/builders/Apple%20Lion%20Release%20%28Build%29/builds/10845 Sure. I've just reverted my patch. The revision is r145061.
Takashi Sakamoto
Comment 24 2013-03-07 05:14:54 PST
Since I broke webkit-build, I will fix the issue and try again tomorrow (JST).
Takashi Sakamoto
Comment 25 2013-03-08 00:57:30 PST
Created attachment 192172 [details] Patch
Takashi Sakamoto
Comment 26 2013-03-08 02:21:07 PST
Created attachment 192185 [details] Patch
Build Bot
Comment 27 2013-03-08 10:04:03 PST
Comment on attachment 192185 [details] Patch Attachment 192185 [details] did not pass mac-ews (mac): Output: http://webkit-commit-queue.appspot.com/results/17113083 New failing tests: editing/selection/selection-modify-crash.html
Takashi Sakamoto
Comment 28 2013-03-10 21:07:37 PDT
Created attachment 192400 [details] Patch
WebKit Review Bot
Comment 29 2013-03-11 02:52:51 PDT
Comment on attachment 192400 [details] Patch Clearing flags on attachment: 192400 Committed r145349: <http://trac.webkit.org/changeset/145349>
WebKit Review Bot
Comment 30 2013-03-11 02:52:57 PDT
All reviewed patches have been landed. Closing bug.
WebKit Review Bot
Comment 31 2013-03-11 02:59:35 PDT
Re-opened since this is blocked by bug 111966
Takashi Sakamoto
Comment 32 2013-03-13 01:47:54 PDT
I manually submitted this patch (by using svn commit) and monitored whether the patch causes any bad things, e.g. build failure and so on. So I think, the patch doesn't cause such bad things. I will mark this bug as "resolved and fixed".
Note You need to log in before you can comment on or make changes to this bug.