Add UMA metric for tracking root level listeners for blocking touch.

Add UMA metric for tracking root level listeners for blocking touch.

Adjust histogram to capture on each phase of targeting the event instead
of the old histogram which targeted only where the event handler was
registered.

BUG=604790
Committed: https://crrev.com/05f9fdad56cd5018c8fb31f1866b232d7444f796
Cr-Commit-Position: refs/heads/master@{#394982}

[dtapuska, 2016-05-16 19:13:40]

Description was changed from

==========
Add UMA metric for tracking root level listeners for blocking touch.

Adjust histogram to capture on each phase of targeting the event instead
of the old histogram which targeted only where the event handler was
registered.

BUG=604790
==========

to

==========
Add UMA metric for tracking root level listeners for blocking touch.

Adjust histogram to capture on each phase of targeting the event instead
of the old histogram which targeted only where the event handler was
registered.

BUG=604790
==========

[dtapuska, 2016-05-16 19:13:40]

dtapuska@chromium.org changed reviewers:
+ rbyers@chromium.org, tdresser@chromium.org

[dtapuska, 2016-05-16 19:17:25]

Does this adequately fix the issue with reporting the event target.

Note that events that don't have event target data won't be reported.

But as soon as it has added any event handler it will start getting reported;
for example if it added a keydown event handler it will (only on touch events
though)

[tdresser, 2016-05-17 14:56:54]

Dave and I just talked over a few changes here:
- Only record touch event listeners
- Only record for blocking events

Rick - what do you think of this super fine grained approach? It's potentially
more data than we need, but with timeline formulas, we'll still be able to
generate exactly the graphs we want to look at.

It seems plausible that there would be some valuable trends we could identify by
breaking down capturing vs non-capturing listeners.

https://codereview.chromium.org/1980113002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/events/Event.h (right):

https://codereview.chromium.org/1980113002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/events/Event.h:121: virtual void
doneDispatchedEventAtCurrentTarget();
doneDispatchingEventAtCurrentTarget

https://codereview.chromium.org/1980113002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/events/TouchEvent.cpp (right):

https://codereview.chromium.org/1980113002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/events/TouchEvent.cpp:96: void
logTouchTargetHistogram(EventTarget* eventTarget, unsigned short phase, bool
wasCanceled, bool isCanceled)
wasCancelled could use a bit more detail. 

wasCancelledBeforeCurrentTarget?

https://codereview.chromium.org/1980113002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/events/TouchEvent.h (right):

https://codereview.chromium.org/1980113002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/events/TouchEvent.h:104: bool
m_previousDefaultPrevented;
Previous what?

Maybe:
bool defaultPreventedBeforeCurrentTarget?

[dtapuska, 2016-05-17 20:50:14]

PTAL; I think I've addressed the concerns Tim and I talked about this am.

https://codereview.chromium.org/1980113002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/events/Event.h (right):

https://codereview.chromium.org/1980113002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/events/Event.h:121: virtual void
doneDispatchedEventAtCurrentTarget();
On 2016/05/17 14:56:54, tdresser wrote:
> doneDispatchingEventAtCurrentTarget

Done.

https://codereview.chromium.org/1980113002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/events/TouchEvent.cpp (right):

https://codereview.chromium.org/1980113002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/events/TouchEvent.cpp:96: void
logTouchTargetHistogram(EventTarget* eventTarget, unsigned short phase, bool
wasCanceled, bool isCanceled)
On 2016/05/17 14:56:54, tdresser wrote:
> wasCancelled could use a bit more detail. 
> 
> wasCancelledBeforeCurrentTarget?

Done.

https://codereview.chromium.org/1980113002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/events/TouchEvent.h (right):

https://codereview.chromium.org/1980113002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/events/TouchEvent.h:104: bool
m_previousDefaultPrevented;
On 2016/05/17 14:56:54, tdresser wrote:
> Previous what?
> 
> Maybe:
> bool defaultPreventedBeforeCurrentTarget?

Done.

[tdresser, 2016-05-18 13:23:10]

LGTM - if it's reasonably straight forward to add a test for this please do, but
I suspect it would be a pain, and we shouldn't block on it.

https://codereview.chromium.org/1980113002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/events/EventTarget.cpp (right):

https://codereview.chromium.org/1980113002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/events/EventTarget.cpp:504: // metric where it
probably shouldn't because it was never fired.
Can you file and link a bug for this?

https://codereview.chromium.org/1980113002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/events/TouchEvent.cpp (right):

https://codereview.chromium.org/1980113002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/events/TouchEvent.cpp:55:
CapturingNonRootScrollerNonScrollableAlreadyHandled, // Non-root-scroller,
non-scrollable document, already handled.
Maybe add a comment per section on Capturing/Bubbling/Target.

[dtapuska, 2016-05-18 15:24:05]

https://codereview.chromium.org/1980113002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/events/EventTarget.cpp (right):

https://codereview.chromium.org/1980113002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/events/EventTarget.cpp:504: // metric where it
probably shouldn't because it was never fired.
On 2016/05/18 13:23:10, tdresser wrote:
> Can you file and link a bug for this?

Done.

https://codereview.chromium.org/1980113002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/events/TouchEvent.cpp (right):

https://codereview.chromium.org/1980113002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/events/TouchEvent.cpp:55:
CapturingNonRootScrollerNonScrollableAlreadyHandled, // Non-root-scroller,
non-scrollable document, already handled.
On 2016/05/18 13:23:10, tdresser wrote:
> Maybe add a comment per section on Capturing/Bubbling/Target.

Done.

[Rick Byers, 2016-05-18 18:37:19]

https://codereview.chromium.org/1980113002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/events/Event.h (right):

https://codereview.chromium.org/1980113002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/events/Event.h:121: virtual void
doneDispatchingEventAtCurrentTarget();
nit: you could just inline the empty function here (consistent with the other
trivial functions nearby).

https://codereview.chromium.org/1980113002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/events/Event.h:121: virtual void
doneDispatchingEventAtCurrentTarget();
please add a comment to this function - i.e. document that it's only invoked
when listeners were actually invoked and it's probably a good idea to mention
that should only be used to influence metrics not functionality (observing the
presence of listeners is dangerous).

https://codereview.chromium.org/1980113002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/events/TouchEvent.cpp (right):

https://codereview.chromium.org/1980113002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/events/TouchEvent.cpp:42: // These offsets change
indicies into the ListenerHistogram
this is quite a lot of complexity / details.  Talking with you and Tim I agree
this is worthwhile in figuring out our touch scroll intervention, but once we've
got that figured out we should probably remove all this.  To make that clear
please add a TODO comment saying to remove this once http://crbug.com/599609 is
fixed.

https://codereview.chromium.org/1980113002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/events/TouchEvent.cpp:230: if (!cancelable())
This histogram is going to be swamped out by the (canceled) touchmove events
that occur during a drag, right?  Don't you really want to focus on the events
that can block scrolling - i.e. touchstart and touchmove events that occur
before "movedBeyondSlopRegion" is true?

[tdresser, 2016-05-18 18:57:13]

https://codereview.chromium.org/1980113002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/events/TouchEvent.cpp (right):

https://codereview.chromium.org/1980113002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/events/TouchEvent.cpp:230: if (!cancelable())
On 2016/05/18 18:37:18, Rick Byers wrote:
> This histogram is going to be swamped out by the (canceled) touchmove events
> that occur during a drag, right?  Don't you really want to focus on the events
> that can block scrolling - i.e. touchstart and touchmove events that occur
> before "movedBeyondSlopRegion" is true?

+1, thanks.

[dtapuska, 2016-05-19 16:11:21]

PTAL

https://codereview.chromium.org/1980113002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/events/Event.h (right):

https://codereview.chromium.org/1980113002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/events/Event.h:121: virtual void
doneDispatchingEventAtCurrentTarget();
On 2016/05/18 18:37:18, Rick Byers wrote:
> please add a comment to this function - i.e. document that it's only invoked
> when listeners were actually invoked and it's probably a good idea to mention
> that should only be used to influence metrics not functionality (observing the
> presence of listeners is dangerous).

Done.

https://codereview.chromium.org/1980113002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/events/Event.h:121: virtual void
doneDispatchingEventAtCurrentTarget();
On 2016/05/18 18:37:18, Rick Byers wrote:
> nit: you could just inline the empty function here (consistent with the other
> trivial functions nearby).

Done.

https://codereview.chromium.org/1980113002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/events/TouchEvent.cpp (right):

https://codereview.chromium.org/1980113002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/events/TouchEvent.cpp:42: // These offsets change
indicies into the ListenerHistogram
On 2016/05/18 18:37:18, Rick Byers wrote:
> this is quite a lot of complexity / details.  Talking with you and Tim I agree
> this is worthwhile in figuring out our touch scroll intervention, but once
we've
> got that figured out we should probably remove all this.  To make that clear
> please add a TODO comment saying to remove this once http://crbug.com/599609
is
> fixed.

Done.

https://codereview.chromium.org/1980113002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/events/TouchEvent.cpp:230: if (!cancelable())
On 2016/05/18 18:57:12, tdresser wrote:
> On 2016/05/18 18:37:18, Rick Byers wrote:
> > This histogram is going to be swamped out by the (canceled) touchmove events
> > that occur during a drag, right?  Don't you really want to focus on the
events
> > that can block scrolling - i.e. touchstart and touchmove events that occur
> > before "movedBeyondSlopRegion" is true?
> 
> +1, thanks.

Done.

[Rick Byers, 2016-05-19 16:30:51]

https://codereview.chromium.org/1980113002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/events/TouchEvent.cpp (right):

https://codereview.chromium.org/1980113002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/events/TouchEvent.cpp:240: // scrolling, have
more than one touch point or aren't on the main frame.
this all makes sense to me except the "aren't on the main frame" part.  Why does
that matter?  Eg. if I touch down inside a youtube frame when scrolling a page
and it's touch listeners result in scroll jank, then I still care right?  Or
maybe the problem is just in interpreting the meaning of "document-level" in
that case.

Certainly focusing on main-frame for now should get us most of the benefit and
simplify the analysis, but we'd have to constrain the first version of an
intervention to apply only to main-frame touches, right?

https://codereview.chromium.org/1980113002/diff/80001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/1980113002/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:12391: An enumeration identifying 3
properties: 1) a classification of the event
please update the histogram description to describe precisely what it's
measuring and when - we always get this confused when looking at the dashboard
:-)

