Don't coerce pointers to compositor layer mappings to booleans.

Don't coerce pointers to compositor layer mappings to booleans.

Using compositedLayerMapping() in a boolean expression is an error -- there are
in fact three compositing states, and there may be more in the future. This
subtlety cannot be captured with a boolean. This CL updates the current code so
that our intentions are explicit, and it prevents future abuses through a thin
wrapper around a CompositedLayerMapping* that cannot be coerced to a boolean
value.

This patch includes much of shawn's change to use a hasCompositedLayerMapping() accessor: https://codereview.chromium.org/58543002/

R=shawnsingh@chromium.org
BUG=

[shawnsingh, 2013-11-05 10:20:26]

So after getting halfway, I began to formulate three thoughts:

(1) Did you remember that hasCompositedLayerMapping() is true when compositing
state is PaintsIntoOwnBacking OR when compositing state is
HasOwnBackingButPaintsIntoAncestor?   It seems like a lot of places where you
replaced compositing state, we can't easily replace it with only
PaintsIntoOwnBacking.

(2) Maybe we should pare down this patch into separate smaller steps.  For
example, there are two FIXMEs that I didn't fix in the first CompositingState
patch because they are likely actual bugs and it would be a good idea to add
layout tests before moving on with those.  Another benefit of splitting this
into smaller pieces - it will make it easier to retain our progress and bisect a
problem, even if we have to revert one of the small pieces.  I know it slows
things down a little bit to split this, but I honestly don't think it would be
too bad - and I would predict that for this particular task, we will save more
time doing things carefully up front than debugging things afterwards.

Rest of my half-review is in comments below.

https://codereview.chromium.org/59063003/diff/40001/Source/core/inspector/Ins...
File Source/core/inspector/InspectorLayerTreeAgent.cpp (right):

https://codereview.chromium.org/59063003/diff/40001/Source/core/inspector/Ins...
Source/core/inspector/InspectorLayerTreeAgent.cpp:156: if
(renderLayer->compositingState() != PaintsIntoOwnBacking)
It makes sense if this would never be called when
HasOwnBackingButPaintsIntoAncestor ... but I wouldn't be surprised if this gets
called in that scenario, either.  Do you know for certain what happens here?

https://codereview.chromium.org/59063003/diff/40001/Source/core/inspector/Ins...
Source/core/inspector/InspectorLayerTreeAgent.cpp:181: if
(root->compositingState() == PaintsIntoOwnBacking) {
I don't think this one is correct.  The previous code could also have had "true"
for the condition where compositingState() == HasOwnBackingButPaintsIntoAncestor

https://codereview.chromium.org/59063003/diff/40001/Source/core/page/scrollin...
File Source/core/page/scrolling/ScrollingCoordinator.cpp (right):

https://codereview.chromium.org/59063003/diff/40001/Source/core/page/scrollin...
Source/core/page/scrolling/ScrollingCoordinator.cpp:188: } while (layer &&
layer->compositingState() == PaintsIntoOwnBacking);
This is not correct - see the comment immediately above.  If we don't want to
use a hasCompositedLayerMapping()  boolean, then this needs to check for both
PaintsIntoOwnBacking or HasOwnBackingButPaintsIntoAncestor.   The scenario here
is a fixed position layer that is positioned with respect to a composited
ancestor that paints into ancestor.  In that case, the fixed-pos element should
still be positioned with respect to that layer that doesn't paint anything -- it
still exists in the composited layer tree in such a way that affects positions
of its descendants.

https://codereview.chromium.org/59063003/diff/40001/Source/core/page/scrollin...
Source/core/page/scrolling/ScrollingCoordinator.cpp:526: if
((*it)->compositingState() == PaintsIntoOwnBacking) {
I think this also needs to include HasOwnBackingButPaintsIntoAncestor... but I'm
not entirely sure.  I have a feeling there may be some existing bugs with touch
rects on composited layers that paint into ancestor already even before this
patch.

https://codereview.chromium.org/59063003/diff/40001/Source/core/rendering/Com...
File Source/core/rendering/CompositedLayerMapping.cpp (right):

https://codereview.chromium.org/59063003/diff/40001/Source/core/rendering/Com...
Source/core/rendering/CompositedLayerMapping.cpp:1360: if
(curLayer->compositingState() == NotComposited
I think this should be written as:
  if (curLayer->compositingState() == NotComposited ||
curLayer->compositingState() == HasOwnBackingButPaintsIntoAncestor)

And same for the next two usages below.


Also, I think we should put a FIXME on this function to (1) rename curLayer to
childLayer and (2) change this code to use the Iterator.

https://codereview.chromium.org/59063003/diff/40001/Source/core/rendering/Com...
Source/core/rendering/CompositedLayerMapping.cpp:1395: // FIXME: By name the
implementation is correct. But the code that uses this function means something
By the way, the 3 lines you are changing in
hasVisibleNonCompositingDescendants() actually addresses this FIXME, so you can
probably remove this FIXME.

However, I'm pretty sure we can construct a layout test that actually reproduces
a bug due to the incorrect usage of compostedLayerMapping() before this patch. 
So we probably should do that (which is why I didn't change these in my first
compositingState patch).

https://codereview.chromium.org/59063003/diff/40001/Source/core/rendering/Com...
File Source/core/rendering/CompositedLayerMappingPtr.h (right):

https://codereview.chromium.org/59063003/diff/40001/Source/core/rendering/Com...
Source/core/rendering/CompositedLayerMappingPtr.h:19: CompositedLayerMapping&
operator*() const { return *m_mapping; }
should we ASSERT(m_mapping) here?  I don't feel strongly, but it seems like a
good idea?

https://codereview.chromium.org/59063003/diff/40001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/59063003/diff/40001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:648: if (compositingState() ==
PaintsIntoOwnBacking || !parent())
I'm really not sure about this one.  If you feel certain, can you please
elaborate why we shouldn't include HasOwnBackingButPaintsIntoAncestor here?

https://codereview.chromium.org/59063003/diff/40001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:1002: if (compositingState() ==
PaintsIntoOwnBacking) {
What about HasOwnBackingButPaintsIntoAncestor ?   composited layer positioning
doesn't care about whether contents paint into it or an ancestor, right?

https://codereview.chromium.org/59063003/diff/40001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:1200: if (compositingState() ==
PaintsIntoOwnBacking)
Should this also include HasOwnBackingButPaintsIntoAncestor ?  I can imagine a
composited layer that paints into ancestor but still has a clip.  That's one of
the common cases where a composited layer paints into its ancestor in the first
place.

