[cc] Change semantics of fixed-pos container layer

[cc] Change semantics of fixed-pos container layer

In the old semantics, when we set a layer to be fixed-pos container,
it means descendant fixed-pos layers don't move relative to the
current layer's parent.

This made it impossible (without extra dummy layers) to correctly
implement fixed-pos container that has overflow scroll, because in
this case fixed-pos descendants do scroll along (don't move relative
to the scrolling layer, instead of its parent).

This CL changes the semantics of the fixed-pos container flag, so
the fixed-pos container layer itself becomes the anchor of fixed-pos 
descendants, instead of its parent.

BUG=715338
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

[trchen, 2017-04-25 23:42:59]

Description was changed from

==========
[cc] Removes a hack that skips scrollable fixed-pos container

There was a hack in cc property tree builder that if a fixed-pos
container layer is scrollable, instead we treat its parent layer
as the fixed-pos container.

This was probably a workaround to old Blink bug that we incorrectly
set fixed-pos container layer for frames. However this made it
impossible (without extra dummy layers) to correctly implement
fixed-pos container with overflow scroll, because fixed-pos descendants
do scroll along in that case.

BUG=
==========

to

==========
[cc] Removes a hack that skips scrollable fixed-pos container

There was a hack in cc property tree builder that if a fixed-pos
container layer is scrollable, instead we treat its parent layer
as the fixed-pos container.

This was probably a workaround to old Blink bug that we incorrectly
set fixed-pos container layer for frames. However this made it
impossible (without extra dummy layers) to correctly implement
fixed-pos container with overflow scroll, because fixed-pos descendants
do scroll along in that case.

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

[trchen, 2017-04-25 23:43:00]

Description was changed from

==========
[cc] Removes a hack that skips scrollable fixed-pos container

There was a hack in cc property tree builder that if a fixed-pos
container layer is scrollable, instead we treat its parent layer
as the fixed-pos container.

This was probably a workaround to old Blink bug that we incorrectly
set fixed-pos container layer for frames. However this made it
impossible (without extra dummy layers) to correctly implement
fixed-pos container with overflow scroll, because fixed-pos descendants
do scroll along in that case.

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

to

==========
[cc] Removes a hack that skips scrollable fixed-pos container

There was a hack in cc property tree builder that if a fixed-pos
container layer is scrollable, instead we treat its parent layer
as the fixed-pos container.

This was probably a workaround to old Blink bug that we incorrectly
set fixed-pos container layer for frames. However this made it
impossible (without extra dummy layers) to correctly implement
fixed-pos container with overflow scroll, because fixed-pos descendants
do scroll along in that case.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[trchen, 2017-04-25 23:43:11]

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

[commit-bot: I haz the power, 2017-04-25 23:43:46]

Dry run: CQ is trying da patch.

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

[trchen, 2017-04-25 23:44:17]

Description was changed from

==========
[cc] Removes a hack that skips scrollable fixed-pos container

There was a hack in cc property tree builder that if a fixed-pos
container layer is scrollable, instead we treat its parent layer
as the fixed-pos container.

This was probably a workaround to old Blink bug that we incorrectly
set fixed-pos container layer for frames. However this made it
impossible (without extra dummy layers) to correctly implement
fixed-pos container with overflow scroll, because fixed-pos descendants
do scroll along in that case.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
[cc] Removes a hack that skips scrollable fixed-pos container

There was a hack in cc property tree builder that if a fixed-pos
container layer is scrollable, instead we treat its parent layer
as the fixed-pos container.

This was probably a workaround to old Blink bug that we incorrectly
set fixed-pos container layer for frames. However this made it
impossible (without extra dummy layers) to correctly implement
fixed-pos container with overflow scroll, because fixed-pos descendants
do scroll along in that case.

BUG=715338
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[trchen, 2017-04-25 23:44:17]

trchen@chromium.org changed reviewers:
+ aelias@chromium.org, ajuma@chromium.org, bokan@chromium.org

[trchen, 2017-04-25 23:46:16]

I've been away from this outer/inner viewport stuff for too long... Needs pairs
of extra eyes to make sure I didn't overlook anything here.

[commit-bot: I haz the power, 2017-04-26 00:40:21]

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

