[Media Controls] Add UMA for timeline scrubber

[Media Controls] Add UMA for timeline scrubber

Adds UMA metrics to the media controls timeline <input type="range">, to
understand current usage, and detect changes in usage.

Specifically this adds:
- Media.Timeline.DragGestureDuration.<timeline width>
- Media.Timeline.DragPercent.<timeline width>
- Media.Timeline.DragSumAbsTimeDelta.<timeline width>
- Media.Timeline.DragTimeDelta.<timeline width>
- Media.Timeline.SeekType.<timeline width>
- Media.Timeline.Width
- Media.Timeline.Width.<Inline|Fullscreen><Portrait|Landscape>

BUG=693630
Review-Url: https://codereview.chromium.org/2779273003
Cr-Commit-Position: refs/heads/master@{#462847}
Committed: https://chromium.googlesource.com/chromium/src/+/00a232273b9dd6705d492dd4fd7218005db6f910

[johnme, 2017-03-29 19:15:14]

Description was changed from

==========
[Media Controls] Add UMA for timeline scrubber

Adds UMA metrics to the media controls timeline <input type="range">, to
understand current usage, and detect changes in usage.

Specifically this adds:
- Media.Timeline.DragGestureDuration.<timeline width>
- Media.Timeline.DragPercent.<timeline width>
- Media.Timeline.DragSumAbsTimeDelta.<timeline width>
- Media.Timeline.DragTimeDelta.<timeline width>
- Media.Timeline.SeekType.<timeline width>
- Media.Timeline.Width
- Media.Timeline.Width.<Inline|Fullscreen><Portrait|Landscape>


BUG=693630
==========

to

==========
[Media Controls] Add UMA for timeline scrubber

Adds UMA metrics to the media controls timeline <input type="range">, to
understand current usage, and detect changes in usage.

Specifically this adds:
- Media.Timeline.DragGestureDuration.<timeline width>
- Media.Timeline.DragPercent.<timeline width>
- Media.Timeline.DragSumAbsTimeDelta.<timeline width>
- Media.Timeline.DragTimeDelta.<timeline width>
- Media.Timeline.SeekType.<timeline width>
- Media.Timeline.Width
- Media.Timeline.Width.<Inline|Fullscreen><Portrait|Landscape>

BUG=693630
==========

[johnme, 2017-03-29 19:15:23]

johnme@chromium.org changed reviewers:
+ mlamouri@chromium.org

[mlamouri (slow - plz ping), 2017-03-30 21:43:00]

Few comments, will finish the review tomorrow.

https://codereview.chromium.org/2779273003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp
(right):

https://codereview.chromium.org/2779273003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:898:
timelineWidth());
I would have a slight preference to pass the orientationType value to this
method and let it do its own thing.

https://codereview.chromium.org/2779273003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/shadow/MediaControlElements.h (right):

https://codereview.chromium.org/2779273003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.h:288: int
timelineWidth();
getTimelineWidth() to be more Chromium friendly? :)

https://codereview.chromium.org/2779273003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/shadow/MediaControlTimelineMetrics.h
(right):

https://codereview.chromium.org/2779273003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/shadow/MediaControlTimelineMetrics.h:42:
kInactive,
nit: drop the k? :)

https://codereview.chromium.org/2779273003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/shadow/MediaControls.cpp (right):

https://codereview.chromium.org/2779273003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:852: }
Would it make sense to have this in onLoadedMetadata instead?
MediaContorlTimelineMetrics could keep track of whether it was recorded already?

https://codereview.chromium.org/2779273003/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/KeyboardCodes.h (right):

https://codereview.chromium.org/2779273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/KeyboardCodes.h:34: #include
"platform/wtf/build_config.h"
why?

https://codereview.chromium.org/2779273003/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2779273003/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:27491: + 
<owner>mlamouri@chromium.org</owner>
Here and below: why an empty line above? Why not adding yourself?

[mlamouri (slow - plz ping), 2017-03-31 11:39:40]

Some more comments. I will need probably another pass to fully digest :)

https://codereview.chromium.org/2779273003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/shadow/MediaControlTimelineMetrics.cpp
(right):

https://codereview.chromium.org/2779273003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/shadow/MediaControlTimelineMetrics.cpp:72:
constexpr int32_t kTimeDeltaBucketCount = 49;
(kAbsTimeDeltaBucketCount * 2) - 1

https://codereview.chromium.org/2779273003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/shadow/MediaControlTimelineMetrics.cpp:76:
"Intervals must match UMA MediaTimelineTimeDelta enum");
If you do the change above, it would make this static_assert moot I believe.

