Correct for enclosing layers scroll position in sticky_data->main_thread_offset

Correct for enclosing layers scroll position in parentRelativeStickyBoxOffset

The offset calculated in CompositedLayerMapping for
parent_relative_sticky_box_offset calls |convertToLayerCoords| on the
enclosing layer. This method relies on location() however which includes
the scroll offset if the enclosing layer is not already the scroll
ancestor, e.g. if you had the following layer setup:

ScrollAncestor
|
+-- IntermediateLayer
    |
    +-- StickyLayer

To correct for this, add back in the scroll offset for the case where
the enclosing layer is not the scroll ancestor.

BUG=692263
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/2698843004
Cr-Commit-Position: refs/heads/master@{#454444}
Committed: https://chromium.googlesource.com/chromium/src/+/e4819386ba4f91ce923c8c870927480566624001

[smcgruer, 2017-02-16 21:38:47]

Description was changed from

==========
Correct for enclosing layers scroll position in sticky_data->main_thread_offset

The offset calculated in CompositedLayerMapping for
parent_relative_sticky_box_offset calls |convertToLayerCoords| on the
enclosing layer. This method relies on location() however which includes
the scroll offset if the enclosing layer is not already the scroll
ancestor, e.g. if you had the following layer setup:

ScrollAncestor
|
+-- IntermediateLayer
    |
    +-- StickyLayer

To correct for this, add back in the scroll offset for the case where
the enclosing layer is not the scroll ancestor.

BUG=692263
==========

to

==========
Correct for enclosing layers scroll position in sticky_data->main_thread_offset

The offset calculated in CompositedLayerMapping for
parent_relative_sticky_box_offset calls |convertToLayerCoords| on the
enclosing layer. This method relies on location() however which includes
the scroll offset if the enclosing layer is not already the scroll
ancestor, e.g. if you had the following layer setup:

ScrollAncestor
|
+-- IntermediateLayer
    |
    +-- StickyLayer

To correct for this, add back in the scroll offset for the case where
the enclosing layer is not the scroll ancestor.

BUG=692263
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[smcgruer, 2017-02-16 21:41:08]

smcgruer@chromium.org changed reviewers:
+ ajuma@chromium.org, flackr@chromium.org

[flackr, 2017-02-16 22:23:59]

https://codereview.chromium.org/2698843004/diff/1/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/2698843004/diff/1/cc/layers/layer.cc#newcode605
cc/layers/layer.cc:605: // We are a scroller, so we need our parent's
scroll_ancestor.
Can't we always get the node from the parent?

https://codereview.chromium.org/2698843004/diff/1/cc/layers/layer.cc#newcode610
cc/layers/layer.cc:610: sticky_box_offset -= gfx::ScrollOffsetToFlooredVector2d(
Do we know that this floored scroll offset matches the main thread offset? There
might be a similar bug to https://crbug.com/681951 here.

I wonder if it might be better to adjust what we send for the parent relative
sticky box offset on the blink side since we will have consistent scroll
snapping there.

[smcgruer, 2017-02-17 13:55:31]

https://codereview.chromium.org/2698843004/diff/1/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/2698843004/diff/1/cc/layers/layer.cc#newcode605
cc/layers/layer.cc:605: // We are a scroller, so we need our parent's
scroll_ancestor.
On 2017/02/16 22:23:59, flackr wrote:
> Can't we always get the node from the parent?

Yes, of course. Done.

https://codereview.chromium.org/2698843004/diff/1/cc/layers/layer.cc#newcode610
cc/layers/layer.cc:610: sticky_box_offset -= gfx::ScrollOffsetToFlooredVector2d(
On 2017/02/16 22:23:59, flackr wrote:
> Do we know that this floored scroll offset matches the main thread offset?
There
> might be a similar bug to https://crbug.com/681951 here.
> 
> I wonder if it might be better to adjust what we send for the parent relative
> sticky box offset on the blink side since we will have consistent scroll
> snapping there.

So the |enclosingLayerOffset| that is calculated blink-side is a LayoutPoint
which is an integer (as is location()). It is added to |stickyBoxOffset| which
is a FloatPoint, but the float here comes from the fact that
scrollContainerRelativeStickyBoxRect() is a FloatRect.

I think we could try to do it blink side if it would feel better, but the offset
from main for scroll (via location()) is coming in as an integer. (FYI the
scroll offset in blink-side updateLayerPosition is
"containingLayer->layoutBox()->scrolledContentOffset();" which is an IntPoint
too.)

[flackr, 2017-02-21 00:41:30]

https://codereview.chromium.org/2698843004/diff/1/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/2698843004/diff/1/cc/layers/layer.cc#newcode610
cc/layers/layer.cc:610: sticky_box_offset -= gfx::ScrollOffsetToFlooredVector2d(
On 2017/02/17 13:55:31, smcgruer wrote:
> On 2017/02/16 22:23:59, flackr wrote:
> > Do we know that this floored scroll offset matches the main thread offset?
> There
> > might be a similar bug to https://crbug.com/681951 here.
> > 
> > I wonder if it might be better to adjust what we send for the parent
relative
> > sticky box offset on the blink side since we will have consistent scroll
> > snapping there.
> 
> So the |enclosingLayerOffset| that is calculated blink-side is a LayoutPoint
> which is an integer (as is location()). It is added to |stickyBoxOffset| which
> is a FloatPoint, but the float here comes from the fact that
> scrollContainerRelativeStickyBoxRect() is a FloatRect.
> 
> I think we could try to do it blink side if it would feel better, but the
offset
> from main for scroll (via location()) is coming in as an integer. (FYI the
> scroll offset in blink-side updateLayerPosition is
> "containingLayer->layoutBox()->scrolledContentOffset();" which is an IntPoint
> too.)

This definitely belongs blink side. If there are cases where blink and cc
disagree about the scroll position (i.e. https://crbug.com/681951) we want to
use the one that blink is using when it sets the layer position of the sticky
layer.

[smcgruer, 2017-02-21 19:16:20]

Description was changed from

==========
Correct for enclosing layers scroll position in sticky_data->main_thread_offset

The offset calculated in CompositedLayerMapping for
parent_relative_sticky_box_offset calls |convertToLayerCoords| on the
enclosing layer. This method relies on location() however which includes
the scroll offset if the enclosing layer is not already the scroll
ancestor, e.g. if you had the following layer setup:

ScrollAncestor
|
+-- IntermediateLayer
    |
    +-- StickyLayer

To correct for this, add back in the scroll offset for the case where
the enclosing layer is not the scroll ancestor.

BUG=692263
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Correct for enclosing layers scroll position in parentRelativeStickyBoxOffset

The offset calculated in CompositedLayerMapping for
parent_relative_sticky_box_offset calls |convertToLayerCoords| on the
enclosing layer. This method relies on location() however which includes
the scroll offset if the enclosing layer is not already the scroll
ancestor, e.g. if you had the following layer setup:

ScrollAncestor
|
+-- IntermediateLayer
    |
    +-- StickyLayer

To correct for this, add back in the scroll offset for the case where
the enclosing layer is not the scroll ancestor.

BUG=692263
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[smcgruer, 2017-02-21 19:17:22]

https://codereview.chromium.org/2698843004/diff/1/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/2698843004/diff/1/cc/layers/layer.cc#newcode610
cc/layers/layer.cc:610: sticky_box_offset -= gfx::ScrollOffsetToFlooredVector2d(
On 2017/02/21 00:41:30, flackr wrote:
> On 2017/02/17 13:55:31, smcgruer wrote:
> > On 2017/02/16 22:23:59, flackr wrote:
> > > Do we know that this floored scroll offset matches the main thread offset?
> > There
> > > might be a similar bug to https://crbug.com/681951 here.
> > > 
> > > I wonder if it might be better to adjust what we send for the parent
> relative
> > > sticky box offset on the blink side since we will have consistent scroll
> > > snapping there.
> > 
> > So the |enclosingLayerOffset| that is calculated blink-side is a LayoutPoint
> > which is an integer (as is location()). It is added to |stickyBoxOffset|
which
> > is a FloatPoint, but the float here comes from the fact that
> > scrollContainerRelativeStickyBoxRect() is a FloatRect.
> > 
> > I think we could try to do it blink side if it would feel better, but the
> offset
> > from main for scroll (via location()) is coming in as an integer. (FYI the
> > scroll offset in blink-side updateLayerPosition is
> > "containingLayer->layoutBox()->scrolledContentOffset();" which is an
IntPoint
> > too.)
> 
> This definitely belongs blink side. If there are cases where blink and cc
> disagree about the scroll position (i.e. https://crbug.com/681951) we want to
> use the one that blink is using when it sets the layer position of the sticky
> layer.

Done.

[flackr, 2017-02-22 14:23:09]

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

https://codereview.chromium.org/2698843004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:322:
->scrolledContentOffset();
scrolledContentOffset requires that the box has an overflow clip but what if the
containing layer is not a scroller? Is this required if the enclosing layer is a
descendant of the scroller (in which case we can use ancestorOverflowLayer's
scroll offset) or just a child?

[smcgruer, 2017-02-22 15:42:31]

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

https://codereview.chromium.org/2698843004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:322:
->scrolledContentOffset();
On 2017/02/22 14:23:09, flackr wrote:
> scrolledContentOffset requires that the box has an overflow clip but what if
the
> containing layer is not a scroller? Is this required if the enclosing layer is
a
> descendant of the scroller (in which case we can use ancestorOverflowLayer's
> scroll offset) or just a child?

Done.

[flackr, 2017-02-22 15:45:18]

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

https://codereview.chromium.org/2698843004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:319:
enclosingLayer->containingLayer()) {
Why the check for containingLayer? If it's not the ancestorOverflowLayer it will
for sure have a containing layer right?

[smcgruer, 2017-02-22 15:58:23]

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

https://codereview.chromium.org/2698843004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:319:
enclosingLayer->containingLayer()) {
Ah, yes, no longer needed.

[flackr, 2017-02-22 16:14:49]

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

https://codereview.chromium.org/2698843004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMappingTest.cpp:1728:
document().view()->updateLifecycleToCompositingCleanPlusScrolling();
Does this actually call updateStickyConstraints? We'd need to do something to
dirty them wouldn't we?

[smcgruer, 2017-02-22 16:54:40]

Description was changed from

==========
Correct for enclosing layers scroll position in parentRelativeStickyBoxOffset

The offset calculated in CompositedLayerMapping for
parent_relative_sticky_box_offset calls |convertToLayerCoords| on the
enclosing layer. This method relies on location() however which includes
the scroll offset if the enclosing layer is not already the scroll
ancestor, e.g. if you had the following layer setup:

ScrollAncestor
|
+-- IntermediateLayer
    |
    +-- StickyLayer

To correct for this, add back in the scroll offset for the case where
the enclosing layer is not the scroll ancestor.

BUG=692263
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Correct for enclosing layers scroll position in parentRelativeStickyBoxOffset

The offset calculated in CompositedLayerMapping for
parent_relative_sticky_box_offset calls |convertToLayerCoords| on the
enclosing layer. This method relies on location() however which includes
the scroll offset if the enclosing layer is not already the scroll
ancestor, e.g. if you had the following layer setup:

ScrollAncestor
|
+-- IntermediateLayer
    |
    +-- StickyLayer

To correct for this, add back in the scroll offset for the case where
the enclosing layer is not the scroll ancestor.

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

[smcgruer, 2017-02-22 18:39:04]

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

https://codereview.chromium.org/2698843004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMappingTest.cpp:1728:
document().view()->updateLifecycleToCompositingCleanPlusScrolling();
On 2017/02/22 16:14:49, flackr wrote:
> Does this actually call updateStickyConstraints? We'd need to do something to
> dirty them wouldn't we?

It appears to be:

[133571:133571:0222/133838.647623:3805035727007:INFO:CompositedLayerMappingTest.cpp(1728)]
Calling update
[133571:133571:0222/133838.647658:3805035727042:INFO:CompositedLayerMapping.cpp(295)]
CompositedLayerMapping::updateStickyConstraints for "LayoutBlockFlow DIV
id='scroller' class='composited'"
[133571:133571:0222/133838.647693:3805035727077:INFO:CompositedLayerMapping.cpp(295)]
CompositedLayerMapping::updateStickyConstraints for "LayoutBlockFlow DIV
class='composited container'"
[133571:133571:0222/133838.647718:3805035727101:INFO:CompositedLayerMapping.cpp(295)]
CompositedLayerMapping::updateStickyConstraints for "LayoutBlockFlow DIV
class='composited container'"
[133571:133571:0222/133838.647740:3805035727123:INFO:CompositedLayerMapping.cpp(295)]
CompositedLayerMapping::updateStickyConstraints for "LayoutBlockFlow (sticky
positioned) DIV id='sticky' class='composited'"
[133571:133571:0222/133838.647764:3805035727148:INFO:CompositedLayerMappingTest.cpp(1730)]
Returning from call to update

[flackr, 2017-02-22 19:53:21]

On 2017/02/22 18:39:04, smcgruer wrote:
>
https://codereview.chromium.org/2698843004/diff/80001/third_party/WebKit/Sour...
> File
>
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMappingTest.cpp
> (right):
> 
>
https://codereview.chromium.org/2698843004/diff/80001/third_party/WebKit/Sour...
>
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMappingTest.cpp:1728:
> document().view()->updateLifecycleToCompositingCleanPlusScrolling();
> On 2017/02/22 16:14:49, flackr wrote:
> > Does this actually call updateStickyConstraints? We'd need to do something
to
> > dirty them wouldn't we?
> 
> It appears to be:
> 
>
[133571:133571:0222/133838.647623:3805035727007:INFO:CompositedLayerMappingTest.cpp(1728)]
> Calling update
>
[133571:133571:0222/133838.647658:3805035727042:INFO:CompositedLayerMapping.cpp(295)]
> CompositedLayerMapping::updateStickyConstraints for "LayoutBlockFlow DIV
> id='scroller' class='composited'"
>
[133571:133571:0222/133838.647693:3805035727077:INFO:CompositedLayerMapping.cpp(295)]
> CompositedLayerMapping::updateStickyConstraints for "LayoutBlockFlow DIV
> class='composited container'"
>
[133571:133571:0222/133838.647718:3805035727101:INFO:CompositedLayerMapping.cpp(295)]
> CompositedLayerMapping::updateStickyConstraints for "LayoutBlockFlow DIV
> class='composited container'"
>
[133571:133571:0222/133838.647740:3805035727123:INFO:CompositedLayerMapping.cpp(295)]
> CompositedLayerMapping::updateStickyConstraints for "LayoutBlockFlow (sticky
> positioned) DIV id='sticky' class='composited'"
>
[133571:133571:0222/133838.647764:3805035727148:INFO:CompositedLayerMappingTest.cpp(1730)]
> Returning from call to update

I guess my question is why do we update the sticky constraints after a scroll? I
think we should just be setting the layer positions unless style / layout
changes.

[smcgruer, 2017-02-22 20:38:09]

> I guess my question is why do we update the sticky constraints after a scroll?
I
> think we should just be setting the layer positions unless style / layout
> changes.

After discussion: we probably shouldn't, but currently do. Will file a bug to
fix that, but also have forced compositing inputs dirty in the test to make sure
its testing what we think its testing.

[flackr, 2017-02-22 20:44:00]

LGTM

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

https://codereview.chromium.org/2698843004/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:322:
enclosingLayerOffset += LayoutSize(enclosingLayer->ancestorOverflowLayer()
nit: use ancestorOverflowLayer (from line 296) here and above

[smcgruer, 2017-02-22 20:48:18]

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

https://codereview.chromium.org/2698843004/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:322:
enclosingLayerOffset += LayoutSize(enclosingLayer->ancestorOverflowLayer()
On 2017/02/22 20:44:00, flackr wrote:
> nit: use ancestorOverflowLayer (from line 296) here and above

Done.

[smcgruer, 2017-02-22 20:49:05]

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

[smcgruer, 2017-02-22 20:49:05]

-ajuma since we moved to blink side

+chrishtr for blink OWNERS

[smcgruer, 2017-02-23 20:04:06]

smcgruer@chromium.org changed reviewers:
+ pdr@google.com

[smcgruer, 2017-02-23 20:04:27]

Description was changed from

==========
Correct for enclosing layers scroll position in parentRelativeStickyBoxOffset

The offset calculated in CompositedLayerMapping for
parent_relative_sticky_box_offset calls |convertToLayerCoords| on the
enclosing layer. This method relies on location() however which includes
the scroll offset if the enclosing layer is not already the scroll
ancestor, e.g. if you had the following layer setup:

ScrollAncestor
|
+-- IntermediateLayer
    |
    +-- StickyLayer

To correct for this, add back in the scroll offset for the case where
the enclosing layer is not the scroll ancestor.

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

to

==========
Correct for enclosing layers scroll position in parentRelativeStickyBoxOffset

The offset calculated in CompositedLayerMapping for
parent_relative_sticky_box_offset calls |convertToLayerCoords| on the
enclosing layer. This method relies on location() however which includes
the scroll offset if the enclosing layer is not already the scroll
ancestor, e.g. if you had the following layer setup:

ScrollAncestor
|
+-- IntermediateLayer
    |
    +-- StickyLayer

To correct for this, add back in the scroll offset for the case where
the enclosing layer is not the scroll ancestor.

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

[smcgruer, 2017-02-23 20:04:27]

smcgruer@chromium.org changed reviewers:
+ pdr@chromium.org
- pdr@google.com

[smcgruer, 2017-02-28 13:48:33]

smcgruer@chromium.org changed reviewers:
+ schenney@chromium.org

[smcgruer, 2017-02-28 13:48:34]

Ping for Blink OWNERS :)

[smcgruer, 2017-02-28 18:38:15]

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

[commit-bot: I haz the power, 2017-02-28 18:38:47]

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

[chrishtr, 2017-02-28 19:36:28]

https://codereview.chromium.org/2698843004/diff/140001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/compositing/overflow/composited-sticky-element-enclosing-layers.html
(right):

https://codereview.chromium.org/2698843004/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/compositing/overflow/composited-sticky-element-enclosing-layers.html:32:
<div class="composited container">
Why have three nested "composited container" elements?

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

https://codereview.chromium.org/2698843004/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:315:
// adjusted to include the scroll offset to keep it relative.
"relative to compositingContainer"

https://codereview.chromium.org/2698843004/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:317:
m_owningLayer.enclosingLayerWithCompositedLayerMapping(ExcludeSelf);
Please pass compositingContainer to updateStickyConstraints
(it's available in the callsite).

Also, please make updateStickyConstraints a private member
of CompositedLayerMapping.

[commit-bot: I haz the power, 2017-02-28 21:10:47]

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

[commit-bot: I haz the power, 2017-02-28 21:10:48]

Dry run: This issue passed the CQ dry run.

[smcgruer, 2017-03-01 19:58:40]

https://codereview.chromium.org/2698843004/diff/140001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/compositing/overflow/composited-sticky-element-enclosing-layers.html
(right):

https://codereview.chromium.org/2698843004/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/compositing/overflow/composited-sticky-element-enclosing-layers.html:32:
<div class="composited container">
On 2017/02/28 19:36:28, chrishtr wrote:
> Why have three nested "composited container" elements?

No real reason. Makes me feel better that it still works correctly in the
presence of multiple intermediate layers, but only one is required. Removed the
rest.

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

https://codereview.chromium.org/2698843004/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:315:
// adjusted to include the scroll offset to keep it relative.
On 2017/02/28 19:36:28, chrishtr wrote:
> "relative to compositingContainer"

Done.

https://codereview.chromium.org/2698843004/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:317:
m_owningLayer.enclosingLayerWithCompositedLayerMapping(ExcludeSelf);
On 2017/02/28 19:36:28, chrishtr wrote:
> Please pass compositingContainer to updateStickyConstraints
> (it's available in the callsite).
> 
> Also, please make updateStickyConstraints a private member
> of CompositedLayerMapping.

Done.

[chrishtr, 2017-03-02 22:33:12]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2017-03-02 22:33:13]

lgtm

[chrishtr, 2017-03-02 22:33:13]

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

[commit-bot: I haz the power, 2017-03-02 22:33:51]

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-03-03 00:04:20]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1488493992475350,
"parent_rev": "abcf7eeec655ec47740a7676a4c61e164e69189d", "commit_rev":
"e4819386ba4f91ce923c8c870927480566624001"}

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

Description was changed from

==========
Correct for enclosing layers scroll position in parentRelativeStickyBoxOffset

The offset calculated in CompositedLayerMapping for
parent_relative_sticky_box_offset calls |convertToLayerCoords| on the
enclosing layer. This method relies on location() however which includes
the scroll offset if the enclosing layer is not already the scroll
ancestor, e.g. if you had the following layer setup:

ScrollAncestor
|
+-- IntermediateLayer
    |
    +-- StickyLayer

To correct for this, add back in the scroll offset for the case where
the enclosing layer is not the scroll ancestor.

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

to

==========
Correct for enclosing layers scroll position in parentRelativeStickyBoxOffset

The offset calculated in CompositedLayerMapping for
parent_relative_sticky_box_offset calls |convertToLayerCoords| on the
enclosing layer. This method relies on location() however which includes
the scroll offset if the enclosing layer is not already the scroll
ancestor, e.g. if you had the following layer setup:

ScrollAncestor
|
+-- IntermediateLayer
    |
    +-- StickyLayer

To correct for this, add back in the scroll offset for the case where
the enclosing layer is not the scroll ancestor.

BUG=692263
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/2698843004
Cr-Commit-Position: refs/heads/master@{#454444}
Committed:
https://chromium.googlesource.com/chromium/src/+/e4819386ba4f91ce923c8c870927...
==========

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

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/e4819386ba4f91ce923c8c870927...