[dtapuska, 2016-05-19 18:14:23]

https://codereview.chromium.org/1980113002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/events/TouchEvent.cpp (right):

https://codereview.chromium.org/1980113002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/events/TouchEvent.cpp:240: // scrolling, have
more than one touch point or aren't on the main frame.
On 2016/05/19 16:30:51, Rick Byers wrote:
> this all makes sense to me except the "aren't on the main frame" part.  Why
does
> that matter?  Eg. if I touch down inside a youtube frame when scrolling a page
> and it's touch listeners result in scroll jank, then I still care right?  Or
> maybe the problem is just in interpreting the meaning of "document-level" in
> that case.
> 
> Certainly focusing on main-frame for now should get us most of the benefit and
> simplify the analysis, but we'd have to constrain the first version of an
> intervention to apply only to main-frame touches, right?

Yes I've updated the histogram description.

https://codereview.chromium.org/1980113002/diff/80001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/1980113002/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:12391: An enumeration identifying 3
properties: 1) a classification of the event
On 2016/05/19 16:30:51, Rick Byers wrote:
> please update the histogram description to describe precisely what it's
> measuring and when - we always get this confused when looking at the dashboard
> :-)

Done.

[Rick Byers, 2016-05-19 19:44:45]

LGTM, thanks!

[dtapuska, 2016-05-19 20:43:49]