https://codereview.chromium.org/2779273003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/shadow/MediaControlTimelineMetrics.cpp:128:
}                                                                        \
Can you add a comment explaining that the if (false) is here so you can have a
bunch of ELSE?

https://codereview.chromium.org/2779273003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/shadow/MediaControlTimelineMetrics.cpp:148:
int32_t sample = static_cast<int32_t>(duration.InMilliseconds());
Here and below, why is it int32_t?

https://codereview.chromium.org/2779273003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/shadow/MediaControlTimelineMetrics.cpp:306:
constexpr int max = 7680;  // Timeline unlikely to be wider than 8K monitors.
+1 with the comment, especially in CSS pixels.

[johnme, 2017-03-31 13:47:24]

Addressed review comments - thanks!

https://codereview.chromium.org/2779273003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp
(right):

https://codereview.chromium.org/2779273003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:898:
timelineWidth());
On 2017/03/30 21:43:00, mlamouri wrote:
> I would have a slight preference to pass the orientationType value to this
> method and let it do its own thing.

Done.

https://codereview.chromium.org/2779273003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/shadow/MediaControlElements.h (right):

https://codereview.chromium.org/2779273003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.h:288: int
timelineWidth();
On 2017/03/30 21:43:00, mlamouri wrote:
> getTimelineWidth() to be more Chromium friendly? :)

Blink style guide says not to:
https://sites.google.com/a/chromium.org/dev/blink/coding-style
[names-setter-getter]

https://codereview.chromium.org/2779273003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/shadow/MediaControlTimelineMetrics.cpp
(right):

https://codereview.chromium.org/2779273003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/shadow/MediaControlTimelineMetrics.cpp:72:
constexpr int32_t kTimeDeltaBucketCount = 49;
On 2017/03/31 11:39:40, mlamouri wrote:
> (kAbsTimeDeltaBucketCount * 2) - 1

I'd rather not compute this, because it's potentially confusing that
kTimeDeltaMSIntervals corresponds to two UMA enums of different sizes (in fact I
updated the comment above kTimeDeltaMSIntervals to spell that out explicitly).

Having the numbers side by side here hopefully makes it a little clearer. I
added comments to each number clarifying that they must match their
corresponding UMA enum.

https://codereview.chromium.org/2779273003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/shadow/MediaControlTimelineMetrics.cpp:76:
"Intervals must match UMA MediaTimelineTimeDelta enum");
On 2017/03/31 11:39:40, mlamouri wrote:
> If you do the change above, it would make this static_assert moot I believe.

Ack. See above though.

https://codereview.chromium.org/2779273003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/shadow/MediaControlTimelineMetrics.cpp:128:
}                                                                        \
On 2017/03/31 11:39:40, mlamouri wrote:
> Can you add a comment explaining that the if (false) is here so you can have a
> bunch of ELSE?

Done.

https://codereview.chromium.org/2779273003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/shadow/MediaControlTimelineMetrics.cpp:148:
int32_t sample = static_cast<int32_t>(duration.InMilliseconds());
On 2017/03/31 11:39:40, mlamouri wrote:
> Here and below, why is it int32_t?

For samples, base::HistogramBase::Sample is typedef'd to int32_t (and I'm not
allowed to include base/ in blink to use that typedef).

For bucket counts, platform/Histogram.h explicitly uses int32_t (which is a
little odd, since base/metrics/histogram.h uses uint32_t, but oh well...).

(I've changed some of the ints in recordSeekTypeByWidth and recordPlaying to be
int32_t for consistency).

https://codereview.chromium.org/2779273003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/shadow/MediaControlTimelineMetrics.cpp:306:
constexpr int max = 7680;  // Timeline unlikely to be wider than 8K monitors.
On 2017/03/31 11:39:40, mlamouri wrote:
> +1 with the comment, especially in CSS pixels.

Changed to "Equivalent to an 80inch wide 8K monitor." to emphasize the impact of
this being in CSS pixels.

https://codereview.chromium.org/2779273003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/shadow/MediaControlTimelineMetrics.h
(right):

https://codereview.chromium.org/2779273003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/shadow/MediaControlTimelineMetrics.h:42:
kInactive,
On 2017/03/30 21:43:00, mlamouri wrote:
> nit: drop the k? :)

https://sites.google.com/a/chromium.org/dev/blink/coding-style
[names-enum-members] says kInterCaps is preferable for new code. Though perhaps
that advice predates enum classes, where the State:: prefix already makes it
clear that it's an enum value?

