Move logic of recording main thread scrolling reasons from cc to blink::ScrollManager

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

Index: third_party/WebKit/Source/core/input/EventHandlerTest.cpp
diff --git a/third_party/WebKit/Source/core/input/EventHandlerTest.cpp b/third_party/WebKit/Source/core/input/EventHandlerTest.cpp
index 2161408f393ec7d446dfdfd76ba156d7e96eecda..933515d0ef77f8498180020211d7442ced0525ca 100644
--- a/third_party/WebKit/Source/core/input/EventHandlerTest.cpp
+++ b/third_party/WebKit/Source/core/input/EventHandlerTest.cpp
@@ -16,6 +16,8 @@
 #include "core/page/AutoscrollController.h"
 #include "core/page/Page.h"
 #include "core/testing/DummyPageHolder.h"
+#include "platform/scroll/MainThreadScrollingReason.h"
+#include "platform/testing/HistogramTester.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace blink {
@@ -29,6 +31,7 @@ class EventHandlerTest : public ::testing::Test {
   FrameSelection& selection() const { return document().frame()->selection(); }
 
   void setHtmlInnerHTML(const char* htmlContent);
+  int getBucketIndex(uint32_t reason);
 
  protected:
   std::unique_ptr<DummyPageHolder> m_dummyPageHolder;
@@ -79,6 +82,29 @@ class MousePressEventBuilder : public WebMouseEvent {
   }
 };
 
+class ScrollBeginEventBuilder : public WebGestureEvent {
+ public:
+  ScrollBeginEventBuilder(IntPoint position,
+                          FloatPoint delta,
+                          WebGestureDevice device)
+      : WebGestureEvent() {
+    m_type = WebInputEvent::GestureScrollBegin;
+    x = globalX = position.x();
+    y = globalY = position.y();
+    data.scrollBegin.deltaYHint = delta.y();
+    sourceDevice = device;
+    m_frameScale = 1;
+  }
+};
+
+class ScrollEndEventBuilder : public WebGestureEvent {
+ public:
+  ScrollEndEventBuilder() : WebGestureEvent() {
+    m_type = WebInputEvent::GestureScrollEnd;
+    m_frameScale = 1;
+  }
+};
+
 void EventHandlerTest::SetUp() {
   m_dummyPageHolder = DummyPageHolder::create(IntSize(300, 400));
 }
@@ -88,6 +114,17 @@ void EventHandlerTest::setHtmlInnerHTML(const char* htmlContent) {
   document().view()->updateAllLifecyclePhases();
 }
 