https://codereview.chromium.org/59063003/diff/40001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:1242: if (compositingState() ==
PaintsIntoOwnBacking)
Again, I think we need to include HasOwnBackingbutPaintsIntoAncestor here...
it's about the composited layer hierarchy, not about who paints into where...

The scenario here is a composited layer with a non-composited ancestor that has
an opacity.  The composited layer has to walk all the way up to the next
composited layer to accumulate the correct opacity that it must inherit from all
non-composited layers.  The walk stops at the first layer that shows up in the
composited layer tree - regardless of whether it paints into its own backing or
not.

https://codereview.chromium.org/59063003/diff/40001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:1782: if (compositingState() ==
PaintsIntoOwnBacking)

So yeah, the theme of my review - did you remember to use
HasOwnBackingButPaintsIntoAncestor? =P  Or am I missing something that makes you
confident we don't need it in so many places?

https://codereview.chromium.org/59063003/diff/40001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:3617: if (flags &
IncludeCompositedDescendants || node->layer()->compositingState() !=
PaintsIntoOwnBacking) {
This was another place where I put a FIXME because it's very likely a bug that
we should repro with a layout test before fixing.  Could we add a layout test
that repros a bug before your patch?

https://codereview.chromium.org/59063003/diff/40001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:4003: else if (compositingState() ==
PaintsIntoOwnBacking)
I'm pretty sure we would need HasOwnBackingButPaintsIntoAncestor here, too?

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

