Record the type of controls used for video persistence.

Record the type of controls used for video persistence.

BUG=703267
R=zqzhang@chromium.org

Review-Url: https://codereview.chromium.org/2763723002
Cr-Commit-Position: refs/heads/master@{#458387}
Committed: https://chromium.googlesource.com/chromium/src/+/9bab651a8a170a6016af11dc0ee4f6407456fcb9

[mlamouri (slow - plz ping), 2017-03-20 18:22:36]

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

[commit-bot: I haz the power, 2017-03-20 18:23:05]

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

[mlamouri (slow - plz ping), 2017-03-20 18:23:43]

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

[mlamouri (slow - plz ping), 2017-03-20 18:23:44]

+isherman@ for histograms.xml

PTAL :)

[Ilya Sherman, 2017-03-20 19:08:40]

Metrics LGTM % nits:

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

https://codereview.chromium.org/2763723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:147:
VideoPersistenceControlsTypeMax
Optional nit: Typically, this would be named "Count" rather than "Max" -- "max"
is usually equal to the last actual item of the enum, whereas this is
max-plus-one (which is what histograms need).

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

https://codereview.chromium.org/2763723002/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:27665: +  <summary>Record the type of
controls a persisted video is using.</summary>
Please document when this is recorded: Once per video when the video is shown? 
Whenever the type of the controls changes?  Periodically sampled?

[commit-bot: I haz the power, 2017-03-20 19:17:45]

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

[commit-bot: I haz the power, 2017-03-20 19:17:46]

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

[Zhiqiang Zhang (Slow), 2017-03-20 21:29:15]

lgtm w/ nits

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

https://codereview.chromium.org/2763723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:147:
VideoPersistenceControlsTypeMax
On 2017/03/20 at 19:08:39, Ilya Sherman wrote:
> Optional nit: Typically, this would be named "Count" rather than "Max" --
"max" is usually equal to the last actual item of the enum, whereas this is
max-plus-one (which is what histograms need).

Or name it "NumberOfTypes"?
Also, use "enum class" maybe?

[mlamouri (slow - plz ping), 2017-03-21 11:44:38]

The CQ bit was checked by mlamouri@chromium.org

[mlamouri (slow - plz ping), 2017-03-21 11:44:39]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org, zqzhang@chromium.org
Link to the patchset: https://codereview.chromium.org/2763723002/#ps20001
(title: "review comments")

[commit-bot: I haz the power, 2017-03-21 11:44:56]

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

[mlamouri (slow - plz ping), 2017-03-21 12:01:21]

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

https://codereview.chromium.org/2763723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:147:
VideoPersistenceControlsTypeMax
On 2017/03/20 at 21:29:15, Zhiqiang Zhang wrote:
> On 2017/03/20 at 19:08:39, Ilya Sherman wrote:
> > Optional nit: Typically, this would be named "Count" rather than "Max" --
"max" is usually equal to the last actual item of the enum, whereas this is
max-plus-one (which is what histograms need).
> 
> Or name it "NumberOfTypes"?
> Also, use "enum class" maybe?

Named it Count as per isherman@ request. I would like to use an enum class but
the histogram code doesn't like enum class and it would forces us to do static
cast int which, given what we are doing with the enum, sounds like mostly
useless.

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

https://codereview.chromium.org/2763723002/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:27665: +  <summary>Record the type of
controls a persisted video is using.</summary>
On 2017/03/20 at 19:08:39, Ilya Sherman wrote:
> Please document when this is recorded: Once per video when the video is shown?
 Whenever the type of the controls changes?  Periodically sampled?

Done.

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

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1490096678514030,
"parent_rev": "7b60b0148294aa25648798b20bb527097c800baf", "commit_rev":
"9bab651a8a170a6016af11dc0ee4f6407456fcb9"}

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

Description was changed from

==========
Record the type of controls used for video persistence.

BUG=703267
R=zqzhang@chromium.org
==========

to

==========
Record the type of controls used for video persistence.

BUG=703267
R=zqzhang@chromium.org

Review-Url: https://codereview.chromium.org/2763723002
Cr-Commit-Position: refs/heads/master@{#458387}
Committed:
https://chromium.googlesource.com/chromium/src/+/9bab651a8a170a6016af11dc0ee4...
==========

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

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/9bab651a8a170a6016af11dc0ee4...