dtapuska@chromium.org changed reviewers:
+ isherman@chromium.org

[dtapuska, 2016-05-19 20:44:13]

On 2016/05/19 19:44:45, Rick Byers wrote:
> LGTM, thanks!

+isherman for histograms.xml

[Ilya Sherman, 2016-05-19 22:23:23]

histogram lgtm

https://codereview.chromium.org/1980113002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/events/TouchEvent.cpp (right):

https://codereview.chromium.org/1980113002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/TouchEvent.cpp:99: };
Optional: Rather than defining one long enum with offsets, it seems a little bit
simpler to define three separate enums, and explicitly compute their
cross-product.

[dtapuska, 2016-05-20 00:08:58]

The CQ bit was checked by dtapuska@chromium.org

[dtapuska, 2016-05-20 00:08:59]

The patchset sent to the CQ was uploaded after l-g-t-m from
tdresser@chromium.org
Link to the patchset: https://codereview.chromium.org/1980113002/#ps100001
(title: "Update histogram description")

[commit-bot: I haz the power, 2016-05-20 00:10:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1980113002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1980113002/100001

[commit-bot: I haz the power, 2016-05-20 03:33:52]

Description was changed from

==========
Add UMA metric for tracking root level listeners for blocking touch.

Adjust histogram to capture on each phase of targeting the event instead
of the old histogram which targeted only where the event handler was
registered.

BUG=604790
==========

to

==========
Add UMA metric for tracking root level listeners for blocking touch.

Adjust histogram to capture on each phase of targeting the event instead
of the old histogram which targeted only where the event handler was
registered.

BUG=604790
==========

[commit-bot: I haz the power, 2016-05-20 03:33:53]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-05-20 03:36:20]

Description was changed from

==========
Add UMA metric for tracking root level listeners for blocking touch.

Adjust histogram to capture on each phase of targeting the event instead
of the old histogram which targeted only where the event handler was
registered.

BUG=604790
==========

to

==========
Add UMA metric for tracking root level listeners for blocking touch.

Adjust histogram to capture on each phase of targeting the event instead
of the old histogram which targeted only where the event handler was
registered.

BUG=604790

Committed: https://crrev.com/05f9fdad56cd5018c8fb31f1866b232d7444f796
Cr-Commit-Position: refs/heads/master@{#394982}
==========

[commit-bot: I haz the power, 2016-05-20 03:36:22]

Patchset 6 (id:??) landed as
https://crrev.com/05f9fdad56cd5018c8fb31f1866b232d7444f796
Cr-Commit-Position: refs/heads/master@{#394982}