Bug 78539 - [chromium] Refactor CCLayerTreeHostCommon: create helper function for complex boolean condition
Summary: [chromium] Refactor CCLayerTreeHostCommon: create helper function for complex...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Shawn Singh
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-02-13 14:36 PST by Shawn Singh
Modified: 2012-02-16 04:16 PST (History)
7 users (show)

See Also:


Attachments
Patch (6.39 KB, patch)
2012-02-13 14:43 PST, Shawn Singh
no flags Details | Formatted Diff | Diff
rewrote as per reviewers comment (6.54 KB, patch)
2012-02-13 18:03 PST, Shawn Singh
no flags Details | Formatted Diff | Diff
Same patch, with minor fix (6.57 KB, patch)
2012-02-14 11:28 PST, Shawn Singh
no flags Details | Formatted Diff | Diff
Patch for landing (6.54 KB, patch)
2012-02-15 16:39 PST, Shawn Singh
no flags Details | Formatted Diff | Diff
Patch for landing (6.53 KB, patch)
2012-02-15 23:44 PST, Shawn Singh
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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 1 Shawn Singh 2012-02-13 14:43:39 PST
Created attachment 126840 [details]
Patch
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.