Compositor Animation Timelines: Fix frame swapping tests in WebFrameTest.cpp

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

 /*
  * Copyright (C) 2011 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright
  *    notice, this list of conditions and the following disclaimer in the
@@ skipping 76 matching lines @@
 }
 
 ScrollingCoordinator::ScrollingCoordinator(Page* page)
     : m_page(page)
     , m_scrollGestureRegionIsDirty(false)
     , m_touchEventTargetRectsAreDirty(false)
     , m_shouldScrollOnMainThreadDirty(false)
     , m_wasFrameScrollable(false)
     , m_lastMainThreadScrollingReasons(0)
 {
-    createProgrammaticScrollAnimatorTimeline();
 }
 
 ScrollingCoordinator::~ScrollingCoordinator()
 {
     ASSERT(!m_page);
 }
 
 DEFINE_TRACE(ScrollingCoordinator)
 {
     visitor->trace(m_page);
@@ skipping 605 matching lines @@
 void ScrollingCoordinator::setShouldUpdateScrollLayerPositionOnMainThread(MainThreadScrollingReasons reasons)
 {
     if (!m_page->mainFrame()->isLocalFrame() || !m_page->deprecatedLocalMainFrame()->view())
         return;
     if (WebLayer* scrollLayer = toWebLayer(m_page->deprecatedLocalMainFrame()->view()->layerForScrolling())) {
         m_lastMainThreadScrollingReasons = reasons;
         scrollLayer->setShouldScrollOnMainThread(reasons);
     }
 }
 
+void ScrollingCoordinator::onSetMainFrame()
+{
+    createProgrammaticScrollAnimatorTimeline();

[dcheng, 2015/11/19 05:09:56]

It feels like this should be tied to initializing the layer tree view, perhaps?

[loyso (OOO), 2015/11/26 06:22:52]

Done.

+}
+
+void ScrollingCoordinator::willSwapMainFrame()
+{
+    destroyProgrammaticScrollAnimatorTimeline();
+}
+
 void ScrollingCoordinator::willCloseLayerTreeView()
 {
     destroyProgrammaticScrollAnimatorTimeline();
 }
 
 void ScrollingCoordinator::willBeDestroyed()
 {
     ASSERT(m_page);
 
     destroyProgrammaticScrollAnimatorTimeline();

[dcheng, 2015/11/19 05:09:56]

See previous comment: I'm a bit confused. Why is this called in both
willBeDestroyed() and willCloseLayerTreeView()? They're both called on page
shutdown, and it seems like if this code was tied to layer tree
creation/cleanup, then we would minimize the amount of code that shows up in
other layers.

[dcheng, 2015/11/25 06:09:44]

Ping? I'm still curious about an answer to this, since this affects what we
might choose to do here.

[loyso (OOO), 2015/11/26 06:22:52]

We have many situations when we call Page::willBeDestroyed and we don't call
Page::willCloseLayerTreeView. I was afraid that we don't destroy it properly in
all configurations.

Now I agree, that if we bind ProgrammaticScrollAnimatorTimeline existence and
lifetime scope to
WebViewImpl::initializeLayerTreeView/WebViewImpl::willCloseLayerTreeView then it
will be just fine.

 
     m_page = nullptr;
     for (const auto& scrollbar : m_horizontalScrollbars)
         GraphicsLayer::unregisterContentsLayer(scrollbar.value->layer());
     for (const auto& scrollbar : m_verticalScrollbars)
         GraphicsLayer::unregisterContentsLayer(scrollbar.value->layer());
 }
 
 bool ScrollingCoordinator::coordinatesScrollingForFrameView(FrameView* frameView) const
 {
@@ skipping 348 matching lines @@
 {
     if (m_programmaticScrollAnimatorTimeline) {
         ASSERT(m_page);
         ASSERT(m_page->mainFrame()->isLocalFrame());
         m_page->chromeClient().detachCompositorAnimationTimeline(m_programmaticScrollAnimatorTimeline.get(), toLocalFrame(m_page->mainFrame()));
         m_programmaticScrollAnimatorTimeline.clear();
     }
 }
 
 } // namespace blink