Unify the calculation of main thread offset of sticky element

Unify the calculation of main thread offset of sticky element

Currently we have different logic of calculating sticky postion offset
on blink and cc which caused many unexpected offset related bugs. The
usage of "source_to_parent" when updating sticky position offset on cc
side makes the debug even harder. In addition we have to use another
variable from transform node "source_offset" to make it work correctly
which makes the logic even more complex.

By passing main thread offset from blink to cc via Layer, we could unify
the logic on both sides and get rid of the dependencies of those two
variables from transform node.

BUG=718188
TEST=compositing/overflow/ancestor-overflow-layer-of-sticky-child-of-compositing-container.html;
CompositedLayerMappingTest.StickyPositionMainThreadOffset
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2911463002
Cr-Commit-Position: refs/heads/master@{#477881}
Committed: https://chromium.googlesource.com/chromium/src/+/65784430dea11bf2e12e947c77dd08ce2c543c6f

[yigu, 2017-05-25 20:35:21]

Description was changed from

==========
Unify the computation of main thread offset of sticky element

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

to

==========
Unify the computation of main thread offset of sticky element

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

[yigu, 2017-05-25 20:35:23]

Description was changed from

==========
Unify the computation of main thread offset of sticky element

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

to

==========
Unify the computation of main thread offset of sticky element

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

[yigu, 2017-05-25 21:10:21]

Description was changed from

==========
Unify the computation of main thread offset of sticky element

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

to

==========
Unify the computation of main thread offset of sticky element

Currently we have different logic of calculating sticky postion offset
on blink and cc which caused many unexpected offset related bugs. The
usage of "source_to_parent" when updating sticky position offset on cc
side makes the debug even harder.

BUG=718188
TEST=compositing/overflow/ancestor-overflow-layer-of-sticky-child-of-compositing-container.html
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[yigu, 2017-05-26 13:56:13]

Description was changed from

==========
Unify the computation of main thread offset of sticky element

Currently we have different logic of calculating sticky postion offset
on blink and cc which caused many unexpected offset related bugs. The
usage of "source_to_parent" when updating sticky position offset on cc
side makes the debug even harder.

BUG=718188
TEST=compositing/overflow/ancestor-overflow-layer-of-sticky-child-of-compositing-container.html
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Unify the computation of main thread offset of sticky element

Currently we have different logic of calculating sticky postion offset
on blink and cc which caused many unexpected offset related bugs. The
usage of "source_to_parent" when updating sticky position offset on cc
side makes the debug even harder. In addition we have to use another
variable from transform node "source_offset" to make it work correctly
which makes the logic even more complex.

By passing main thread offset from blink to cc via Layer, we could unify
the logic on both sides and get rid of the dependencies of those two
variables from transform node.

BUG=718188
TEST=compositing/overflow/ancestor-overflow-layer-of-sticky-child-of-compositing-container.html
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[yigu, 2017-05-26 16:14:54]

Description was changed from

==========
Unify the computation of main thread offset of sticky element

Currently we have different logic of calculating sticky postion offset
on blink and cc which caused many unexpected offset related bugs. The
usage of "source_to_parent" when updating sticky position offset on cc
side makes the debug even harder. In addition we have to use another
variable from transform node "source_offset" to make it work correctly
which makes the logic even more complex.

By passing main thread offset from blink to cc via Layer, we could unify
the logic on both sides and get rid of the dependencies of those two
variables from transform node.

BUG=718188
TEST=compositing/overflow/ancestor-overflow-layer-of-sticky-child-of-compositing-container.html
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Unify the computation of main thread offset of sticky element

Currently we have different logic of calculating sticky postion offset
on blink and cc which caused many unexpected offset related bugs. The
usage of "source_to_parent" when updating sticky position offset on cc
side makes the debug even harder. In addition we have to use another
variable from transform node "source_offset" to make it work correctly
which makes the logic even more complex.

By passing main thread offset from blink to cc via Layer, we could unify
the logic on both sides and get rid of the dependencies of those two
variables from transform node.

BUG=718188
TEST=compositing/overflow/ancestor-overflow-layer-of-sticky-child-of-compositing-container.html;
CompositedLayerMappingTest.StickyPositionMainThreadOffset
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[yigu, 2017-05-26 16:17:57]

yigu@chromium.org changed reviewers:
+ chrishtr@chromium.org, flackr@chromium.org, smcgruer@chromium.org,
trchen@chromium.org

[yigu, 2017-05-26 16:17:58]

Hello. I uploaded a new patch addressing this issue. PTAL. Thanks!

[yigu, 2017-05-26 16:18:09]

Description was changed from

==========
Unify the computation of main thread offset of sticky element

Currently we have different logic of calculating sticky postion offset
on blink and cc which caused many unexpected offset related bugs. The
usage of "source_to_parent" when updating sticky position offset on cc
side makes the debug even harder. In addition we have to use another
variable from transform node "source_offset" to make it work correctly
which makes the logic even more complex.

By passing main thread offset from blink to cc via Layer, we could unify
the logic on both sides and get rid of the dependencies of those two
variables from transform node.

BUG=718188
TEST=compositing/overflow/ancestor-overflow-layer-of-sticky-child-of-compositing-container.html;
CompositedLayerMappingTest.StickyPositionMainThreadOffset
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Unify the calculation of main thread offset of sticky element

Currently we have different logic of calculating sticky postion offset
on blink and cc which caused many unexpected offset related bugs. The
usage of "source_to_parent" when updating sticky position offset on cc
side makes the debug even harder. In addition we have to use another
variable from transform node "source_offset" to make it work correctly
which makes the logic even more complex.

By passing main thread offset from blink to cc via Layer, we could unify
the logic on both sides and get rid of the dependencies of those two
variables from transform node.

BUG=718188
TEST=compositing/overflow/ancestor-overflow-layer-of-sticky-child-of-compositing-container.html;
CompositedLayerMappingTest.StickyPositionMainThreadOffset
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[smcgruer, 2017-05-26 17:14:12]

https://codereview.chromium.org/2911463002/diff/20001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/2911463002/diff/20001/cc/layers/layer.cc#newc...
cc/layers/layer.cc:1104: SetPropertyTreesNeedRebuild();
Do we definitely need to rebuild property trees here? Could we not propagate to
the existing TransformNode in this method instead?

https://codereview.chromium.org/2911463002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
(right):

https://codereview.chromium.org/2911463002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:299:
WebSize sticky_main_thread_offset;
Nit; this method is explicitly about sticky, we could just name this
'main_thread_offset' within this method.

[yigu, 2017-05-26 18:04:21]

https://codereview.chromium.org/2911463002/diff/20001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/2911463002/diff/20001/cc/layers/layer.cc#newc...
cc/layers/layer.cc:1104: SetPropertyTreesNeedRebuild();
On 2017/05/26 17:14:12, smcgruer wrote:
> Do we definitely need to rebuild property trees here? Could we not propagate
to
> the existing TransformNode in this method instead?

We don't need to rebuild the tree. Done.

https://codereview.chromium.org/2911463002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
(right):

https://codereview.chromium.org/2911463002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:299:
WebSize sticky_main_thread_offset;
On 2017/05/26 17:14:12, smcgruer wrote:
> Nit; this method is explicitly about sticky, we could just name this
> 'main_thread_offset' within this method.

Done.

[smcgruer, 2017-05-26 19:19:08]

lgtm

[flackr, 2017-05-31 14:08:23]

Thanks for doing this cleanup, this should fix all of our compositing only
sticky bugs.

https://codereview.chromium.org/2911463002/diff/40001/cc/trees/property_tree.cc
File cc/trees/property_tree.cc (right):

https://codereview.chromium.org/2911463002/diff/40001/cc/trees/property_tree....
cc/trees/property_tree.cc:459: gfx::Vector2dF sticky_main_thread_offset(
Rather than converting to a Vector2dF every time, perhaps we should store this
as a Vector2dF. This also matches many of the other offsets on TransformNode.

https://codereview.chromium.org/2911463002/diff/40001/cc/trees/transform_node.h
File cc/trees/transform_node.h (right):

https://codereview.chromium.org/2911463002/diff/40001/cc/trees/transform_node...
cc/trees/transform_node.h:58: gfx::Size sticky_main_thread_offset;
nit: Comment what this means.

https://codereview.chromium.org/2911463002/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
(right):

https://codereview.chromium.org/2911463002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:311:
RoundedIntSize(GetLayoutObject().StickyPositionOffset());
We should probably use the cached sticky position offsets so that we don't have
to recompute unnecessarily here. It would just be
total_sticky_box_sticky_offset_ -
NearestStickyBoxShiftingStickyBox().total_sticky_box_sticky_offset_ i think.

https://codereview.chromium.org/2911463002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:364:
graphics_layer_->SetStickyMainThreadOffset(main_thread_offset);
Don't we need to update the sticky main thread offset whenever we update the
layer position? I.e. after scrolling an overflow we call
PaintLayer::UpdateLayerPositionsAfterOverflowScroll which just updates the layer
positions recursively but shouldn't produce new sticky constraints.

[yigu, 2017-06-01 15:15:19]

Thanks! PTAL.

https://codereview.chromium.org/2911463002/diff/40001/cc/trees/property_tree.cc
File cc/trees/property_tree.cc (right):

https://codereview.chromium.org/2911463002/diff/40001/cc/trees/property_tree....
cc/trees/property_tree.cc:459: gfx::Vector2dF sticky_main_thread_offset(
On 2017/05/31 14:08:22, flackr wrote:
> Rather than converting to a Vector2dF every time, perhaps we should store this
> as a Vector2dF. This also matches many of the other offsets on TransformNode.

Done.

https://codereview.chromium.org/2911463002/diff/40001/cc/trees/transform_node.h
File cc/trees/transform_node.h (right):

https://codereview.chromium.org/2911463002/diff/40001/cc/trees/transform_node...
cc/trees/transform_node.h:58: gfx::Size sticky_main_thread_offset;
On 2017/05/31 14:08:23, flackr wrote:
> nit: Comment what this means.

Done.

https://codereview.chromium.org/2911463002/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
(right):

https://codereview.chromium.org/2911463002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:311:
RoundedIntSize(GetLayoutObject().StickyPositionOffset());
On 2017/05/31 14:08:23, flackr wrote:
> We should probably use the cached sticky position offsets so that we don't
have
> to recompute unnecessarily here. It would just be
> total_sticky_box_sticky_offset_ -
> NearestStickyBoxShiftingStickyBox().total_sticky_box_sticky_offset_ i think.

Moved this logic to UpdateLayerPosition.

https://codereview.chromium.org/2911463002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:364:
graphics_layer_->SetStickyMainThreadOffset(main_thread_offset);
On 2017/05/31 14:08:23, flackr wrote:
> Don't we need to update the sticky main thread offset whenever we update the
> layer position? I.e. after scrolling an overflow we call
> PaintLayer::UpdateLayerPositionsAfterOverflowScroll which just updates the
layer
> positions recursively but shouldn't produce new sticky constraints.

Done.

[flackr, 2017-06-01 19:01:48]

https://codereview.chromium.org/2911463002/diff/60001/cc/trees/transform_node.h
File cc/trees/transform_node.h (right):

https://codereview.chromium.org/2911463002/diff/60001/cc/trees/transform_node...
cc/trees/transform_node.h:59: // thread. It's used to calculate the actual
sticky position offset from cc.
nit: s/actual sticky position offset/additional sticky position offset

https://codereview.chromium.org/2911463002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintLayer.cpp (right):

https://codereview.chromium.org/2911463002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayer.cpp:854:
->SetStickyMainThreadOffset(RoundedIntSize(new_offset));
I don't think this is the right place to set the sticky main thread offset on
the graphics layer since it may be created later. We should be doing this when
we set the graphics layer's position based, which is I believe in
CompositedLayerMapping::UpdateMainGraphicsLayerGeometry.

[yigu, 2017-06-01 19:28:40]

PTAL. Thanks!

https://codereview.chromium.org/2911463002/diff/60001/cc/trees/transform_node.h
File cc/trees/transform_node.h (right):

https://codereview.chromium.org/2911463002/diff/60001/cc/trees/transform_node...
cc/trees/transform_node.h:59: // thread. It's used to calculate the actual
sticky position offset from cc.
On 2017/06/01 19:01:47, flackr wrote:
> nit: s/actual sticky position offset/additional sticky position offset

Done.

https://codereview.chromium.org/2911463002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintLayer.cpp (right):

https://codereview.chromium.org/2911463002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayer.cpp:854:
->SetStickyMainThreadOffset(RoundedIntSize(new_offset));
On 2017/06/01 19:01:47, flackr wrote:
> I don't think this is the right place to set the sticky main thread offset on
> the graphics layer since it may be created later. We should be doing this when
> we set the graphics layer's position based, which is I believe in
> CompositedLayerMapping::UpdateMainGraphicsLayerGeometry.

Done.

[flackr, 2017-06-01 19:47:49]

https://codereview.chromium.org/2911463002/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
(right):

https://codereview.chromium.org/2911463002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:1139:
RoundedIntSize(GetLayoutObject().StickyPositionOffset()));
Only if the element is sticky, otherwise this should be 0.

Also, did you look into using the cached sticky offset rather than recomputing
it?

[yigu, 2017-06-01 20:32:54]

The cached values are used to avoid re-computation. PTAL. Thanks!

https://codereview.chromium.org/2911463002/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
(right):

https://codereview.chromium.org/2911463002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:1139:
RoundedIntSize(GetLayoutObject().StickyPositionOffset()));
On 2017/06/01 19:47:48, flackr wrote:
> Only if the element is sticky, otherwise this should be 0.
> 
> Also, did you look into using the cached sticky offset rather than recomputing
> it?

Done.

[flackr, 2017-06-01 22:09:09]

https://codereview.chromium.org/2911463002/diff/100001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
(right):

https://codereview.chromium.org/2911463002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:1146:
graphics_layer_->SetStickyMainThreadOffset(RoundedIntSize(
What if the layer becomes no longer sticky? I think we should set this to 0
otherwise. i.e.

FloatSize main_thread_sticky_offset;
if (sticky) {
  // calculate offset
}
graphics_layer_->SetStickyMainThreadOffset(main_thread_sticky_offset);

https://codereview.chromium.org/2911463002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:1152:
: FloatSize())));
nit: It would probably make sense to turn this into a function on
StickyPositionScrollingConstraints, like localStickyOffset().