https://codereview.chromium.org/2779273003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/shadow/MediaControls.cpp (right):

https://codereview.chromium.org/2779273003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:852: }
On 2017/03/30 21:43:00, mlamouri wrote:
> Would it make sense to have this in onLoadedMetadata instead?

Moved to a new onPlayed event as discussed.

> MediaContorlTimelineMetrics could keep track of whether it was recorded
already?

Done.

https://codereview.chromium.org/2779273003/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/KeyboardCodes.h (right):

https://codereview.chromium.org/2779273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/KeyboardCodes.h:34: #include
"platform/wtf/build_config.h"
On 2017/03/30 21:43:00, mlamouri wrote:
> why?

Because OS(WIN) below requires the OS macro defined in build_config.h (it fails
to compile otherwise).

https://codereview.chromium.org/2779273003/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2779273003/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:27491: + 
<owner>mlamouri@chromium.org</owner>
On 2017/03/30 21:43:00, mlamouri wrote:
> Here and below: why an empty line above? Why not adding yourself?

Mandatory formatting tool requires an empty line after comments. Switched to
myself as owner.

[johnme, 2017-03-31 16:49:54]

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

[johnme, 2017-03-31 16:49:54]

isherman: Please review histograms.xml and
third_party/WebKit/Source/core/html/shadow/MediaControlTimelineMetrics.cpp which
calculates/reports all of them. Note that I do some unusual things here, in
particular:

- Media.Timeline.DragPercent and Media.Timeline.DragTimeDelta both need to
report negative values (with exponential buckets to either side of zero), which
isn't normally possible, so I defined custom enums for these, with code to
calculate the corresponding bucket. This seems much more usable than the
alternative of splitting the histogram into a negative and a positive one. A
nice side-effect of this is that it allowed human-readable bucket names, e.g.
"(30m, 1h]" rather than 1800000..3600000 (or worse, since exponential buckets
wouldn't naturally align to minutes).

- I reused the positive part of the custom enum calculation from
Media.Timeline.DragTimeDelta for Media.Timeline.DragSumAbsTimeDelta so that the
values can be compared easily.

- I used ranges of numbers for histogram suffixes, so the usual fun with
multiple DEFINE_STATIC_LOCALs, with an if-elseif-elseif instead of a switch
statement. Unfortunately, the interpretation of these metrics depends heavily on
the width of the timeline element so they all need to be suffixed, but I tried
to minimise the number of suffixes to avoid histogram bloat (hence for example
there are no suffixes <32 or >512).

Whilst unusual, these are the best compromises I can find between histogram
usability, number of histograms*suffixes, and code complexity (which is
carefully isolated to MediaControlTimelineMetrics.cpp). I've tried to clearly
comment the unusual behavior.

[Ilya Sherman, 2017-04-01 00:42:27]

On 2017/03/31 16:49:54, johnme wrote:
> isherman: Please review histograms.xml and
> third_party/WebKit/Source/core/html/shadow/MediaControlTimelineMetrics.cpp
which
> calculates/reports all of them. Note that I do some unusual things here, in
> particular:
> 
> - Media.Timeline.DragPercent and Media.Timeline.DragTimeDelta both need to
> report negative values (with exponential buckets to either side of zero),
which
> isn't normally possible, so I defined custom enums for these, with code to
> calculate the corresponding bucket. This seems much more usable than the
> alternative of splitting the histogram into a negative and a positive one. A
> nice side-effect of this is that it allowed human-readable bucket names, e.g.
> "(30m, 1h]" rather than 1800000..3600000 (or worse, since exponential buckets
> wouldn't naturally align to minutes).
> 
> - I reused the positive part of the custom enum calculation from
> Media.Timeline.DragTimeDelta for Media.Timeline.DragSumAbsTimeDelta so that
the
> values can be compared easily.
> 
> - I used ranges of numbers for histogram suffixes, so the usual fun with
> multiple DEFINE_STATIC_LOCALs, with an if-elseif-elseif instead of a switch
> statement. Unfortunately, the interpretation of these metrics depends heavily
on
> the width of the timeline element so they all need to be suffixed, but I tried
> to minimise the number of suffixes to avoid histogram bloat (hence for example
> there are no suffixes <32 or >512).
> 
> Whilst unusual, these are the best compromises I can find between histogram
> usability, number of histograms*suffixes, and code complexity (which is
> carefully isolated to MediaControlTimelineMetrics.cpp). I've tried to clearly
> comment the unusual behavior.

Thanks!  I think these are actually quite nice solutions for the issues that
you're facing.  Sorry that it's such a hassle -- you're definitely pushing the
limits of what histograms are designed to work with!  Metrics LGTM.

[Ilya Sherman, 2017-04-01 00:42:41]

+inline comments:

https://codereview.chromium.org/2779273003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/shadow/MediaControlTimelineMetrics.cpp
(right):

https://codereview.chromium.org/2779273003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlTimelineMetrics.cpp:63:
15000,     // 15s
Optional nit: Maybe write this as 15*1000, and similarly split up the constants
below, for ease of scanning.  But, might not be necessary with the comments
(thanks for including those!)

https://codereview.chromium.org/2779273003/diff/40001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2779273003/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:27488: +<histogram
name="Media.Timeline.DragGestureDuration" units="ms">
nit: Please add base="true" for this histogram and others that are just
providing name fragments.

https://codereview.chromium.org/2779273003/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:102383: +  <int value="0" label="[0,
0]"/>
nit: We tend to have min be inclusive and max be inclusive on the UMA
dashboards.  You might want to follow that same pattern here, and update the
bucket boundaries accordingly.  It's just marginally less likely to confuse
people who are familiar with the way that UMA tends to work.

[johnme, 2017-04-03 16:16:48]

https://codereview.chromium.org/2779273003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/shadow/MediaControlTimelineMetrics.cpp
(right):

https://codereview.chromium.org/2779273003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlTimelineMetrics.cpp:63:
15000,     // 15s
On 2017/04/01 00:42:41, Ilya Sherman wrote:
> Optional nit: Maybe write this as 15*1000, and similarly split up the
constants
> below, for ease of scanning.  But, might not be necessary with the comments
> (thanks for including those!)

Done.

https://codereview.chromium.org/2779273003/diff/40001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2779273003/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:27488: +<histogram
name="Media.Timeline.DragGestureDuration" units="ms">
On 2017/04/01 00:42:41, Ilya Sherman wrote:
> nit: Please add base="true" for this histogram and others that are just
> providing name fragments.

Done.

https://codereview.chromium.org/2779273003/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:102383: +  <int value="0" label="[0,
0]"/>
On 2017/04/01 00:42:41, Ilya Sherman wrote:
> nit: We tend to have min be inclusive and max be inclusive on the UMA
> dashboards.  You might want to follow that same pattern here, and update the
> bucket boundaries accordingly.  It's just marginally less likely to confuse
> people who are familiar with the way that UMA tends to work.

