Summary: | [Refactoring] Implement RuleCollector | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Takashi Sakamoto <tasak> | ||||||||||||||||||||
Component: | CSS | Assignee: | 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
Takashi Sakamoto
2013-02-15 02:13:04 PST
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". |