Only scroll on main if the targeted frames need to scroll on main

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 fe04c5a66b7f62cd407af5223674f1da69f07753..1421f22a809547064d6726fa49570346607d5be2 100644
--- a/third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp
+++ b/third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp
@@ -190,7 +190,12 @@ void ScrollingCoordinator::updateAfterCompositingChangeIfNeeded() {
   if (m_shouldScrollOnMainThreadDirty ||
       m_wasFrameScrollable != frameIsScrollable) {
     setShouldUpdateScrollLayerPositionOnMainThread(
-        mainThreadScrollingReasons());
+        mainThreadScrollingReasons(m_page->deprecatedLocalMainFrame()));

[pdr., 2016/12/13 05:51:17]

Nit: lets refactor this to not use deprecated functions (here, and on line 188).
The first line of this function checks "if
(!m_page->mainFrame()->isLocalFrame())" so I think we can just use
"toLocalFrame(frame)" here.

[yigu, 2016/12/14 21:07:27]

Done.

+
+    // Need to update scroll on main thread reasons for subframe because
+    // subframe (e.g. iframe with background-attachment:fixed) should
+    // scroll on main thread while the main frame scrolls on impl.
+    updateSubFrameScrollOnMainReason();
     m_shouldScrollOnMainThreadDirty = false;
   }
   m_wasFrameScrollable = frameIsScrollable;
@@ -835,6 +840,35 @@ void ScrollingCoordinator::setShouldUpdateScrollLayerPositionOnMainThread(
   }
 }
 