[yigu, 2017-06-02 04:24:27]

PTAL. Thanks!

https://codereview.chromium.org/2911463002/diff/100001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
(right):

https://codereview.chromium.org/2911463002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:1146:
graphics_layer_->SetStickyMainThreadOffset(RoundedIntSize(
On 2017/06/01 22:09:09, flackr wrote:
> What if the layer becomes no longer sticky? I think we should set this to 0
> otherwise. i.e.
> 
> FloatSize main_thread_sticky_offset;
> if (sticky) {
>   // calculate offset
> }
> graphics_layer_->SetStickyMainThreadOffset(main_thread_sticky_offset);

Done.

https://codereview.chromium.org/2911463002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:1152:
: FloatSize())));
On 2017/06/01 22:09:09, flackr wrote:
> nit: It would probably make sense to turn this into a function on
> StickyPositionScrollingConstraints, like localStickyOffset().

Done. Currently ancestor_overflow_layer is passed to the function to get
constraints_map. Is there a better way to compute the offset of
NearestStickyBoxShiftingStickyBox?

[flackr, 2017-06-02 15:33:58]

LGTM with nits

https://codereview.chromium.org/2911463002/diff/120001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
(right):

https://codereview.chromium.org/2911463002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:1147:
*GetLayoutObject().Layer()->AncestorOverflowLayer());
This can just be owning_layer_.AncestorOverflowLayer().

