Bug 78539 – [chromium] Refactor CCLayerTreeHostCommon: create helper function for complex boolean condition

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Shawn Singh]

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.

[Shawn Singh]

Created attachment 126840 [details]
Patch

[Adrienne Walker]

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.

[Shawn Singh]

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.

[WebKit Review Bot]

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

[James Robinson]

Is the EWS output plausible?  I didn't think that these tests could run through the composited path

[Shawn Singh]

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

[James Robinson]

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?

[Shawn Singh]

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.

[Stephen White]

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

[Shawn Singh]

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.

[WebKit Review Bot]

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.

[James Robinson]

Comment on attachment 127005 [details]
Same patch, with minor fix

R=me

[Shawn Singh]

Created attachment 127273 [details]
Patch for landing

[WebKit Review Bot]

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

[Shawn Singh]

Created attachment 127321 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 127321 [details]
Patch for landing

Clearing flags on attachment: 127321

Committed r107921: <http://trac.webkit.org/changeset/107921>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.