Done (assuming you mean min be inclusive and max be exclusive). I kept the
separate zero buckets (since those are likely quite common for TimeDelta &
Percent), and for negative ranges I did the opposite (min exclusive, max
inclusive) for symmetry.

[johnme, 2017-04-03 16:16:48]

https://codereview.chromium.org/2779273003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/shadow/MediaControlTimelineMetrics.cpp
(right):

https://codereview.chromium.org/2779273003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlTimelineMetrics.cpp:63:
15000,     // 15s
On 2017/04/01 00:42:41, Ilya Sherman wrote:
> Optional nit: Maybe write this as 15*1000, and similarly split up the
constants
> below, for ease of scanning.  But, might not be necessary with the comments
> (thanks for including those!)

Done.

https://codereview.chromium.org/2779273003/diff/40001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2779273003/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:27488: +<histogram
name="Media.Timeline.DragGestureDuration" units="ms">
On 2017/04/01 00:42:41, Ilya Sherman wrote:
> nit: Please add base="true" for this histogram and others that are just
> providing name fragments.

Done.

https://codereview.chromium.org/2779273003/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:102383: +  <int value="0" label="[0,
0]"/>
On 2017/04/01 00:42:41, Ilya Sherman wrote:
> nit: We tend to have min be inclusive and max be inclusive on the UMA
> dashboards.  You might want to follow that same pattern here, and update the
> bucket boundaries accordingly.  It's just marginally less likely to confuse
> people who are familiar with the way that UMA tends to work.

Done (assuming you mean min be inclusive and max be exclusive). I kept the
separate zero buckets (since those are likely quite common for TimeDelta &
Percent), and for negative ranges I did the opposite (min exclusive, max
inclusive) for symmetry.

[johnme, 2017-04-03 16:24:31]

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

[johnme, 2017-04-03 16:24:31]

esprehn@chromium.org: Please review changes in:

