Bug 109916 – [Refactoring] Implement RuleCollector

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Takashi Sakamoto]

Move collectingRules and matchRules code from StyleResolver to another class.

[Takashi Sakamoto]

Created attachment 188518 [details]
WIP

[Takashi Sakamoto]

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 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 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 on attachment 188518 [details]
WIP

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

[Build Bot]

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 on attachment 188518 [details]
WIP

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

[WebKit Review Bot]

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 on attachment 188518 [details]
WIP

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

[Takashi Sakamoto]

Created attachment 189469 [details]
WIP

[Dimitri Glazkov (Google)]

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]

Created attachment 189694 [details]
Patch

[Takashi Sakamoto]

Created attachment 189698 [details]
Patch

[Takashi Sakamoto]

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)]

The EWSes are all purple..

[Antti Koivisto]

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]

Created attachment 191943 [details]
Patch

[Takashi Sakamoto]

Created attachment 191948 [details]
Patch

[Takashi Sakamoto]

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]

> 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]

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]

and the Apple Mac build: http://build.webkit.org/builders/Apple%20Lion%20Release%20%28Build%29/builds/10845

[Takashi Sakamoto]

(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]

Since I broke webkit-build, I will fix the issue and try again tomorrow (JST).

[Takashi Sakamoto]

Created attachment 192172 [details]
Patch

[Takashi Sakamoto]

Created attachment 192185 [details]
Patch

[Build Bot]

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]

Created attachment 192400 [details]
Patch

[WebKit Review Bot]

Comment on attachment 192400 [details]
Patch

Clearing flags on attachment: 192400

Committed r145349: <http://trac.webkit.org/changeset/145349>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by bug 111966

[Takashi Sakamoto]

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".