Fix scroll chaining for non-descendants of root scroller.

cc/trees/layer_tree_host_impl_unittest.cc

Index: cc/trees/layer_tree_host_impl_unittest.cc
diff --git a/cc/trees/layer_tree_host_impl_unittest.cc b/cc/trees/layer_tree_host_impl_unittest.cc
index af8b817f649a5b0a9cd081f6197420fd9332886f..9ed4267993307b9629c3caba58ad6b4650a539e6 100644
--- a/cc/trees/layer_tree_host_impl_unittest.cc
+++ b/cc/trees/layer_tree_host_impl_unittest.cc
@@ -4155,7 +4155,7 @@ TEST_F(LayerTreeHostImplTopControlsTest,
                 ->ScrollBegin(BeginState(gfx::Point()).get(),
                               InputHandler::TOUCHSCREEN)
                 .thread);
-  EXPECT_EQ(host_impl_->CurrentlyScrollingLayer(), inner_scroll);
+  EXPECT_EQ(host_impl_->CurrentlyScrollingLayer(), outer_scroll);
 
   host_impl_->ScrollBy(
       UpdateState(gfx::Point(), gfx::Vector2dF(0.f, -50.f)).get());
@@ -4611,7 +4611,7 @@ TEST_F(LayerTreeHostImplTopControlsTest, TopControlsScrollOuterViewport) {
                 .thread);
   host_impl_->ScrollBy(UpdateState(gfx::Point(), scroll_delta).get());
 