https://codereview.chromium.org/2911463002/diff/120001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.cpp
(right):

https://codereview.chromium.org/2911463002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.cpp:93:
const PaintLayer& ancestor_overflow_layer) const {
Instead of passing in the ancestor_overflow_layer it'd probably have a cleaner
API if we just pass the StickyConstraintsMap.

https://codereview.chromium.org/2911463002/diff/120001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.h
(right):

https://codereview.chromium.org/2911463002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.h:60:
FloatSize ComputeMainThreadStickyOffset(
Let's make this like GetTotalStickyBoxStickyOffset - i.e. maybe call it
GetLocalStickyOffset and move it amongst those functions.

[yigu, 2017-06-02 17:59:56]

Thanks Rob! Patch updated.

https://codereview.chromium.org/2911463002/diff/120001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
(right):

https://codereview.chromium.org/2911463002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:1147:
*GetLayoutObject().Layer()->AncestorOverflowLayer());
On 2017/06/02 15:33:58, flackr wrote:
> This can just be owning_layer_.AncestorOverflowLayer().

Done.

https://codereview.chromium.org/2911463002/diff/120001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.cpp
(right):

https://codereview.chromium.org/2911463002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.cpp:93:
const PaintLayer& ancestor_overflow_layer) const {
On 2017/06/02 15:33:58, flackr wrote:
> Instead of passing in the ancestor_overflow_layer it'd probably have a cleaner
> API if we just pass the StickyConstraintsMap.

Done.

https://codereview.chromium.org/2911463002/diff/120001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.h
(right):

https://codereview.chromium.org/2911463002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.h:60:
FloatSize ComputeMainThreadStickyOffset(
On 2017/06/02 15:33:58, flackr wrote:
> Let's make this like GetTotalStickyBoxStickyOffset - i.e. maybe call it
> GetLocalStickyOffset and move it amongst those functions.

Done.

[chrishtr, 2017-06-05 15:23:27]

https://codereview.chromium.org/2911463002/diff/140001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/2911463002/diff/140001/cc/layers/layer.cc#new...
cc/layers/layer.cc:1111:
property_trees->transform_tree.UpdateNodeFromOwningLayerId(id())) {
Use element id -> node id map instead. Mapping from Layer id is being removed.

https://codereview.chromium.org/2911463002/diff/140001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
(left):

https://codereview.chromium.org/2911463002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:306:
// Find the layout offset of the unshifted sticky box within its parent
It seems it turned out that this logic was equivalent to what was already
computed
in the sticky constraints map code. Was this just a case of not fully realizing
that at the time this original code was written?

[flackr, 2017-06-05 16:45:12]

https://codereview.chromium.org/2911463002/diff/140001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
(left):

https://codereview.chromium.org/2911463002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:306:
// Find the layout offset of the unshifted sticky box within its parent
On 2017/06/05 15:23:27, chrishtr wrote:
> It seems it turned out that this logic was equivalent to what was already
> computed
> in the sticky constraints map code. Was this just a case of not fully
realizing
> that at the time this original code was written?

Well this logic is actually just trying to work out the relative position of the
sticky box and ancestor scroller so that it can figure out how much the main
thread has already pushed this sticky element down from setting the layer
position. We had thought about just passing this main thread offset through
initially and at the time it seemed simple enough to work out what it was from
the layer position but has since proven to be rather hard to get right with all
of the various ways the tree can be constructed.

So it's not that the main thread does this, it doesn't need to because it's
using the layout boxes and if we just pass the main thread offset the only thing
we need from the compositor is the current ancestor scroll position to work out
the additional sticky offset.

[chrishtr, 2017-06-05 17:17:34]

https://codereview.chromium.org/2911463002/diff/140001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.h
(right):

https://codereview.chromium.org/2911463002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.h:122:
FloatSize GetLocalStickyOffset(const StickyConstraintsMap&) const;
Document clearly what this returns.

Not your fault, but: actually, on further inspection
of this file, each of these public accessors and setters needs documentation.
I suggest a class-level comment explaining each of the different types of
information, with examples.

[yigu, 2017-06-05 18:16:34]

Thanks Chris! PTAL.

https://codereview.chromium.org/2911463002/diff/140001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/2911463002/diff/140001/cc/layers/layer.cc#new...
cc/layers/layer.cc:1111:
property_trees->transform_tree.UpdateNodeFromOwningLayerId(id())) {
On 2017/06/05 15:23:27, chrishtr wrote:
> Use element id -> node id map instead. Mapping from Layer id is being removed.

Done.

https://codereview.chromium.org/2911463002/diff/140001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.h
(right):

https://codereview.chromium.org/2911463002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.h:122:
FloatSize GetLocalStickyOffset(const StickyConstraintsMap&) const;
On 2017/06/05 17:17:34, chrishtr wrote:
> Document clearly what this returns.
> 
> Not your fault, but: actually, on further inspection
> of this file, each of these public accessors and setters needs documentation.
> I suggest a class-level comment explaining each of the different types of
> information, with examples.

Added a TODO for adding the class-level comment with examples.

[chrishtr, 2017-06-05 18:28:45]

https://codereview.chromium.org/2911463002/diff/160001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.h
(right):

https://codereview.chromium.org/2911463002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.h:125:
// that it can figure out how much the main thread has already pushed this
You shouldn't specify use cases. It's clearer to just say what this
represents.

https://codereview.chromium.org/2911463002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.h:127:
FloatSize GetLocalStickyOffset(const StickyConstraintsMap&) const;
GetPositionRelativeToScrollAncestor?

[yigu, 2017-06-05 18:35:08]

Patch updated. PTAL. Thanks!

https://codereview.chromium.org/2911463002/diff/160001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.h
(right):

https://codereview.chromium.org/2911463002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.h:125:
// that it can figure out how much the main thread has already pushed this
On 2017/06/05 18:28:45, chrishtr wrote:
> You shouldn't specify use cases. It's clearer to just say what this
> represents.

Done.

https://codereview.chromium.org/2911463002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.h:127:
FloatSize GetLocalStickyOffset(const StickyConstraintsMap&) const;
On 2017/06/05 18:28:45, chrishtr wrote:
> GetPositionRelativeToScrollAncestor?

Done.

[chrishtr, 2017-06-05 18:58:33]

https://codereview.chromium.org/2911463002/diff/180001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.cpp
(right):

https://codereview.chromium.org/2911463002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.cpp:101:
return total_sticky_box_sticky_offset_ -
Could you explain why this logic is correct? I'm not sure
I recall what total_sticky_box_sticky_offset_ and 
nearest_sticky_box_shifting_sticky_box_constraints_offset are
exactly. (and the comments are still not there for this file. :))

[yigu, 2017-06-05 21:03:49]

https://codereview.chromium.org/2911463002/diff/180001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.cpp
(right):

https://codereview.chromium.org/2911463002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.cpp:101:
return total_sticky_box_sticky_offset_ -
On 2017/06/05 18:58:33, chrishtr wrote:
> Could you explain why this logic is correct? I'm not sure
> I recall what total_sticky_box_sticky_offset_ and 
> nearest_sticky_box_shifting_sticky_box_constraints_offset are
> exactly. (and the comments are still not there for this file. :))

