Give OOPIF FrameViews their own scroll animation timelines and hosts

third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp

Index: third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp
diff --git a/third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp b/third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp
index 27681120185f9dd0c4b8948ce1472000fa4bfc26..f24ce00faa2c8483067d5086080473afedc57e3d 100644
--- a/third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp
+++ b/third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp
@@ -491,8 +491,15 @@ bool ScrollingCoordinator::scrollableAreaScrollLayerDidChange(
       isForRootLayer(scrollableArea))
     m_page->chromeClient().registerViewportLayers();
 
-  scrollableArea->layerForScrollingDidChange(
-      m_programmaticScrollAnimatorTimeline.get());
+  CompositorAnimationTimeline* timeline;

[bokan, 2017/01/17 20:40:15]

For my own clarification, each process has its own ScrollingCoordinator, but the
issue is that there's just one, where we might have multiple LayerTrees in a
single process. Is that right?

[kenrb, 2017/01/19 19:07:40]

Correct. The problem reproduces when we navigate two iframes to the same origin
as each other (but different from the parent), so they become OOPIFs in the same
process. Then there are two compositors and LayerTrees, but only one
ScrollingCoordinator.

+  // FrameView::compositorAnimationTimeline() can indirectly return
+  // m_programmaticScrollAnimatorTimeline if it does not have its own
+  // timeline.
+  if (scrollableArea->isFrameView())
+    timeline = toFrameView(scrollableArea)->compositorAnimationTimeline();
+  else
+    timeline = m_programmaticScrollAnimatorTimeline.get();

[bokan, 2017/01/17 20:40:15]

In this case, the scrollableArea is a PLSA but those seem to return the timeline
that's associated with their FrameView so shouldn't we be returning
scrollableArea->layoutBox()->frameView()->compositorAnimationTimeline?

[kenrb, 2017/01/19 19:07:40]

That sounds reasonable, and simpler, but it breaks tests. It looks like this
method is called during attachment of the root graphics layer in the main frame,
and at that time scrollableArea->layoutBox() returns nullptr, but the work done
here appears to be important.

[bokan, 2017/01/19 20:05:39]

Ok, so to see if I understand - for the following configuration:

    A1     A1 = main frame in process A
   / \
  B1 B2   B1,B2 = OOPIF frames in process B


The current behavior in this patch is:

Scrolling A1 uses A::ScrollingCoordinator::m_programmaticScrollAnimatorTimeline
Scrolling a <div> in A1 uses
A::ScrollingCoordinator::m_programmaticScrollAnimatorTimeline

Scrolling B1 uses B1::m_animationTimeline
Scrolling B2 uses B2::m_animationTimeline
Scrolling a <div> inside either B1 or B2 uses
B::ScrollingCoordinator::m_programmaticScrollAnimatorTimeline?

[kenrb, 2017/01/19 21:36:19]

Scrolling a div in B1 or B2 should use B{1,2}::m_animationTimeline, because
PaintLayerScrollableArea gets the timeline and animation host from its local
frame root.

[bokan, 2017/01/19 21:37:50]

In that case, we're passing the wrong timeline to it here then, no?

[kenrb, 2017/01/20 00:15:32]

Yes, that's a bug. I just uploaded a CL that fixes it, and also adds the DCHECKs
in VisualViewport.

+  scrollableArea->layerForScrollingDidChange(timeline);
 
   return !!webLayer;
 }
@@ -835,20 +842,37 @@ void ScrollingCoordinator::setShouldUpdateScrollLayerPositionOnMainThread(
 }
 
 void ScrollingCoordinator::layerTreeViewInitialized(
-    WebLayerTreeView& layerTreeView) {
+    WebLayerTreeView& layerTreeView,
+    FrameView* view) {
   if (Platform::current()->isThreadedAnimationEnabled() &&
       layerTreeView.compositorAnimationHost()) {
-    m_animationHost = WTF::makeUnique<CompositorAnimationHost>(
-        layerTreeView.compositorAnimationHost());
-    m_programmaticScrollAnimatorTimeline =
+    std::unique_ptr<CompositorAnimationTimeline> timeline =
         CompositorAnimationTimeline::create();
-    m_animationHost->addTimeline(*m_programmaticScrollAnimatorTimeline.get());
+    std::unique_ptr<CompositorAnimationHost> host =
+        WTF::makeUnique<CompositorAnimationHost>(
+            layerTreeView.compositorAnimationHost());

[bokan, 2017/01/17 20:40:15]

This seems suspect (and did before your
patch)...layerTreeView.compositorAnimationHost() returns and stores into a
unique_ptr a raw pointer to RenderWidgetCompositor::animation_host_ which is
itself a unique_ptr. Unless I'm missing something, we have two unique_ptrs to
one object, no?

[kenrb, 2017/01/19 19:07:40]

This does look strange, and I don't know for sure if current behavior is
intended, but that's not quite what happens. makeUnique copies the object, so we
end up owning a distinct CompositorAnimationHost.

[bokan, 2017/01/19 20:05:39]

Oh, nevermind, I confused makeUnique with wrapUnique.

+    if (view && view->frame().localFrameRoot() != m_page->mainFrame()) {
+      view->setAnimationHost(std::move(host));
+      view->setAnimationTimeline(std::move(timeline));
+      view->compositorAnimationHost()->addTimeline(
+          *view->compositorAnimationTimeline());
+    } else {
+      m_animationHost = std::move(host);
+      m_programmaticScrollAnimatorTimeline = std::move(timeline);
+      m_animationHost->addTimeline(*m_programmaticScrollAnimatorTimeline.get());
+    }
   }
 }
 
 void ScrollingCoordinator::willCloseLayerTreeView(
-    WebLayerTreeView& layerTreeView) {
-  if (m_programmaticScrollAnimatorTimeline) {
+    WebLayerTreeView& layerTreeView,
+    FrameView* view) {
+  if (view && view->frame().localFrameRoot() != m_page->mainFrame()) {
+    view->compositorAnimationHost()->removeTimeline(
+        *view->compositorAnimationTimeline());
+    view->setAnimationTimeline(nullptr);
+    view->setAnimationHost(nullptr);
+  } else if (m_programmaticScrollAnimatorTimeline) {
     m_animationHost->removeTimeline(
         *m_programmaticScrollAnimatorTimeline.get());
     m_programmaticScrollAnimatorTimeline = nullptr;