third_party/WebKit/Source/core/html/BUILD.gn
third_party/WebKit/Source/core/html/shadow/SliderThumbElement.h
third_party/WebKit/Source/core/html/shadow/SliderThumbElement.cpp
third_party/WebKit/Source/platform/Histogram.h
third_party/WebKit/Source/platform/KeyboardCodes.h

(i.e. Blink excluding third_party/WebKit/Source/core/html/shadow/MediaControl*.*
which mlamouri already reviewed)

Thanks!

[Ilya Sherman, 2017-04-03 21:39:27]

(Still LGTM, thanks!)

https://codereview.chromium.org/2779273003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/shadow/MediaControlTimelineMetrics.cpp
(right):

https://codereview.chromium.org/2779273003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlTimelineMetrics.cpp:68:
120 * 1000,    // 2m
nit: If you're going to write these as products, then "2 * 60 * 1000" is more
clearly 2 minutes.

https://codereview.chromium.org/2779273003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlTimelineMetrics.cpp:76:
28800 * 1000,  // 8h
nit: Likewise, "8 * 60 * 60 * 1000" is more clearly 8 hours.

https://codereview.chromium.org/2779273003/diff/60001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2779273003/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:102387: +  <int value="1" label="(0,
16ms)"/>
nit: Is this accurately described as "[1, 16ms)"?

[mlamouri (slow - plz ping), 2017-04-04 13:40:16]

this lgtm but can you add unit tests as the logic is fairly complex and I'm
afraid it will be hard to maintain

https://codereview.chromium.org/2779273003/diff/60001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2779273003/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:27491: + 
<owner>johnme@chromium.org</owner>
Actually, would you mind adding media-dev@chromium.org in addition of you? :)

[esprehn, 2017-04-04 18:56:01]

esprehn@chromium.org changed reviewers:
+ dglazkov@chromium.org, oysteine@chromium.org
- esprehn@chromium.org

[esprehn, 2017-04-04 18:56:02]

This is a really massive amount of code and metrics for the scrubber, I don't
understand what you're trying to learn by recording so much stuff and how that
would impact your decision process. I'll defer this review to dglazkov@,
personally I'd prefer you improved the UI instead for example doing what Apple
does.

https://codereview.chromium.org/2779273003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/KeyboardCodes.h (right):

https://codereview.chromium.org/2779273003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/KeyboardCodes.h:34: #include
"platform/wtf/build_config.h"
Why is this needed?

[johnme, 2017-04-06 16:23:25]

johnme@chromium.org changed reviewers:
- oysteine@chromium.org

[johnme, 2017-04-06 16:23:26]

Added tests and addressed nits.

On 2017/04/04 13:40:16, mlamouri wrote:
> this lgtm but can you add unit tests as the logic is fairly complex and I'm
> afraid it will be hard to maintain

Done - I'd been struggling to find a nice way to deliver touch events from the
unit test, but I found a clean way of delivering mouse events that should give
us pretty good coverage.


On 2017/04/04 18:56:02, esprehn wrote:
> This is a really massive amount of code and metrics for the scrubber, I don't
> understand what you're trying to learn by recording so much stuff and how that
> would impact your decision process. I'll defer this review to dglazkov@,
> personally I'd prefer you improved the UI instead for example doing what Apple
> does.

This is the minimal set of metrics, carefully agreed with PM, UX and already
reviewed by the Media Controls owner. These metrics will be useful precisely
because UX is looking to improve the scrubber UI (especially in cases where
it's narrow), so want to a) quantify current pain points, and b) be able to
confirm that upcoming UX changes do in fact make things uniformly better.

https://codereview.chromium.org/2779273003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/shadow/MediaControlTimelineMetrics.cpp
(right):

https://codereview.chromium.org/2779273003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlTimelineMetrics.cpp:68:
120 * 1000,    // 2m
On 2017/04/03 21:39:26, Ilya Sherman wrote:
> nit: If you're going to write these as products, then "2 * 60 * 1000" is more
> clearly 2 minutes.

Ah; in that case, I'll go back to writing them as raw values (e.g. 120000) since
the comments already contain the human readable duration, and it's sometimes
useful to be able to see the raw values as well in case you need to compare to a
TimeDelta.

https://codereview.chromium.org/2779273003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlTimelineMetrics.cpp:76:
28800 * 1000,  // 8h
On 2017/04/03 21:39:26, Ilya Sherman wrote:
> nit: Likewise, "8 * 60 * 60 * 1000" is more clearly 8 hours.

Ditto I'll go back to writing these as raw values then.

https://codereview.chromium.org/2779273003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/KeyboardCodes.h (right):

https://codereview.chromium.org/2779273003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/KeyboardCodes.h:34: #include
"platform/wtf/build_config.h"
On 2017/04/04 18:56:02, esprehn wrote:
> Why is this needed?

Because OS(WIN) below requires the OS macro defined in build_config.h (it fails
to compile otherwise). Presumably in other .cpp files that include this the
build_config.h happened to have already been included.

https://codereview.chromium.org/2779273003/diff/60001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2779273003/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:27491: + 
<owner>johnme@chromium.org</owner>
On 2017/04/04 13:40:16, mlamouri wrote:
> Actually, would you mind adding mailto:media-dev@chromium.org in addition of
you? :)

