cc: Fix for synced property main thread updates with MFBA.

cc/trees/layer_tree_host_unittest_scroll.cc

Index: cc/trees/layer_tree_host_unittest_scroll.cc
diff --git a/cc/trees/layer_tree_host_unittest_scroll.cc b/cc/trees/layer_tree_host_unittest_scroll.cc
index 6e685a5959b0bc68d3f404ada0608d9fa285971c..45e1b0e8ece7476324a20bfaf00e972b3d84b0b0 100644
--- a/cc/trees/layer_tree_host_unittest_scroll.cc
+++ b/cc/trees/layer_tree_host_unittest_scroll.cc
@@ -1515,5 +1515,355 @@ TEST_F(LayerTreeHostScrollTestLayerStructureChange, ScrollDestroyWholeTree) {
   RunTest(CompositorMode::THREADED, false);
 }
 
+class LayerTreeHostScrollTestScrollMFBA : public LayerTreeHostScrollTest {
+ public:
+  LayerTreeHostScrollTestScrollMFBA()
+      : initial_scroll_(10, 20),
+        second_scroll_(40, 5),
+        third_scroll_(20, 10),
+        scroll_amount_(2, -1),
+        num_commits_(0),
+        num_scrolls_(0) {}
+
+  void InitializeSettings(LayerTreeSettings* settings) override {
+    settings->main_frame_before_activation_enabled = true;
+  }
+
+  void BeginTest() override {
+    outer_viewport_container_layer_id_ = layer_tree_host()
+                                             ->outer_viewport_scroll_layer()
+                                             ->scroll_clip_layer()
+                                             ->id();
+    layer_tree_host()->outer_viewport_scroll_layer()->SetScrollOffset(
+        initial_scroll_);
+    PostSetNeedsCommitToMainThread();
+  }
+
+  void StartCommitOnImpl() override {
+    switch (num_commits_) {
+      case 0:
+        PostSetNeedsCommitToMainThread();
+        break;
+      case 1:
+        PostSetNeedsCommitToMainThread();
+        // Block activation after second commit until third commit is ready.
+        GetProxyImplForTest()->BlockNotifyReadyToActivateForTesting(true);
+        break;
+      case 2:
+        // Unblock activation after third commit is ready.
+        GetProxyImplForTest()->BlockNotifyReadyToActivateForTesting(false);
+        break;
+    }
+    num_commits_++;
+  }
+
+  void UpdateLayerTreeHost() override {
+    Layer* scroll_layer = layer_tree_host()->outer_viewport_scroll_layer();
+    switch (layer_tree_host()->source_frame_number()) {
+      case 0:
+        EXPECT_VECTOR_EQ(initial_scroll_, scroll_layer->scroll_offset());
+        break;
+      case 1:
+        EXPECT_VECTOR_EQ(
+            gfx::ScrollOffsetWithDelta(initial_scroll_, scroll_amount_),
+            scroll_layer->scroll_offset());
+        // Pretend like Javascript updated the scroll position itself.
+        scroll_layer->SetScrollOffset(second_scroll_);
+        break;
+      case 2:
+        // Third frame does not see a scroll delta because we only did one
+        // scroll for the second and third frames.
+        EXPECT_VECTOR_EQ(second_scroll_, scroll_layer->scroll_offset());
+        // Pretend like Javascript updated the scroll position itself.
+        scroll_layer->SetScrollOffset(third_scroll_);
+        break;
+    }
+  }
+
+  void DrawLayersOnThread(LayerTreeHostImpl* impl) override {
+    LayerImpl* scroll_layer = impl->OuterViewportScrollLayer();
+    switch (impl->active_tree()->source_frame_number()) {
+      case 0:
+        EXPECT_VECTOR_EQ(gfx::Vector2d(), ScrollDelta(scroll_layer));
+        EXPECT_VECTOR_EQ(initial_scroll_, ScrollTreeForLayer(scroll_layer)
+                                              ->GetScrollOffsetBaseForTesting(
+                                                  scroll_layer->id()));
+        Scroll(impl);
+        EXPECT_VECTOR_EQ(scroll_amount_, ScrollDelta(scroll_layer));
+        break;
+      case 1:
+        EXPECT_VECTOR_EQ(gfx::Vector2d(), ScrollDelta(scroll_layer));
+        EXPECT_VECTOR_EQ(second_scroll_, ScrollTreeForLayer(scroll_layer)
+                                             ->GetScrollOffsetBaseForTesting(
+                                                 scroll_layer->id()));
+        Scroll(impl);
+        EXPECT_VECTOR_EQ(scroll_amount_, ScrollDelta(scroll_layer));
+        break;
+      case 2:
+        // The scroll hasn't been consumed by the main thread.
+        EXPECT_VECTOR_EQ(scroll_amount_, ScrollDelta(scroll_layer));
+        EXPECT_VECTOR_EQ(third_scroll_, ScrollTreeForLayer(scroll_layer)
+                                            ->GetScrollOffsetBaseForTesting(
+                                                scroll_layer->id()));
+        EndTest();
+        break;
+    }
+  }
+
+  void ApplyViewportDeltas(const gfx::Vector2dF& inner_delta,
+                           const gfx::Vector2dF& outer_delta,
+                           const gfx::Vector2dF& elastic_overscroll_delta,
+                           float scale,
+                           float top_controls_delta) override {
+    num_scrolls_++;
+  }
+
+  void AfterTest() override {
+    EXPECT_EQ(3, num_commits_);
+    EXPECT_EQ(1, num_scrolls_);
+  }
+
+ private:
+  void Scroll(LayerTreeHostImpl* impl) {
+    LayerImpl* root = impl->active_tree()->root_layer();
+    LayerImpl* scroll_layer = impl->OuterViewportScrollLayer();
+
+    scroll_layer->SetScrollClipLayer(outer_viewport_container_layer_id_);
+    scroll_layer->SetBounds(
+        gfx::Size(root->bounds().width() + 100, root->bounds().height() + 100));
+    scroll_layer->ScrollBy(scroll_amount_);
+  }
+
+  gfx::ScrollOffset initial_scroll_;
+  gfx::ScrollOffset second_scroll_;
+  gfx::ScrollOffset third_scroll_;
+  gfx::Vector2dF scroll_amount_;
+  int num_commits_;
+  int num_scrolls_;
+  int outer_viewport_container_layer_id_;
+};
+
+MULTI_THREAD_TEST_F(LayerTreeHostScrollTestScrollMFBA);
+
+class LayerTreeHostScrollTestScrollAbortedCommitMFBA
+    : public LayerTreeHostScrollTest {
+ public:
+  LayerTreeHostScrollTestScrollAbortedCommitMFBA()
+      : initial_scroll_(50, 60),
+        impl_scroll_(-3, 2),
+        second_main_scroll_(14, -3),
+        num_will_begin_main_frames_(0),
+        num_did_begin_main_frames_(0),
+        num_will_commits_(0),
+        num_did_commits_(0),
+        num_impl_commits_(0),
+        num_aborted_commits_(0),
+        num_impl_scrolls_(0),
+        num_draws_(0) {}
+
+  void InitializeSettings(LayerTreeSettings* settings) override {
+    settings->main_frame_before_activation_enabled = true;
+  }
+
+  void BeginTest() override {
+    layer_tree_host()->outer_viewport_scroll_layer()->SetScrollOffset(
+        initial_scroll_);
+    PostSetNeedsCommitToMainThread();
+  }
+
+  void SetupTree() override {
+    LayerTreeHostScrollTest::SetupTree();
+
+    gfx::Size scroll_layer_bounds(200, 200);
+    layer_tree_host()->outer_viewport_scroll_layer()->SetBounds(
+        scroll_layer_bounds);
+    layer_tree_host()->SetPageScaleFactorAndLimits(1.f, 0.01f, 100.f);
+  }
+
+  void WillBeginMainFrame() override {
+    num_will_begin_main_frames_++;
+    Layer* root_scroll_layer = layer_tree_host()->outer_viewport_scroll_layer();
+    switch (num_will_begin_main_frames_) {
+      case 1:
+        // This will not be aborted because of the initial prop changes.
+        EXPECT_EQ(0, num_impl_scrolls_);
+        EXPECT_EQ(0, layer_tree_host()->source_frame_number());
+        EXPECT_VECTOR_EQ(initial_scroll_, root_scroll_layer->scroll_offset());
+        break;
+      case 2:
+        // This commit will not be aborted because of the scroll change.
+        EXPECT_EQ(1, num_impl_scrolls_);
+        EXPECT_EQ(1, layer_tree_host()->source_frame_number());
+        EXPECT_VECTOR_EQ(
+            gfx::ScrollOffsetWithDelta(initial_scroll_, impl_scroll_),
+            root_scroll_layer->scroll_offset());
+        root_scroll_layer->SetScrollOffset(gfx::ScrollOffsetWithDelta(
+            root_scroll_layer->scroll_offset(), second_main_scroll_));
+        break;
+      case 3: {
+        // This commit will be aborted.
+        EXPECT_EQ(2, num_impl_scrolls_);
+        // The source frame number still increases even with the abort.
+        EXPECT_EQ(2, layer_tree_host()->source_frame_number());
+        gfx::Vector2dF delta =
+            impl_scroll_ + impl_scroll_ + second_main_scroll_;
+        EXPECT_VECTOR_EQ(gfx::ScrollOffsetWithDelta(initial_scroll_, delta),
+                         root_scroll_layer->scroll_offset());
+        break;
+      }
+      case 4: {
+        // This commit will also be aborted.
+        EXPECT_EQ(3, num_impl_scrolls_);
+        EXPECT_EQ(3, layer_tree_host()->source_frame_number());
+        gfx::Vector2dF delta =
+            impl_scroll_ + impl_scroll_ + impl_scroll_ + second_main_scroll_;
+        EXPECT_VECTOR_EQ(gfx::ScrollOffsetWithDelta(initial_scroll_, delta),
+                         root_scroll_layer->scroll_offset());
+        break;
+      }
+    }
+  }
+
+  void DidBeginMainFrame() override { num_did_begin_main_frames_++; }
+
+  void WillCommit() override { num_will_commits_++; }
+
+  void DidCommit() override { num_did_commits_++; }
+
+  void BeginCommitOnThread(LayerTreeHostImpl* impl) override {
+    switch (num_impl_commits_) {
+      case 0:
+        impl->SetNeedsCommit();
+        break;
+      case 1:
+        // Redraw so that we keep scrolling.
+        impl->SetNeedsRedraw();
+        // Ask for another commit (which will abort).
+        impl->SetNeedsCommit();
+        // Block activation until third commit is aborted.
+        impl->BlockNotifyReadyToActivateForTesting(true);
+        break;
+      default:
+        NOTREACHED();
+    }
+    num_impl_commits_++;
+  }
+
+  void BeginMainFrameAbortedOnThread(LayerTreeHostImpl* impl,
+                                     CommitEarlyOutReason reason) override {
+    switch (num_aborted_commits_) {
+      case 0:
+        EXPECT_EQ(2, num_impl_commits_);
+        // Ask for another commit (which will also abort).
+        impl->SetNeedsCommit();
+        // Unblock activation when third commit is aborted.
+        impl->BlockNotifyReadyToActivateForTesting(false);

[enne (OOO), 2016/06/09 00:57:44]

Is this racy? It seems like this means that the scroll commit activation and the
aborted commit are potentially running against each other.  Right now it looks
like the activation occurs before the second commit aborts?

If that's the case, then I think a better testing structure would be this:
* initial commit + activation
* scroll change commit + hold activation
* aborted commit
* aborted commit (+ unblock activation)
* scroll change activation (check scroll positions, end tests after draw here)

That way you always test that two aborted commits have the right scroll
positions, and that the pending tree has the correct value after it finally
activates as well.

[sunnyps, 2016/06/09 01:08:57]

The reason for blocking NotifyReadyToActivate is to ensure the second commit to
be sent while the previous commit activation is pending. Activation is
fundamentally racy with BeginMainFrame, right? I.e. if we activate in the same
frame as commit, we'll do a non-MFBA BeginMainFrame but if activation gets
delayed beyond the frame boundary then we'll do an MFBA BeginMainFrame. Blocking
NotifyReadyToActivate is the only safe solution here.

I like what I have here (i.e. first aborted commit unblocks activation) better
because we're testing both when activation is pending (first aborted commit) and
when it's not (second aborted commit).

+        break;
+      case 1:
+        EXPECT_EQ(2, num_impl_commits_);
+        // Redraw to end the test.
+        impl->SetNeedsRedraw();
+        break;
+    }
+    num_aborted_commits_++;
+  }
+
+  void DrawLayersOnThread(LayerTreeHostImpl* impl) override {
+    LayerImpl* root_scroll_layer = impl->OuterViewportScrollLayer();
+    switch (impl->active_tree()->source_frame_number()) {
+      case 0:
+        if (num_impl_commits_ == 1) {
+          // First draw
+          EXPECT_VECTOR_EQ(gfx::Vector2d(), ScrollDelta(root_scroll_layer));
+          root_scroll_layer->ScrollBy(impl_scroll_);
+          EXPECT_VECTOR_EQ(impl_scroll_, ScrollDelta(root_scroll_layer));
+          EXPECT_VECTOR_EQ(
+              initial_scroll_,
+              ScrollTreeForLayer(root_scroll_layer)
+                  ->GetScrollOffsetBaseForTesting(root_scroll_layer->id()));
+        } else if (num_impl_commits_ == 2) {
+          // Second draw but no new active tree because activation is blocked.
+          EXPECT_VECTOR_EQ(impl_scroll_, ScrollDelta(root_scroll_layer));
+          root_scroll_layer->ScrollBy(impl_scroll_);
+          EXPECT_VECTOR_EQ(impl_scroll_ + impl_scroll_,
+                           ScrollDelta(root_scroll_layer));
+          EXPECT_VECTOR_EQ(
+              initial_scroll_,
+              ScrollTreeForLayer(root_scroll_layer)
+                  ->GetScrollOffsetBaseForTesting(root_scroll_layer->id()));
+        } else {
+          NOTREACHED();
+        }
+        break;
+      case 1: {
+        EXPECT_EQ(2, num_impl_commits_);
+        // All scroll deltas so far should be consumed.
+        EXPECT_EQ(gfx::Vector2d(), ScrollDelta(root_scroll_layer));
+        if (num_aborted_commits_ == 1) {
+          root_scroll_layer->ScrollBy(impl_scroll_);
+          EXPECT_VECTOR_EQ(impl_scroll_, ScrollDelta(root_scroll_layer));
+          gfx::Vector2dF prev_delta =
+              impl_scroll_ + impl_scroll_ + second_main_scroll_;
+          EXPECT_VECTOR_EQ(
+              gfx::ScrollOffsetWithDelta(initial_scroll_, prev_delta),
+              ScrollTreeForLayer(root_scroll_layer)
+                  ->GetScrollOffsetBaseForTesting(root_scroll_layer->id()));
+        } else if (num_aborted_commits_ == 2) {
+          gfx::Vector2dF delta =
+              impl_scroll_ + impl_scroll_ + impl_scroll_ + second_main_scroll_;
+          EXPECT_VECTOR_EQ(
+              gfx::ScrollOffsetWithDelta(initial_scroll_, delta),
+              ScrollTreeForLayer(root_scroll_layer)
+                  ->GetScrollOffsetBaseForTesting(root_scroll_layer->id()));
+          // End test after second aborted commit (fourth commit request).
+          EndTest();
+        } else {
+          NOTREACHED();
+        }
+        break;
+      }
+    }
+    num_draws_++;
+  }
+
+  void ApplyViewportDeltas(const gfx::Vector2dF& inner_delta,
+                           const gfx::Vector2dF& outer_delta,
+                           const gfx::Vector2dF& elastic_overscroll_delta,
+                           float scale,
+                           float top_controls_delta) override {
+    num_impl_scrolls_++;
+  }
+
+  void AfterTest() override {
+    EXPECT_EQ(3, num_impl_scrolls_);
+    // Verify that the embedder sees aborted commits as real commits.
+    EXPECT_EQ(4, num_will_begin_main_frames_);
+    EXPECT_EQ(4, num_did_begin_main_frames_);
+    EXPECT_EQ(4, num_will_commits_);
+    EXPECT_EQ(4, num_did_commits_);
+    // ...but the compositor thread only sees two real ones.
+    EXPECT_EQ(2, num_impl_commits_);
+    // ...and two aborted ones.
+    EXPECT_EQ(2, num_aborted_commits_);
+    // ...and four draws.
+    EXPECT_EQ(4, num_draws_);
+  }
+
+ private:
+  gfx::ScrollOffset initial_scroll_;
+  gfx::Vector2dF impl_scroll_;
+  gfx::Vector2dF second_main_scroll_;
+  int num_will_begin_main_frames_;
+  int num_did_begin_main_frames_;
+  int num_will_commits_;
+  int num_did_commits_;
+  int num_impl_commits_;
+  int num_aborted_commits_;
+  int num_impl_scrolls_;
+  int num_draws_;
+};
+
+MULTI_THREAD_TEST_F(LayerTreeHostScrollTestScrollAbortedCommitMFBA);
+
 }  // namespace
 }  // namespace cc