Implement PaintLayer::scrollsWithViewport for SPv2.

Implement PaintLayer::scrollsWithViewport for SPv2.

Also removes a call to
ScrollingCoordinator::updateAfterCompositingChangeIfNeeded that is
SPv2-incompatible, and add force lifecycle updates for two cases of querying
scrolling state via web editingselection APIs.

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

Committed: https://crrev.com/1cf98b57647fa75d2aa18a94ba2128f1dc770386
Cr-Commit-Position: refs/heads/master@{#437051}

[chrishtr, 2016-12-06 01:16:41]

Description was changed from

==========
none


none

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

to

==========
none


none

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

[chrishtr, 2016-12-06 01:17:36]

Description was changed from

==========
none


none

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

to

==========
Implement PaintLayer::scrollsWithViewport for SPv2.

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

[chrishtr, 2016-12-06 01:18:16]

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

[chrishtr, 2016-12-06 01:18:21]

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

[commit-bot: I haz the power, 2016-12-06 01:18:44]

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, 2016-12-06 02:52:31]

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

[commit-bot: I haz the power, 2016-12-06 02:52:33]

Dry run: This issue passed the CQ dry run.

[pdr., 2016-12-06 04:38:51]

https://codereview.chromium.org/2549353002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/paint/PaintLayer.cpp (right):

https://codereview.chromium.org/2549353002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PaintLayer.cpp:342: if
(layoutObject()->style()->position() == FixedPosition &&
Can you move the spv2 codepath outside like so?
if (RuntimeEnabledFeatures::slimmingPaintInvalidationEnabled()) {
  DCHECK(layoutObject()->paintProperties());
  return layoutObject()
              ->paintProperties()
              ->localBorderBoxProperties()
              ->propertyTreeState.scroll() ==
          layoutObject()
              ->view()
              ->paintProperties()
              ->contentsProperties()
              .propertyTreeState.scroll();
} else {
  return (layoutObject()->style()->position() == FixedPosition &&
          layoutObject()->containerForFixedPosition() ==
              layoutObject()->view()) ||
          (layoutObject()->style()->position() == StickyPosition &&
          !ancestorScrollingLayer());
}

https://codereview.chromium.org/2549353002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/paint/PaintLayerTest.cpp (right):

https://codereview.chromium.org/2549353002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PaintLayerTest.cpp:105: "<div
style='transform: translateZ(0)'>"
This isn't correct for spv2 because the viewport doesn't actually scroll. Can
you use the following where the fixed scroller is the transformed element?
"<div style='transform: translateZ(0); height: 10px; overflow: scroll;'>"
"  <div id='target' style='position: fixed;'></div>"
"  <div class='forcescroll' style='height: 5555px;'></div>"
"</div>"
"<div class='forcescroll' style='height: 5555px;'></div>"

Similarly for the other tests, please add "<div class='forcescroll'
style='height: 5555px;'></div>".

https://codereview.chromium.org/2549353002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PaintLayerTest.cpp:137:
TEST_P(PaintLayerTest, CompositedScrollingNoNeedsRepaint) {
Nit: Can you file a bug for this and include it here (similarly, for the todo in
CompositedBoundsAbsPosGrandchild)?
// TODO(chrishtr, crbug.com/90210): Despline the spv2 frobinator in the b-tree
hive.

Nit²: CompositedScrollingNoNeedsRepaint -> CompositedScrollingDoesNotNeedRepaint

[chrishtr, 2016-12-06 19:29:23]

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

[commit-bot: I haz the power, 2016-12-06 19:29:45]

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, 2016-12-06 19:33:07]

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

[commit-bot: I haz the power, 2016-12-06 19:33:08]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[chrishtr, 2016-12-06 19:59:08]

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

[commit-bot: I haz the power, 2016-12-06 19:59:36]

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, 2016-12-06 21:07:18]

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

[commit-bot: I haz the power, 2016-12-06 21:07:19]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[chrishtr, 2016-12-06 23:56:07]

https://codereview.chromium.org/2549353002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/paint/PaintLayer.cpp (right):