We do the subtraction because total_sticky_box_sticky_offset is calculated by
ancestor_sticky_box_offset + sticky_offset. The subtraction gives us the
sticky_offset we need.
Alternatively, we could cache the sticky_offset upon calculation as
sticky_offset_ and pass it to cc as what we did with
"main_thread_sticky_offset". The downside of it is we are adding a variable to
store a value which we could compute from existing cached values.
WDYT?

[trchen, 2017-06-05 22:43:11]

https://codereview.chromium.org/2911463002/diff/180001/cc/layers/layer.h
File cc/layers/layer.h (right):

https://codereview.chromium.org/2911463002/diff/180001/cc/layers/layer.h#newc...
cc/layers/layer.h:585: gfx::Size sticky_main_thread_offset;
I think we should document what this value means. At the beginning I thought it
is accumulated offset up to the enclosing overflow clip, but it is in fact the
stick offset applied locally.

https://codereview.chromium.org/2911463002/diff/180001/cc/trees/property_tree.cc
File cc/trees/property_tree.cc (right):

https://codereview.chromium.org/2911463002/diff/180001/cc/trees/property_tree...
cc/trees/property_tree.cc:466: sticky_box_rect.OffsetFromOrigin();
This is quite confusing. I think the logic is to compute the local sticky offset
using only layout information (prior to blink adjustment), then subtract blink
adjustment from target adjustment, i.e. 

