[BGPT] Update cc::Layer offset from visual viewport

In the linked bug, we try scrolling the page using the keyboard. While all the scroll offsets in Blink are correctly updated, we don't see the scrolling occur on the screen. The reason for this is that we don't realize anything has changed and the commit is aborted due to no changes. It turns out we don't update the offset on the VisualViewport's cc::Layer. The reason this doesn't manifest in other contexts is that we usually perform viewport scrolling using touch or wheel events which are handled on the compositor thread. The fix here is to do what we do for PaintLayerScrollableAreas and call UpdateCompositedScrollOffset on the VisualViewport when the offset changes from the main thread. We need to update the cc::Layer's offset since we use a change in this offset to determine that a commit is needed. This is a similar issue to the one fixed in https://crrev.com/dfe90fdc496af64515756ba1b5970d2760b7314d Bug: 956579 Change-Id: Ie9854f3df1b7065bc2d021b131ff6a8998a8f59d Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1617545Reviewed-by: Philip Rogers <pdr@chromium.org> Commit-Queue: David Bokan <bokan@chromium.org> Cr-Commit-Position: refs/heads/master@{#660930}

[BGPT] Update cc::Layer offset from visual viewport
In the linked bug, we try scrolling the page using the keyboard. While all the scroll offsets in Blink are correctly updated, we don't see the scrolling occur on the screen. The reason for this is that we don't realize anything has changed and the commit is aborted due to no changes. It turns out we don't update the offset on the VisualViewport's cc::Layer. The reason this doesn't manifest in other contexts is that we usually perform viewport scrolling using touch or wheel events which are handled on the compositor thread. The fix here is to do what we do for PaintLayerScrollableAreas and call UpdateCompositedScrollOffset on the VisualViewport when the offset changes from the main thread. We need to update the cc::Layer's offset since we use a change in this offset to determine that a commit is needed. This is a similar issue to the one fixed in https://crrev.com/dfe90fdc496af64515756ba1b5970d2760b7314d Bug: 956579 Change-Id: Ie9854f3df1b7065bc2d021b131ff6a8998a8f59d Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1617545Reviewed-by: Philip Rogers <pdr@chromium.org> Commit-Queue: David Bokan <bokan@chromium.org> Cr-Commit-Position: refs/heads/master@{#660930}
e6ca4549 · David Bokan · Commit Bot · 1dc2e479 · e6ca4549 · e6ca4549
Commit e6ca4549 authored May 17, 2019 by David Bokan Committed by Commit Bot May 17, 2019
2 changed files
--- a/third_party/blink/renderer/core/frame/visual_viewport.cc
+++ b/third_party/blink/renderer/core/frame/visual_viewport.cc
@@ -552,13 +552,17 @@ bool VisualViewport::DidSetScaleOrLocation(float scale,
    // SVG runs with accelerated compositing disabled so no
    // ScrollingCoordinator.
-    // TODO(bokan): This can go away with BGPT because it's used only to
+    if (ScrollingCoordinator* coordinator =
-    // propagate scroll offsets to the cc::Layers. In BGPT, scroll offsets are
+            GetPage().GetScrollingCoordinator()) {
-    // set directly on the TransformPaintPropertyNodes so this becomes a no-op.
+      // In BGPT, scroll offsets and related properties are set directly on the
-    if (!RuntimeEnabledFeatures::BlinkGenPropertyTreesEnabled()) {
+      // property trees so all we need to do is update the scroll offset on the
-      if (ScrollingCoordinator* coordinator =
+      // corresponding cc::Layer. Pre-BGPT we used ScrollLayerDidChange but
-              GetPage().GetScrollingCoordinator())
+      // since all we need this for is to update the cc::Layer's offset, we now
+      // use the more purpose-built UpdateCompositedScrollOffset, as in PLSA.
+      if (!RuntimeEnabledFeatures::BlinkGenPropertyTreesEnabled())
        coordinator->ScrollableAreaScrollLayerDidChange(this);
+      else
+        coordinator->UpdateCompositedScrollOffset(this);
    }
    EnqueueScrollEvent();

--- a/third_party/blink/renderer/core/page/scrolling/scrolling_coordinator_test.cc
+++ b/third_party/blink/renderer/core/page/scrolling/scrolling_coordinator_test.cc
@@ -1672,6 +1672,39 @@ TEST_P(ScrollingCoordinatorTest, ScrollOffsetClobberedBeforeCompositingUpdate) {
  EXPECT_EQ(gfx::ScrollOffset(), cc_layer->CurrentScrollOffset());
 }
+TEST_P(ScrollingCoordinatorTest, UpdateVisualViewportScrollLayer) {
+  // This test fails without BGPT enabled. https://crbug.com/930636.
+  if (!RuntimeEnabledFeatures::BlinkGenPropertyTreesEnabled())
+    return;
+  LoadHTML(R"HTML(
+          <!DOCTYPE html>
+          <style>
+            #box {
+              width: 300px;
+              height: 1000px;
+              background-color: red;
+            }
+          </style>
+          <div id="box">
+          </div>
+      )HTML");
+  ForceFullCompositingUpdate();
+  Page* page = GetFrame()->GetPage();
+  cc::Layer* inner_viewport_scroll_layer =
+      page->GetVisualViewport().ScrollLayer()->CcLayer();
+  page->GetVisualViewport().SetScale(2);
+  EXPECT_EQ(gfx::ScrollOffset(0, 0),
+            inner_viewport_scroll_layer->CurrentScrollOffset());
+  page->GetVisualViewport().SetLocation(FloatPoint(10, 20));
+  EXPECT_EQ(gfx::ScrollOffset(10, 20),
+            inner_viewport_scroll_layer->CurrentScrollOffset());
+}
 TEST_P(ScrollingCoordinatorTest, UpdateUMAMetricUpdated) {
  HistogramTester histogram_tester;
  LoadHTML(R"HTML(