[commit-bot: I haz the power, 2017-04-26 00:40:22]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[bokan, 2017-04-26 21:17:56]

https://codereview.chromium.org/2840143002/diff/1/cc/trees/layer_tree_host.cc
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/2840143002/diff/1/cc/trees/layer_tree_host.cc...
cc/trees/layer_tree_host.cc:1159:
DCHECK(page_scale_layer_->IsContainerForFixedPositionLayers());
I think this DCHECK should be for the outer container layer.

https://codereview.chromium.org/2840143002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/VisualViewport.cpp (right):

https://codereview.chromium.org/2840143002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/VisualViewport.cpp:351: // by the
VisualViewport actually?
I believe this was a temporary state meant only to support running in either
modes that never got removed. IMO, we can probably get rid of this, I think only
the outer viewport container layer (the PLC's container layer) should be a
ContainerForFixedPositionLayers.

[trchen, 2017-05-01 21:20:58]

Description was changed from

==========
[cc] Removes a hack that skips scrollable fixed-pos container

There was a hack in cc property tree builder that if a fixed-pos
container layer is scrollable, instead we treat its parent layer
as the fixed-pos container.

This was probably a workaround to old Blink bug that we incorrectly
set fixed-pos container layer for frames. However this made it
impossible (without extra dummy layers) to correctly implement
fixed-pos container with overflow scroll, because fixed-pos descendants
do scroll along in that case.

BUG=715338
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
[cc] Change semantics of fixed-pos container layer

In the old semantics, when we set a layer to be fixed-pos container,
it means descendant fixed-pos layers don't move relative to the
current layer's parent.

This made it impossible (without extra dummy layers) to correctly
implement fixed-pos container that has overflow scroll, because in
this case fixed-pos descendants do scroll along (don't move relative
to the scrolling layer, instead of its parent).

This CL changes the semantics of the fixed-pos container flag, so
the fixed-pos container layer itself becomes the anchor of fixed-pos 
descendants, instead of its parent.

BUG=715338
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[trchen, 2017-05-01 21:24:13]

Updated unittests. Also changed CL description because the old behavior feels
more like a (badly) designed API than a hack.

https://codereview.chromium.org/2840143002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/VisualViewport.cpp (right):

https://codereview.chromium.org/2840143002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/VisualViewport.cpp:351: // by the
VisualViewport actually?
On 2017/04/26 21:17:56, bokan (OOO until May 9) wrote:
> I believe this was a temporary state meant only to support running in either
> modes that never got removed. IMO, we can probably get rid of this, I think
only
> the outer viewport container layer (the PLC's container layer) should be a
> ContainerForFixedPositionLayers.

I removed it in patch set 2, but on a second thought, I think we'd better change
on thing at a time so added it back.

[trchen, 2017-05-01 21:24:22]

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

[commit-bot: I haz the power, 2017-05-01 21:25:39]

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-05-02 00:01:45]

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

[commit-bot: I haz the power, 2017-05-02 00:01:46]

Dry run: This issue passed the CQ dry run.

[trchen, 2017-05-04 20:37:26]

Ping. What do you think?

I'm a little bit uncertain about this CL because it's refactoring only, but the
code getting refactored is likely to be removed in SPv2. i.e. Adding risks
despite limited long-term gain from code quality.

I feel we should still land it though. IMO if a code still live and kicking
today, we should not give up its maintenance. Also I believe it will save time
for those who study the code to implement its SPv2 counterpart.

[aelias_OOO_until_Jul13, 2017-05-04 20:40:08]

lgtm, but please also wait for bokan@ review

[bokan, 2017-05-09 22:27:21]

Sorry for the delay, just got back from vacay.

lgtm % comment.

https://codereview.chromium.org/2840143002/diff/40001/cc/test/layer_tree_test.cc
File cc/test/layer_tree_test.cc (left):

https://codereview.chromium.org/2840143002/diff/40001/cc/test/layer_tree_test...
cc/test/layer_tree_test.cc:68:
inner_viewport_scroll_layer->SetIsContainerForFixedPositionLayers(true);
If we're going to keep the inner viewport as a container for fixed position
layers we should keep it so in tests too.