https://codereview.chromium.org/59063003/diff/40001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayerCompositor.cpp:521: if
(layer->compositingState() == NotComposited) {
I strongly prefer these ones to use hasCompositedLayerMapping or the pointer
somehow.  This function is really hte internal guts of allocating or destroying
the compositedlayermapping and I feel it's meaningless and misleading to make
people think in terms of compositing state of the layer here.

https://codereview.chromium.org/59063003/diff/40001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayerCompositor.cpp:544: if
(layer->compositingState() != NotComposited) {
Ditto same as previous comment here.

https://codereview.chromium.org/59063003/diff/40001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayerCompositor.cpp:663: if
(child->compositingState() != PaintsIntoOwnBacking ||
parent->renderer()->documentBeingDestroyed())
or HasOwnBackingButPaintsIntoAncestor...

https://codereview.chromium.org/59063003/diff/40001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayerCompositor.cpp:985: if (!parentLayer ||
parentLayer->compositingState() != PaintsIntoOwnBacking)
and again....

https://codereview.chromium.org/59063003/diff/40001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayerModelObject.cpp (right):

https://codereview.chromium.org/59063003/diff/40001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayerModelObject.cpp:209: return m_layer ?
m_layer->compositedLayerMapping() : 0;
Does 0 actually work here, or should it be CompositedLayerMappingPtr(0) ?

[shawnsingh, 2013-11-05 10:22:48]

Oh - two thoughts not three.

[Ian Vollick, 2013-11-08 03:35:11]

Blergh, you're right. I misunderstood the point of the
composited-but-paints-into-ancestor state. I think there are some cases where we
only care about PaintsIntoOwnBacking, but most of my other changes have been
updated to include the paints-into-ancestor state as well.

PTAL.

https://codereview.chromium.org/59063003/diff/40001/Source/core/inspector/Ins...
File Source/core/inspector/InspectorLayerTreeAgent.cpp (right):

https://codereview.chromium.org/59063003/diff/40001/Source/core/inspector/Ins...
Source/core/inspector/InspectorLayerTreeAgent.cpp:156: if
(renderLayer->compositingState() != PaintsIntoOwnBacking)
On 2013/11/05 10:20:27, shawnsingh wrote:
> It makes sense if this would never be called when
> HasOwnBackingButPaintsIntoAncestor ... but I wouldn't be surprised if this
gets
> called in that scenario, either.  Do you know for certain what happens here?

Yep, I'm pretty certain. I've updated the code with a comment and added an
ASSERT.

https://codereview.chromium.org/59063003/diff/40001/Source/core/inspector/Ins...
Source/core/inspector/InspectorLayerTreeAgent.cpp:181: if
(root->compositingState() == PaintsIntoOwnBacking) {
On 2013/11/05 10:20:27, shawnsingh wrote:
> I don't think this one is correct.  The previous code could also have had
"true"
> for the condition where compositingState() ==
HasOwnBackingButPaintsIntoAncestor
I think there may have been a bug in the old code. I've added my rationale as a
comment in the code.

https://codereview.chromium.org/59063003/diff/40001/Source/core/page/scrollin...
File Source/core/page/scrolling/ScrollingCoordinator.cpp (right):

https://codereview.chromium.org/59063003/diff/40001/Source/core/page/scrollin...
Source/core/page/scrolling/ScrollingCoordinator.cpp:188: } while (layer &&
layer->compositingState() == PaintsIntoOwnBacking);
On 2013/11/05 10:20:27, shawnsingh wrote:
> This is not correct - see the comment immediately above.  If we don't want to
> use a hasCompositedLayerMapping()  boolean, then this needs to check for both
> PaintsIntoOwnBacking or HasOwnBackingButPaintsIntoAncestor.   The scenario
here
> is a fixed position layer that is positioned with respect to a composited
> ancestor that paints into ancestor.  In that case, the fixed-pos element
should
> still be positioned with respect to that layer that doesn't paint anything --
it
> still exists in the composited layer tree in such a way that affects positions
> of its descendants.

I think the comment (and possibly the code) are wrong here. I've added comments
in the code and reverted the logic to match the current behavior until we decide
what this function should really be doing.

https://codereview.chromium.org/59063003/diff/40001/Source/core/page/scrollin...
Source/core/page/scrolling/ScrollingCoordinator.cpp:526: if
((*it)->compositingState() == PaintsIntoOwnBacking) {
On 2013/11/05 10:20:27, shawnsingh wrote:
> I think this also needs to include HasOwnBackingButPaintsIntoAncestor... but
I'm
> not entirely sure.  I have a feeling there may be some existing bugs with
touch
> rects on composited layers that paint into ancestor already even before this
> patch.

I think this is a bug, but I'm not going to fix it in this CL. I've described
the problem in a comment and changed the code to match the previous behavior.

https://codereview.chromium.org/59063003/diff/40001/Source/core/rendering/Com...
File Source/core/rendering/CompositedLayerMapping.cpp (right):

https://codereview.chromium.org/59063003/diff/40001/Source/core/rendering/Com...
Source/core/rendering/CompositedLayerMapping.cpp:1360: if
(curLayer->compositingState() == NotComposited
On 2013/11/05 10:20:27, shawnsingh wrote:
> I think this should be written as:
>   if (curLayer->compositingState() == NotComposited ||
> curLayer->compositingState() == HasOwnBackingButPaintsIntoAncestor)
> 
> And same for the next two usages below.
> 
> 
> Also, I think we should put a FIXME on this function to (1) rename curLayer to
> childLayer and (2) change this code to use the Iterator.

I agree with you on this, but it is a behavior change. I'd like to retain the
current behavior and do the bugfix in a subsequent patch. I've updated things to
use the iterator, though.

https://codereview.chromium.org/59063003/diff/40001/Source/core/rendering/Com...
Source/core/rendering/CompositedLayerMapping.cpp:1395: // FIXME: By name the
implementation is correct. But the code that uses this function means something
On 2013/11/05 10:20:27, shawnsingh wrote:
> By the way, the 3 lines you are changing in
> hasVisibleNonCompositingDescendants() actually addresses this FIXME, so you
can
> probably remove this FIXME.
> 
> However, I'm pretty sure we can construct a layout test that actually
reproduces
> a bug due to the incorrect usage of compostedLayerMapping() before this patch.

> So we probably should do that (which is why I didn't change these in my first
> compositingState patch).
I think I've spoken to this with my previous comment.

https://codereview.chromium.org/59063003/diff/40001/Source/core/rendering/Com...
File Source/core/rendering/CompositedLayerMappingPtr.h (right):

https://codereview.chromium.org/59063003/diff/40001/Source/core/rendering/Com...
Source/core/rendering/CompositedLayerMappingPtr.h:19: CompositedLayerMapping&
operator*() const { return *m_mapping; }
On 2013/11/05 10:20:27, shawnsingh wrote:
> should we ASSERT(m_mapping) here?  I don't feel strongly, but it seems like a
> good idea?

Sure, done. I've also added the assert to 'operator->'. We really shouldn't be
using either of these fns if m_mapping is NULL.

https://codereview.chromium.org/59063003/diff/40001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/59063003/diff/40001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:1002: if (compositingState() ==
PaintsIntoOwnBacking) {
On 2013/11/05 10:20:27, shawnsingh wrote:
> What about HasOwnBackingButPaintsIntoAncestor ?   composited layer positioning
> doesn't care about whether contents paint into it or an ancestor, right?

That was the point I was trying to address with this comment: if we paint into a
composited layer that is already in its column-correct location, executing this
code will be a double compensation. The code below is quite hairy and I may be
misunderstanding it; but that's my current impression.

https://codereview.chromium.org/59063003/diff/40001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:3617: if (flags &
IncludeCompositedDescendants || node->layer()->compositingState() !=
PaintsIntoOwnBacking) {
On 2013/11/05 10:20:27, shawnsingh wrote:
> This was another place where I put a FIXME because it's very likely a bug that
> we should repro with a layout test before fixing.  Could we add a layout test
> that repros a bug before your patch?

Sure. I've reinstated the FIXME and matched the existing, buggy behavior.

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

https://codereview.chromium.org/59063003/diff/40001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayerCompositor.cpp:521: if
(layer->compositingState() == NotComposited) {
On 2013/11/05 10:20:27, shawnsingh wrote:
> I strongly prefer these ones to use hasCompositedLayerMapping or the pointer
> somehow.  This function is really hte internal guts of allocating or
destroying
> the compositedlayermapping and I feel it's meaningless and misleading to make
> people think in terms of compositing state of the layer here.

Done.

https://codereview.chromium.org/59063003/diff/40001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayerCompositor.cpp:544: if
(layer->compositingState() != NotComposited) {
On 2013/11/05 10:20:27, shawnsingh wrote:
> Ditto same as previous comment here.

Done.

https://codereview.chromium.org/59063003/diff/40001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayerModelObject.cpp (right):

https://codereview.chromium.org/59063003/diff/40001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayerModelObject.cpp:209: return m_layer ?
m_layer->compositedLayerMapping() : 0;
On 2013/11/05 10:20:27, shawnsingh wrote:
> Does 0 actually work here, or should it be CompositedLayerMappingPtr(0) ?

I think that 0 will be coerced into a CompositedLayerMappingPtr, actually.
(We'll see what all the other build bots say, though :))

[Ian Vollick, 2013-11-08 16:12:34]

There was a bug in the timing of computing
require-own-backing-for-ancestor-reasons that showed up as a layout test
failure. I've rearranged the code to cope with this. See
CompositedLayerMapping::updateCompositedBounds, and
RenderLayer::calculateLayerBounds.

https://codereview.chromium.org/59063003/diff/230001/Source/core/rendering/Re...
File Source/core/rendering/RenderLayerRepainter.cpp (right):

https://codereview.chromium.org/59063003/diff/230001/Source/core/rendering/Re...
Source/core/rendering/RenderLayerRepainter.cpp:140: if (curr->compositingState()
== NotComposited)
I've kept the compositingState() == Blah model here as a reminder that this
needs to be changed. (When I've used hasCompositedLayerMapping, I intend for
that code to continue working when we expand the number of compositing states).

[hartmanng, 2013-11-08 22:51:04]

Looks good for the most part, I plan to take another look later (tomorrow?)
though to convince myself of a few of the compositingState() checks.

https://codereview.chromium.org/59063003/diff/380001/Source/core/core.gypi
File Source/core/core.gypi (right):

https://codereview.chromium.org/59063003/diff/380001/Source/core/core.gypi#ne...
Source/core/core.gypi:1353: 'rendering/CompositedLayerMappingPtr.h',
not sure if this is necessary?

https://codereview.chromium.org/59063003/diff/380001/Source/core/rendering/Co...
File Source/core/rendering/CompositedLayerMapping.cpp (right):

https://codereview.chromium.org/59063003/diff/380001/Source/core/rendering/Co...
Source/core/rendering/CompositedLayerMapping.cpp:329:
updateDrawsContent(isSimpleContainerCompositingLayer());
this seems out of place in this patch

[Ian Vollick, 2013-11-11 15:29:48]

On 2013/11/08 22:51:04, hartmanng wrote:
> Looks good for the most part, I plan to take another look later (tomorrow?)
> though to convince myself of a few of the compositingState() checks.
> 
> https://codereview.chromium.org/59063003/diff/380001/Source/core/core.gypi
> File Source/core/core.gypi (right):
> 
>
https://codereview.chromium.org/59063003/diff/380001/Source/core/core.gypi#ne...
> Source/core/core.gypi:1353: 'rendering/CompositedLayerMappingPtr.h',
> not sure if this is necessary?
I don't know what the point of a header in a gypi file is, but it's pretty
common, so I think this is the thing to do.
> 
>
https://codereview.chromium.org/59063003/diff/380001/Source/core/rendering/Co...
> File Source/core/rendering/CompositedLayerMapping.cpp (right):
> 
>
https://codereview.chromium.org/59063003/diff/380001/Source/core/rendering/Co...
> Source/core/rendering/CompositedLayerMapping.cpp:329:
> updateDrawsContent(isSimpleContainerCompositingLayer());
> this seems out of place in this patch
K, I've created a bug for this, added a TestExpectations entry for the test that
will fail when I remove this code and will submit the fix as a follow on.

[shawnsingh, 2013-11-12 09:55:49]

So far so good - but still a lot of comments.   This patch is definitely a
painfully tricky one (as was the previous compositingState patch)!

Some of my comments include "We need to make a layout test for
HasOwnBackingButPaintsIntoAncestor".  I wonder if it may be worth you trying
your "get rid of paintsIntoAncestor" patch which might supersede making those
layout tests anyway?

https://codereview.chromium.org/59063003/diff/450001/Source/core/inspector/In...
File Source/core/inspector/InspectorLayerTreeAgent.cpp (right):

https://codereview.chromium.org/59063003/diff/450001/Source/core/inspector/In...
Source/core/inspector/InspectorLayerTreeAgent.cpp:157: // paint into its own
backing.

s/paint/paints/

https://codereview.chromium.org/59063003/diff/450001/Source/core/inspector/In...
Source/core/inspector/InspectorLayerTreeAgent.cpp:188: if
(root->compositingState() == PaintsIntoOwnBacking) {

Isn't this a scenario where we really only care whether the
compositedLayerMapping exists or not, so that the layer-to-node map represents
the same information as RenderLayer-owns-CLM relationships? If you disagree, can
you please explain more clearly why we cannot just use
hasCompositedLayerMapping()?

https://codereview.chromium.org/59063003/diff/450001/Source/core/page/scrolli...
File Source/core/page/scrolling/ScrollingCoordinator.cpp (right):

https://codereview.chromium.org/59063003/diff/450001/Source/core/page/scrolli...
Source/core/page/scrolling/ScrollingCoordinator.cpp:216: } while (layer &&
layer->hasCompositedLayerMapping());
Actually the code existing before your patch is wrong because I made a mistake
in my compositingState patch. 
https://codereview.chromium.org/26156006/diff/8001/Source/core/page/scrolling...

I accidentally lost the "!" negation operator in the refactoring process.  I
think we need to fix that in a separate patch because there is almost certainly
something we will need to merge back to the m32 branch for this.  It turns out
that there is no difference in correctness (possibly wrong for pinch-zoom on the
impl thread, but not for basic positioning), because either way the layer will
get fixed to something else that is fixed.

So I believe your FIXME is unnecessary - please feel free to discuss if you
think otherwise =)

https://codereview.chromium.org/59063003/diff/450001/Source/core/rendering/Re...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/59063003/diff/450001/Source/core/rendering/Re...
Source/core/rendering/RenderLayer.cpp:539: if (compositingState() ==
PaintsIntoOwnBacking)

Wouldn't blend-mode still work for a layer that paints into an ancestor, because
it would then use the software implementation of blend-modes within the other
layer?  So I think it is valid to accept both compositing states
PaintsIntoOwnBacking and HasOwnBackingButPaintsIntoAncestor.  Code elsewhere
with paintsWithBlendMode() will recognize in that scenario that it needs to use
the software blend mode implementation.

If you agree, I think this is a scenario where we could go ahead and use
compositingState() to define the condition rather than
hasCompositedLayerMapping().

https://codereview.chromium.org/59063003/diff/450001/Source/core/rendering/Re...
Source/core/rendering/RenderLayer.cpp:1004: if (compositingState() ==
PaintsIntoOwnBacking) {
I think this will break for a scenario where a layer has compositingState
HasOwnBackingButPaintsIntoAncestor and is located on the second column.  Either
it will get positioned incorrectly because the CompositedLayerMapping assumes
that the "composited" layer is positioned correctly including it's multi-column
offset, or it may get hit tested incorrectly for a similar reason.

https://codereview.chromium.org/59063003/diff/450001/Source/core/rendering/Re...
Source/core/rendering/RenderLayer.cpp:1207: if (compositingState() ==
PaintsIntoOwnBacking)
red flag in comment - "only for layers that own a backing" --> includes layers
that HasOwnBackingButPaintsIntoAncestor.  So either the comment is wrong or the
condition is wrong?

https://codereview.chromium.org/59063003/diff/450001/Source/core/rendering/Re...
Source/core/rendering/RenderLayer.cpp:1251: if (compositingState() ==
PaintsIntoOwnBacking)
This seems like something we should make a layout test for the
paintsIntoAncestor case...

https://codereview.chromium.org/59063003/diff/450001/Source/core/rendering/Re...
Source/core/rendering/RenderLayer.cpp:3621: if (flags &
IncludeCompositedDescendants || node->layer()->compositingState() !=
PaintsIntoOwnBacking) {

Can we get a layout test for this?  I had made this a FIXME (which you can
remove 2 lines above this) because we ought to regard this a bug and get a
layout test that reproduces the incorrect composited layer bounds.

https://codereview.chromium.org/59063003/diff/450001/Source/core/rendering/Re...
File Source/core/rendering/RenderLayer.h (right):

https://codereview.chromium.org/59063003/diff/450001/Source/core/rendering/Re...
Source/core/rendering/RenderLayer.h:353: bool hasCompositedLayerMapping() const
{ return m_compositedLayerMapping.get(); }
I think the NOTE 3 lines above should apply to this new bool function, now.

https://codereview.chromium.org/59063003/diff/450001/Source/core/rendering/Re...
File Source/core/rendering/RenderLayerCompositor.cpp (right):

https://codereview.chromium.org/59063003/diff/450001/Source/core/rendering/Re...
Source/core/rendering/RenderLayerCompositor.cpp:1535: if
(layer->compositingState() != PaintsIntoOwnBacking || !layer->parent())
Seems correct, but I have to admit I'm too tired right now to reason through
this one.

https://codereview.chromium.org/59063003/diff/450001/Source/core/rendering/Re...
File Source/core/rendering/RenderLayerRepainter.cpp (right):

https://codereview.chromium.org/59063003/diff/450001/Source/core/rendering/Re...
Source/core/rendering/RenderLayerRepainter.cpp:140: if (curr->compositingState()
== NotComposited)

I don't understand this FIXME... why cant we just write:
  if (curr->compositingState() == NotComposited || curr->compositingState() ==
HasOwnBackingButPaintsIntoAncestor)

which would be equivalent to the previous code, and address your FIXME all at
the same time?

https://codereview.chromium.org/59063003/diff/450001/Source/core/rendering/Re...
Source/core/rendering/RenderLayerRepainter.cpp:152: if
(child->compositingState() != NotComposited)
This code doesn't seem to require a FIXME, either?

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

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

I think this is incorrect - the searchRoot would still own its CLM even if it
painted into ancestor.

https://codereview.chromium.org/59063003/diff/450001/Source/web/LinkHighlight...
File Source/web/LinkHighlight.cpp (right):

https://codereview.chromium.org/59063003/diff/450001/Source/web/LinkHighlight...
Source/web/LinkHighlight.cpp:134: ASSERT(renderLayer->compositingState() ==
PaintsIntoOwnBacking &&
renderLayer->compositedLayerMapping()->scrollingContentsLayer());

Can you please explain? I actually am not familiar with the role/function of
link highlights.

[shawnsingh, 2013-11-12 10:05:25]

I'm beginning to feel that we should definitely break this patch into baby
steps:

(1) create CompositedLayerMappingPtr and hasCompositedLayerMapping, but don't
convert any of the cases into compositingState() usage yet.

(2) one by one, address usages of hasCompositedLayerMapping() with the goal of
eventually removing most of them.  I doubt we'll want to ever remove all of them
since it really is legit to use at times... that's beyond foresight though.


My primary thought is that t will be easier to revert (and more importantly -
re-land) a small piece that fails than the whole huge patch.  And we definitely
won't need to revert step (1), yet it would still be nice to have the distinct
grep-ability for the boolean usages versus the real usages.

[caseq, 2013-11-13 15:45:19]

https://codereview.chromium.org/59063003/diff/450001/Source/core/inspector/In...
File Source/core/inspector/InspectorLayerTreeAgent.cpp (right):

https://codereview.chromium.org/59063003/diff/450001/Source/core/inspector/In...
Source/core/inspector/InspectorLayerTreeAgent.cpp:156: // Note, that this only
gets called with the FrameView's RenderView which, if it is composited, only
This statement is not correct, there are two call sites for
InspectorInstrumentation::didPaint() -- one in RenderView, another in
CompositedLayerMapping. The former is on the non-composited code path only (so
all we have to do here is bail out) and the latter is naturally on composited
path. The ASSERT() below looks reasonable, but I'd move it after if
(renderLayer->compositingState() != PaintsIntoOwnBacking) and remove the first
clause.

https://codereview.chromium.org/59063003/diff/450001/Source/core/inspector/In...
Source/core/inspector/InspectorLayerTreeAgent.cpp:188: if
(root->compositingState() == PaintsIntoOwnBacking) {
On 2013/11/12 09:55:50, shawnsingh wrote:
> 
> Isn't this a scenario where we really only care whether the
> compositedLayerMapping exists or not, so that the layer-to-node map represents
> the same information as RenderLayer-owns-CLM relationships? If you disagree,
can
> you please explain more clearly why we cannot just use
> hasCompositedLayerMapping()?

+1 to what Shawn says.
Since the mere presence of CLM means we do have at least a main graphics layer,
event if this render layer is painted elsewhere, we want that layer to be
associated with the actual owner node that caused it to appear.

[Ian Vollick, 2013-11-13 16:47:43]

On 2013/11/13 15:45:19, caseq wrote:
>
https://codereview.chromium.org/59063003/diff/450001/Source/core/inspector/In...
> File Source/core/inspector/InspectorLayerTreeAgent.cpp (right):
> 
>
https://codereview.chromium.org/59063003/diff/450001/Source/core/inspector/In...
> Source/core/inspector/InspectorLayerTreeAgent.cpp:156: // Note, that this only
> gets called with the FrameView's RenderView which, if it is composited, only
> This statement is not correct, there are two call sites for
> InspectorInstrumentation::didPaint() -- one in RenderView, another in
> CompositedLayerMapping. The former is on the non-composited code path only (so
> all we have to do here is bail out) and the latter is naturally on composited
> path. The ASSERT() below looks reasonable, but I'd move it after if
> (renderLayer->compositingState() != PaintsIntoOwnBacking) and remove the first
> clause.
> 
>
https://codereview.chromium.org/59063003/diff/450001/Source/core/inspector/In...
> Source/core/inspector/InspectorLayerTreeAgent.cpp:188: if
> (root->compositingState() == PaintsIntoOwnBacking) {
> On 2013/11/12 09:55:50, shawnsingh wrote:
> > 
> > Isn't this a scenario where we really only care whether the
> > compositedLayerMapping exists or not, so that the layer-to-node map
represents
> > the same information as RenderLayer-owns-CLM relationships? If you disagree,
> can
> > you please explain more clearly why we cannot just use
> > hasCompositedLayerMapping()?
> 
> +1 to what Shawn says.
> Since the mere presence of CLM means we do have at least a main graphics
layer,
> event if this render layer is painted elsewhere, we want that layer to be
> associated with the actual owner node that caused it to appear.

Yep, this is true, and it's good you've noted it here for posterity, but this
patch is no longer needed so I will not be updating it. I'm currently looking at
removing the composited-but-paints-into-ancestor mode (in another CL), and that
work will make obsolete much of what's been done in this patch.

[Ian Vollick, 2013-11-14 04:12:46]

Even though this patch shouldn't be necessary, I thought a response to your
comments could be helpful when you go to migrate the hasCompositedLayerMapping()
call sites to compositingState() for squashing.

https://codereview.chromium.org/59063003/diff/450001/Source/core/inspector/In...
File Source/core/inspector/InspectorLayerTreeAgent.cpp (right):

https://codereview.chromium.org/59063003/diff/450001/Source/core/inspector/In...
Source/core/inspector/InspectorLayerTreeAgent.cpp:188: if
(root->compositingState() == PaintsIntoOwnBacking) {
On 2013/11/13 15:45:20, caseq wrote:
> On 2013/11/12 09:55:50, shawnsingh wrote:
> > 
> > Isn't this a scenario where we really only care whether the
> > compositedLayerMapping exists or not, so that the layer-to-node map
represents
> > the same information as RenderLayer-owns-CLM relationships? If you disagree,
> can
> > you please explain more clearly why we cannot just use
> > hasCompositedLayerMapping()?
> 
> +1 to what Shawn says.
> Since the mere presence of CLM means we do have at least a main graphics
layer,
> event if this render layer is painted elsewhere, we want that layer to be
> associated with the actual owner node that caused it to appear.
> 

Agreed.

https://codereview.chromium.org/59063003/diff/450001/Source/core/page/scrolli...
File Source/core/page/scrolling/ScrollingCoordinator.cpp (right):

https://codereview.chromium.org/59063003/diff/450001/Source/core/page/scrolli...
Source/core/page/scrolling/ScrollingCoordinator.cpp:216: } while (layer &&
layer->hasCompositedLayerMapping());
On 2013/11/12 09:55:50, shawnsingh wrote:
> Actually the code existing before your patch is wrong because I made a mistake
> in my compositingState patch. 
>
https://codereview.chromium.org/26156006/diff/8001/Source/core/page/scrolling...
> 
> I accidentally lost the "!" negation operator in the refactoring process.  I
> think we need to fix that in a separate patch because there is almost
certainly
> something we will need to merge back to the m32 branch for this.  It turns out
> that there is no difference in correctness (possibly wrong for pinch-zoom on
the
> impl thread, but not for basic positioning), because either way the layer will
> get fixed to something else that is fixed.
> 
> So I believe your FIXME is unnecessary - please feel free to discuss if you
> think otherwise =)
K, cool :)

https://codereview.chromium.org/59063003/diff/450001/Source/core/rendering/Re...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/59063003/diff/450001/Source/core/rendering/Re...
Source/core/rendering/RenderLayer.cpp:539: if (compositingState() ==
PaintsIntoOwnBacking)
On 2013/11/12 09:55:50, shawnsingh wrote:
> 
> Wouldn't blend-mode still work for a layer that paints into an ancestor,
because
> it would then use the software implementation of blend-modes within the other
> layer?  So I think it is valid to accept both compositing states
> PaintsIntoOwnBacking and HasOwnBackingButPaintsIntoAncestor.  Code elsewhere
> with paintsWithBlendMode() will recognize in that scenario that it needs to
use
> the software blend mode implementation.
> 
> If you agree, I think this is a scenario where we could go ahead and use
> compositingState() to define the condition rather than
> hasCompositedLayerMapping().

It's my impression that the paints-into-ancestor mode was for creating GL's for
clipping, preserves3d'ing and perspective'ifying composited descendants -- not
for painting. The blend mode is still available via the style. Isn't that what
will be consulted when we paint this layer into its composited ancestor?

https://codereview.chromium.org/59063003/diff/450001/Source/core/rendering/Re...
Source/core/rendering/RenderLayer.cpp:1004: if (compositingState() ==
PaintsIntoOwnBacking) {
On 2013/11/12 09:55:50, shawnsingh wrote:
> I think this will break for a scenario where a layer has compositingState
> HasOwnBackingButPaintsIntoAncestor and is located on the second column. 
Either
> it will get positioned incorrectly because the CompositedLayerMapping assumes
> that the "composited" layer is positioned correctly including it's
multi-column
> offset, or it may get hit tested incorrectly for a similar reason.

I think a layer that paints-into-ancestor would itself draw fine (I think the
painting code would get it positioned correctly), but I'm now nervous about the
composited descendants -- I bet they won't be positioned correctly if we skip
this. I think we need a layout test here.

https://codereview.chromium.org/59063003/diff/450001/Source/core/rendering/Re...
Source/core/rendering/RenderLayer.cpp:1207: if (compositingState() ==
PaintsIntoOwnBacking)
On 2013/11/12 09:55:50, shawnsingh wrote:
> red flag in comment - "only for layers that own a backing" --> includes layers
> that HasOwnBackingButPaintsIntoAncestor.  So either the comment is wrong or
the
> condition is wrong?

VWhen I say "own a backing", I mean owns a mapping that is painted into. Could
we distinguish between mapping and backing? Is there better verbage we could be
using?

https://codereview.chromium.org/59063003/diff/450001/Source/core/rendering/Re...
Source/core/rendering/RenderLayer.cpp:3604: if
(!reflectionLayer->hasCompositedLayerMapping()) {
I should mention that my assumption (which I should have enforced with an ASSERT
here) was the reflection would either paint into the same backing as |this| or
it would paint into its own backing.

https://codereview.chromium.org/59063003/diff/450001/Source/core/rendering/Re...
Source/core/rendering/RenderLayer.cpp:3621: if (flags &
IncludeCompositedDescendants || node->layer()->compositingState() !=
PaintsIntoOwnBacking) {
On 2013/11/12 09:55:50, shawnsingh wrote:
> 
> Can we get a layout test for this?  I had made this a FIXME (which you can
> remove 2 lines above this) because we ought to regard this a bug and get a
> layout test that reproduces the incorrect composited layer bounds. 

Actually, this is hit by a layout test:
transforms/3d/point-mapping/3d-point-mapping-deep.html

https://codereview.chromium.org/59063003/diff/450001/Source/core/rendering/Re...
File Source/core/rendering/RenderLayer.h (right):

https://codereview.chromium.org/59063003/diff/450001/Source/core/rendering/Re...
Source/core/rendering/RenderLayer.h:353: bool hasCompositedLayerMapping() const
{ return m_compositedLayerMapping.get(); }
On 2013/11/12 09:55:50, shawnsingh wrote:
> I think the NOTE 3 lines above should apply to this new bool function, now.
They have been moved in the hasCLM patch.

https://codereview.chromium.org/59063003/diff/450001/Source/core/rendering/Re...
File Source/core/rendering/RenderLayerCompositor.cpp (right):

https://codereview.chromium.org/59063003/diff/450001/Source/core/rendering/Re...
Source/core/rendering/RenderLayerCompositor.cpp:1535: if
(layer->compositingState() != PaintsIntoOwnBacking || !layer->parent())
On 2013/11/12 09:55:50, shawnsingh wrote:
> Seems correct, but I have to admit I'm too tired right now to reason through
> this one.

It's another one that could use a layout test.

https://codereview.chromium.org/59063003/diff/450001/Source/core/rendering/Re...
File Source/core/rendering/RenderLayerRepainter.cpp (right):

https://codereview.chromium.org/59063003/diff/450001/Source/core/rendering/Re...
Source/core/rendering/RenderLayerRepainter.cpp:140: if (curr->compositingState()
== NotComposited)
On 2013/11/12 09:55:50, shawnsingh wrote:
> 
> I don't understand this FIXME... why cant we just write:
>   if (curr->compositingState() == NotComposited || curr->compositingState() ==
> HasOwnBackingButPaintsIntoAncestor)
> 
> which would be equivalent to the previous code, and address your FIXME all at
> the same time?

Ah right. We want to paint those layers that paint into our repaint container,
but I was worried that the paints-into-ancestor layers may paint into some other
container (is that impossible? seems quite possible in the world of squashing).
I put the FIXME here so I could add the container comparison code in another
patch.

https://codereview.chromium.org/59063003/diff/450001/Source/core/rendering/Re...
Source/core/rendering/RenderLayerRepainter.cpp:152: if
(child->compositingState() != NotComposited)
On 2013/11/12 09:55:50, shawnsingh wrote:
> This code doesn't seem to require a FIXME, either?
Same as above.

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

https://codereview.chromium.org/59063003/diff/450001/Source/core/testing/Inte...
Source/core/testing/Internals.cpp:1275: if (searchRoot->compositingState() ==
PaintsIntoOwnBacking && graphicsLayer ==
searchRoot->compositedLayerMapping()->mainGraphicsLayer())
On 2013/11/12 09:55:50, shawnsingh wrote:
> 
> I think this is incorrect - the searchRoot would still own its CLM even if it
> painted into ancestor.

You're totally right. This was a mistake.

https://codereview.chromium.org/59063003/diff/450001/Source/web/LinkHighlight...
File Source/web/LinkHighlight.cpp (right):

https://codereview.chromium.org/59063003/diff/450001/Source/web/LinkHighlight...
Source/web/LinkHighlight.cpp:134: ASSERT(renderLayer->compositingState() ==
PaintsIntoOwnBacking &&
renderLayer->compositedLayerMapping()->scrollingContentsLayer());
On 2013/11/12 09:55:50, shawnsingh wrote:
> 
> Can you please explain? I actually am not familiar with the role/function of
> link highlights.

It's just that if the renderLayer uses composited scrolling, it'll totally paint
into its own backing.