Record size of scroller that user scrolls

third_party/WebKit/Source/core/input/ScrollManager.cpp

Index: third_party/WebKit/Source/core/input/ScrollManager.cpp
diff --git a/third_party/WebKit/Source/core/input/ScrollManager.cpp b/third_party/WebKit/Source/core/input/ScrollManager.cpp
index 6d05fe059f6f8a7ddb8b1370866f43cd5abda5ea..1b503e07ef6e136c286063407a821e42ab71c7cd 100644
--- a/third_party/WebKit/Source/core/input/ScrollManager.cpp
+++ b/third_party/WebKit/Source/core/input/ScrollManager.cpp
@@ -22,6 +22,7 @@
 #include "core/page/scrolling/RootScrollerController.h"
 #include "core/page/scrolling/ScrollState.h"
 #include "core/paint/PaintLayer.h"
+#include "platform/Histogram.h"
 #include "platform/RuntimeEnabledFeatures.h"
 #include "wtf/PtrUtil.h"
 
@@ -221,6 +222,13 @@ WebInputEventResult ScrollManager::handleGestureScrollBegin(
   passScrollGestureEvent(gestureEvent,
                          m_scrollGestureHandlingNode->layoutObject());
 
+  if (m_scrollGestureHandlingNode->layoutObject() &&
+      (gestureEvent.sourceDevice == WebGestureDeviceTouchpad ||
+       gestureEvent.sourceDevice == WebGestureDeviceTouchscreen)) {
+    recordScrollerSize(*(m_scrollGestureHandlingNode->layoutObject()),
+                       gestureEvent.sourceDevice);
+  }
+
   m_currentScrollChain.clear();
   std::unique_ptr<ScrollStateData> scrollStateData =
       WTF::makeUnique<ScrollStateData>();
@@ -559,4 +567,26 @@ bool ScrollManager::canHandleGestureEvent(
   return false;
 }
 
+void ScrollManager::recordScrollerSize(const LayoutObject& layoutObject,
+                                       const WebGestureDevice device) {
+  if (!layoutObject.enclosingLayer())
+    return;
+
+  if (device == WebGestureDeviceTouchpad) {
+    DEFINE_STATIC_LOCAL(
+        CustomCountHistogram, sizeHistogram,

[tdresser, 2017/03/28 13:30:12]

It looks like these macros are defining a static local with the same name.

Don't these names need to be different? Why is this currently working?

[yigu, 2017/03/29 15:32:03]

It's better we differentiate the names. There are existing logic that uses same
histogram name
(https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/html/HTML...).
Could it be static method overload? But the param type looks identical. I need
to dig into the WTF::StaticSingleton.

+        ("Event.Scroll.ScrollerSize.OnScroll_Wheel", 1, 90000, 50));

[flackr, 2017/03/28 14:40:23]

As I mentioned in the cc file we should use constants for the max and number of
buckets.

I'm not sure if 90000 is the right maximum for small scrollers, I'd recommend
about 200000 to cover any scroller that's smaller than 500x400 at and above
which size you could argue it is intended to be a/the main scroller.
Alternately, we could have a maximum that goes well above what we would consider
to be large scrollers which would help us to determine the relative proportion
of small scroller scrolling vs large scroller scrolling.

How will we tell if there are small scrollers that do not get scrolled (i.e.
prime candidates to not save layers for)? I think we should also record scroller
sizes that we see occur on pages.

[yigu, 2017/03/29 15:32:03]

500*400 makes sense to me.
There is gonna be another patch that adds recording scrollers on load/create and
that's why I name the histograms *OnScroll*.

+    sizeHistogram.count(
+        layoutObject.enclosingLayer()->visualRect().width().toInt() *
+        layoutObject.enclosingLayer()->visualRect().height().toInt());
+  } else {
+    DEFINE_STATIC_LOCAL(
+        CustomCountHistogram, sizeHistogram,
+        ("Event.Scroll.ScrollerSize.OnScroll_Touch", 1, 90000, 50));
+    sizeHistogram.count(
+        layoutObject.enclosingLayer()->visualRect().width().toInt() *
+        layoutObject.enclosingLayer()->visualRect().height().toInt());
+  }
+}
+
 }  // namespace blink