Shifting layer position for sticky element to avoid passing unnessary variable to cc

Shifting layer position for sticky element to avoid passing unnecessary
variable to cc.

BUG=730655
TEST=CompositedLayerMappingTest.LayerPositionForStickyElement
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/2929873002
Cr-Commit-Position: refs/heads/master@{#478983}
Committed: https://chromium.googlesource.com/chromium/src/+/23b68f6ceca08d1d213d4af869ba8336c9aa597a

[yigu, 2017-06-08 16:15:53]

Description was changed from

==========
Shifting layer position for sticky element to avoid passing unnessary
variable to cc.

BUG=730655
TEST=CompositedLayerMappingTest.LayerPositionForStickyElement
==========

to

==========
Shifting layer position for sticky element to avoid passing unnessary
variable to cc.

BUG=730655
TEST=CompositedLayerMappingTest.LayerPositionForStickyElement
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[yigu, 2017-06-08 16:15:54]

Description was changed from

==========
Shifting layer position for sticky element to avoid passing unnessary
variable to cc.

BUG=730655
TEST=CompositedLayerMappingTest.LayerPositionForStickyElement
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Shifting layer position for sticky element to avoid passing unnessary
variable to cc.

BUG=730655
TEST=CompositedLayerMappingTest.LayerPositionForStickyElement
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[yigu, 2017-06-08 16:16:37]

Description was changed from

==========
Shifting layer position for sticky element to avoid passing unnessary
variable to cc.

BUG=730655
TEST=CompositedLayerMappingTest.LayerPositionForStickyElement
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Shifting layer position for sticky element to avoid passing unnessary
variable to cc.

BUG=730655
TEST=CompositedLayerMappingTest.LayerPositionForStickyElement
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[yigu, 2017-06-08 17:21:58]

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

[yigu, 2017-06-08 17:21:58]

This patch is to remove the new main thread sticky offset we added in the
previous patch. By shifting the layer position, we don't need to pass that
variable any more. PTAL. Thanks.

[flackr, 2017-06-08 18:56:19]

https://codereview.chromium.org/2929873002/diff/1/cc/trees/property_tree.cc
File cc/trees/property_tree.cc (right):

https://codereview.chromium.org/2929873002/diff/1/cc/trees/property_tree.cc#n...
cc/trees/property_tree.cc:356: gfx::Vector2dF
StickyPositionOffset(TransformTree* tree, TransformNode* node) {
I think now it definitely makes sense to add the comment that trchen mentioned
in your last review:
https://codereview.chromium.org/2911463002/diff/200001/cc/trees/property_tree...

These functions need to be kept in sync (in case changes are made in one)

https://codereview.chromium.org/2929873002/diff/1/third_party/WebKit/Source/c...
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
(right):

https://codereview.chromium.org/2929873002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:1135:
if (GetLayoutObject().Style()->GetPosition() == EPosition::kSticky &&
This definitely needs a comment at a minimum, although I think to ensure that we
don't make the wrong decision we should use the same condition to determine
whether we set sticky position constraints on the graphics layer.

I.e. add a method bool CompositedLayerMapping::UsesCompositedStickyPosition, and
then check that this returns true before unshifting the graphics layer position
here and before transferring the sticky position constraints in
CompositedLayerMapping::UpdateStickyConstraints. That way we don't risk doing
composited sticky position on the compositor with an unshifted layer.

https://codereview.chromium.org/2929873002/diff/1/third_party/WebKit/Source/c...
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMappingTest.cpp
(right):

https://codereview.chromium.org/2929873002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMappingTest.cpp:1583:
EXPECT_FLOAT_EQ(0, main_graphics_layer->GetPosition().Y());
You should have both a composited scroller and non composited scroller test
since these have different layer positions.

[flackr, 2017-06-08 22:26:55]

https://codereview.chromium.org/2929873002/diff/1/third_party/WebKit/Source/c...
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
(right):

https://codereview.chromium.org/2929873002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:1137:
owning_layer_.AncestorOverflowLayer()->NeedsCompositedScrolling())) {
chrishtr, trchen, I'm wondering if it's safe to try to determine the compositing
state of the scrolling ancestor at this point.

[chrishtr, 2017-06-08 22:38:40]

https://codereview.chromium.org/2929873002/diff/1/third_party/WebKit/Source/c...
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
(right):

https://codereview.chromium.org/2929873002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:1137:
owning_layer_.AncestorOverflowLayer()->NeedsCompositedScrolling())) {
On 2017/06/08 at 22:26:55, flackr wrote:
> chrishtr, trchen, I'm wondering if it's safe to try to determine the
compositing state of the scrolling ancestor at this point.

Yes, it is safe. Paint layer compositor works in this sequence:

1. Compute compositing inputs (including sticky position constraints, for
example)
2. Compute compositing requirements (triggers for compositing)
3. Assign  composited layers
4. Update graphics layer configurations

Step 4 is where this function lies. Step 3 determined everything needed to
answer
NeedsCompositedScrolling.

[yigu, 2017-06-08 23:30:03]

Thanks! Patch updated. PTAL.

https://codereview.chromium.org/2929873002/diff/1/cc/trees/property_tree.cc
File cc/trees/property_tree.cc (right):

https://codereview.chromium.org/2929873002/diff/1/cc/trees/property_tree.cc#n...
cc/trees/property_tree.cc:356: gfx::Vector2dF
StickyPositionOffset(TransformTree* tree, TransformNode* node) {
On 2017/06/08 18:56:18, flackr wrote:
> I think now it definitely makes sense to add the comment that trchen mentioned
> in your last review:
>
https://codereview.chromium.org/2911463002/diff/200001/cc/trees/property_tree...
> 
> These functions need to be kept in sync (in case changes are made in one)

Done.

https://codereview.chromium.org/2929873002/diff/1/third_party/WebKit/Source/c...
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
(right):

https://codereview.chromium.org/2929873002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:1135:
if (GetLayoutObject().Style()->GetPosition() == EPosition::kSticky &&
On 2017/06/08 18:56:19, flackr wrote:
> This definitely needs a comment at a minimum, although I think to ensure that
we
> don't make the wrong decision we should use the same condition to determine
> whether we set sticky position constraints on the graphics layer.
> 
> I.e. add a method bool CompositedLayerMapping::UsesCompositedStickyPosition,
and
> then check that this returns true before unshifting the graphics layer
position
> here and before transferring the sticky position constraints in
> CompositedLayerMapping::UpdateStickyConstraints. That way we don't risk doing
> composited sticky position on the compositor with an unshifted layer.

Done.

https://codereview.chromium.org/2929873002/diff/1/third_party/WebKit/Source/c...
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMappingTest.cpp
(right):

https://codereview.chromium.org/2929873002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMappingTest.cpp:1583:
EXPECT_FLOAT_EQ(0, main_graphics_layer->GetPosition().Y());
On 2017/06/08 18:56:19, flackr wrote:
> You should have both a composited scroller and non composited scroller test
> since these have different layer positions.

I added a test with non-composited scroller. As the scroller doesn't need
composited scrolling we don't compute the local sticky offset as we do in the
other test. The layer position is therefore not affected.

[flackr, 2017-06-08 23:54:46]

https://codereview.chromium.org/2929873002/diff/1/third_party/WebKit/Source/c...
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMappingTest.cpp
(right):

https://codereview.chromium.org/2929873002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMappingTest.cpp:1583:
EXPECT_FLOAT_EQ(0, main_graphics_layer->GetPosition().Y());
On 2017/06/08 23:30:03, yigu wrote:
> On 2017/06/08 18:56:19, flackr wrote:
> > You should have both a composited scroller and non composited scroller test
> > since these have different layer positions.
> 
> I added a test with non-composited scroller. As the scroller doesn't need
> composited scrolling we don't compute the local sticky offset as we do in the
> other test. The layer position is therefore not affected.

All else being equal, one test should have the sticky offset subtracted off of
the layer position and the other not (i.e. these two tests should exercise both
code branches, the one you added and the existing one).

[yigu, 2017-06-09 17:23:24]

Thanks for the suggestion! Patch updated. PTAL.

[flackr, 2017-06-09 17:33:33]

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

https://codereview.chromium.org/2929873002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:1131:
// keep the element stuck under compositor scrolling.
This comment is confusing because it seems like it's suggesting
offset_for_sticky_position is the layout offset of the unshifted sticky box.
Perhaps it would be better to say "Find and remove the offset applied for sticky
position if the compositor will shift the layer for sticky position to avoid
offsetting the layer twice."

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

https://codereview.chromium.org/2929873002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.h:373:
bool UsesCompositedStickyPosition() const;
Add a comment. This function is determining whether the compositor will be
responsible for applying the sticky position offset for this composited layer.

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

https://codereview.chromium.org/2929873002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMappingTest.cpp:1597:
" <div class='composited container'>"
Add this above too so the cases are identical other than the composited
scroller.

[yigu, 2017-06-09 17:41:16]

Thanks! Comments updated. PTAL.

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

https://codereview.chromium.org/2929873002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:1131:
// keep the element stuck under compositor scrolling.
On 2017/06/09 17:33:33, flackr wrote:
> This comment is confusing because it seems like it's suggesting
> offset_for_sticky_position is the layout offset of the unshifted sticky box.
> Perhaps it would be better to say "Find and remove the offset applied for
sticky
> position if the compositor will shift the layer for sticky position to avoid
> offsetting the layer twice."

Done.

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

https://codereview.chromium.org/2929873002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.h:373:
bool UsesCompositedStickyPosition() const;
On 2017/06/09 17:33:33, flackr wrote:
> Add a comment. This function is determining whether the compositor will be
> responsible for applying the sticky position offset for this composited layer.

Done.

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

https://codereview.chromium.org/2929873002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMappingTest.cpp:1597:
" <div class='composited container'>"
On 2017/06/09 17:33:33, flackr wrote:
> Add this above too so the cases are identical other than the composited
> scroller.

Done.

[flackr, 2017-06-09 17:49:52]

LGTM with nit

https://codereview.chromium.org/2929873002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.h
(right):

https://codereview.chromium.org/2929873002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.h:373:
// This function is determining whether the compositor will be responsible for
Sorry, I didn't mean that comment literally. For consistency this should be
written shorter and descriptively - see
https://google.github.io/styleguide/cppguide.html#Function_Comments for
examples.
i.e. Returns true if the compositor will...

[yigu, 2017-06-09 18:06:07]

Ah. Thanks!

https://codereview.chromium.org/2929873002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.h
(right):

https://codereview.chromium.org/2929873002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.h:373:
// This function is determining whether the compositor will be responsible for
On 2017/06/09 17:49:52, flackr wrote:
> Sorry, I didn't mean that comment literally. For consistency this should be
> written shorter and descriptively - see
> https://google.github.io/styleguide/cppguide.html#Function_Comments for
> examples.
> i.e. Returns true if the compositor will...

Done.

[chrishtr, 2017-06-09 18:42:45]

Description was changed from

==========
Shifting layer position for sticky element to avoid passing unnessary
variable to cc.

BUG=730655
TEST=CompositedLayerMappingTest.LayerPositionForStickyElement
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Shifting layer position for sticky element to avoid passing unnecessary
variable to cc.

BUG=730655
TEST=CompositedLayerMappingTest.LayerPositionForStickyElement
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[trchen, 2017-06-09 20:20:36]

lgtm except one nit.

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

https://codereview.chromium.org/2929873002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:297:
if (UsesCompositedStickyPosition()) {
nits: Prefer shorter branch first, early out first.

if (!UsesCompositedStickyPosition()) {
 
graphics_layer_->SetStickyPositionConstraint(WebLayerStickyPositionConstraint());
  return;
}

Then we can unident 50 lines of code. :)

[chrishtr, 2017-06-09 20:28:08]

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

https://codereview.chromium.org/2929873002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:297:
if (UsesCompositedStickyPosition()) {
Is this if statement just an optimization? If so, is it necessary? What if
we just always compute the constraints?

[yigu, 2017-06-09 20:52:39]

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

https://codereview.chromium.org/2929873002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:297:
if (UsesCompositedStickyPosition()) {
On 2017/06/09 20:28:07, chrishtr wrote:
> Is this if statement just an optimization? If so, is it necessary? What if
> we just always compute the constraints?

It seems that we call UpdateStickyConstraints unconditionally so we need this
check to exclude non-sticky elements. How about we do something like
if (UsesCompositedStickyPosition())
  UpdateStickyConstraints(...);
That way we don't need this condition within UpdateStickyConstraints any more.
It's just we don't SetStickyPositionConstraints for non-sticky element's
graphics_layer neither but I think that's OK? WDYT?

[chrishtr, 2017-06-09 20:58:30]

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

https://codereview.chromium.org/2929873002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:297:
if (UsesCompositedStickyPosition()) {
On 2017/06/09 at 20:52:39, yigu wrote:
> On 2017/06/09 20:28:07, chrishtr wrote:
> > Is this if statement just an optimization? If so, is it necessary? What if
> > we just always compute the constraints?
> 
> It seems that we call UpdateStickyConstraints unconditionally so we need this
check to exclude non-sticky elements. How about we do something like
> if (UsesCompositedStickyPosition())
>   UpdateStickyConstraints(...);
> That way we don't need this condition within UpdateStickyConstraints any more.
It's just we don't SetStickyPositionConstraints for non-sticky element's
graphics_layer neither but I think that's OK? WDYT?

I would like to get rid of the UsesCompositedStickyPosition() to reduce
complexity if perf
doesn't matter, so how about just checking if owning_layer_ has sticky
constraints at all?

[flackr, 2017-06-10 00:25:05]

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

https://codereview.chromium.org/2929873002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:297:
if (UsesCompositedStickyPosition()) {
On 2017/06/09 20:58:29, chrishtr wrote:
> On 2017/06/09 at 20:52:39, yigu wrote:
> > On 2017/06/09 20:28:07, chrishtr wrote:
> > > Is this if statement just an optimization? If so, is it necessary? What if
> > > we just always compute the constraints?
> > 
> > It seems that we call UpdateStickyConstraints unconditionally so we need
this
> check to exclude non-sticky elements. How about we do something like
> > if (UsesCompositedStickyPosition())
> >   UpdateStickyConstraints(...);
> > That way we don't need this condition within UpdateStickyConstraints any
more.
> It's just we don't SetStickyPositionConstraints for non-sticky element's
> graphics_layer neither but I think that's OK? WDYT?
> 
> I would like to get rid of the UsesCompositedStickyPosition() to reduce
> complexity if perf
> doesn't matter, so how about just checking if owning_layer_ has sticky
> constraints at all?

We would still have to check UsesCompositedStickyPosition() in
UpdateMainGraphicsLayerGeometry in case the ancestor scroller is not composited,
because if the scroller isn't known about on the compositor it can't do the
sticky offset correctly. Even worse, if we set constraints for something with a
non composited scroller I think on the compositor we'll just use the next
scroller up because that will be the nearest ancestor scroller the compositor
knows about. And now that we don't pass the main thread offset over we won't
know that the element has already been offset for sticky position.

[chrishtr, 2017-06-10 00:29:16]

On 2017/06/10 at 00:25:05, flackr wrote:
>
https://codereview.chromium.org/2929873002/diff/80001/third_party/WebKit/Sour...
> File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
(right):
> 
>
https://codereview.chromium.org/2929873002/diff/80001/third_party/WebKit/Sour...
>
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:297:
if (UsesCompositedStickyPosition()) {
> On 2017/06/09 20:58:29, chrishtr wrote:
> > On 2017/06/09 at 20:52:39, yigu wrote:
> > > On 2017/06/09 20:28:07, chrishtr wrote:
> > > > Is this if statement just an optimization? If so, is it necessary? What
if
> > > > we just always compute the constraints?
> > > 
> > > It seems that we call UpdateStickyConstraints unconditionally so we need
this
> > check to exclude non-sticky elements. How about we do something like
> > > if (UsesCompositedStickyPosition())
> > >   UpdateStickyConstraints(...);
> > > That way we don't need this condition within UpdateStickyConstraints any
more.
> > It's just we don't SetStickyPositionConstraints for non-sticky element's
> > graphics_layer neither but I think that's OK? WDYT?
> > 
> > I would like to get rid of the UsesCompositedStickyPosition() to reduce
> > complexity if perf
> > doesn't matter, so how about just checking if owning_layer_ has sticky
> > constraints at all?
> 
> We would still have to check UsesCompositedStickyPosition() in
UpdateMainGraphicsLayerGeometry in case the ancestor scroller is not composited,
because if the scroller isn't known about on the compositor it can't do the
sticky offset correctly. Even worse, if we set constraints for something with a
non composited scroller I think on the compositor we'll just use the next
scroller up because that will be the nearest ancestor scroller the compositor
knows about. And now that we don't pass the main thread offset over we won't
know that the element has already been offset for sticky position.

Is that because the presence of the sticky constraints on the compositor is a
trigger for determining whether to do cc-side sticky adjustments?
If so that's a good reason to keep the code as-is.

[flackr, 2017-06-10 00:31:40]

On 2017/06/10 00:29:16, chrishtr wrote:
> On 2017/06/10 at 00:25:05, flackr wrote:
> >
>
https://codereview.chromium.org/2929873002/diff/80001/third_party/WebKit/Sour...
> > File
> third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
> (right):
> > 
> >
>
https://codereview.chromium.org/2929873002/diff/80001/third_party/WebKit/Sour...
> >
>
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:297:
> if (UsesCompositedStickyPosition()) {
> > On 2017/06/09 20:58:29, chrishtr wrote:
> > > On 2017/06/09 at 20:52:39, yigu wrote:
> > > > On 2017/06/09 20:28:07, chrishtr wrote:
> > > > > Is this if statement just an optimization? If so, is it necessary?
What
> if
> > > > > we just always compute the constraints?
> > > > 
> > > > It seems that we call UpdateStickyConstraints unconditionally so we need
> this
> > > check to exclude non-sticky elements. How about we do something like
> > > > if (UsesCompositedStickyPosition())
> > > >   UpdateStickyConstraints(...);
> > > > That way we don't need this condition within UpdateStickyConstraints any
> more.
> > > It's just we don't SetStickyPositionConstraints for non-sticky element's
> > > graphics_layer neither but I think that's OK? WDYT?
> > > 
> > > I would like to get rid of the UsesCompositedStickyPosition() to reduce
> > > complexity if perf
> > > doesn't matter, so how about just checking if owning_layer_ has sticky
> > > constraints at all?
> > 
> > We would still have to check UsesCompositedStickyPosition() in
> UpdateMainGraphicsLayerGeometry in case the ancestor scroller is not
composited,
> because if the scroller isn't known about on the compositor it can't do the
> sticky offset correctly. Even worse, if we set constraints for something with
a
> non composited scroller I think on the compositor we'll just use the next
> scroller up because that will be the nearest ancestor scroller the compositor
> knows about. And now that we don't pass the main thread offset over we won't
> know that the element has already been offset for sticky position.
> 
> Is that because the presence of the sticky constraints on the compositor is a
> trigger for determining whether to do cc-side sticky adjustments?
> If so that's a good reason to keep the code as-is.

Right, if we set sticky constraints we will do cc-side sticky positioning. But
that will only be correct if the ancestorOverflowLayer is composited because we
just use the nearest scroller cc is aware of.

[chrishtr, 2017-06-10 00:34:19]

lgtm

LGTM as-is then.

[yigu, 2017-06-10 01:28:14]

The CQ bit was checked by yigu@chromium.org

[yigu, 2017-06-10 01:28:14]

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

[commit-bot: I haz the power, 2017-06-10 01:28:33]

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-10 04:05:37]

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

[commit-bot: I haz the power, 2017-06-10 04:05:38]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[yigu, 2017-06-12 14:59:53]

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

[commit-bot: I haz the power, 2017-06-12 15:00:08]

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

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

[commit-bot: I haz the power, 2017-06-12 16:28:13]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[yigu, 2017-06-13 12:30:05]

The CQ bit was checked by yigu@chromium.org

[commit-bot: I haz the power, 2017-06-13 12:30:21]

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-13 12:35:09]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1497357005175380,
"parent_rev": "0492fc281b7eb603be6e7f3f00afb7d9802ef3d5", "commit_rev":
"23b68f6ceca08d1d213d4af869ba8336c9aa597a"}

[commit-bot: I haz the power, 2017-06-13 12:35:26]

Description was changed from

==========
Shifting layer position for sticky element to avoid passing unnecessary
variable to cc.

BUG=730655
TEST=CompositedLayerMappingTest.LayerPositionForStickyElement
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Shifting layer position for sticky element to avoid passing unnecessary
variable to cc.

BUG=730655
TEST=CompositedLayerMappingTest.LayerPositionForStickyElement
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/2929873002
Cr-Commit-Position: refs/heads/master@{#478983}
Committed:
https://chromium.googlesource.com/chromium/src/+/23b68f6ceca08d1d213d4af869ba...
==========

[commit-bot: I haz the power, 2017-06-13 12:35:29]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/23b68f6ceca08d1d213d4af869ba...