Make sure we set "should flatten transform" correctly.

Make sure we set "should flatten transform" correctly.

The bookkeeping for this property has gotten a little stale, and it's
prevented transforms from propagating in some cases. This CL ensures
that we use the same code everywhere for updating
should-flatten-transform on all applicable layers, and that we call it
at the right times.

BUG=None

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

[chrishtr, 2014-07-17 17:37:46]

Is there a bug this CL fixes?

[Ian Vollick, 2014-07-17 17:45:01]

On 2014/07/17 17:37:46, chrishtr wrote:
> Is there a bug this CL fixes?

Yup. I noticed that
accelerated-overflow-scroll-should-not-affect-perspective.html no longer
rendered correctly due to the incorrect should-flatten-transform values on a new
CLM layer.

That is to say, the expectation for that and several other layout tests were
incorrect for a while. This is corrected with this patch and these tests provide
our coverage.

I didn't bother logging a bug, but I certainly do that if you think it would be
helpful.

[chrishtr, 2014-07-17 17:47:01]

Filing a bug is not necessary, but could you include one of the changed platform
expectation files that were updated by this fix?

[hartmanng, 2014-07-17 17:52:22]

lgtm

[abarth-chromium, 2014-07-17 18:11:01]

https://codereview.chromium.org/399733002/diff/50001/LayoutTests/TestExpectat...
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/399733002/diff/50001/LayoutTests/TestExpectat...
LayoutTests/TestExpectations:170: #Bug(vollick)
virtual/softwarecompositing/update-paint-phases.html [ NeedsRebaseline ]
Did you meant to comment out these lines?

[Ian Vollick, 2014-07-17 18:17:00]

On 2014/07/17 18:11:01, abarth wrote:
>
https://codereview.chromium.org/399733002/diff/50001/LayoutTests/TestExpectat...
> File LayoutTests/TestExpectations (right):
> 
>
https://codereview.chromium.org/399733002/diff/50001/LayoutTests/TestExpectat...
> LayoutTests/TestExpectations:170: #Bug(vollick)
> virtual/softwarecompositing/update-paint-phases.html [ NeedsRebaseline ]
> Did you meant to comment out these lines?

Heh, yeah, I did that so I could run the trybots on all platforms to take a look
at the failures again. I was going to remove this patch set once I'd gathered my
info.

[Ian Vollick, 2014-07-17 19:10:13]

On 2014/07/17 17:47:01, chrishtr wrote:
> Filing a bug is not necessary, but could you include one of the changed
platform
> expectation files that were updated by this fix?

Done.

[abarth-chromium, 2014-07-17 20:05:18]

(Looks fine to me, but hartmanng is a better reviewer than me, so I'm deferring
to him.)

[chrishtr, 2014-07-17 20:07:33]

https://codereview.chromium.org/399733002/diff/70001/Source/core/rendering/co...
File Source/core/rendering/compositing/CompositedLayerMapping.cpp (right):

https://codereview.chromium.org/399733002/diff/70001/Source/core/rendering/co...
Source/core/rendering/compositing/CompositedLayerMapping.cpp:1154:
updateShouldFlattenTransform();
Is this call needed? Why isn't the other one sufficient?

https://codereview.chromium.org/399733002/diff/70001/Source/core/rendering/co...
Source/core/rendering/compositing/CompositedLayerMapping.cpp:1273:
f(mapping->scrollingBlockSelectionLayer());
Is this an unrelated-ish change?

[Ian Vollick, 2014-07-17 20:30:15]

https://codereview.chromium.org/399733002/diff/70001/Source/core/rendering/co...
File Source/core/rendering/compositing/CompositedLayerMapping.cpp (right):

https://codereview.chromium.org/399733002/diff/70001/Source/core/rendering/co...
Source/core/rendering/compositing/CompositedLayerMapping.cpp:1154:
updateShouldFlattenTransform();
On 2014/07/17 20:07:32, chrishtr wrote:
> Is this call needed? Why isn't the other one sufficient?

Good point! I think you're right that this is redundant. I've removed this and
the tests seem to pass (at least the ones that this patch affected).

https://codereview.chromium.org/399733002/diff/70001/Source/core/rendering/co...
Source/core/rendering/compositing/CompositedLayerMapping.cpp:1273:
f(mapping->scrollingBlockSelectionLayer());
On 2014/07/17 20:07:32, chrishtr wrote:
> Is this an unrelated-ish change?

It isn't, actually. In updateShouldFlattenTransform there are two phases. The
first updates should-flatten-transform as required by preserves-3d, and the
second for perspective. Neither of these phases was operating on the scrolling
block selection layer, but they needed to -- this is the bug this CL is trying
to address. The first phase applies the functor using ApplyToCoreLayers and the
block selection layer needed to be included in that pass. This is what
ApplyToCoreLayers is for, so it's not going to cause the block selection layer
to be included in other, unexpected operations.

[chrishtr, 2014-07-17 20:38:40]

https://codereview.chromium.org/399733002/diff/70001/Source/core/rendering/co...
File Source/core/rendering/compositing/CompositedLayerMapping.cpp (right):

https://codereview.chromium.org/399733002/diff/70001/Source/core/rendering/co...
Source/core/rendering/compositing/CompositedLayerMapping.cpp:1273:
f(mapping->scrollingBlockSelectionLayer());
On 2014/07/17 20:30:15, Ian Vollick wrote:
> On 2014/07/17 20:07:32, chrishtr wrote:
> > Is this an unrelated-ish change?
> 
> It isn't, actually. In updateShouldFlattenTransform there are two phases. The
> first updates should-flatten-transform as required by preserves-3d, and the
> second for perspective. Neither of these phases was operating on the scrolling
> block selection layer, but they needed to -- this is the bug this CL is trying
> to address. The first phase applies the functor using ApplyToCoreLayers and
the
> block selection layer needed to be included in that pass. This is what
> ApplyToCoreLayers is for, so it's not going to cause the block selection layer
> to be included in other, unexpected operations.

Great. Thanks for the explanation.

[Ian Vollick, 2014-07-17 20:39:45]

The CQ bit was checked by vollick@chromium.org

[Ian Vollick, 2014-07-17 20:39:46]

The CQ bit was unchecked by vollick@chromium.org

[Ian Vollick, 2014-07-17 20:52:13]

The CQ bit was checked by vollick@chromium.org

[commit-bot: I haz the power, 2014-07-17 20:52:23]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/vollick@chromium.org/399733002/90001

[chrishtr, 2014-07-17 20:53:28]

lgtm

[commit-bot: I haz the power, 2014-07-17 22:23:09]

Change committed as 178400