(sticky_offset - sticky_box_rect.OffsetFromOrigin()) -
node->sticky_main_thread_offset

which is the same value, but with correct associating order.

However it raised the question why sticky_offset included
sticky_box_rect.OffsetFromOrigin() in the first place. Can we just do

gfx::Vector2dF sticky_offset;

on line #408? Then we can replace "sticky_offset -
sticky_box_rect.OffsetFromOrigin()" with just "sticky_offset" on line #458 #461
#465.

https://codereview.chromium.org/2911463002/diff/180001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.cpp
(right):

https://codereview.chromium.org/2911463002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.cpp:101:
return total_sticky_box_sticky_offset_ -
On 2017/06/05 21:03:49, yigu wrote:
> On 2017/06/05 18:58:33, chrishtr wrote:
> > Could you explain why this logic is correct? I'm not sure
> > I recall what total_sticky_box_sticky_offset_ and 
> > nearest_sticky_box_shifting_sticky_box_constraints_offset are
> > exactly. (and the comments are still not there for this file. :))

+1 We need more comments in the headers. It took me a while to read through the
relevant code and label each variables in my mental map. :)

Following is my understanding of them. Feel free to rephrase/make
correction/copy them to comments.

total_sticky_box_sticky_offset_ : The accumulated layout location adjustment
applied to a sticky box, up to its containing block.