Done.

https://codereview.chromium.org/2779273003/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:102387: +  <int value="1" label="(0,
16ms)"/>
On 2017/04/03 21:39:26, Ilya Sherman wrote:
> nit: Is this accurately described as "[1, 16ms)"?

Done, and changed [0, 0] to [0, 1ms) as well :)

[dglazkov, 2017-04-06 20:46:08]

LGTM.

I share esprehn@'s worries though. Could we somehow make sure this code is
removed after we're done gathering data? Is this a permanent fixture?

[johnme, 2017-04-07 10:25:33]

On 2017/04/06 20:46:08, dglazkov wrote:
> LGTM.
> 
> I share esprehn@'s worries though. Could we somehow make sure this code is
> removed after we're done gathering data? Is this a permanent fixture?

Yes, I anticipate that once the improved scrubber has been designed, implemented
and the metrics compared, it'll make sense to remove most of these metrics. I'll
file a bug so we don't forget.

[johnme, 2017-04-07 10:25:41]

The CQ bit was checked by johnme@chromium.org

[johnme, 2017-04-07 10:25:41]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org, mlamouri@chromium.org
Link to the patchset: https://codereview.chromium.org/2779273003/#ps140001
(title: "Cleanup test setup")

[commit-bot: I haz the power, 2017-04-07 10:25:52]

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-04-07 10:53:36]

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

[commit-bot: I haz the power, 2017-04-07 10:53:37]

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_...)

[johnme, 2017-04-07 11:33:24]

The CQ bit was checked by johnme@chromium.org

[johnme, 2017-04-07 11:33:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org, dglazkov@chromium.org, mlamouri@chromium.org
Link to the patchset: https://codereview.chromium.org/2779273003/#ps160001
(title: "Appease MSVC")

[commit-bot: I haz the power, 2017-04-07 11:33:35]

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-04-07 13:33:41]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1491564804970880,
"parent_rev": "3f07d979d8161ed82181c1cd89eef17214df78bf", "commit_rev":
"00a232273b9dd6705d492dd4fd7218005db6f910"}

[commit-bot: I haz the power, 2017-04-07 13:34:31]

Description was changed from

==========
[Media Controls] Add UMA for timeline scrubber

Adds UMA metrics to the media controls timeline <input type="range">, to
understand current usage, and detect changes in usage.

Specifically this adds:
- Media.Timeline.DragGestureDuration.<timeline width>
- Media.Timeline.DragPercent.<timeline width>
- Media.Timeline.DragSumAbsTimeDelta.<timeline width>
- Media.Timeline.DragTimeDelta.<timeline width>
- Media.Timeline.SeekType.<timeline width>
- Media.Timeline.Width
- Media.Timeline.Width.<Inline|Fullscreen><Portrait|Landscape>

BUG=693630
==========

to

==========
[Media Controls] Add UMA for timeline scrubber

Adds UMA metrics to the media controls timeline <input type="range">, to
understand current usage, and detect changes in usage.

Specifically this adds:
- Media.Timeline.DragGestureDuration.<timeline width>
- Media.Timeline.DragPercent.<timeline width>
- Media.Timeline.DragSumAbsTimeDelta.<timeline width>
- Media.Timeline.DragTimeDelta.<timeline width>
- Media.Timeline.SeekType.<timeline width>
- Media.Timeline.Width
- Media.Timeline.Width.<Inline|Fullscreen><Portrait|Landscape>

BUG=693630

Review-Url: https://codereview.chromium.org/2779273003
Cr-Commit-Position: refs/heads/master@{#462847}
Committed:
https://chromium.googlesource.com/chromium/src/+/00a232273b9dd6705d492dd4fd72...
==========

[commit-bot: I haz the power, 2017-04-07 13:34:34]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/00a232273b9dd6705d492dd4fd72...