https://codereview.chromium.org/2549353002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PaintLayer.cpp:342: if
(layoutObject()->style()->position() == FixedPosition &&
On 2016/12/06 at 04:38:51, pdr. wrote:
> Can you move the spv2 codepath outside like so?
> if (RuntimeEnabledFeatures::slimmingPaintInvalidationEnabled()) {
>   DCHECK(layoutObject()->paintProperties());
>   return layoutObject()
>               ->paintProperties()
>               ->localBorderBoxProperties()
>               ->propertyTreeState.scroll() ==
>           layoutObject()
>               ->view()
>               ->paintProperties()
>               ->contentsProperties()
>               .propertyTreeState.scroll();
> } else {
>   return (layoutObject()->style()->position() == FixedPosition &&
>           layoutObject()->containerForFixedPosition() ==
>               layoutObject()->view()) ||
>           (layoutObject()->style()->position() == StickyPosition &&
>           !ancestorScrollingLayer());
> }

This isn't quite right, because the method should return whether the PaintLayer
sticks to the viewport of its LayoutView.

https://codereview.chromium.org/2549353002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/paint/PaintLayerTest.cpp (right):

https://codereview.chromium.org/2549353002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PaintLayerTest.cpp:105: "<div
style='transform: translateZ(0)'>"
On 2016/12/06 at 04:38:51, pdr. wrote:
> This isn't correct for spv2 because the viewport doesn't actually scroll. Can
you use the following where the fixed scroller is the transformed element?
> "<div style='transform: translateZ(0); height: 10px; overflow: scroll;'>"
> "  <div id='target' style='position: fixed;'></div>"
> "  <div class='forcescroll' style='height: 5555px;'></div>"
> "</div>"
> "<div class='forcescroll' style='height: 5555px;'></div>"
> 
> Similarly for the other tests, please add "<div class='forcescroll'
style='height: 5555px;'></div>".

Added here, and also added another test without scrolling.

https://codereview.chromium.org/2549353002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2549353002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:2874: if
(!RuntimeEnabledFeatures::slimmingPaintV2Enabled()) {
This code is not needed for SPv2, and tries to call sticksToViewport at an
illegal lifecycle phase.

https://codereview.chromium.org/2549353002/diff/120001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/FlagExpectations/enable-slimming-paint-v2
(right):

https://codereview.chromium.org/2549353002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/FlagExpectations/enable-slimming-paint-v2:249:
crbug.com/671838
compositing/layer-creation/iframe-background-attachment-fixed.html [ Failure ]
This was an existing bug exposed by removing SPv2-incompatible scrolling update
code in FrameView.

https://codereview.chromium.org/2549353002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/FlagExpectations/enable-slimming-paint-v2:320:
crbug.com/667946 compositing/overflow/scrolls-with-respect-to-nested.html [
Failure ]
These are broken because viewport scrolling in general is not yet working for
SPv2,
so we can't detect that a fixed-position element doesn't scroll w.r.t. the
viewport.

[chrishtr, 2016-12-06 23:56:12]

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

[commit-bot: I haz the power, 2016-12-06 23:56:55]

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

[chrishtr, 2016-12-06 23:57:14]

Description was changed from

==========
Implement PaintLayer::scrollsWithViewport for SPv2.

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

to

==========
Implement PaintLayer::scrollsWithViewport for SPv2.

Also removes a call to
ScrollingCoordinator::updateAfterCompositingChangeIfNeeded that is
SPv2-incompatible, and add force lifecycle updates for two cases of querying
scrolling state via web editingselection APIs.

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

[pdr., 2016-12-07 00:02:05]

LGTM

https://codereview.chromium.org/2549353002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintLayer.cpp (right):

https://codereview.chromium.org/2549353002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintLayer.cpp:372:
(!ancestorScrollingLayer() || ancestorScrollingLayer() == root()));
Is the new "ancestorScrollingLayer() == root()" check fixing a bug?

https://codereview.chromium.org/2549353002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintLayerTest.cpp (right):

https://codereview.chromium.org/2549353002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintLayerTest.cpp:38: // TODO(chrishtr):
fix this test for SPv2
Can you file a bug for this?

[commit-bot: I haz the power, 2016-12-07 00:04:24]

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

[commit-bot: I haz the power, 2016-12-07 00:04:25]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[chrishtr, 2016-12-07 01:00:34]

https://codereview.chromium.org/2549353002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintLayer.cpp (right):

https://codereview.chromium.org/2549353002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintLayer.cpp:372:
(!ancestorScrollingLayer() || ancestorScrollingLayer() == root()));
On 2016/12/07 at 00:02:05, pdr. wrote:
> Is the new "ancestorScrollingLayer() == root()" check fixing a bug?

Yes. When root layer scrolling is on, the LayoutView is the scroller, instead
of FrameView. ancestorScrollingLayer() == root() is to handle that.

https://codereview.chromium.org/2549353002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintLayerTest.cpp (right):

https://codereview.chromium.org/2549353002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintLayerTest.cpp:38: // TODO(chrishtr):
fix this test for SPv2
On 2016/12/07 at 00:02:05, pdr. wrote:
> Can you file a bug for this?

Done. crbug.com/671864

[chrishtr, 2016-12-07 18:07:19]

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

https://codereview.chromium.org/2549353002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:679:
containerForFixedPosition() == view()) {
This one was pretty easy to adjust to not call sticksToViewport, so I just
changed it. This method is called during layout, so it's not ok to update
all lifecycle phases.

[chrishtr, 2016-12-07 18:07:24]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2016-12-07 18:07:25]

The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org
Link to the patchset: https://codereview.chromium.org/2549353002/#ps140001
(title: "none")

[commit-bot: I haz the power, 2016-12-07 18:07: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, 2016-12-07 18:53:22]

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

[commit-bot: I haz the power, 2016-12-07 18:53:23]

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

[chrishtr, 2016-12-07 19:13:21]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2016-12-07 19:13:21]

The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org
Link to the patchset: https://codereview.chromium.org/2549353002/#ps160001
(title: "none")

[commit-bot: I haz the power, 2016-12-07 19:14:09]

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, 2016-12-07 20:46:41]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1481138001331560,
"parent_rev": "8d7a43d6af9c5485df29bc3a9db758e6a44f8811", "commit_rev":
"ab5b18419bee5f551b284201ad2dcd5d89c0193a"}

[commit-bot: I haz the power, 2016-12-07 20:47:05]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-12-07 20:50:17]

Description was changed from

==========
Implement PaintLayer::scrollsWithViewport for SPv2.

Also removes a call to
ScrollingCoordinator::updateAfterCompositingChangeIfNeeded that is
SPv2-incompatible, and add force lifecycle updates for two cases of querying
scrolling state via web editingselection APIs.

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

to

==========
Implement PaintLayer::scrollsWithViewport for SPv2.

Also removes a call to
ScrollingCoordinator::updateAfterCompositingChangeIfNeeded that is
SPv2-incompatible, and add force lifecycle updates for two cases of querying
scrolling state via web editingselection APIs.

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

Committed: https://crrev.com/1cf98b57647fa75d2aa18a94ba2128f1dc770386
Cr-Commit-Position: refs/heads/master@{#437051}
==========

[commit-bot: I haz the power, 2016-12-07 20:50:18]

Patchset 9 (id:??) landed as
https://crrev.com/1cf98b57647fa75d2aa18a94ba2128f1dc770386
Cr-Commit-Position: refs/heads/master@{#437051}