-  EXPECT_EQ(host_impl_->InnerViewportScrollLayer(),
+  EXPECT_EQ(host_impl_->OuterViewportScrollLayer(),
             host_impl_->CurrentlyScrollingLayer());
   host_impl_->ScrollEnd(EndState().get());
 
@@ -4628,7 +4628,7 @@ TEST_F(LayerTreeHostImplTopControlsTest, TopControlsScrollOuterViewport) {
   host_impl_->ScrollBy(UpdateState(gfx::Point(), scroll_delta).get());
 
   EXPECT_EQ(0, host_impl_->top_controls_manager()->ContentTopOffset());
-  EXPECT_EQ(host_impl_->InnerViewportScrollLayer(),
+  EXPECT_EQ(host_impl_->OuterViewportScrollLayer(),
             host_impl_->CurrentlyScrollingLayer());
 
   host_impl_->ScrollEnd(EndState().get());
@@ -6476,6 +6476,180 @@ TEST_F(LayerTreeHostImplTest, ScrollChainingWithReplacedOuterViewport) {
                      scroll_layer->CurrentScrollOffset());
   }
 }
+// Test that scrolls chain correctly when a child scroller on the page (e.g. a
+// scrolling div) is set as the outer viewport but scrolls start from a layer
+// that's not a descendant of the outer viewport. This happens in the
+// rootScroller proposal.
+TEST_F(LayerTreeHostImplTest, RootScrollerScrollNonDescendant) {
+  const gfx::Size content_size(300, 300);
+  const gfx::Size viewport_size(300, 300);
+
+  LayerTreeImpl* layer_tree_impl = host_impl_->active_tree();
+
+  LayerImpl* content_layer =
+      CreateBasicVirtualViewportLayers(viewport_size, content_size);
+  LayerImpl* inner_scroll_layer = host_impl_->InnerViewportScrollLayer();
+
+  LayerImpl* outer_scroll_layer = nullptr;
+  LayerImpl* sibling_scroll_layer = nullptr;
+
+  // Initialization: Add a scrolling layer, simulating an ordinary DIV, to be
+  // set as the outer viewport. Add a sibling scrolling layer that isn't a child
+  // of the outer viewport scroll layer.
+  {
+    std::unique_ptr<LayerImpl> clip = LayerImpl::Create(layer_tree_impl, 10);
+    clip->SetBounds(content_size);
+    clip->SetPosition(gfx::PointF(100, 100));
+
+    std::unique_ptr<LayerImpl> scroll = LayerImpl::Create(layer_tree_impl, 11);
+    scroll->SetBounds(gfx::Size(1200, 1200));
+    scroll->SetScrollClipLayer(clip->id());
+    scroll->SetDrawsContent(true);
+
+    outer_scroll_layer = scroll.get();

[tdresser, 2016/09/23 15:59:58]

Having a raw pointer point to something owned by a unique_ptr is a bit scary.
I'm not sure why this doesn't crash.

[bokan, 2016/09/23 19:57:01]

It's because we std::move them into the layer tree below, the LayerTreeImpl
maintains a unique_ptr to each layer so I think using a raw ptr here should be
ok since the LayerTreeImpl is long lived. 

[tdresser, 2016/09/23 20:40:30]

D'oh, sure, that's fine.

+
+    clip->test_properties()->AddChild(std::move(scroll));
+    content_layer->test_properties()->AddChild(std::move(clip));
+
+    // Create the non-descendant.
+    std::unique_ptr<LayerImpl> clip2 = LayerImpl::Create(layer_tree_impl, 14);
+    clip2->SetBounds(gfx::Size(600, 600));
+    clip2->SetPosition(gfx::PointF());
+
+    std::unique_ptr<LayerImpl> scroll2 = LayerImpl::Create(layer_tree_impl, 15);
+    scroll2->SetBounds(gfx::Size(1200, 1200));
+    scroll2->SetScrollClipLayer(clip2->id());
+    scroll2->SetDrawsContent(true);
+
+    sibling_scroll_layer = scroll2.get();
+
+    clip2->test_properties()->AddChild(std::move(scroll2));
+    content_layer->test_properties()->AddChild(std::move(clip2));
+
+    layer_tree_impl->SetViewportLayersFromIds(
+        Layer::INVALID_ID, layer_tree_impl->PageScaleLayer()->id(),
+        inner_scroll_layer->id(), outer_scroll_layer->id());
+    layer_tree_impl->BuildPropertyTreesForTesting();
+
+    ASSERT_EQ(outer_scroll_layer, layer_tree_impl->OuterViewportScrollLayer());
+  }
+
+  // Scrolls should target the non-descendant scroller. Chaining should not
+  // propagate to the outer viewport scroll layer.
+  {
+    // This should fully scroll the layer.
+    host_impl_->ScrollBegin(BeginState(gfx::Point(0, 0)).get(),
+                            InputHandler::TOUCHSCREEN);
+    host_impl_->ScrollBy(
+        UpdateState(gfx::Point(0, 0), gfx::Vector2dF(1000.f, 1000.f)).get());
+    host_impl_->ScrollEnd(EndState().get());
+
+    EXPECT_VECTOR_EQ(gfx::Vector2dF(600.f, 600.f),
+                     sibling_scroll_layer->CurrentScrollOffset());
+    EXPECT_VECTOR_EQ(gfx::Vector2dF(),
+                     outer_scroll_layer->CurrentScrollOffset());

[tdresser, 2016/09/23 15:59:58]

Shouldn't outer_scroll_layer be deleted by the time you get here?

[bokan, 2016/09/23 19:57:01]

The unique_ptr's are std::moved into the layer tree so it's kept around.

+
+    // Scrolling now should chain up but, since the outer viewport is a sibling
+    // rather than an ancestor, we shouldn't chain to it.
+    host_impl_->ScrollBegin(BeginState(gfx::Point(0, 0)).get(),
+                            InputHandler::TOUCHSCREEN);
+    host_impl_->ScrollBy(
+        UpdateState(gfx::Point(0, 0), gfx::Vector2dF(1000.f, 1000.f)).get());
+    host_impl_->ScrollEnd(EndState().get());
+
+    EXPECT_VECTOR_EQ(gfx::Vector2dF(600.f, 600.f),
+                     sibling_scroll_layer->CurrentScrollOffset());
+    EXPECT_VECTOR_EQ(gfx::Vector2dF(),
+                     outer_scroll_layer->CurrentScrollOffset());
+  }
+
+  float min_page_scale = 1.f, max_page_scale = 4.f;
+  float page_scale_factor = 1.f;
+  host_impl_->active_tree()->PushPageScaleFromMainThread(
+      page_scale_factor, min_page_scale, max_page_scale);
+
+  gfx::Vector2dF viewport_size_vec(viewport_size.width(),
+                                   viewport_size.height());
+
+  // Reset the scroll offset.
+  sibling_scroll_layer->SetCurrentScrollOffset(gfx::ScrollOffset());
+
+  // Now pinch-zoom in. Anchoring should cause scrolling only on the inner
+  // viewport layer.
+  {
+    // Pinch in to the middle of the screen. The inner viewport should scroll
+    // to keep the gesture anchored but not the outer or the sibling scroller.
+    page_scale_factor = 2.f;
+    gfx::Point anchor(viewport_size.width() / 2, viewport_size.height() / 2);
+    host_impl_->ScrollBegin(BeginState(anchor).get(),
+                            InputHandler::TOUCHSCREEN);
+    host_impl_->PinchGestureBegin();
+    host_impl_->PinchGestureUpdate(page_scale_factor, anchor);
+    host_impl_->PinchGestureEnd();
+
+    EXPECT_VECTOR_EQ(gfx::Vector2dF(anchor.x() / 2, anchor.y() / 2),
+                     inner_scroll_layer->CurrentScrollOffset());
+
+    host_impl_->ScrollBy(UpdateState(anchor, viewport_size_vec).get());
+
+    EXPECT_VECTOR_EQ(ScaleVector2d(viewport_size_vec, 1.f / page_scale_factor),
+                     inner_scroll_layer->CurrentScrollOffset());
+    // TODO(bokan): This doesn't yet work but we'll probably want to fix this
+    // at some point.
+    // EXPECT_VECTOR_EQ(
+    //     gfx::Vector2dF(),
+    //     outer_scroll_layer->CurrentScrollOffset());
+    EXPECT_VECTOR_EQ(gfx::Vector2dF(),
+                     sibling_scroll_layer->CurrentScrollOffset());
+
+    host_impl_->ScrollEnd(EndState().get());
+  }
+
+  // Reset the scroll offsets
+  sibling_scroll_layer->SetCurrentScrollOffset(gfx::ScrollOffset());
+  inner_scroll_layer->SetCurrentScrollOffset(gfx::ScrollOffset());
+  outer_scroll_layer->SetCurrentScrollOffset(gfx::ScrollOffset());
+
+  // Scrolls over the sibling while pinched in should scroll the sibling first,
+  // but then chain up to the inner viewport so that the user can still pan
+  // around. The outer viewport should be unaffected.
+  {
+    // This should fully scroll the sibling but, because we latch to the
+    // scroller, it shouldn't chain up to the inner viewport yet.
+    host_impl_->ScrollBegin(BeginState(gfx::Point(0, 0)).get(),
+                            InputHandler::TOUCHSCREEN);
+    host_impl_->ScrollBy(
+        UpdateState(gfx::Point(0, 0), gfx::Vector2dF(2000.f, 2000.f)).get());
+    host_impl_->ScrollEnd(EndState().get());
+
+    EXPECT_VECTOR_EQ(gfx::Vector2dF(600.f, 600.f),
+                     sibling_scroll_layer->CurrentScrollOffset());
+    EXPECT_VECTOR_EQ(gfx::Vector2dF(),
+                     inner_scroll_layer->CurrentScrollOffset());
+
+    // Scrolling now should chain up to the inner viewport.
+    host_impl_->ScrollBegin(BeginState(gfx::Point(0, 0)).get(),
+                            InputHandler::TOUCHSCREEN);
+    host_impl_->ScrollBy(
+        UpdateState(gfx::Point(0, 0), gfx::Vector2dF(2000.f, 2000.f)).get());
+    host_impl_->ScrollEnd(EndState().get());
+
+    EXPECT_VECTOR_EQ(ScaleVector2d(viewport_size_vec, 1 / page_scale_factor),
+                     inner_scroll_layer->CurrentScrollOffset());
+    EXPECT_VECTOR_EQ(gfx::Vector2dF(),
+                     outer_scroll_layer->CurrentScrollOffset());
+
+    // No more scrolling should be possible.
+    host_impl_->ScrollBegin(BeginState(gfx::Point(0, 0)).get(),
+                            InputHandler::TOUCHSCREEN);
+    host_impl_->ScrollBy(
+        UpdateState(gfx::Point(0, 0), gfx::Vector2dF(2000.f, 2000.f)).get());
+    host_impl_->ScrollEnd(EndState().get());
+
+    EXPECT_VECTOR_EQ(gfx::Vector2dF(),
+                     outer_scroll_layer->CurrentScrollOffset());
+  }
+}
 
 TEST_F(LayerTreeHostImplTest, OverscrollOnImplThread) {
   InputHandlerScrollResult scroll_result;
@@ -8377,13 +8551,13 @@ TEST_F(LayerTreeHostImplTest, NotScrollInvisibleScroller) {
   // any layer that is a drawn RSLL member, then we can ignore the hit.
   //
   // Why SCROLL_STARTED? In this case, it's because we've bubbled out and
-  // started scrolling the inner viewport.
+  // started scrolling the viewport.
   EXPECT_EQ(InputHandler::SCROLL_ON_IMPL_THREAD,
             host_impl_->ScrollBegin(BeginState(gfx::Point()).get(),
                                     InputHandler::WHEEL)
                 .thread);
 
-  EXPECT_EQ(2, host_impl_->CurrentlyScrollingLayer()->id());
+  EXPECT_EQ(117, host_impl_->CurrentlyScrollingLayer()->id());
 }
 
 TEST_F(LayerTreeHostImplTest, ScrollInvisibleScrollerWithVisibleDescendent) {
@@ -9363,16 +9537,16 @@ TEST_F(LayerTreeHostImplVirtualViewportTest, FlingScrollBubblesToInner) {
                   ->ScrollBegin(BeginState(gfx::Point()).get(),
                                 InputHandler::TOUCHSCREEN)
                   .thread);
-    EXPECT_EQ(inner_scroll, host_impl_->CurrentlyScrollingLayer());
+    EXPECT_EQ(outer_scroll, host_impl_->CurrentlyScrollingLayer());
     EXPECT_EQ(InputHandler::SCROLL_ON_IMPL_THREAD,
               host_impl_->FlingScrollBegin().thread);
-    EXPECT_EQ(inner_scroll, host_impl_->CurrentlyScrollingLayer());
+    EXPECT_EQ(outer_scroll, host_impl_->CurrentlyScrollingLayer());
 
     gfx::Vector2d scroll_delta(inner_viewport.width() / 2.f,
                                inner_viewport.height() / 2.f);
     host_impl_->ScrollBy(UpdateState(gfx::Point(), scroll_delta).get());
     inner_expected += gfx::Vector2dF(scroll_delta.x(), scroll_delta.y());
-    EXPECT_EQ(inner_scroll, host_impl_->CurrentlyScrollingLayer());
+    EXPECT_EQ(outer_scroll, host_impl_->CurrentlyScrollingLayer());
 
     host_impl_->ScrollEnd(EndState().get());
     host_impl_->ClearCurrentlyScrollingLayerForTesting();
@@ -9387,18 +9561,18 @@ TEST_F(LayerTreeHostImplVirtualViewportTest, FlingScrollBubblesToInner) {
                   ->ScrollBegin(BeginState(gfx::Point()).get(),
                                 InputHandler::TOUCHSCREEN)
                   .thread);
-    EXPECT_EQ(inner_scroll, host_impl_->CurrentlyScrollingLayer());
+    EXPECT_EQ(outer_scroll, host_impl_->CurrentlyScrollingLayer());
     EXPECT_EQ(InputHandler::SCROLL_ON_IMPL_THREAD,
               host_impl_->FlingScrollBegin().thread);
-    EXPECT_EQ(inner_scroll, host_impl_->CurrentlyScrollingLayer());
+    EXPECT_EQ(outer_scroll, host_impl_->CurrentlyScrollingLayer());
 
     host_impl_->ScrollBy(UpdateState(gfx::Point(), scroll_delta).get());
     inner_expected += gfx::Vector2dF(scroll_delta.x(), scroll_delta.y());
-    EXPECT_EQ(inner_scroll, host_impl_->CurrentlyScrollingLayer());
+    EXPECT_EQ(outer_scroll, host_impl_->CurrentlyScrollingLayer());
 
     host_impl_->ScrollBy(UpdateState(gfx::Point(), scroll_delta).get());
     outer_expected += gfx::Vector2dF(scroll_delta.x(), scroll_delta.y());
-    EXPECT_EQ(inner_scroll, host_impl_->CurrentlyScrollingLayer());
+    EXPECT_EQ(outer_scroll, host_impl_->CurrentlyScrollingLayer());
 
     host_impl_->ScrollEnd(EndState().get());
     host_impl_->ClearCurrentlyScrollingLayerForTesting();
@@ -11166,7 +11340,7 @@ TEST_F(LayerTreeHostImplTest, JitterTest) {
     LayerTreeImpl* pending_tree = host_impl_->pending_tree();
     pending_tree->PushPageScaleFromMainThread(1.f, 1.f, 1.f);
     LayerImpl* last_scrolled_layer = pending_tree->LayerById(
-        host_impl_->active_tree()->LastScrolledLayerId());
+        host_impl_->active_tree()->InnerViewportScrollLayer()->id());

[bokan, 2016/09/23 01:53:55]

The LastScrolledLayerId is set using SetCurrentlyScrollingLayer which isn't
completely precise w.r.t the viewport. For viewport scrolling, we always set the
currently scrolling layer as the one layer that represents the viewport.
Previously this was inner but this patch switched it to outer.

In this test, its the inner viewport that gets scrolled and so the last scrolled
was previously correct but breaks now since the below "hacking" is applied to
the wrong layer.

I'm not sure if this is a problem in practice, however, it's already broken
today for the opposite case. So if the user scrolls the outer viewport, the last
scrolled layer id will be set to the inner. +jaydasika@ as FYI. 

[tdresser, 2016/09/23 15:59:58]

This seems a bit scary – it should be a pretty simple fix, shouldn't it?

[bokan, 2016/09/23 19:57:01]

It's not as straightforward as it seems. This currently gets set during
SetCurrentlyScrollingLayer but, to correctly scroll the viewport, currently
scrolling layer needs to be set to the "representative" viewport layer rather
than the actually scrolled layer. So we'd have to split this out from the
currently scrolling layer. I suspect that it might be best to set this from the
ScrollTree itself whenever a layer's scroll offset changes but I'm not up to
speed on all the details there. This is only ever used to calculate the "jitter"
metric so I'm not sure what the impact is.

I could make cc::Viewport keep track of its last_scrolled_layer and then query
that from SetCurrentlyScrollingLayer but that seems like extra machinery and
it's not clear what that should do when we scroll both layers in one scroll
update. jaydasika@ added this so he might have some insight in the right way
forward here.

 
     // When building property trees from impl side, the builder uses the scroll
     // offset of layer_impl to initialize the scroll offset in scroll tree: