UMA metrics for use count of wheel and touch scrolls.

UMA metrics for use count of wheel and touch scrolls.

For Impl thread scrolling test, run the virtual threaded/fast/scroll-behavior
testSuite.

BUG=699272
TEST=LayoutTests/fast/scroll-behavior/wheel-and-touch-scroll-use-count.html
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2737653004
Cr-Commit-Position: refs/heads/master@{#460140}
Committed: https://chromium.googlesource.com/chromium/src/+/1b47fda711331d9d39563ce0749c5f515b186169

[sahel, 2017-03-07 22:41:54]

Description was changed from

==========
UMA metrics for use count of wheel and touch scrolls.

For Impl thread scrolling test, run the virtual threaded/fast/scroll-behavior
testSuite.

BUG=699272
TEST=LayoutTests/fast/scroll-behavior/wheel-and-touch-scroll-use-count.html
==========

to

==========
UMA metrics for use count of wheel and touch scrolls.

For Impl thread scrolling test, run the virtual threaded/fast/scroll-behavior
testSuite.

BUG=699272
TEST=LayoutTests/fast/scroll-behavior/wheel-and-touch-scroll-use-count.html
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[sahel, 2017-03-07 22:44:00]

The CQ bit was checked by sahel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-07 22:44:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-07 23:12:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-07 23:12:56]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[sahel, 2017-03-08 16:13:20]

The CQ bit was checked by sahel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-08 16:13:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-08 16:15:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-08 16:15:46]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[sahel, 2017-03-08 16:54:07]

The CQ bit was checked by sahel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-08 16:54:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-08 18:27:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-08 18:27:18]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sahel, 2017-03-08 20:21:16]

Description was changed from

==========
UMA metrics for use count of wheel and touch scrolls.

For Impl thread scrolling test, run the virtual threaded/fast/scroll-behavior
testSuite.

BUG=699272
TEST=LayoutTests/fast/scroll-behavior/wheel-and-touch-scroll-use-count.html
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
UMA metrics for use count of wheel and touch scrolls.

For Impl thread scrolling test, run the virtual threaded/fast/scroll-behavior
testSuite.

BUG=699272
TEST=LayoutTests/fast/scroll-behavior/wheel-and-touch-scroll-use-count.html
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[sahel, 2017-03-08 20:26:17]

sahel@chromium.org changed reviewers:
+ bokan@chromium.org, dtapuska@chromium.org

[sahel, 2017-03-08 21:11:59]

Patchset #3 (id:40001) has been deleted

[sahel, 2017-03-08 21:12:28]

Patchset #2 (id:20001) has been deleted

[sahel, 2017-03-08 21:12:39]

The CQ bit was checked by sahel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-08 21:13:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dtapuska, 2017-03-08 21:18:00]

https://codereview.chromium.org/2737653004/diff/60001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/2737653004/diff/60001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host.cc:749: break;
Can you return here?

https://codereview.chromium.org/2737653004/diff/60001/third_party/WebKit/publ...
File third_party/WebKit/public/web/WebWidget.h (right):

https://codereview.chromium.org/2737653004/diff/60001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebWidget.h:137: virtual void
RecordWheelAndTouchScrollingCount(bool hasScrolledByWheel,
lower case the first letter according to the coding conventions for blink (and
elsewhere).

[commit-bot: I haz the power, 2017-03-08 22:09:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-08 22:09:56]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sahel, 2017-03-09 16:15:20]

The CQ bit was checked by sahel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-09 16:16:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[bokan, 2017-03-09 17:31:03]

https://codereview.chromium.org/2737653004/diff/60001/cc/input/input_handler.h
File cc/input/input_handler.h (right):

https://codereview.chromium.org/2737653004/diff/60001/cc/input/input_handler....
cc/input/input_handler.h:115: SCROLLED_BY_TOUCH,
Make this a bitfield. i.e. SCROLLED_BY_TOUCH = 0x1, SCROLLED_BY_WHEEL = 0x2.
That way you can avoid the awkward translation in
LayerTreeHostImpl::UpdateScrollSourceInfo and just OR the values together.

https://codereview.chromium.org/2737653004/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/scroll-behavior/wheel-and-touch-scroll-use-count.html
(right):

https://codereview.chromium.org/2737653004/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/scroll-behavior/wheel-and-touch-scroll-use-count.html:45:
// is not supported for mac.
Just curious, where does this limitation come from? I don't see anything Mac
specific in GpuBenchmarking so I presume at somepoint in the pipeline the
touches/gestures are dropped on Mac because we assume Mac == NoTouch?

https://codereview.chromium.org/2737653004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/ScrollManager.cpp (right):

https://codereview.chromium.org/2737653004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/ScrollManager.cpp:240: if
(gestureEvent.sourceDevice == WebGestureDeviceTouchscreen)
Put this in a method on Page and share it between here and the WebViewImpl
method.

[commit-bot: I haz the power, 2017-03-09 17:57:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-09 17:57:08]

Dry run: This issue passed the CQ dry run.

[bokan, 2017-03-09 18:31:16]

https://codereview.chromium.org/2737653004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/ScrollManager.cpp (right):

https://codereview.chromium.org/2737653004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/ScrollManager.cpp:240: if
(gestureEvent.sourceDevice == WebGestureDeviceTouchscreen)
On 2017/03/09 17:31:03, bokan wrote:
> Put this in a method on Page and share it between here and the WebViewImpl
> method.

Actually, I guess this wouldn't really help much. Feel free to disregard.

[sahel, 2017-03-09 19:33:50]

The CQ bit was checked by sahel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-09 19:34:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-09 19:47:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-09 19:47:38]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[sahel, 2017-03-09 20:36:58]

Patchset #4 (id:100001) has been deleted

[sahel, 2017-03-09 20:41:15]

The CQ bit was checked by sahel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-09 20:41:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-09 20:55:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-09 20:55:37]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[sahel, 2017-03-09 22:00:42]

The CQ bit was checked by sahel@chromium.org to run a CQ dry run

[sahel, 2017-03-09 22:00:51]

Patchset #4 (id:120001) has been deleted

[sahel, 2017-03-09 22:01:05]

Patchset #4 (id:140001) has been deleted

[commit-bot: I haz the power, 2017-03-09 22:01:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-09 23:39:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-09 23:39:06]

Dry run: This issue passed the CQ dry run.

[sahel, 2017-03-10 16:21:33]

https://codereview.chromium.org/2737653004/diff/60001/cc/input/input_handler.h
File cc/input/input_handler.h (right):

https://codereview.chromium.org/2737653004/diff/60001/cc/input/input_handler....
cc/input/input_handler.h:115: SCROLLED_BY_TOUCH,
On 2017/03/09 17:31:03, bokan wrote:
> Make this a bitfield. i.e. SCROLLED_BY_TOUCH = 0x1, SCROLLED_BY_WHEEL = 0x2.
> That way you can avoid the awkward translation in
> LayerTreeHostImpl::UpdateScrollSourceInfo and just OR the values together.

Done.

https://codereview.chromium.org/2737653004/diff/60001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/2737653004/diff/60001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host.cc:749: break;
On 2017/03/08 21:18:00, dtapuska wrote:
> Can you return here?

Done.

https://codereview.chromium.org/2737653004/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/scroll-behavior/wheel-and-touch-scroll-use-count.html
(right):

https://codereview.chromium.org/2737653004/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/scroll-behavior/wheel-and-touch-scroll-use-count.html:45:
// is not supported for mac.
On 2017/03/09 17:31:03, bokan wrote:
> Just curious, where does this limitation come from? I don't see anything Mac
> specific in GpuBenchmarking so I presume at somepoint in the pipeline the
> touches/gestures are dropped on Mac because we assume Mac == NoTouch?

synthetic_gesture_target_mac doesn't override the following function and the
check fails:
https://cs.chromium.org/chromium/src/content/browser/renderer_host/input/synt...

https://codereview.chromium.org/2737653004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/ScrollManager.cpp (right):