nearest_sticky_box_shifting_sticky_box_ : The nearest ancestor in location
container chain that is sticky, up to its containing block. Can be nullptr if
there is no such ancestor.

total_containing_block_sticky_offset_ : The accumulated layout location
adjustment applied to a sticky box, up to its enclosing overflow clip.

> We do the subtraction because total_sticky_box_sticky_offset is calculated by
> ancestor_sticky_box_offset + sticky_offset. The subtraction gives us the
> sticky_offset we need.
> Alternatively, we could cache the sticky_offset upon calculation as
> sticky_offset_ and pass it to cc as what we did with
> "main_thread_sticky_offset". The downside of it is we are adding a variable to
> store a value which we could compute from existing cached values.
> WDYT?

I think it is okay to use the derived value than caching it, but the naming
could be clearer. It is definitely not "position relative to scroll ancestor".
How about something like GetPreviouslyComputedStickyOffset? Also documenting
that this function is only safe to call if ComputeStickyOffset has been invoked
previously (better if DCHECK can be added, but that seems to be complicated).

[yigu, 2017-06-06 19:33:18]

Thanks for the suggestion! PTAL.
p.s. smcgruer@ has the documentation work in progress and I've filed a bug for
it (crbug.com/730043).

https://codereview.chromium.org/2911463002/diff/180001/cc/layers/layer.h
File cc/layers/layer.h (right):

https://codereview.chromium.org/2911463002/diff/180001/cc/layers/layer.h#newc...
cc/layers/layer.h:585: gfx::Size sticky_main_thread_offset;
On 2017/06/05 22:43:11, trchen wrote:
> I think we should document what this value means. At the beginning I thought
it
> is accumulated offset up to the enclosing overflow clip, but it is in fact the
> stick offset applied locally.

Done.

https://codereview.chromium.org/2911463002/diff/180001/cc/trees/property_tree.cc
File cc/trees/property_tree.cc (right):

https://codereview.chromium.org/2911463002/diff/180001/cc/trees/property_tree...
cc/trees/property_tree.cc:466: sticky_box_rect.OffsetFromOrigin();
On 2017/06/05 22:43:11, trchen wrote:
> This is quite confusing. I think the logic is to compute the local sticky
offset
> using only layout information (prior to blink adjustment), then subtract blink
> adjustment from target adjustment, i.e. 
> 
> (sticky_offset - sticky_box_rect.OffsetFromOrigin()) -
> node->sticky_main_thread_offset
> 
> which is the same value, but with correct associating order.
> 
> However it raised the question why sticky_offset included
> sticky_box_rect.OffsetFromOrigin() in the first place. Can we just do
> 
> gfx::Vector2dF sticky_offset;
> 
> on line #408? Then we can replace "sticky_offset -
> sticky_box_rect.OffsetFromOrigin()" with just "sticky_offset" on line #458
#461
> #465.

Done.

https://codereview.chromium.org/2911463002/diff/180001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.cpp
(right):