+void ScrollingCoordinator::updateSubFrameScrollOnMainReason() {

[pdr., 2016/12/13 05:51:16]

Ah, I misread this code. You are correct that this is no longer O(n^2). I think
the general design of how you split up the calls into per-frame reasons and
global reasons is a good approach.

[yigu, 2016/12/14 21:07:27]

Done.

+  MainThreadScrollingReasons reasons =
+      static_cast<MainThreadScrollingReasons>(0);
+  MainThreadScrollingReasons mainFrameReasons =
+      static_cast<MainThreadScrollingReasons>(0);
+
+  if (!m_page->settings().threadedScrollingEnabled())
+    mainFrameReasons |= MainThreadScrollingReason::kThreadedScrollingDisabled;
+
+  for (Frame* frame = m_page->mainFrame(); frame;

[pdr., 2016/12/13 05:51:16]

This loop doesn't quite look right, or I may misunderstand.

Imagine we have the following tree:
A
|
B
|
C

If B has background attachment fixed objects, it will have local main thread
scrolling reasons. In that case, I think A's scroll layer should also have main
thread scrolling reasons, but C's scroll layer should not--is that correct?
Won't this loop set main thread scrolling reasons incorrectly on C?

A slightly more complex example:
..A
./.\
B...D
|
C

If B has background attachment fixed objects, what main thread scrolling reasons
should the scroll layers of A, C, and D have? I thought they would be: A(has
main thread reasons), B(has main thread reasons), C(no main thread reasons), and
D(no main thread reasons).

[pdr., 2016/12/13 23:07:10]

I was wrong in how this works. For A-B-C, if B has main thread reasons, only A
needs it too (not C).

I am still insure this loop is correct though, because of how
frame->tree().traverseNext() works.

Consider the following:
..A
./.\
B...D
|
C

If B has main thread scrolling reasons, won't this loop also set them
unnecessarily on D?

+       frame = frame->tree().traverseNext()) {
+    if (!frame->isLocalFrame())
+      continue;
+
+    reasons = mainFrameReasons |
+              mainThreadScrollingReasonsPerFrame(toLocalFrame(frame));
+    if (WebLayer* scrollLayer =
+            toWebLayer(toLocalFrame(frame)->view()->layerForScrolling())) {
+      if (reasons) {
+        scrollLayer->addMainThreadScrollingReasons(reasons);
+      } else {
+        reasons = ~reasons;
+        reasons &= ~MainThreadScrollingReason::kHandlingScrollFromMainThread;
+        scrollLayer->clearMainThreadScrollingReasons(reasons);
+      }
+    }
+  }
+}
+
 void ScrollingCoordinator::layerTreeViewInitialized(
     WebLayerTreeView& layerTreeView) {
   if (Platform::current()->isThreadedAnimationEnabled()) {
@@ -952,9 +986,12 @@ static void accumulateDocumentTouchEventTargetRects(LayerHitTestRects& rects,
   if (!targets)
     return;
 
-  // If there's a handler on the window, document, html or body element (fairly
-  // common in practice), then we can quickly mark the entire document and skip
-  // looking at any other handlers.  Note that technically a handler on the body
+  // If there's a handler on the window, document, html or body element

[pdr., 2016/12/13 05:51:16]

Nit: please re-flow this multi-line comment so it's 80 cols.

[yigu, 2016/12/14 21:07:27]

Done.

+  // (fairly
+  // common in practice), then we can quickly mark the entire document and
+  // skip
+  // looking at any other handlers.  Note that technically a handler on the
+  // body
   // doesn't cover the whole document, but it's reasonable to be conservative
   // and report the whole document anyway.
   //
@@ -1141,8 +1178,39 @@ bool ScrollingCoordinator::hasVisibleSlowRepaintViewportConstrainedObjects(
   return false;
 }
 
-MainThreadScrollingReasons ScrollingCoordinator::mainThreadScrollingReasons()
-    const {
+MainThreadScrollingReasons
+ScrollingCoordinator::mainThreadScrollingReasonsPerFrame(

[pdr., 2016/12/13 05:51:16]

Does this need to be on ScrollingCoordinator? I wonder if we could just move all
of this code into FrameView::getScrollingReasons()? Then it's a little easier to
understand that the reasons are local to a single FrameView.

+    LocalFrame* frame) const {
+  MainThreadScrollingReasons reasons =
+      static_cast<MainThreadScrollingReasons>(0);
+
+  FrameView* frameView = frame->view();
+  if (!frameView || frameView->shouldThrottleRendering())
+    return reasons;
+
+  if (frameView->hasBackgroundAttachmentFixedObjects())
+    reasons |= MainThreadScrollingReason::kHasBackgroundAttachmentFixedObjects;
+  FrameView::ScrollingReasons scrollingReasons =
+      frameView->getScrollingReasons();
+  const bool mayBeScrolledByInput = (scrollingReasons == FrameView::Scrollable);
+  const bool mayBeScrolledByScript =
+      mayBeScrolledByInput ||
+      (scrollingReasons == FrameView::NotScrollableExplicitlyDisabled);
+
+  // TODO(awoloszyn) Currently crbug.com/304810 will let certain
+  // overflow:hidden elements scroll on the compositor thread, so we should
+  // not let this move there path as an optimization, when we have
+  // slow-repaint elements.
+  if (mayBeScrolledByScript &&
+      hasVisibleSlowRepaintViewportConstrainedObjects(frameView)) {
+    reasons |=
+        MainThreadScrollingReason::kHasNonLayerViewportConstrainedObjects;
+  }
+  return reasons;
+}
+
+MainThreadScrollingReasons ScrollingCoordinator::mainThreadScrollingReasons(
+    LocalFrame* targetFrame) const {
   MainThreadScrollingReasons reasons =
       static_cast<MainThreadScrollingReasons>(0);
 
@@ -1152,55 +1220,27 @@ MainThreadScrollingReasons ScrollingCoordinator::mainThreadScrollingReasons()
   if (!m_page->mainFrame()->isLocalFrame())
     return reasons;
 
-  // TODO(flackr) Currently we combine reasons for main thread scrolling from
-  // all frames but we should only look at the targetted frame (and its
-  // ancestors if the scroll bubbles up). http://crbug.com/568901
-  for (Frame* frame = m_page->mainFrame(); frame;
-       frame = frame->tree().traverseNext()) {
-    if (!frame->isLocalFrame())
-      continue;
-
-    // TODO(alexmos,kenrb): For OOPIF, local roots that are different from
-    // the main frame can't be used in the calculation, since they use
-    // different compositors with unrelated state, which breaks some of the
-    // calculations below.
-    if (toLocalFrame(frame)->localFrameRoot() != m_page->mainFrame())
-      continue;
+  if (targetFrame->localFrameRoot() != m_page->mainFrame())

[pdr., 2016/12/13 05:51:16]

Please add the comment back about OOPIF:
// TODO(alexmos,kenrb): For OOPIF, local roots that are different from
// the main frame can't be used in the calculation, since they use
// different compositors with unrelated state, which breaks some of the
// calculations below.

+    return reasons;
 
-    FrameView* frameView = toLocalFrame(frame)->view();
-    if (!frameView || frameView->shouldThrottleRendering())
+  // Walk through the subtree from the target frame to root. Use the gathered
+  // reasons to determine whether the target frame should be scrolled on main
+  // thread regardless other subframes on the same page.
+  for (Frame* frame = targetFrame; frame; frame = frame->tree().parent()) {
+    if (!frame->isLocalFrame())
       continue;
-
-    if (frameView->hasBackgroundAttachmentFixedObjects())
-      reasons |=
-          MainThreadScrollingReason::kHasBackgroundAttachmentFixedObjects;
-    FrameView::ScrollingReasons scrollingReasons =
-        frameView->getScrollingReasons();
-    const bool mayBeScrolledByInput =
-        (scrollingReasons == FrameView::Scrollable);
-    const bool mayBeScrolledByScript =
-        mayBeScrolledByInput ||
-        (scrollingReasons == FrameView::NotScrollableExplicitlyDisabled);
-
-    // TODO(awoloszyn) Currently crbug.com/304810 will let certain
-    // overflow:hidden elements scroll on the compositor thread, so we should
-    // not let this move there path as an optimization, when we have
-    // slow-repaint elements.
-    if (mayBeScrolledByScript &&
-        hasVisibleSlowRepaintViewportConstrainedObjects(frameView)) {
-      reasons |=
-          MainThreadScrollingReason::kHasNonLayerViewportConstrainedObjects;
-    }
+    reasons |= mainThreadScrollingReasonsPerFrame(toLocalFrame(frame));
   }
 
   return reasons;
 }
 
-String ScrollingCoordinator::mainThreadScrollingReasonsAsText() const {
-  DCHECK(m_page->deprecatedLocalMainFrame()->document()->lifecycle().state() >=
+String ScrollingCoordinator::mainThreadScrollingReasonsAsText(
+    const Frame* frame) const {
+  DCHECK(toLocalFrame(frame)->document()->lifecycle().state() >=
          DocumentLifecycle::CompositingClean);
-  if (WebLayer* scrollLayer = toWebLayer(
-          m_page->deprecatedLocalMainFrame()->view()->layerForScrolling())) {
+  if (WebLayer* scrollLayer =
+          toWebLayer(toLocalFrame(frame)->view()->layerForScrolling())) {
     String result(MainThreadScrollingReason::mainThreadScrollingReasonsAsText(
                       scrollLayer->mainThreadScrollingReasons())
                       .c_str());