+int EventHandlerTest::getBucketIndex(uint32_t reason) {
+  if (reason & (reason - 1))

[bokan, 2017/04/07 21:05:57]

I had to google what this does. A clearer way to ensure we only have a single
reason is:

int index = 1;
while (!(reason & 1)) {
  reason >>= 1;
  ++index;
}
DCHECK(reason == 1);
return index;

And DCHECK is preferable to returning error condition.

[yigu, 2017/04/10 20:39:04]

Done.

+    return -1;
+  int index = 0;
+  while (reason > 0) {
+    reason >>= 1;
+    ++index;
+  }
+  return index;
+}
+
 TEST_F(EventHandlerTest, dragSelectionAfterScroll) {
   setHtmlInnerHTML(
       "<style> body { margin: 0px; } .upper { width: 300px; height: 400px; }"
@@ -461,6 +498,178 @@ TEST_F(EventHandlerTest, dragEndInNewDrag) {
   // This test passes if it doesn't crash.
 }
 
+TEST_F(EventHandlerTest, NonCompositedMainThreadScrollingReasonTest) {
+  setHtmlInnerHTML(
+      "<style>"
+      " .box { overflow:scroll; width: 100px; height: 100px; }"
+      " .translucent { opacity: 0.5; }"
+      " .spacer { height: 1000px; }"
+      "</style>"
+      "<div id='box' class='translucent box'>"
+      " <div class='spacer'></div>"
+      "</div>");
+
+  document().view()->updateAllLifecyclePhases();
+
+  HistogramTester histogramTester;
+
+  // Test touch scroll.
+  ScrollBeginEventBuilder touchScrollBegin(
+      IntPoint(50, 50), FloatPoint(0.f, 1.f), WebGestureDeviceTouchscreen);
+  ScrollEndEventBuilder touchScrollEnd;
+  document().frame()->eventHandler().handleGestureEvent(touchScrollBegin);
+  document().frame()->eventHandler().handleGestureEvent(touchScrollEnd);
+  histogramTester.expectBucketCount(
+      "Renderer4.MainThreadGestureScrollReason",
+      getBucketIndex(MainThreadScrollingReason::kHasOpacityAndLCDText), 1);
+
+  document().frame()->eventHandler().handleGestureEvent(touchScrollBegin);
+  document().frame()->eventHandler().handleGestureEvent(touchScrollEnd);

[bokan, 2017/04/07 21:05:56]

I'd throw two GestureScrollUpdates between this, just to be sure that we aren't
counting in each Update.

[yigu, 2017/04/10 20:39:05]

Done.

+  histogramTester.expectBucketCount(
+      "Renderer4.MainThreadGestureScrollReason",
+      getBucketIndex(MainThreadScrollingReason::kHasOpacityAndLCDText), 2);
+
+  // Test wheel scroll.
+  ScrollBeginEventBuilder wheelScrollBegin(
+      IntPoint(50, 50), FloatPoint(0.f, 1.f), WebGestureDeviceTouchpad);
+  ScrollEndEventBuilder wheelScrollEnd;
+  document().frame()->eventHandler().handleGestureEvent(wheelScrollBegin);
+  document().frame()->eventHandler().handleGestureEvent(wheelScrollEnd);
+  histogramTester.expectBucketCount(
+      "Renderer4.MainThreadWheelScrollReason",

[bokan, 2017/04/07 21:05:57]

You can improve readability of your expectations with a macro

#define EXPECT_WHEEL_BUCKET(reason, count) \
  histogramTester.expectBucketCount("Renderer4.MainThreadScrollReason", \
    getBucketIndex(MainThreadScrollingReason::##reason), count);

So that your checks look like this:

EXPECT_WHEEL_BUCKET(kHasOpacityAndLCDText, 1);

And equivalently for touch.

*disclaimer: my macro skills are rusty so you may have to jig the above a bit.

[yigu, 2017/04/10 20:39:05]

It looks MUCH better with this improvement. Thanks for the suggestion!
btw, here we should remove [##] ahead of reason otherwise we would get an error
message "pasting formed '::kHasOpacityAndLCDText', an invalid preprocessing
token"

+      getBucketIndex(MainThreadScrollingReason::kHasOpacityAndLCDText), 1);
+  histogramTester.expectBucketCount(
+      "Renderer4.MainThreadWheelScrollReason",
+      getBucketIndex(
+          MainThreadScrollingReason::kBackgroundNotOpaqueInRectAndLCDText),

[bokan, 2017/04/07 21:05:57]

Why do we get this reason for touch but not wheel? Oh, or do we but you just
didn't check above? You should check it for touch as well.

[yigu, 2017/04/10 20:39:05]

Done.

+      1);
+  histogramTester.expectTotalCount("Renderer4.MainThreadWheelScrollReason", 2);
+}
+
+TEST_F(EventHandlerTest,
+       NonCompositedMainThreadScrollingReasonWithCompositedScrollableAreaTest) {
+  setHtmlInnerHTML(
+      "<style>"
+      " .box { overflow:scroll; width: 100px; height: 100px; }"
+      " .translucent { opacity: 0.5; }"
+      " .composited { will-change: transform; }"
+      " .spacer { height: 1000px; }"
+      "</style>"
+      "<div id='box' class='translucent box'>"
+      " <div class='spacer'></div>"
+      "</div>");
+
+  page().settings().setAcceleratedCompositingEnabled(true);
+  document().view()->setParentVisible(true);

[bokan, 2017/04/07 21:05:57]

Why is setParentVisible and setSelfVisible needed? I've never seen these used.

[yigu, 2017/04/10 20:39:05]

We have to use these to make sure the test box gets composited.

+  document().view()->setSelfVisible(true);
+  document().view()->updateAllLifecyclePhases();
+
+  HistogramTester histogramTester;
+
+  ScrollBeginEventBuilder wheelScrollBegin(
+      IntPoint(50, 50), FloatPoint(0.f, 1.f), WebGestureDeviceTouchpad);
+  ScrollEndEventBuilder wheelScrollEnd;
+  document().frame()->eventHandler().handleGestureEvent(wheelScrollBegin);
+  document().frame()->eventHandler().handleGestureEvent(wheelScrollEnd);

[bokan, 2017/04/07 21:05:57]

Another way to make this more readable is to wrap up these wheel and touch
events into a method on the class (you can subclass EventHandlerTest into
NonCompositedMainThreadScrollingReasonTest, might help shorted your test names
too). You should also pass in the Element you want to scroll so it's clear what
the scroll is targeting, e.g.:

Element* box = document().getElementById("box");
wheelScroll(box);

And the method can figure out the Element's position.

[yigu, 2017/04/10 20:39:05]

Done.

+  histogramTester.expectBucketCount(
+      "Renderer4.MainThreadWheelScrollReason",
+      getBucketIndex(MainThreadScrollingReason::kHasOpacityAndLCDText), 1);
+
+  Element* box = document().getElementById("box");
+  box->setAttribute("class", "composited translucent box");
+  document().view()->updateAllLifecyclePhases();
+  document().frame()->eventHandler().handleGestureEvent(wheelScrollBegin);
+  document().frame()->eventHandler().handleGestureEvent(wheelScrollEnd);

[bokan, 2017/04/07 21:05:57]

Lets also EXPECT box's PLSA::getNonCompositedMainThreadScrollReasons is 0

[yigu, 2017/04/10 20:39:04]

Done.

+  histogramTester.expectBucketCount(
+      "Renderer4.MainThreadWheelScrollReason",
+      getBucketIndex(MainThreadScrollingReason::kHasOpacityAndLCDText), 1);

[bokan, 2017/04/07 21:05:57]

It'd be safer to check that the total count didn't change here.

[yigu, 2017/04/10 20:39:04]

Done.

+}
+
+TEST_F(EventHandlerTest,
+       NonCompositedMainThreadScrollingReasonWithNotScrollableAreaTest) {
+  setHtmlInnerHTML(
+      "<style>.box { overflow:scroll; width: 100px; height: 100px; }"
+      " .translucent { opacity: 0.5; }"
+      " .hidden { overflow: hidden; }"
+      " .spacer { height: 1000px; }"
+      "</style>"
+      "<div id='box' class='translucent box'>"
+      " <div class='spacer'></div>"
+      "</div>");
+
+  document().view()->updateAllLifecyclePhases();
+
+  HistogramTester histogramTester;
+
+  ScrollBeginEventBuilder wheelScrollBegin(
+      IntPoint(50, 50), FloatPoint(0.f, 1.f), WebGestureDeviceTouchpad);
+  ScrollEndEventBuilder wheelScrollEnd;
+  document().frame()->eventHandler().handleGestureEvent(wheelScrollBegin);
+  document().frame()->eventHandler().handleGestureEvent(wheelScrollEnd);
+  histogramTester.expectBucketCount(
+      "Renderer4.MainThreadWheelScrollReason",
+      getBucketIndex(MainThreadScrollingReason::kHasOpacityAndLCDText), 1);
+
+  Element* box = document().getElementById("box");
+  box->setAttribute("class", "hidden translucent box");
+  document().view()->updateAllLifecyclePhases();
+  document().frame()->eventHandler().handleGestureEvent(wheelScrollBegin);
+  document().frame()->eventHandler().handleGestureEvent(wheelScrollEnd);
+  histogramTester.expectBucketCount(

[bokan, 2017/04/07 21:05:57]

Check total count.

[yigu, 2017/04/10 20:39:05]

Done.

+      "Renderer4.MainThreadWheelScrollReason",
+      getBucketIndex(MainThreadScrollingReason::kHasOpacityAndLCDText), 1);
+}
+
+TEST_F(EventHandlerTest,
+       NonCompositedMainThreadScrollingReasonWithNestedScrollersTest) {
+  setHtmlInnerHTML(
+      "<style>"
+      " .container { overflow:scroll; width: 200px; height: 200px; }"
+      " .box { overflow:scroll; width: 100px; height: 100px; }"
+      " .translucent { opacity: 0.5; }"
+      " .transform { transform: scale(0.8); }"
+      " .with-border-radius { border: 5px solid; border-radius: 5px; }"
+      " .spacer { height: 1000px; }"
+      " .composited { will-change: transform; }"
+      "</style>"
+      "<div id='container' class='container with-border-radius'>"
+      "  <div class='translucent box'>"
+      "    <div class='composited transform box'>"
+      "      <div class='spacer'></div>"
+      "    </div>"
+      "  </div>"
+      "  <div class='spacer'></div>"
+      "</div>");
+
+  page().settings().setAcceleratedCompositingEnabled(true);
+  document().view()->setParentVisible(true);
+  document().view()->setSelfVisible(true);
+  document().view()->updateAllLifecyclePhases();
+
+  HistogramTester histogramTester;
+
+  ScrollBeginEventBuilder wheelScrollBegin(
+      IntPoint(50, 50), FloatPoint(0.f, 1.f), WebGestureDeviceTouchpad);
+  ScrollEndEventBuilder wheelScrollEnd;
+  document().frame()->eventHandler().handleGestureEvent(wheelScrollBegin);
+  document().frame()->eventHandler().handleGestureEvent(wheelScrollEnd);
+  histogramTester.expectBucketCount(

[bokan, 2017/04/07 21:05:57]

This test is a little more complex, some comments above each expectation would
help the reader know where each one is coming from.

[yigu, 2017/04/10 20:39:05]

Done.

+      "Renderer4.MainThreadWheelScrollReason",
+      getBucketIndex(MainThreadScrollingReason::kHasOpacityAndLCDText), 1);

[bokan, 2017/04/07 21:05:57]

This one comes from "translucent box" right? Isn't that one unscrollable? Since
it's the same size as its child and its child is scaled down even...?

[yigu, 2017/04/10 20:39:05]

It's actually scrollable (see jsbin.com/muluhes).

+  histogramTester.expectBucketCount(
+      "Renderer4.MainThreadWheelScrollReason",
+      getBucketIndex(
+          MainThreadScrollingReason::kBackgroundNotOpaqueInRectAndLCDText),

[bokan, 2017/04/07 21:05:57]

This one comes from all of them, right?

[yigu, 2017/04/10 20:39:05]

In this case yes. Usually we set this reason whenever we have a not opaque
background.

+      1);
+  histogramTester.expectBucketCount(
+      "Renderer4.MainThreadWheelScrollReason",
+      getBucketIndex(MainThreadScrollingReason::kHasBorderRadius), 1);
+  histogramTester.expectBucketCount(
+      "Renderer4.MainThreadWheelScrollReason",
+      getBucketIndex(MainThreadScrollingReason::kHasTransformAndLCDText), 0);
+  histogramTester.expectTotalCount("Renderer4.MainThreadWheelScrollReason", 3);
+}
+
 class TooltipCapturingChromeClient : public EmptyChromeClient {
  public:
   TooltipCapturingChromeClient() {}