Use a boolean hasCompositedLayerMapping() accessor instead of the pointer

Use a boolean hasCompositedLayerMapping() accessor instead of the pointer

In order to add the ability to paint multiple RenderLayers into one
composited layer backing, we need to complexity the possible "states"
that a RenderLayer identifies as its compositingState. There are many
places in code that still use compositedLayerMapping() as a boolean
accessor to indicate compositingState, but doing so now is already
inadequate, and with layer squashing, it may even be wrong in many
places.

To gradually evolve the codebase away from the compositedLayerMapping()
accessor as a boolean, it is helpful to create a boolean accessor,
hasCompositedLayerMapping(). The purpose of doing this is to more easily
distinguish between places in code where compositedLayerMapping() is
actually used as an object versus places where it is used as a boolean.

BUG=261605
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=161973

[shawnsingh, 2013-11-04 22:20:56]

Glenn, can you please take the first look so that Ian can rubberstamp it when
he's back?  Thanks in advance =)

[hartmanng, 2013-11-04 22:56:09]

non-OWNER lgtm

https://codereview.chromium.org/58543002/diff/80001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayerCompositor.cpp (right):

https://codereview.chromium.org/58543002/diff/80001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayerCompositor.cpp:458:
ASSERT(compositedLayerMapping);
Personally, I'd just take this out - seems inconsistent to ASSERT this here, but
everywhere else just checking hasCompositedLayerMapping() is enough.

[shawnsingh, 2013-11-12 09:57:01]

Ian has taken over this patch and is working on this here -
https://codereview.chromium.org/59063003/

[Ian Vollick, 2013-11-12 21:02:06]

On 2013/11/12 09:57:01, shawnsingh wrote:
> Ian has taken over this patch and is working on this here -
> https://codereview.chromium.org/59063003/

Refreshed this patch. PTAL, folks.

[shawnsingh, 2013-11-12 21:12:50]

LGTM with one nit

https://codereview.chromium.org/58543002/diff/150001/Source/core/testing/Inte...
File Source/core/testing/Internals.cpp (right):

https://codereview.chromium.org/58543002/diff/150001/Source/core/testing/Inte...
Source/core/testing/Internals.cpp:1275: if (searchRoot->compositingState() ==
PaintsIntoOwnBacking && graphicsLayer ==
searchRoot->compositedLayerMapping()->mainGraphicsLayer())

Option 1: maybe we should keep this as hasCompositedLayerMapping for now

Option 2: can you please explain why it should not be true when
HasOwnBackingButPaintsIntoAncestor?  based on the comment, it should include
that condition.  the mainGraphicsLayer will still exist for composited
RenderLayers that paint into ancestor.

[hartmanng, 2013-11-12 21:20:51]

slgtm modulo Shawn's comment

[Ian Vollick, 2013-11-12 22:06:26]

On 2013/11/12 21:12:50, shawnsingh wrote:
> LGTM with one nit
> 
>
https://codereview.chromium.org/58543002/diff/150001/Source/core/testing/Inte...
> File Source/core/testing/Internals.cpp (right):
> 
>
https://codereview.chromium.org/58543002/diff/150001/Source/core/testing/Inte...
> Source/core/testing/Internals.cpp:1275: if (searchRoot->compositingState() ==
> PaintsIntoOwnBacking && graphicsLayer ==
> searchRoot->compositedLayerMapping()->mainGraphicsLayer())
> 
> Option 1: maybe we should keep this as hasCompositedLayerMapping for now
> 
> Option 2: can you please explain why it should not be true when
> HasOwnBackingButPaintsIntoAncestor?  based on the comment, it should include
> that condition.  the mainGraphicsLayer will still exist for composited
> RenderLayers that paint into ancestor.

This was an omission when I was removing all of my changes to move towards
compositingState. It's removed in the latest patch set.

[commit-bot: I haz the power, 2013-11-12 22:06:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shawnsingh@chromium.org/58543002/500001

[commit-bot: I haz the power, 2013-11-12 22:33:32]

Retried try job too often on blink_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=blink_pres...

[Ian Vollick, 2013-11-13 00:34:35]

On 2013/11/12 22:33:32, I haz the power (commit-bot) wrote:
> Retried try job too often on blink_presubmit for step(s) presubmit
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=blink_pres...

+jamesr for OWNERS

[jamesr, 2013-11-13 00:38:36]

Can you have a /core/ owner take a look at those parts of the patch first,
please?

[Ian Vollick, 2013-11-13 14:48:18]

On 2013/11/13 00:38:36, jamesr wrote:
> Can you have a /core/ owner take a look at those parts of the patch first,
> please?

+enne,jchaffraix

Could you please take a look at core/ ? The change is pretty trivial.
::compositedLayerMapping now returns a thin wrapper around a pointer that can't
be coerced to a bool, and in places where we had been coercing a
CompositedLayerMapping* to a bool, I've switched us to use
::hasCompositedLayerMapping instead. Using this function broadly is unfortunate
-- ultimately we want to be using compositingState() and
::hasCompositedLayerMapping could be misused in the same way that a coerced
CompositedLayerMapping pointer had been -- but this is a first step towards that
goal, separating cases where we really want to use a CompositedLayerMapping from
places where we're asking questions about the compositing situation.

[enne (OOO), 2013-11-13 20:55:54]

I think it would be clearer to just go directly to compositingState and don't
really see the need for this intermediate state.  It also seems like having
hasCompositedLayerMapping around is not any safer, since folks can just call
that now.

lgtm rubber stamp if you want it.  It sounds like there's been enough discussion
on this already and I don't think this is bad, just unnecessary.

[Ian Vollick, 2013-11-13 23:21:39]

On 2013/11/13 20:55:54, enne wrote:
> I think it would be clearer to just go directly to compositingState and don't
> really see the need for this intermediate state.  It also seems like having
> hasCompositedLayerMapping around is not any safer, since folks can just call
> that now.
> 
> lgtm rubber stamp if you want it.  It sounds like there's been enough
discussion
> on this already and I don't think this is bad, just unnecessary.

Yep, I agree that this isn't safer, but I think the goal isn't so much safety
but to make it easier to distinguish between the types of compositedLayerMapping
call site -- accessing it to make use of the mapping vs. accessing it to check
the state of compositing. When we move to compositingState, we should just be
able to grep for hasCompositedLayerMapping and the number of fixups will be a
lot smaller. The only danger I see is that we could be tempted to use
hasCompositedLayerMapping in new code, but we'll keep an eye out for that. I've
updated the comment for hasCompositedLayerMapping to discourage incorrect use of
this function.

James, can you please take a look at web/ ?

[jamesr, 2013-11-14 00:47:37]

/web/ lgtm

[commit-bot: I haz the power, 2013-11-14 02:34:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shawnsingh@chromium.org/58543002/590001

[commit-bot: I haz the power, 2013-11-14 03:33:58]

Change committed as 161973