https://codereview.chromium.org/2737653004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/ScrollManager.cpp:240: if
(gestureEvent.sourceDevice == WebGestureDeviceTouchscreen)
On 2017/03/09 18:31:15, bokan wrote:
> On 2017/03/09 17:31:03, bokan wrote:
> > Put this in a method on Page and share it between here and the WebViewImpl
> > method.
> 
> Actually, I guess this wouldn't really help much. Feel free to disregard.

Acknowledged.

https://codereview.chromium.org/2737653004/diff/60001/third_party/WebKit/publ...
File third_party/WebKit/public/web/WebWidget.h (right):

https://codereview.chromium.org/2737653004/diff/60001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebWidget.h:137: virtual void
RecordWheelAndTouchScrollingCount(bool hasScrolledByWheel,
On 2017/03/08 21:18:00, dtapuska wrote:
> lower case the first letter according to the coding conventions for blink (and
> elsewhere).

Done.

[bokan, 2017-03-10 16:27:13]

lgtm

https://codereview.chromium.org/2737653004/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/scroll-behavior/wheel-and-touch-scroll-use-count.html
(right):

https://codereview.chromium.org/2737653004/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/scroll-behavior/wheel-and-touch-scroll-use-count.html:45:
// is not supported for mac.
On 2017/03/10 16:21:33, sahel wrote:
> On 2017/03/09 17:31:03, bokan wrote:
> > Just curious, where does this limitation come from? I don't see anything Mac
> > specific in GpuBenchmarking so I presume at somepoint in the pipeline the
> > touches/gestures are dropped on Mac because we assume Mac == NoTouch?
> 
> synthetic_gesture_target_mac doesn't override the following function and the
> check fails:
>
https://cs.chromium.org/chromium/src/content/browser/renderer_host/input/synt...

Acknowledged.

[sahel, 2017-03-10 20:28:58]

sahel@chromium.org changed reviewers:
+ aelias@chromium.org, danakj@chromium.org, esprehn@chromium.org

[sahel, 2017-03-10 20:35:26]

@danakj please review changes at:
cc/*
ui/compositor/compositor.h

@aelias please review changes at:
content/browser/renderer_host/compositor_impl_android.h

@esprehn please review changes at:
content/renderer/*
third_party/WebKit/Source/*

[aelias_OOO_until_Jul13, 2017-03-10 20:41:36]

renderer_host lgtm

[danakj, 2017-03-10 20:52:48]

https://codereview.chromium.org/2737653004/diff/160001/cc/input/input_handler.h
File cc/input/input_handler.h (right):

https://codereview.chromium.org/2737653004/diff/160001/cc/input/input_handler...
cc/input/input_handler.h:114: enum : uint32_t {
Why do you specify the storage type?

https://codereview.chromium.org/2737653004/diff/160001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/2737653004/diff/160001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host.cc:741: if (!info || info->scroll_source_info == 0)
When will info be null?

https://codereview.chromium.org/2737653004/diff/160001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_common.h (right):

https://codereview.chromium.org/2737653004/diff/160001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_common.h:180: uint32_t scroll_source_info;
Why did you go with a bitmask here instead of 2 bools? You can make them 1 bit
large each here.

[sahel, 2017-03-10 21:48:36]

The CQ bit was checked by sahel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-10 21:48:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-11 00:19:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-11 00:19:46]

Dry run: This issue passed the CQ dry run.

[sahel, 2017-03-13 20:13:43]

https://codereview.chromium.org/2737653004/diff/160001/cc/input/input_handler.h
File cc/input/input_handler.h (right):

https://codereview.chromium.org/2737653004/diff/160001/cc/input/input_handler...
cc/input/input_handler.h:114: enum : uint32_t {
On 2017/03/10 20:52:48, danakj wrote:
> Why do you specify the storage type?

Done, I made it uint8_t to address the
feedback about info struct size.

https://codereview.chromium.org/2737653004/diff/160001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/2737653004/diff/160001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host.cc:741: if (!info || info->scroll_source_info == 0)
On 2017/03/10 20:52:48, danakj wrote:
> When will info be null?

Done.

https://codereview.chromium.org/2737653004/diff/160001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_common.h (right):

https://codereview.chromium.org/2737653004/diff/160001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_common.h:180: uint32_t scroll_source_info;
On 2017/03/10 20:52:48, danakj wrote:
> Why did you go with a bitmask here instead of 2 bools? You can make them 1 bit
> large each here.

If other types of scrolling (e.g. keyboard) are handled by the compositor thread
in future (there are some plans for keyboard events generating gesture events)
this implementation is extendable.

In terms of struct size I didn't think it's critical because the struct already
has vectors. I changed it to uint8_32 though, if it's still not good enough I
can change it to two boolean values.

[danakj, 2017-03-13 20:16:23]

https://codereview.chromium.org/2737653004/diff/160001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_common.h (right):

https://codereview.chromium.org/2737653004/diff/160001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_common.h:180: uint32_t scroll_source_info;
On 2017/03/13 20:13:43, sahel wrote:
> On 2017/03/10 20:52:48, danakj wrote:
> > Why did you go with a bitmask here instead of 2 bools? You can make them 1
bit
> > large each here.
> 
> If other types of scrolling (e.g. keyboard) are handled by the compositor
thread
> in future (there are some plans for keyboard events generating gesture events)
> this implementation is extendable.

I don't see how a bitflag is any more extendable than a set of bools. Maybe you
can explain?

> In terms of struct size I didn't think it's critical because the struct
already
> has vectors. I changed it to uint8_32 though, if it's still not good enough I
> can change it to two boolean values.

I was wondering why you specify a size at all rather than the size you did. The
compiler will pick a reasonable size for an enum.

[sahel, 2017-03-13 21:03:52]

https://codereview.chromium.org/2737653004/diff/160001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_common.h (right):

https://codereview.chromium.org/2737653004/diff/160001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_common.h:180: uint32_t scroll_source_info;
On 2017/03/13 20:16:23, danakj wrote:
> On 2017/03/13 20:13:43, sahel wrote:
> > On 2017/03/10 20:52:48, danakj wrote:
> > > Why did you go with a bitmask here instead of 2 bools? You can make them 1
> bit
> > > large each here.
> > 
> > If other types of scrolling (e.g. keyboard) are handled by the compositor
> thread
> > in future (there are some plans for keyboard events generating gesture
events)
> > this implementation is extendable.
> 
> I don't see how a bitflag is any more extendable than a set of bools. Maybe
you
> can explain?
> 
> > In terms of struct size I didn't think it's critical because the struct
> already
> > has vectors. I changed it to uint8_32 though, if it's still not good enough
I
> > can change it to two boolean values.
> 
> I was wondering why you specify a size at all rather than the size you did.
The
> compiler will pick a reasonable size for an enum.

what I meant is: if I want to add other sources of scrolling, with having one
flag per source type I will keep updating the struct. On the other hand with an
enum, all source info is wrapped into one field regardless of the number of
resources and I won't need to change the struct and its populating when I need
to add a new resource. Anyways, I can definitely change it to one flag per type
if you prefer them.  

I didn't know that compilers do that. Thanks for explaining.

[danakj, 2017-03-13 22:29:15]

https://codereview.chromium.org/2737653004/diff/160001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_common.h (right):

https://codereview.chromium.org/2737653004/diff/160001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_common.h:180: uint32_t scroll_source_info;
On 2017/03/13 21:03:52, sahel wrote:
> On 2017/03/13 20:16:23, danakj wrote:
> > On 2017/03/13 20:13:43, sahel wrote:
> > > On 2017/03/10 20:52:48, danakj wrote:
> > > > Why did you go with a bitmask here instead of 2 bools? You can make them
1
> > bit
> > > > large each here.
> > > 
> > > If other types of scrolling (e.g. keyboard) are handled by the compositor
> > thread
> > > in future (there are some plans for keyboard events generating gesture
> events)
> > > this implementation is extendable.
> > 
> > I don't see how a bitflag is any more extendable than a set of bools. Maybe
> you
> > can explain?
> > 
> > > In terms of struct size I didn't think it's critical because the struct
> > already
> > > has vectors. I changed it to uint8_32 though, if it's still not good
enough
> I
> > > can change it to two boolean values.
> > 
> > I was wondering why you specify a size at all rather than the size you did.
> The
> > compiler will pick a reasonable size for an enum.
> 
> what I meant is: if I want to add other sources of scrolling, with having one
> flag per source type I will keep updating the struct. On the other hand with
an
> enum, all source info is wrapped into one field regardless of the number of
> resources and I won't need to change the struct and its populating when I need
> to add a new resource. Anyways, I can definitely change it to one flag per
type
> if you prefer them.  
> 
> I didn't know that compilers do that. Thanks for explaining. 

Optimize for the reader, not the writer. A uint32_t will explain much less about
what you're reading here than a couple bools will. So yeah, I prefer that.

[sahel, 2017-03-15 20:52:42]

The CQ bit was checked by sahel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-15 20:53:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-15 23:28:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-15 23:28:50]

Dry run: This issue passed the CQ dry run.

[sahel, 2017-03-16 12:58:26]

https://codereview.chromium.org/2737653004/diff/160001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_common.h (right):

https://codereview.chromium.org/2737653004/diff/160001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_common.h:180: uint32_t scroll_source_info;
On 2017/03/13 22:29:15, danakj wrote:
> On 2017/03/13 21:03:52, sahel wrote:
> > On 2017/03/13 20:16:23, danakj wrote:
> > > On 2017/03/13 20:13:43, sahel wrote:
> > > > On 2017/03/10 20:52:48, danakj wrote:
> > > > > Why did you go with a bitmask here instead of 2 bools? You can make
them
> 1
> > > bit
> > > > > large each here.
> > > > 
> > > > If other types of scrolling (e.g. keyboard) are handled by the
compositor
> > > thread
> > > > in future (there are some plans for keyboard events generating gesture
> > events)
> > > > this implementation is extendable.
> > > 
> > > I don't see how a bitflag is any more extendable than a set of bools.
Maybe
> > you
> > > can explain?
> > > 
> > > > In terms of struct size I didn't think it's critical because the struct
> > > already
> > > > has vectors. I changed it to uint8_32 though, if it's still not good
> enough
> > I
> > > > can change it to two boolean values.
> > > 
> > > I was wondering why you specify a size at all rather than the size you
did.
> > The
> > > compiler will pick a reasonable size for an enum.
> > 
> > what I meant is: if I want to add other sources of scrolling, with having
one
> > flag per source type I will keep updating the struct. On the other hand with
> an
> > enum, all source info is wrapped into one field regardless of the number of
> > resources and I won't need to change the struct and its populating when I
need
> > to add a new resource. Anyways, I can definitely change it to one flag per
> type
> > if you prefer them.  
> > 
> > I didn't know that compilers do that. Thanks for explaining. 
> 
> Optimize for the reader, not the writer. A uint32_t will explain much less
about
> what you're reading here than a couple bools will. So yeah, I prefer that.

Done.

[danakj, 2017-03-16 15:19:21]

Thanks, I like this

https://codereview.chromium.org/2737653004/diff/200001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_common.h (right):

https://codereview.chromium.org/2737653004/diff/200001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_common.h:180: bool has_scrolled_by_wheel;
where do these get init to false?

https://codereview.chromium.org/2737653004/diff/200001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_impl.h (right):

https://codereview.chromium.org/2737653004/diff/200001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.h:861: bool has_scrolled_by_wheel_;
where do these get init to false?

https://codereview.chromium.org/2737653004/diff/200001/content/renderer/gpu/r...
File content/renderer/gpu/render_widget_compositor_delegate.h (right):

https://codereview.chromium.org/2737653004/diff/200001/content/renderer/gpu/r...
content/renderer/gpu/render_widget_compositor_delegate.h:40: // tread.
thread

[sahel, 2017-03-17 18:45:28]

The CQ bit was checked by sahel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-17 18:46:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-17 20:56:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-17 20:56:42]

Dry run: This issue passed the CQ dry run.

[sahel, 2017-03-17 21:02:34]

https://codereview.chromium.org/2737653004/diff/200001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_common.h (right):

https://codereview.chromium.org/2737653004/diff/200001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_common.h:180: bool has_scrolled_by_wheel;
On 2017/03/16 15:19:21, danakj wrote:
> where do these get init to false?

Done.

https://codereview.chromium.org/2737653004/diff/200001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_impl.h (right):

https://codereview.chromium.org/2737653004/diff/200001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.h:861: bool has_scrolled_by_wheel_;
On 2017/03/16 15:19:21, danakj wrote:
> where do these get init to false?

Done.

https://codereview.chromium.org/2737653004/diff/200001/content/renderer/gpu/r...
File content/renderer/gpu/render_widget_compositor_delegate.h (right):

https://codereview.chromium.org/2737653004/diff/200001/content/renderer/gpu/r...
content/renderer/gpu/render_widget_compositor_delegate.h:40: // tread.
On 2017/03/16 15:19:21, danakj wrote:
> thread

Done.

[danakj, 2017-03-20 15:22:31]

cc LGTM

[esprehn, 2017-03-20 23:12:42]

esprehn@chromium.org changed reviewers:
+ dglazkov@chromium.org

[esprehn, 2017-03-20 23:12:54]

esprehn@chromium.org changed reviewers:
- esprehn@chromium.org

[sahel, 2017-03-22 14:45:31]

sahel@chromium.org changed reviewers:
- dglazkov@chromium.org

[sahel, 2017-03-22 14:48:52]

sahel@chromium.org changed reviewers:
+ pfeldman@chromium.org

[sahel, 2017-03-22 14:48:53]

pfeldman@chromium.org: Please review changes in 

content/renderer/*
third_party/Webkit/*

[pfeldman, 2017-03-22 17:16:54]

That's a lot of plumbing for a use counter signal. Could you explain the data
flow and reasoning behind it? What bit do you need to plumb and where?

[sahel, 2017-03-22 18:34:20]

On 2017/03/22 17:16:54, pfeldman wrote:
> That's a lot of plumbing for a use counter signal. Could you explain the data
> flow and reasoning behind it? What bit do you need to plumb and where?

I want to record usage of different scroll sources by use counters. I can update
the counters in scrollManager if the scroll is handled by main thread. However;
for scrolls that are handled in the compositor side I need to plumb the usage
info to the main thread to record it.

To avoid extra communication overhead, I use already existing ScrollAndScaleSet
struct to save the scroll source info. In proxy_impl scroll_info is stored as
part of the begin_frame_state and in proxy_main,
layer_tree_host_->ApplyScrollAndScale is called with the stored scroll_info.

Starting form layer_tree_host, I followed the identical path that exists for
ApplyViewportDeltas:
A helper function in layer_tree_host that calls
client_->RecordWheelAndTouchScrollingCount, the client_ which is
RenderWidgetCompositor, calls delegate_->RecordWheelAndTouchScrollingCount, the
delegate_ which is RenderWidget calls
GetWebWidget()->RecordWheelAndTouchScrollingCount, and GetWebWidget returns
WebViewFrameWidget which calls m_webView->RecordWheelAndTouchScrollingCount.
Finally m_webview is a pointer to WebViewImpl which updates the use counters.

[bokan, 2017-03-22 18:40:57]

Just to add: an obvious followup question is "why not just record the metric
from CC"? The reason being that we can't use the Blink UseCounter from the CC
impl-threa. We want this metric to record a yes/no per page load. Recording
stats for per page navigation is filled with subtle issues that have been
painfully worked out in UseCounter so I think the plumbing is preferable to
rolling our own per-page stats tracking.

[pfeldman, 2017-03-22 18:58:26]

I see. I ask because there is an ongoing project called "GRC" that aims at
removing this exact use case. It is supposed to eliminate the need for custom
plumbing for the sake of UMA and scheduling altogether. Unfortunately, it is too
early to point you at the API that you would be able to use immediately. We have
instrumentation in Blink covered, platform instrumentation is being landed as we
speak. But CC is not there yet and the part that plumbs it into the browser is
not yet there.

I'm adding oystein@ to track your use case and make sure it is covered with the
GRC. If it is essential to land it asap, there is no good reason to block you.
If that is something that can wait, it be best if we could work on this use case
together and make sure it is covered.

[pfeldman, 2017-03-22 18:58:48]

pfeldman@chromium.org changed reviewers:
+ oysteine@chromium.org

[pfeldman, 2017-03-22 19:06:29]

(/public/web/WebWidget.h public API that you expose is particularly awkward -
you plumb this information cc -> renderer -> blink only for it to go back into
renderer -> browser to be accounted for later). I understand your rationale here
though, it is just sad that we don't have an API for you.

[bokan, 2017-03-22 19:16:13]

What's the approximate timeline like for the new API? This metric isn't urgent,
but we've already landed metrics for the main thread including all scrollbar and
keyboard, in addition to these, so for comparison purposes it'd be useful to
have these landed in a reasonable time frame.

[oystein (OOO til 10th of July), 2017-03-23 21:07:50]

On 2017/03/22 at 19:16:13, bokan wrote:
> What's the approximate timeline like for the new API? This metric isn't
urgent, but we've already landed metrics for the main thread including all
scrollbar and keyboard, in addition to these, so for comparison purposes it'd be
useful to have these landed in a reasonable time frame.

The GRC service itself should hopefully land very shortly (I have a prototype CL
which has gotten good feedback, and will start splitting that up and landing it
ASAP). That mainly solves the plumbing issue itself though; using it for
UseCounters is a use-case we haven't discussed before, but one that does make
sense to me given that (as obvious from this CL) non-Blink code will want to
interact with it.

I think the right move here is to not block on the GRC, especially given that we
need to put some thought into how UseCounters would interact with the GRC
strictly beyond the plumbing issue, but adding a TODO and linking to
crbug.com/691886 would be appreciated. I'll add this case to the list of
usecases we want the GRC to be able to handle over time.

[sahel, 2017-03-24 20:01:08]

Patchset #8 (id:240001) has been deleted

[sahel, 2017-03-24 20:06:21]

The CQ bit was checked by sahel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-24 20:06:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-24 21:39:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-24 21:39:29]

Dry run: This issue passed the CQ dry run.

[sahel, 2017-03-27 14:34:09]

I added the TODO for GRC use.

[pfeldman, 2017-03-27 21:10:45]

lgtm

[sahel, 2017-03-27 21:25:27]

The CQ bit was checked by sahel@chromium.org

[sahel, 2017-03-27 21:25:28]

The patchset sent to the CQ was uploaded after l-g-t-m from bokan@chromium.org,
aelias@chromium.org, danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/2737653004/#ps280001
(title: "TODO for GRC usage added.")

[commit-bot: I haz the power, 2017-03-27 21:26:00]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-27 23:48:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-27 23:48:06]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[sahel, 2017-03-28 14:46:03]

The CQ bit was checked by sahel@chromium.org

[commit-bot: I haz the power, 2017-03-28 14:46:25]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-28 17:03:13]

CQ is committing da patch.

Bot data: {"patchset_id": 280001, "attempt_start_ts": 1490712363753540,
"parent_rev": "3a1acac93195ccb06ca7d19fc40c1d81be32987b", "commit_rev":
"1b47fda711331d9d39563ce0749c5f515b186169"}

[commit-bot: I haz the power, 2017-03-28 17:08:42]

Description was changed from

==========
UMA metrics for use count of wheel and touch scrolls.

For Impl thread scrolling test, run the virtual threaded/fast/scroll-behavior
testSuite.

BUG=699272
TEST=LayoutTests/fast/scroll-behavior/wheel-and-touch-scroll-use-count.html
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
UMA metrics for use count of wheel and touch scrolls.

For Impl thread scrolling test, run the virtual threaded/fast/scroll-behavior
testSuite.

BUG=699272
TEST=LayoutTests/fast/scroll-behavior/wheel-and-touch-scroll-use-count.html
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2737653004
Cr-Commit-Position: refs/heads/master@{#460140}
Committed:
https://chromium.googlesource.com/chromium/src/+/1b47fda711331d9d39563ce0749c...
==========

[commit-bot: I haz the power, 2017-03-28 17:08:44]

Committed patchset #9 (id:280001) as
https://chromium.googlesource.com/chromium/src/+/1b47fda711331d9d39563ce0749c...