Summary: | [chromium] Refactor CCLayerTreeHostCommon: create helper function for complex boolean condition | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Shawn Singh <shawnsingh> | ||||||||||||
Component: | Layout and Rendering | Assignee: | Shawn Singh <shawnsingh> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | cc-bugs, dglazkov, enne, jamesr, senorblanco, vangelis, webkit.review.bot | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Attachments: |
|
Description
Shawn Singh
2012-02-13 14:36:56 PST
Created attachment 126840 [details]
Patch
Comment on attachment 126840 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=126840&action=review > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:130 > + bool shouldCreateSurface = (useSurfaceForMasking || useSurfaceForReflection || useSurfaceForFlatDescendants || useSurfaceForFilters > + || ((useSurfaceForClipping || useSurfaceForOpacity) && layer->descendantDrawsContent())); I think this would be clearer as a series of "if (X) return Y;" conditionals rather than one giant boolean expression. Created attachment 126879 [details]
rewrote as per reviewers comment
Thanks for the suggestion. It is definitely much more readable now. Also added a FIXME based on discussion with Vangelis, which revealed a bug that should be addressed separately.
Comment on attachment 126879 [details] rewrote as per reviewers comment Attachment 126879 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11511717 New failing tests: css3/filters/effect-grayscale-hw.html css3/filters/effect-brightness-hw.html css3/filters/effect-sepia-hw.html css3/filters/effect-opacity-hw.html css3/filters/effect-blur-hw.html css3/filters/effect-combined-hw.html css3/filters/effect-hue-rotate-hw.html css3/filters/effect-invert-hw.html css3/filters/effect-contrast-hw.html css3/filters/effect-saturate-hw.html Is the EWS output plausible? I didn't think that these tests could run through the composited path (In reply to comment #5) > Is the EWS output plausible? I didn't think that these tests could run through the composited path I think so... there are a few lines of logic that deal with css filters in the code I modified. I'll look into it, probably just a small mistake on my part. I didn't think we were using any of this stuff for the software filters path, and I didn't think we ran layout tests for css3 filters through the composited path. Any ideas, Stephen? Comment on attachment 126879 [details] rewrote as per reviewers comment View in context: https://bugs.webkit.org/attachment.cgi?id=126879&action=review > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:131 > + if (layer->filters().size() > 0 && descendantDrawsContent) For equivalence to the previous code, this should not have descendantDrawsContent on it. I thought I removed it, but probably forgot to commit locally before uploading this. (In reply to comment #7) > I didn't think we were using any of this stuff for the software filters path, and I didn't think we ran layout tests for css3 filters through the composited path. Any ideas, Stephen? The -hw.html flavour of those tests use a translateZ(0) to force the composited path. So yes, it is exercising the compositor. Created attachment 127005 [details]
Same patch, with minor fix
Was able to test locally on css3/filters. removing the descendantDrawsContent mistake did fix the problem.
Attachment 127005 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9
Updating OpenSource
Index mismatch: 5815fcf469e98a33fa62d63a6d97a2a16a04fde6 != 77131e3c5b7dde3035199f6c531a4837160a56cb
rereading 2ce77d76874b9ee77991fecb540696485733202a
M Source/WebCore/ChangeLog
M Source/WebCore/platform/graphics/FontCache.cpp
107794 = c59b9dab74417b86c7b5b60a4408ab3e5d1659a8 already exists! Why are we refetching it?
at /usr/lib/git-core/git-svn line 5210
Died at Tools/Scripts/update-webkit line 164.
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 127005 [details]
Same patch, with minor fix
R=me
Created attachment 127273 [details]
Patch for landing
Comment on attachment 127273 [details] Patch for landing Rejecting attachment 127273 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1 ERROR: /mnt/git/webkit-commit-queue/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Full output: http://queues.webkit.org/results/11534635 Created attachment 127321 [details]
Patch for landing
Comment on attachment 127321 [details] Patch for landing Clearing flags on attachment: 127321 Committed r107921: <http://trac.webkit.org/changeset/107921> All reviewed patches have been landed. Closing bug. |