https://codereview.chromium.org/2911463002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.cpp:101:
return total_sticky_box_sticky_offset_ -
On 2017/06/05 22:43:11, trchen wrote:
> On 2017/06/05 21:03:49, yigu wrote:
> > On 2017/06/05 18:58:33, chrishtr wrote:
> > > Could you explain why this logic is correct? I'm not sure
> > > I recall what total_sticky_box_sticky_offset_ and 
> > > nearest_sticky_box_shifting_sticky_box_constraints_offset are
> > > exactly. (and the comments are still not there for this file. :))
> 
> +1 We need more comments in the headers. It took me a while to read through
the
> relevant code and label each variables in my mental map. :)
> 
> Following is my understanding of them. Feel free to rephrase/make
> correction/copy them to comments.
> 
> total_sticky_box_sticky_offset_ : The accumulated layout location adjustment
> applied to a sticky box, up to its containing block.
> 
> nearest_sticky_box_shifting_sticky_box_ : The nearest ancestor in location
> container chain that is sticky, up to its containing block. Can be nullptr if
> there is no such ancestor.
> 
> total_containing_block_sticky_offset_ : The accumulated layout location
> adjustment applied to a sticky box, up to its enclosing overflow clip.
> 
> > We do the subtraction because total_sticky_box_sticky_offset is calculated
by
> > ancestor_sticky_box_offset + sticky_offset. The subtraction gives us the
> > sticky_offset we need.
> > Alternatively, we could cache the sticky_offset upon calculation as
> > sticky_offset_ and pass it to cc as what we did with
> > "main_thread_sticky_offset". The downside of it is we are adding a variable
to
> > store a value which we could compute from existing cached values.
> > WDYT?
> 
> I think it is okay to use the derived value than caching it, but the naming
> could be clearer. It is definitely not "position relative to scroll ancestor".
> How about something like GetPreviouslyComputedStickyOffset? Also documenting
> that this function is only safe to call if ComputeStickyOffset has been
invoked
> previously (better if DCHECK can be added, but that seems to be complicated).

I've updated the function name.

[trchen, 2017-06-06 21:17:21]

lgtm, except I'd wish one more comment.

The two extra questions I have are just for verifying my understand to this CL.
Don't mean to block it.

https://codereview.chromium.org/2911463002/diff/200001/cc/trees/property_tree.cc
File cc/trees/property_tree.cc (right):

https://codereview.chromium.org/2911463002/diff/200001/cc/trees/property_tree...
cc/trees/property_tree.cc:356: gfx::Vector2dF
StickyPositionOffset(TransformTree* tree, TransformNode* node) {
nits: I think it'd be nice to add a comment here to tell this function should
match StickyPositionScrollingConstraints::ComputeStickyOffset().

Extra question: Can we assume in absence of impl-side scroll delta, this
function will always return (0, 0)? If yes, is it possible to add a DCHECK for
it? If not, could you make an example?

Extra question 2: Can we just offset layer position by
-sticky_main_thread_offset in CLM, as if the main thread never applied its
sticky offset? That way sticky_main_thread_offset will always be 0 thus one less
variable to pass around, no?

[yigu, 2017-06-07 15:52:33]

Thanks trchen@ for the feedback! I'll work on the layer shifting in a follow up
patch as we need to do more investigation.

https://codereview.chromium.org/2911463002/diff/200001/cc/trees/property_tree.cc
File cc/trees/property_tree.cc (right):

https://codereview.chromium.org/2911463002/diff/200001/cc/trees/property_tree...
cc/trees/property_tree.cc:356: gfx::Vector2dF
StickyPositionOffset(TransformTree* tree, TransformNode* node) {
On 2017/06/06 21:17:21, trchen wrote:
> nits: I think it'd be nice to add a comment here to tell this function should
> match StickyPositionScrollingConstraints::ComputeStickyOffset().
> 

This function doesn't really match the one from blink. It returns the delta.
e.g. function from blink returns (0,100) then this function returns (0, 100) -
(0, 100) = (0, 0).

> Extra question: Can we assume in absence of impl-side scroll delta, this
> function will always return (0, 0)? If yes, is it possible to add a DCHECK for
> it? If not, could you make an example?
 
This is not true due to snapping and rounding.

> Extra question 2: Can we just offset layer position by
> -sticky_main_thread_offset in CLM, as if the main thread never applied its
> sticky offset? That way sticky_main_thread_offset will always be 0 thus one
less
> variable to pass around, no?

This is actually a very good suggestion. Shifting the layer position works when
the scroller is composited and it has scrolling layer. We tried to add condition
checks to make it work in all cases but it seems that we need further
investigation on this. Will open a bug and start to work on it.

[yigu, 2017-06-07 15:52:42]

The CQ bit was checked by yigu@chromium.org

[yigu, 2017-06-07 15:52:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
smcgruer@chromium.org, flackr@chromium.org
Link to the patchset: https://codereview.chromium.org/2911463002/#ps200001
(title: "Address comments")

[commit-bot: I haz the power, 2017-06-07 15:53:35]

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-06-07 16:04:12]

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

[commit-bot: I haz the power, 2017-06-07 16:04:14]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[chrishtr, 2017-06-07 19:28:38]

https://codereview.chromium.org/2911463002/diff/200001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.h
(right):

https://codereview.chromium.org/2911463002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.h:124:
// Returns the relative position of the sticky box and its original position.
Original, meaning before scroll? If so, please incorporate that into your
comment.

https://codereview.chromium.org/2911463002/diff/200001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebLayer.h (right):

https://codereview.chromium.org/2911463002/diff/200001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebLayer.h:205: // Sets the main thread
offset applied to the sticky element. This will be
The "main thread offset" is the of the sticky element from its non-scrolled
position right? IOW the offset from where it would be if it was
position: relative?

This is the same as GetOffsetRelativeToOriginalLayoutPosition. How about calling
the concept in cc the same? "main thread" is not necessary, because Layer is
always on the main thread and is updated only by Blink. LayerImpl is on the
impl thread and so needs to differentiate.

[yigu, 2017-06-07 21:09:14]

Thanks Chris! I've renamed the functions for clarification. PTAL.

https://codereview.chromium.org/2911463002/diff/200001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.h
(right):

https://codereview.chromium.org/2911463002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.h:124:
// Returns the relative position of the sticky box and its original position.
On 2017/06/07 19:28:38, chrishtr wrote:
> Original, meaning before scroll? If so, please incorporate that into your
> comment.

Done.

https://codereview.chromium.org/2911463002/diff/200001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebLayer.h (right):

https://codereview.chromium.org/2911463002/diff/200001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebLayer.h:205: // Sets the main thread
offset applied to the sticky element. This will be
On 2017/06/07 19:28:38, chrishtr wrote:
> The "main thread offset" is the of the sticky element from its non-scrolled
> position right? IOW the offset from where it would be if it was
> position: relative?
> 
> This is the same as GetOffsetRelativeToOriginalLayoutPosition. How about
calling
> the concept in cc the same? "main thread" is not necessary, because Layer is
> always on the main thread and is updated only by Blink. LayerImpl is on the
> impl thread and so needs to differentiate.

Done.

[chrishtr, 2017-06-08 00:00:30]

lgtm

[yigu, 2017-06-08 00:08:52]

The CQ bit was checked by yigu@chromium.org

[yigu, 2017-06-08 00:08:52]

The patchset sent to the CQ was uploaded after l-g-t-m from flackr@chromium.org,
smcgruer@chromium.org, trchen@chromium.org
Link to the patchset: https://codereview.chromium.org/2911463002/#ps220001
(title: "Renaming functions")

[commit-bot: I haz the power, 2017-06-08 00:09:18]

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-06-08 00:29:08]

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

[commit-bot: I haz the power, 2017-06-08 00:29:10]

Try jobs failed on following builders:
  linux_trusty_blink_rel on master.tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_trusty_blink_rel/b...)

