Add more specific metrics for main thread scrolling reasons

third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp

Index: third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp
diff --git a/third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp b/third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp
index 01fcf7caf800c3006633f899fbc15749bb1896a5..b116c8b73726c5017987974320a6592404170f34 100644
--- a/third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp
+++ b/third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp
@@ -106,7 +106,8 @@ PaintLayerScrollableArea::PaintLayerScrollableArea(PaintLayer& layer)
       m_scrollbarManager(*this),
       m_scrollCorner(nullptr),
       m_resizer(nullptr),
-      m_scrollAnchor(this)
+      m_scrollAnchor(this),
+      m_reasons(0)
 #if DCHECK_IS_ON()
       ,
       m_hasBeenDisposed(false)
@@ -1721,9 +1722,15 @@ bool PaintLayerScrollableArea::shouldScrollOnMainThread() const {
   return ScrollableArea::shouldScrollOnMainThread();
 }
 
-static bool layerNeedsCompositedScrolling(
-    PaintLayerScrollableArea::LCDTextMode mode,
+bool PaintLayerScrollableArea::layerNeedsCompositedScrolling(
+    const LCDTextMode mode,
     const PaintLayer* layer) {
+  // Clear all style related main thread scrolling reasons, if any,
+  // for current layer
+  if (getScrollingCoordinator()) {
+    getScrollingCoordinator()

[bokan, 2016/12/19 21:25:15]

Doing this in here means that calling what looks like a simple getter that
should be "const" actually mutates the object. That's unexpected. Simplest
solution would be to just inline this function directly into
updateNeedsCompositedScrolling where mutation is expected based on the function
name.

[yigu, 2016/12/20 01:41:20]

Agreed! Is it OK to use ScrollingCoordinator* scrollingCoordinator =
getScrollingCoordinator(); then scrollingCoordinator->adjust* ?
I saw something similar in dispose().

[bokan, 2016/12/20 15:53:15]

Yah, unless I'm misunderstanding your question. My comment was about folding
this into the updateNeedsCompositedScrolling method so someone in the future
isn't surprised that calling a method like this makes changes locally or in
ScrollingCoordinator. I don't see a problem with this call on
ScrollingCoordinator as is (just that a caller wouldn't expect it from a method
named like this).

+        ->removeStyleRelatedMainThreadScrollingReasonsForLayer(this);
+  }
   if (!layer->scrollsOverflow())
     return false;
 
@@ -1743,10 +1750,16 @@ static bool layerNeedsCompositedScrolling(
       layer->backgroundIsKnownToBeOpaqueInRect(
           toLayoutBox(layer->layoutObject())->paddingBoxRect()) &&
       !layer->compositesWithTransform() && !layer->compositesWithOpacity();
+
   if (mode == PaintLayerScrollableArea::ConsiderLCDText &&
       !layer->compositor()->preferCompositingToLCDTextEnabled() &&
-      !backgroundSupportsLCDText)
+      !backgroundSupportsLCDText) {
+    if (layer->compositesWithOpacity()) {
+      addStyleRelatedMainThreadScrollingReasons(
+          MainThreadScrollingReason::kHasOpacity);
+    }
     return false;
+  }
 
   // TODO(schenney) Tests fail if we do not also exclude
   // layer->layoutObject()->style()->hasBorderDecoration() (missing background
@@ -1757,10 +1770,25 @@ static bool layerNeedsCompositedScrolling(
            layer->layoutObject()->style()->hasBorderRadius());
 }
 
+// Check if current layer has layout object that contains properties that cause

[bokan, 2016/12/19 21:25:14]

This comment is no longer accurate.

[yigu, 2016/12/20 01:41:20]

Done.

+// main thread scrolling.
+void PaintLayerScrollableArea::addStyleRelatedMainThreadScrollingReasons(
+    const uint32_t reason) {
+  // If current layer has opacity and has not been recorded, add the layer into

[bokan, 2016/12/19 21:25:14]

This comment can be removed, IMO.

[yigu, 2016/12/20 01:41:20]

Done.

+  // frameView.
+  if (!getScrollingCoordinator())
+    return;
+  getScrollingCoordinator()->adjustStyleRelatedMainThreadScrollingReasons(
+      reason, true);
+
+  flipMainThreadScrollingReason(reason);

[bokan, 2016/12/19 21:25:15]

Rather than "flip", this should now be "set" and take a bool. Right now, calling
add twice will remove it the second time (but add it twice in
ScrollingCoordinator).

[yigu, 2016/12/20 01:41:20]

I am not sure about this. Since we remove all existed reasons before each
addStyleRelatedMainThreadScrollingReasons(..), flip in this function should
never remove it. Right? Same for the one used in "remove". To make sure that
removing works, we only "flip" it if we "has" it.

[bokan, 2016/12/20 15:53:15]

That might be true today, but code often evolves in ways we don't predict :).
 
Someone might want to later add a reason from another place in code for which
this may not be true (and may miss this fact by reading the code, after all, the
method says "add").

You could just return if the reason already exists and that would make it
correct. IMO, there's no reason to prefer the "flip" approach though since all
you're ever doing is setting and removing, you might as well make the underlying
method support that directly.

+}
+
 void PaintLayerScrollableArea::updateNeedsCompositedScrolling(
     LCDTextMode mode) {
   const bool needsCompositedScrolling =
       layerNeedsCompositedScrolling(mode, layer());
+
   if (static_cast<bool>(m_needsCompositedScrolling) !=
       needsCompositedScrolling) {
     m_needsCompositedScrolling = needsCompositedScrolling;