Add more specific metrics for main thread scrolling reasons

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

 /*
  * Copyright (C) 2006, 2007, 2008, 2009, 2010, 2011, 2012 Apple Inc. All rights
  * reserved.
  *
  * Portions are Copyright (C) 1998 Netscape Communications Corporation.
  *
  * Other contributors:
  *   Robert O'Callahan <roc+@cs.cmu.edu>
  *   David Baron <dbaron@fas.harvard.edu>
  *   Christian Biesinger <cbiesinger@gmail.com>
@@ skipping 88 matching lines @@
       m_needsCompositedScrolling(false),
       m_rebuildHorizontalScrollbarLayer(false),
       m_rebuildVerticalScrollbarLayer(false),
       m_needsScrollOffsetClamp(false),
       m_needsRelayout(false),
       m_hadHorizontalScrollbarBeforeRelayout(false),
       m_hadVerticalScrollbarBeforeRelayout(false),
       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)
 #endif
 {
   Node* node = box().node();
   if (node && node->isElementNode()) {
     // We save and restore only the scrollOffset as the other scroll values are
     // recalculated.
     Element* element = toElement(node);
@@ skipping 1594 matching lines @@
 }
 
 bool PaintLayerScrollableArea::shouldScrollOnMainThread() const {
   if (ScrollingCoordinator* scrollingCoordinator = getScrollingCoordinator()) {
     if (scrollingCoordinator->shouldUpdateScrollLayerPositionOnMainThread())
       return true;
   }
   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;
 
   Node* node = layer->enclosingNode();
   if (node && node->isElementNode() &&
       (toElement(node)->compositorMutableProperties() &
        (CompositorMutableProperty::kScrollTop |
         CompositorMutableProperty::kScrollLeft)))
     return true;
 
   // TODO(flackr): Allow integer transforms as long as all of the ancestor
   // transforms are also integer.
   bool backgroundSupportsLCDText =
       RuntimeEnabledFeatures::compositeOpaqueScrollersEnabled() &&
       layer->layoutObject()->style()->isStackingContext() &&
       layer->backgroundPaintLocation() & BackgroundPaintInScrollingContents &&
       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
   // behind dashed borders). Resolve this case, or not, and update this check
   // with the results.
   return !(layer->size().isEmpty() || layer->hasDescendantWithClipPath() ||
            layer->hasAncestorWithClipPath() ||
            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;
     layer()->didUpdateNeedsCompositedScrolling();
   }
 }
 
 void PaintLayerScrollableArea::setTopmostScrollChild(PaintLayer* scrollChild) {
   // We only want to track the topmost scroll child for scrollable areas with
   // overlay scrollbars.
@@ skipping 252 matching lines @@
 
 void PaintLayerScrollableArea::DelayScrollOffsetClampScope::
     clampScrollableAreas() {
   for (auto& scrollableArea : *s_needsClamp)
     scrollableArea->clampScrollOffsetAfterOverflowChange();
   delete s_needsClamp;
   s_needsClamp = nullptr;
 }
 
 }  // namespace blink