[yigu, 2017-06-08 01:08:47]

The CQ bit was checked by yigu@chromium.org

[yigu, 2017-06-08 01:08:48]

The patchset sent to the CQ was uploaded after l-g-t-m from flackr@chromium.org,
smcgruer@chromium.org, chrishtr@chromium.org, trchen@chromium.org
Link to the patchset: https://codereview.chromium.org/2911463002/#ps240001
(title: "Bug fix")

[commit-bot: I haz the power, 2017-06-08 01:09:35]

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-06-08 04:09:10]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1496884127565130,
"parent_rev": "1723af89f9dca5b35dc342ccdf36970fc49b548b", "commit_rev":
"65784430dea11bf2e12e947c77dd08ce2c543c6f"}

[commit-bot: I haz the power, 2017-06-08 04:09:21]

Description was changed from

==========
Unify the calculation of main thread offset of sticky element

Currently we have different logic of calculating sticky postion offset
on blink and cc which caused many unexpected offset related bugs. The
usage of "source_to_parent" when updating sticky position offset on cc
side makes the debug even harder. In addition we have to use another
variable from transform node "source_offset" to make it work correctly
which makes the logic even more complex.

By passing main thread offset from blink to cc via Layer, we could unify
the logic on both sides and get rid of the dependencies of those two
variables from transform node.

BUG=718188
TEST=compositing/overflow/ancestor-overflow-layer-of-sticky-child-of-compositing-container.html;
CompositedLayerMappingTest.StickyPositionMainThreadOffset
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Unify the calculation of main thread offset of sticky element

Currently we have different logic of calculating sticky postion offset
on blink and cc which caused many unexpected offset related bugs. The
usage of "source_to_parent" when updating sticky position offset on cc
side makes the debug even harder. In addition we have to use another
variable from transform node "source_offset" to make it work correctly
which makes the logic even more complex.

By passing main thread offset from blink to cc via Layer, we could unify
the logic on both sides and get rid of the dependencies of those two
variables from transform node.

BUG=718188
TEST=compositing/overflow/ancestor-overflow-layer-of-sticky-child-of-compositing-container.html;
CompositedLayerMappingTest.StickyPositionMainThreadOffset
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2911463002
Cr-Commit-Position: refs/heads/master@{#477881}
Committed:
https://chromium.googlesource.com/chromium/src/+/65784430dea11bf2e12e947c77dd...
==========

[commit-bot: I haz the power, 2017-06-08 04:09:23]

Committed patchset #13 (id:240001) as
https://chromium.googlesource.com/chromium/src/+/65784430dea11bf2e12e947c77dd...