Bug 88895 - [chromium] Don't set visible layer rect in CCLayerTreeHost paint iteration
Summary: [chromium] Don't set visible layer rect in CCLayerTreeHost paint iteration
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adrienne Walker
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-06-12 11:30 PDT by Adrienne Walker
Modified: 2012-06-12 15:13 PDT (History)
6 users (show)

See Also:


Attachments
Patch (3.52 KB, patch)
2012-06-12 11:32 PDT, Adrienne Walker
no flags Details | Formatted Diff | Diff
Handle root mask (3.70 KB, patch)
2012-06-12 11:46 PDT, Adrienne Walker
no flags Details | Formatted Diff | Diff
Conditional cleanup (3.98 KB, patch)
2012-06-12 12:12 PDT, Adrienne Walker
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adrienne Walker 2012-06-12 11:30:29 PDT
[chromium] Don't set visible layer rect in CCLayerTreeHost paint iteration
Comment 1 Adrienne Walker 2012-06-12 11:32:08 PDT
Created attachment 147120 [details]
Patch
Comment 2 Dana Jansens 2012-06-12 11:36:02 PDT
Comment on attachment 147120 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=147120&action=review

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:840
>          } else if (it.representsContributingRenderSurface()) {

Can the root layer have a mask? (it won't be contributing)
Comment 3 Adrienne Walker 2012-06-12 11:46:19 PDT
Created attachment 147126 [details]
Handle root mask
Comment 4 Dana Jansens 2012-06-12 12:02:31 PDT
Comment on attachment 147126 [details]
Handle root mask

View in context: https://bugs.webkit.org/attachment.cgi?id=147126&action=review

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:839
> +        if (it.representsTargetRenderSurface()) {
> +            LayerType* maskLayer = it->maskLayer();
> +            if (maskLayer)
> +                maskLayer->setVisibleLayerRect(IntRect(IntPoint(), it->contentBounds()));
> +            LayerType* replicaMaskLayer = it->replicaLayer() ? it->replicaLayer()->maskLayer() : 0;
> +            if (replicaMaskLayer)
> +                replicaMaskLayer->setVisibleLayerRect(IntRect(IntPoint(), it->contentBounds()));

Cool, that's better. What do you think of a continue at the end of this block? The If/else situation is a bit confusing, and that might clear it up, letting us drop the else and un-nest that block below. 

Either way, the change looks correct to me.
Comment 5 Adrienne Walker 2012-06-12 12:12:25 PDT
Created attachment 147132 [details]
Conditional cleanup
Comment 6 Dana Jansens 2012-06-12 12:15:15 PDT
Comment on attachment 147132 [details]
Conditional cleanup

This LGTM, thanks for the changes. The logic is changed a bit, and we won't have a visibleLayerRect set on layers that own a surface but don't drawContents themselves, but that is probably just fine.
Comment 7 James Robinson 2012-06-12 14:08:20 PDT
Comment on attachment 147132 [details]
Conditional cleanup

Nice!
Comment 8 WebKit Review Bot 2012-06-12 15:13:07 PDT
Comment on attachment 147132 [details]
Conditional cleanup

Clearing flags on attachment: 147132

Committed r120129: <http://trac.webkit.org/changeset/120129>
Comment 9 WebKit Review Bot 2012-06-12 15:13:12 PDT
All reviewed patches have been landed.  Closing bug.