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

Description Takashi Sakamoto 2013-02-15 02:13:04 PST
Move collectingRules and matchRules code from StyleResolver to another class.
Comment 1 Takashi Sakamoto 2013-02-15 02:19:35 PST
Created attachment 188518 [details]
WIP
Comment 2 Takashi Sakamoto 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.
Comment 3 Early Warning System Bot 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
Comment 4 Early Warning System Bot 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
Comment 5 EFL EWS Bot 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
Comment 6 Build Bot 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
Comment 7 Build Bot 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
Comment 8 WebKit Review Bot 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.
Comment 9 Build Bot 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
Comment 10 Takashi Sakamoto 2013-02-21 00:52:37 PST
Created attachment 189469 [details]
WIP
Comment 11 Dimitri Glazkov (Google) 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.
Comment 12 Takashi Sakamoto 2013-02-21 23:21:18 PST
Created attachment 189694 [details]
Patch
Comment 13 Takashi Sakamoto 2013-02-21 23:35:59 PST
Created attachment 189698 [details]
Patch
Comment 14 Takashi Sakamoto 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.
Comment 15 Dimitri Glazkov (Google) 2013-02-22 09:10:06 PST
The EWSes are all purple..
Comment 16 Antti Koivisto 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?
Comment 17 Takashi Sakamoto 2013-03-07 01:15:09 PST
Created attachment 191943 [details]
Patch
Comment 18 Takashi Sakamoto 2013-03-07 01:40:53 PST
Created attachment 191948 [details]
Patch
Comment 19 Takashi Sakamoto 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.
Comment 20 Takashi Sakamoto 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.
Comment 21 Csaba Osztrogonác 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?
Comment 22 Csaba Osztrogonác 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
Comment 23 Takashi Sakamoto 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.
Comment 24 Takashi Sakamoto 2013-03-07 05:14:54 PST
Since I broke webkit-build, I will fix the issue and try again tomorrow (JST).
Comment 25 Takashi Sakamoto 2013-03-08 00:57:30 PST
Created attachment 192172 [details]
Patch
Comment 26 Takashi Sakamoto 2013-03-08 02:21:07 PST
Created attachment 192185 [details]
Patch
Comment 27 Build Bot 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
Comment 28 Takashi Sakamoto 2013-03-10 21:07:37 PDT
Created attachment 192400 [details]
Patch
Comment 29 WebKit Review Bot 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>
Comment 30 WebKit Review Bot 2013-03-11 02:52:57 PDT
All reviewed patches have been landed.  Closing bug.
Comment 31 WebKit Review Bot 2013-03-11 02:59:35 PDT
Re-opened since this is blocked by bug 111966
Comment 32 Takashi Sakamoto 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".