media: Add Permissions.Action.ProtectedMedia UMA on ChromeOS

media: Add Permissions.Action.ProtectedMedia UMA on ChromeOS

BUG=719111
TEST=Maually tested

Review-Url: https://codereview.chromium.org/2864113002
Cr-Commit-Position: refs/heads/master@{#470160}
Committed: https://chromium.googlesource.com/chromium/src/+/caeedfbcb7eb8a9a9a1a262c385af7c49045a864

[xhwang, 2017-05-06 00:21:54]

xhwang@chromium.org changed reviewers:
+ ddorwin@chromium.org, jrummell@chromium.org

[xhwang, 2017-05-06 00:21:54]

xhwang@chromium.org changed required reviewers:
+ jrummell@chromium.org

[xhwang, 2017-05-06 00:21:55]

jrummell: PTAL
ddorwin: FYI

[xhwang, 2017-05-06 00:22:25]

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

[commit-bot: I haz the power, 2017-05-06 00:23:21]

Dry run: CQ is trying da patch.

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

[jrummell, 2017-05-06 00:31:11]

Don't you need to update histograms.xml with the new entry? Otherwise lgtm.

[commit-bot: I haz the power, 2017-05-06 01:51:21]

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

[commit-bot: I haz the power, 2017-05-06 01:51:21]

Dry run: This issue passed the CQ dry run.

[xhwang, 2017-05-06 03:55:40]

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

[xhwang, 2017-05-06 03:55:41]

isherman: FYI, this CL enables UMA reporting of an existing UMA on ChromeOS.
Previously we only have data reported on Android. This is why I don't need to
update histograms.xml.

[raymes, 2017-05-07 22:13:47]

raymes@chromium.org changed reviewers:
+ raymes@chromium.org

[raymes, 2017-05-07 22:13:47]

Thanks for adding this. Should we add a test for this?

[xhwang, 2017-05-08 17:52:17]

On 2017/05/07 22:13:47, raymes wrote:
> Thanks for adding this. Should we add a test for this?

Good question. How do we add tests for UMAs? Also this would require user
actions. Do you have any examples?

[Ilya Sherman, 2017-05-08 18:05:29]

Metrics LGTM, thanks.  The HistogramTester class would probably be useful for
adding test coverage.

[xhwang, 2017-05-08 18:24:15]

On 2017/05/08 17:52:17, xhwang wrote:
> On 2017/05/07 22:13:47, raymes wrote:
> > Thanks for adding this. Should we add a test for this?
> 
> Good question. How do we add tests for UMAs? Also this would require user
> actions. Do you have any examples?

raymes@: It seems there's no unittest for PlatformVerificationDialog either,
probably for the same reason: needs user action. So I don't see any easy way of
adding tests. We have a plan to switch to use standard permission bubble for
this: https://bugs.chromium.org/p/chromium/issues/detail?id=454847, after which
it'll be easier to add tests I guess.

[xhwang, 2017-05-08 18:24:15]

On 2017/05/08 17:52:17, xhwang wrote:
> On 2017/05/07 22:13:47, raymes wrote:
> > Thanks for adding this. Should we add a test for this?
> 
> Good question. How do we add tests for UMAs? Also this would require user
> actions. Do you have any examples?

raymes@: It seems there's no unittest for PlatformVerificationDialog either,
probably for the same reason: needs user action. So I don't see any easy way of
adding tests. We have a plan to switch to use standard permission bubble for
this: https://bugs.chromium.org/p/chromium/issues/detail?id=454847, after which
it'll be easier to add tests I guess.

[raymes, 2017-05-08 21:40:00]

On 2017/05/08 18:24:15, xhwang wrote:
> On 2017/05/08 17:52:17, xhwang wrote:
> > On 2017/05/07 22:13:47, raymes wrote:
> > > Thanks for adding this. Should we add a test for this?
> > 
> > Good question. How do we add tests for UMAs? Also this would require user
> > actions. Do you have any examples?
> 
> raymes@: It seems there's no unittest for PlatformVerificationDialog either,
> probably for the same reason: needs user action. So I don't see any easy way
of
> adding tests. We have a plan to switch to use standard permission bubble for
> this: https://bugs.chromium.org/p/chromium/issues/detail?id=454847, after
which
> it'll be easier to add tests I guess.

Ok - I don't know that code so well so I will leave it up to you :)

[xhwang, 2017-05-08 22:03:47]

On 2017/05/08 21:40:00, raymes wrote:
> On 2017/05/08 18:24:15, xhwang wrote:
> > On 2017/05/08 17:52:17, xhwang wrote:
> > > On 2017/05/07 22:13:47, raymes wrote:
> > > > Thanks for adding this. Should we add a test for this?
> > > 
> > > Good question. How do we add tests for UMAs? Also this would require user
> > > actions. Do you have any examples?
> > 
> > raymes@: It seems there's no unittest for PlatformVerificationDialog either,
> > probably for the same reason: needs user action. So I don't see any easy way
> of
> > adding tests. We have a plan to switch to use standard permission bubble for
> > this: https://bugs.chromium.org/p/chromium/issues/detail?id=454847, after
> which
> > it'll be easier to add tests I guess.
> 
> Ok - I don't know that code so well so I will leave it up to you :)

Okay, thanks for the comments!

[xhwang, 2017-05-08 22:03:54]

The CQ bit was checked by xhwang@chromium.org

[commit-bot: I haz the power, 2017-05-08 22:05:48]

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-05-08 23:26:51]

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

[commit-bot: I haz the power, 2017-05-08 23:26:53]

Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[xhwang, 2017-05-08 23:48:28]

The CQ bit was checked by xhwang@chromium.org

[commit-bot: I haz the power, 2017-05-09 00:06:43]

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-05-09 03:45:02]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1494287308580690, "parent_rev":
"3d83dad2db679c3924ffe74599cfd48a03211778", "commit_rev":
"caeedfbcb7eb8a9a9a1a262c385af7c49045a864"}

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

Description was changed from

==========
media: Add Permissions.Action.ProtectedMedia UMA on ChromeOS

BUG=719111
TEST=Maually tested
==========

to

==========
media: Add Permissions.Action.ProtectedMedia UMA on ChromeOS

BUG=719111
TEST=Maually tested

Review-Url: https://codereview.chromium.org/2864113002
Cr-Commit-Position: refs/heads/master@{#470160}
Committed:
https://chromium.googlesource.com/chromium/src/+/caeedfbcb7eb8a9a9a1a262c385a...
==========

[commit-bot: I haz the power, 2017-05-09 03:45:24]

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