Created attachment 188518 [details] WIP

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 on attachment 188518 [details] WIP Attachment 188518 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/16590038

Comment on attachment 188518 [details] WIP Attachment 188518 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/16580772

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

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 on attachment 188518 [details] WIP Attachment 188518 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/16586233

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 on attachment 188518 [details] WIP Attachment 188518 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/16603535

Created attachment 189469 [details] WIP

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.

Created attachment 189694 [details] Patch

Created attachment 189698 [details] Patch

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.

The EWSes are all purple..

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?

Created attachment 191943 [details] Patch

Created attachment 191948 [details] Patch

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.

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

It broke the Qt build: http://build.webkit.org/builders/Qt%20Linux%20Release/builds/58233 Could you fix it as soon is possible?

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

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

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

Created attachment 192172 [details] Patch

Created attachment 192185 [details] Patch

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

Created attachment 192400 [details] Patch

Comment on attachment 192400 [details] Patch Clearing flags on attachment: 192400 Committed r145349: <http://trac.webkit.org/changeset/145349>

All reviewed patches have been landed. Closing bug.

Re-opened since this is blocked by bug 111966

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