Add UMA metric for tracking listeners for blocking touch before page finished loading

Add UMA metric for tracking listeners for blocking touch before page finished loading

In the touch scrolling intervention proposal, rbyers@ proposed that we could treat
all touch event listeners as passive until the load event fires. We are adding metrics
that record when touchstart or first touchmove events are successfully preventDefaulted()
before the load event to measure breakage of event handlers.

BUG=601179
Committed: https://crrev.com/0e80d7ffdef2a817d66be17a0e9fe68029f6d8d7
Cr-Commit-Position: refs/heads/master@{#390257}

[lanwei, 2016-04-18 21:27:56]

Description was changed from

==========
pageload


 mpageload

BUG=601179
==========

to

==========
Add UMA metric for tracking listeners for blocking touch before page finished
loading

In the touch scrolling intervention proposal, rbyers@ proposed that we could
treat
all touch event listeners as passive until the load event fires. We are adding
metrics
that record when touchstart or first touchmove events are successfully
preventDefaulted()
before the load event to measure breakage of event handlers.
 
BUG=601179
==========

[lanwei, 2016-04-18 21:27:57]

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

[lanwei, 2016-04-21 19:02:26]

Patchset #1 (id:1) has been deleted

[lanwei, 2016-04-21 19:02:37]

Patchset #1 (id:20001) has been deleted

[lanwei, 2016-04-21 19:02:45]

Patchset #1 (id:40001) has been deleted

[lanwei, 2016-04-22 01:58:53]

Patchset #1 (id:60001) has been deleted

[lanwei, 2016-04-22 01:59:09]

Patchset #1 (id:80001) has been deleted

[lanwei, 2016-04-22 16:17:13]

Patchset #2 (id:120001) has been deleted

[dtapuska, 2016-04-22 17:34:14]

https://codereview.chromium.org/1879233005/diff/130004/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/1879233005/diff/130004/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/EventHandler.cpp:3901:
DEFINE_STATIC_LOCAL(EnumerationHistogram, pageloadListenerHistogram,
("Event.Touch.TouchBlockingOnPageload", 2));
I'd prefer if these were called out as enums; as opposed to a magical 0, 1,
boolean.

[tdresser, 2016-04-22 17:34:52]

In histograms.xml, Touchs -> Touches.

https://codereview.chromium.org/1879233005/diff/130004/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/1879233005/diff/130004/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/EventHandler.cpp:3901:
DEFINE_STATIC_LOCAL(EnumerationHistogram, pageloadListenerHistogram,
("Event.Touch.TouchBlockingOnPageload", 2));
Let's have two histograms, once for before pageload, and once for after
pageload.

Event.Touch.TouchesCancelledBeforePageLoad
Event.Touch.TouchesCancelledAfterPageLoad

or something like that?

We should probably only be looking at cancelable touch events, right? Or we
should break the histogram up into more buckets, similar to what Dave has done.

[tdresser, 2016-04-22 17:37:06]

https://codereview.chromium.org/1879233005/diff/130004/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/1879233005/diff/130004/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/EventHandler.cpp:3901:
DEFINE_STATIC_LOCAL(EnumerationHistogram, pageloadListenerHistogram,
("Event.Touch.TouchBlockingOnPageload", 2));
On 2016/04/22 17:34:52, tdresser wrote:
> Let's have two histograms, once for before pageload, and once for after
> pageload.
> 
> Event.Touch.TouchesCancelledBeforePageLoad
> Event.Touch.TouchesCancelledAfterPageLoad
> 
> or something like that?
> 
> We should probably only be looking at cancelable touch events, right? Or we
> should break the histogram up into more buckets, similar to what Dave has
done.

Sorry, missed the condition above that checked that we're only looking at
cancelable events.

[dtapuska, 2016-04-22 17:39:10]

https://codereview.chromium.org/1879233005/diff/130004/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/1879233005/diff/130004/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/EventHandler.cpp:3901:
DEFINE_STATIC_LOCAL(EnumerationHistogram, pageloadListenerHistogram,
("Event.Touch.TouchBlockingOnPageload", 2));
On 2016/04/22 17:37:06, tdresser wrote:
> On 2016/04/22 17:34:52, tdresser wrote:
> > Let's have two histograms, once for before pageload, and once for after
> > pageload.
> > 
> > Event.Touch.TouchesCancelledBeforePageLoad
> > Event.Touch.TouchesCancelledAfterPageLoad
> > 
> > or something like that?
> > 
> > We should probably only be looking at cancelable touch events, right? Or we
> > should break the histogram up into more buckets, similar to what Dave has
> done.
> 
> Sorry, missed the condition above that checked that we're only looking at
> cancelable events.

How do we want to report this for iframes?

[tdresser, 2016-04-22 17:51:14]

https://codereview.chromium.org/1879233005/diff/130004/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/1879233005/diff/130004/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/EventHandler.cpp:3901:
DEFINE_STATIC_LOCAL(EnumerationHistogram, pageloadListenerHistogram,
("Event.Touch.TouchBlockingOnPageload", 2));
On 2016/04/22 17:39:10, dtapuska wrote:
> On 2016/04/22 17:37:06, tdresser wrote:
> > On 2016/04/22 17:34:52, tdresser wrote:
> > > Let's have two histograms, once for before pageload, and once for after
> > > pageload.
> > > 
> > > Event.Touch.TouchesCancelledBeforePageLoad
> > > Event.Touch.TouchesCancelledAfterPageLoad
> > > 
> > > or something like that?
> > > 
> > > We should probably only be looking at cancelable touch events, right? Or
we
> > > should break the histogram up into more buckets, similar to what Dave has
> > done.
> > 
> > Sorry, missed the condition above that checked that we're only looking at
> > cancelable events.
> 
> How do we want to report this for iframes?

I think we should only care about the top frame's document. It won't be
considered loaded until all subframes are loaded.

[dtapuska, 2016-04-22 18:02:35]

https://codereview.chromium.org/1879233005/diff/130004/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/1879233005/diff/130004/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/EventHandler.cpp:3901:
DEFINE_STATIC_LOCAL(EnumerationHistogram, pageloadListenerHistogram,
("Event.Touch.TouchBlockingOnPageload", 2));
On 2016/04/22 17:51:13, tdresser wrote:
> On 2016/04/22 17:39:10, dtapuska wrote:
> > On 2016/04/22 17:37:06, tdresser wrote:
> > > On 2016/04/22 17:34:52, tdresser wrote:
> > > > Let's have two histograms, once for before pageload, and once for after
> > > > pageload.
> > > > 
> > > > Event.Touch.TouchesCancelledBeforePageLoad
> > > > Event.Touch.TouchesCancelledAfterPageLoad
> > > > 
> > > > or something like that?
> > > > 
> > > > We should probably only be looking at cancelable touch events, right? Or
> we
> > > > should break the histogram up into more buckets, similar to what Dave
has
> > > done.
> > > 
> > > Sorry, missed the condition above that checked that we're only looking at
> > > cancelable events.
> > 
> > How do we want to report this for iframes?
> 
> I think we should only care about the top frame's document. It won't be
> considered loaded until all subframes are loaded.

Right so the !m_frame->document()->ownerElement() should move back up to the top
if conditional.

[dtapuska, 2016-04-22 22:48:12]

https://codereview.chromium.org/1879233005/diff/170001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/1879233005/diff/170001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/EventHandler.cpp:3904: if
(m_frame->document()->isLoadCompleted()) {
Why can't this be a single histogram? Also canceled I believe is the preferred
US spelling; but best to do a grep to confirm.

[lanwei, 2016-04-22 23:55:24]

https://codereview.chromium.org/1879233005/diff/170001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/1879233005/diff/170001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/EventHandler.cpp:3904: if
(m_frame->document()->isLoadCompleted()) {
On 2016/04/22 22:48:12, dtapuska wrote:
> Why can't this be a single histogram? Also canceled I believe is the preferred
> US spelling; but best to do a grep to confirm. 

I did a search of 'canceled' and 'cancelled', they occur 1,639 and 1,663 time in
the code respectively. You are right, in American English, it is one 'l'.

We can use one histogram, and have four different buckets. I do not know which
way is better. Tim, what do you think?

[tdresser, 2016-04-25 15:45:19]

https://codereview.chromium.org/1879233005/diff/170001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/1879233005/diff/170001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/EventHandler.cpp:3904: if
(m_frame->document()->isLoadCompleted()) {
On 2016/04/22 23:55:23, lanwei wrote:
> On 2016/04/22 22:48:12, dtapuska wrote:
> > Why can't this be a single histogram? Also canceled I believe is the
preferred
> > US spelling; but best to do a grep to confirm. 
> 
> I did a search of 'canceled' and 'cancelled', they occur 1,639 and 1,663 time
in
> the code respectively. You are right, in American English, it is one 'l'.
> 
> We can use one histogram, and have four different buckets. I do not know which
> way is better. Tim, what do you think?

It's somewhat easier to view a graph of the proportion that are cancelled
before/after pageload if we use two separate histograms.

The tools will now let us do this either way, but the tools will be simpler to
use if we split it up.

[dtapuska, 2016-04-25 19:49:09]

https://codereview.chromium.org/1879233005/diff/170001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/1879233005/diff/170001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/EventHandler.cpp:3904: if
(m_frame->document()->isLoadCompleted()) {
On 2016/04/25 15:45:19, tdresser wrote:
> On 2016/04/22 23:55:23, lanwei wrote:
> > On 2016/04/22 22:48:12, dtapuska wrote:
> > > Why can't this be a single histogram? Also canceled I believe is the
> preferred
> > > US spelling; but best to do a grep to confirm. 
> > 
> > I did a search of 'canceled' and 'cancelled', they occur 1,639 and 1,663
time
> in
> > the code respectively. You are right, in American English, it is one 'l'.
> > 
> > We can use one histogram, and have four different buckets. I do not know
which
> > way is better. Tim, what do you think?
> 
> It's somewhat easier to view a graph of the proportion that are cancelled
> before/after pageload if we use two separate histograms.
> 
> The tools will now let us do this either way, but the tools will be simpler to
> use if we split it up.

Two buckets seems fine to me. Inside blink source Canceled is used 240 times vs
Cancelled which is 107. Being that the 4 lines you add here it appears 2
different ways I'd prefer consistent and uses the US spelling.

[lanwei, 2016-04-25 20:29:34]

https://codereview.chromium.org/1879233005/diff/170001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/1879233005/diff/170001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/EventHandler.cpp:3904: if
(m_frame->document()->isLoadCompleted()) {
On 2016/04/25 19:49:09, dtapuska wrote:
> On 2016/04/25 15:45:19, tdresser wrote:
> > On 2016/04/22 23:55:23, lanwei wrote:
> > > On 2016/04/22 22:48:12, dtapuska wrote:
> > > > Why can't this be a single histogram? Also canceled I believe is the
> > preferred
> > > > US spelling; but best to do a grep to confirm. 
> > > 
> > > I did a search of 'canceled' and 'cancelled', they occur 1,639 and 1,663
> time
> > in
> > > the code respectively. You are right, in American English, it is one 'l'.
> > > 
> > > We can use one histogram, and have four different buckets. I do not know
> which
> > > way is better. Tim, what do you think?
> > 
> > It's somewhat easier to view a graph of the proportion that are cancelled
> > before/after pageload if we use two separate histograms.
> > 
> > The tools will now let us do this either way, but the tools will be simpler
to
> > use if we split it up.
> 
> Two buckets seems fine to me. Inside blink source Canceled is used 240 times
vs
> Cancelled which is 107. Being that the 4 lines you add here it appears 2
> different ways I'd prefer consistent and uses the US spelling.

Done.

[tdresser, 2016-04-26 15:54:58]

This LGTM, assuming you've done thorough local testing.

[lanwei, 2016-04-26 19:59:47]

On 2016/04/26 15:54:58, tdresser wrote:
> This LGTM, assuming you've done thorough local testing.

Yes, I did local testing on chrome://histograms/, thanks!

[dtapuska, 2016-04-26 20:05:09]

On 2016/04/26 19:59:47, lanwei wrote:
> On 2016/04/26 15:54:58, tdresser wrote:
> > This LGTM, assuming you've done thorough local testing.
> 
> Yes, I did local testing on chrome://histograms/, thanks!

lgtm

[lanwei, 2016-04-26 20:12:50]

lanwei@chromium.org changed reviewers:
+ bokan@chromium.org, isherman@chromium.org

[bokan, 2016-04-26 20:25:21]

lgtm

[Ilya Sherman, 2016-04-26 20:27:12]

https://codereview.chromium.org/1879233005/diff/190001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/1879233005/diff/190001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/EventHandler.cpp:260: NotHandledTouches, //
Not handled touch events.
"Not handled events" -- does that mean "Unhandled events"?  Or "no (zero)
handled events"?  I'm finding this phrasing somewhat confusing.

https://codereview.chromium.org/1879233005/diff/190001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1879233005/diff/190001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:12144: +    finishes loading.
Hmm... when after the page finishes loading?

https://codereview.chromium.org/1879233005/diff/190001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:12153: +    finishes loading.
Hmm, I'm having a really hard time making sense of these descriptions.  Could
you please try to clarify them, perhaps writing a few sentences rather than just
one?

[lanwei, 2016-04-27 03:42:57]

Patchset #6 (id:210001) has been deleted

[lanwei, 2016-04-27 04:43:30]

https://codereview.chromium.org/1879233005/diff/190001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/1879233005/diff/190001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/EventHandler.cpp:260: NotHandledTouches, //
Not handled touch events.
On 2016/04/26 20:27:11, Ilya Sherman wrote:
> "Not handled events" -- does that mean "Unhandled events"?  Or "no (zero)
> handled events"?  I'm finding this phrasing somewhat confusing.

Done.

https://codereview.chromium.org/1879233005/diff/190001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1879233005/diff/190001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:12144: +    finishes loading.
On 2016/04/26 20:27:11, Ilya Sherman wrote:
> Hmm... when after the page finishes loading?

Done.

https://codereview.chromium.org/1879233005/diff/190001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:12153: +    finishes loading.
On 2016/04/26 20:27:11, Ilya Sherman wrote:
> Hmm, I'm having a really hard time making sense of these descriptions.  Could
> you please try to clarify them, perhaps writing a few sentences rather than
just
> one?

Done.

[tdresser, 2016-04-27 18:31:43]

In terms of metric naming and descriptions, how about something like:

Event.Touch.TouchDispositionsBeforePageLoad

Records the disposition (handled or not handled) of touch start events and the
first touchmove event per scroll. Only recorded before the page is fully loaded.

Event.Touch.TouchDispositionsAfterPageLoad

Records the disposition (handled or not handled) of touch start events and the
first touchmove event per scroll. Only recorded after the page is fully loaded.

[Ilya Sherman, 2016-04-27 19:51:16]

On 2016/04/27 18:31:43, tdresser wrote:
> In terms of metric naming and descriptions, how about something like:
> 
> Event.Touch.TouchDispositionsBeforePageLoad
> 
> Records the disposition (handled or not handled) of touch start events and the
> first touchmove event per scroll. Only recorded before the page is fully
loaded.
> 
> Event.Touch.TouchDispositionsAfterPageLoad
> 
> Records the disposition (handled or not handled) of touch start events and the
> first touchmove event per scroll. Only recorded after the page is fully
loaded.

Thanks, Tim :)  LGTM with these names and descriptions in place.

[lanwei, 2016-04-27 21:15:23]

The CQ bit was checked by lanwei@chromium.org

[lanwei, 2016-04-27 21:15:24]

The patchset sent to the CQ was uploaded after l-g-t-m from bokan@chromium.org,
tdresser@chromium.org, dtapuska@chromium.org, isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/1879233005/#ps250001
(title: "Change the histogram name and discription")

[commit-bot: I haz the power, 2016-04-27 21:15:58]

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

[commit-bot: I haz the power, 2016-04-28 00:36:35]

Description was changed from

==========
Add UMA metric for tracking listeners for blocking touch before page finished
loading

In the touch scrolling intervention proposal, rbyers@ proposed that we could
treat
all touch event listeners as passive until the load event fires. We are adding
metrics
that record when touchstart or first touchmove events are successfully
preventDefaulted()
before the load event to measure breakage of event handlers.
 
BUG=601179
==========

to

==========
Add UMA metric for tracking listeners for blocking touch before page finished
loading

In the touch scrolling intervention proposal, rbyers@ proposed that we could
treat
all touch event listeners as passive until the load event fires. We are adding
metrics
that record when touchstart or first touchmove events are successfully
preventDefaulted()
before the load event to measure breakage of event handlers.
 
BUG=601179
==========

[commit-bot: I haz the power, 2016-04-28 00:36:37]

Committed patchset #7 (id:250001)

[commit-bot: I haz the power, 2016-04-30 17:15:05]

Patchset 7 (id:??) landed as
https://crrev.com/0e80d7ffdef2a817d66be17a0e9fe68029f6d8d7
Cr-Commit-Position: refs/heads/master@{#390257}

[commit-bot: I haz the power, 2016-04-30 17:53:23]

Description was changed from

==========
Add UMA metric for tracking listeners for blocking touch before page finished
loading

In the touch scrolling intervention proposal, rbyers@ proposed that we could
treat
all touch event listeners as passive until the load event fires. We are adding
metrics
that record when touchstart or first touchmove events are successfully
preventDefaulted()
before the load event to measure breakage of event handlers.
 
BUG=601179
==========

to

==========
Add UMA metric for tracking listeners for blocking touch before page finished
loading

In the touch scrolling intervention proposal, rbyers@ proposed that we could
treat
all touch event listeners as passive until the load event fires. We are adding
metrics
that record when touchstart or first touchmove events are successfully
preventDefaulted()
before the load event to measure breakage of event handlers.

BUG=601179

Committed: https://crrev.com/0e80d7ffdef2a817d66be17a0e9fe68029f6d8d7
Cr-Commit-Position: refs/heads/master@{#390257}
==========