|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>|
|Severity:||Normal||CC:||cc-bugs, dglazkov, enne, jamesr, senorblanco, vangelis, webkit.review.bot|
|Version:||528+ (Nightly build)|
Description Shawn Singh 2012-02-13 14:36:56 PST
Hopefully this is the first of many small micro changes to CCLayerTreeHostCommon::calculateDrawTransformsAndVisibility. My preference is to be very strict about the extent of these changes, so that we can easily prove by inspection that behavior remains unchanged, in addition to testing. This particular change is also covered by essentially all existing tests that use accelerated compositing, so there are no new tests. Patch coming in a moment.
Comment 2 Adrienne Walker 2012-02-13 16:14:40 PST
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.
Comment 3 Shawn Singh 2012-02-13 18:03:13 PST
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 4 WebKit Review Bot 2012-02-13 19:22:08 PST
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
Comment 5 James Robinson 2012-02-13 20:43:22 PST
Is the EWS output plausible? I didn't think that these tests could run through the composited path
Comment 6 Shawn Singh 2012-02-13 23:17:23 PST
(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.
Comment 7 James Robinson 2012-02-13 23:35:53 PST
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 8 Shawn Singh 2012-02-13 23:45:14 PST
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.
Comment 9 Stephen White 2012-02-14 10:33:59 PST
(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.
Comment 10 Shawn Singh 2012-02-14 11:28:29 PST
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.
Comment 11 WebKit Review Bot 2012-02-15 01:31:34 PST
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 12 James Robinson 2012-02-15 14:40:42 PST
Comment on attachment 127005 [details] Same patch, with minor fix R=me
Comment 13 Shawn Singh 2012-02-15 16:39:59 PST
Created attachment 127273 [details] Patch for landing
Comment 14 WebKit Review Bot 2012-02-15 21:11:28 PST
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
Comment 15 Shawn Singh 2012-02-15 23:44:27 PST
Created attachment 127321 [details] Patch for landing
Comment 16 WebKit Review Bot 2012-02-16 04:16:07 PST
Comment on attachment 127321 [details] Patch for landing Clearing flags on attachment: 127321 Committed r107921: <http://trac.webkit.org/changeset/107921>
Comment 17 WebKit Review Bot 2012-02-16 04:16:16 PST
All reviewed patches have been landed. Closing bug.