Set m_graphicsLayer as fixed position container

Set m_graphicsLayer as fixed position container

This changes the fixed position container set by CompositedLayerMapping
from childForSuperlayers() to m_graphicsLayer. When childForSuperlayers()
is different from m_graphicsLayer, setting the fixed position container
to childForSuperlayers() means that the contained fixed position layers
incorrectly ignore changes to m_graphicsLayer's transform that happen
on the compositor thread (since they're fixed to m_graphicsLayer's parent
rather than to m_graphicsLayer itself).

BUG=686748
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2683433002
Cr-Commit-Position: refs/heads/master@{#449448}
Committed: https://chromium.googlesource.com/chromium/src/+/d290cbee660c449245346abe4a9f2c7948df4c9e

[ajuma, 2017-02-06 21:09:09]

Description was changed from

==========
Set m_graphicsLayer as fixed position container

BUG=686748
==========

to

==========
Set m_graphicsLayer as fixed position container

BUG=686748
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[ajuma, 2017-02-06 21:09:26]

The CQ bit was checked by ajuma@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-06 21:09:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-07 00:59:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-07 00:59:27]

Dry run: This issue passed the CQ dry run.

[ajuma, 2017-02-07 16:45:15]

Description was changed from

==========
Set m_graphicsLayer as fixed position container

BUG=686748
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Set m_graphicsLayer as fixed position container

This changes the fixed position container set by CompositedLayerMapping
from childForSuperlayers() to m_graphicsLayer. When childForSuperlayers()
is different from m_graphicsLayer, setting the fixed position container
to childForSuperlayers() means that the contained fixed position layers
incorrectly ignore changes to m_graphicsLayer's transform that happen
on the compositor thread (since they're fixed to m_graphicsLayer's parent
rather than to m_graphicsLayer itself).

BUG=686748
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[ajuma, 2017-02-07 16:50:47]

Description was changed from

==========
Set m_graphicsLayer as fixed position container

This changes the fixed position container set by CompositedLayerMapping
from childForSuperlayers() to m_graphicsLayer. When childForSuperlayers()
is different from m_graphicsLayer, setting the fixed position container
to childForSuperlayers() means that the contained fixed position layers
incorrectly ignore changes to m_graphicsLayer's transform that happen
on the compositor thread (since they're fixed to m_graphicsLayer's parent
rather than to m_graphicsLayer itself).

BUG=686748
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Set m_graphicsLayer as fixed position container

This changes the fixed position container set by CompositedLayerMapping
from childForSuperlayers() to m_graphicsLayer. When childForSuperlayers()
is different from m_graphicsLayer, setting the fixed position container
to childForSuperlayers() means that the contained fixed position layers
incorrectly ignore changes to m_graphicsLayer's transform that happen
on the compositor thread (since they're fixed to m_graphicsLayer's parent
rather than to m_graphicsLayer itself).

BUG=686748
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[ajuma, 2017-02-07 16:50:48]

ajuma@chromium.org changed reviewers:
+ chrishtr@chromium.org

[ajuma, 2017-02-07 16:59:40]

I'm not sure how to write a good test for this, since the position gets set
correctly by blink at each commit, it's only during compositor transform
animations that the layer gets positioned incorrectly.

I'm also not sure why we were using childForSuperlayers here in the first place.
Looking through the history, the most recent change to this logic is
https://codereview.chromium.org/475553002, but that was for squashing fixed
position layers (and we don't do that anymore). I didn't find anything
interesting looking further back a bit more.

The old CalcDrawProps implementation of fixed position involved undoing scroll
deltas in between the fixed position layer and its container. Since neither
m_graphicsLayer nor childForSuperlayers is ever scrollable, setting
childForSuperlayers to be the fixed position container wouldn't have been any
different than setting m_graphicsLayer itself as the container.

[chrishtr, 2017-02-08 02:02:27]

On 2017/02/07 at 16:59:40, ajuma wrote:
> I'm not sure how to write a good test for this, since the position gets set
correctly by blink at each commit, it's only during compositor transform
animations that the layer gets positioned incorrectly.

Is it possible to trigger the inline-transform fast-path in Blink, which doesn't
do as much work? Maybe that would allow wedging things into a broken state.

> 
> I'm also not sure why we were using childForSuperlayers here in the first
place. Looking through the history, the most recent change to this logic is
https://codereview.chromium.org/475553002, but that was for squashing fixed
position layers (and we don't do that anymore). I didn't find anything
interesting looking further back a bit more.
> 
> The old CalcDrawProps implementation of fixed position involved undoing scroll
deltas in between the fixed position layer and its container. Since neither
m_graphicsLayer nor childForSuperlayers is ever scrollable, setting
childForSuperlayers to be the fixed position container wouldn't have been any
different than setting m_graphicsLayer itself as the container.

[ajuma, 2017-02-09 16:56:18]

The CQ bit was checked by ajuma@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-09 16:56:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-09 18:51:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-09 18:51:35]

Dry run: This issue passed the CQ dry run.

[ajuma, 2017-02-09 18:52:06]

Added a test. What made this tricky is that cc was inferring that layers with
non-translation transforms (and, confusingly, their children too) were
fixed-position containers even when Blink didn't set these layers as fixed
position containers. (And translation transforms got baked into the
source_to_parent_offset computed during property tree building, so still didn't
cause incorrect rendering.)

So I've removed cc's inference of fixed position containers from non-translation
transforms -- it seemed odd to single out non-translation transforms as
containers and cc should really just be respecting whatever fixed-position
containers are set by the embedder (which is just blink, since ui doesn't use
fixed-position).

With that removed, the new test fails without the change to
CompositedLayerMapping and passes with the change.

[chrishtr, 2017-02-09 22:47:29]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2017-02-09 22:47:29]

lgtm

[commit-bot: I haz the power, 2017-02-09 22:48:07]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-09 22:54:13]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1486680449385890,
"parent_rev": "1334c0aa8505dc48616233442f89e4b1fd1fb778", "commit_rev":
"d290cbee660c449245346abe4a9f2c7948df4c9e"}

[commit-bot: I haz the power, 2017-02-09 22:54:49]

Description was changed from

==========
Set m_graphicsLayer as fixed position container

This changes the fixed position container set by CompositedLayerMapping
from childForSuperlayers() to m_graphicsLayer. When childForSuperlayers()
is different from m_graphicsLayer, setting the fixed position container
to childForSuperlayers() means that the contained fixed position layers
incorrectly ignore changes to m_graphicsLayer's transform that happen
on the compositor thread (since they're fixed to m_graphicsLayer's parent
rather than to m_graphicsLayer itself).

BUG=686748
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Set m_graphicsLayer as fixed position container

This changes the fixed position container set by CompositedLayerMapping
from childForSuperlayers() to m_graphicsLayer. When childForSuperlayers()
is different from m_graphicsLayer, setting the fixed position container
to childForSuperlayers() means that the contained fixed position layers
incorrectly ignore changes to m_graphicsLayer's transform that happen
on the compositor thread (since they're fixed to m_graphicsLayer's parent
rather than to m_graphicsLayer itself).

BUG=686748
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2683433002
Cr-Commit-Position: refs/heads/master@{#449448}
Committed:
https://chromium.googlesource.com/chromium/src/+/d290cbee660c449245346abe4a9f...
==========

[commit-bot: I haz the power, 2017-02-09 22:54:50]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/d290cbee660c449245346abe4a9f...