Add UMA to report requests for key systems using RequestMediaKeyAccess

Add UMA to report requests for key systems using RequestMediaKeyAccess

Adding a UMA to keep track of the number of times RequestMediaKeyAccess
is called with a particular key system, and the number of times the
request succeeds. The counts are only incremented once per renderer
frame.

BUG=351501
TEST=existing EME tests pass, verified UMA locally

Committed: https://crrev.com/de870139c90af7fc5957d42c8c2b0fbb09f95591
Cr-Commit-Position: refs/heads/master@{#313956}

[jrummell, 2015-01-27 18:36:21]

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

[jrummell, 2015-01-27 18:36:21]

PTAL.

[sandersd (OOO until July 31), 2015-01-27 19:04:07]

https://codereview.chromium.org/881853002/diff/1/media/blink/webencryptedmedi...
File media/blink/webencryptedmediaclient_impl.h (right):

https://codereview.chromium.org/881853002/diff/1/media/blink/webencryptedmedi...
media/blink/webencryptedmediaclient_impl.h:41: Reporter* LocateReporter(const
std::string& key_system);
GetReporter()?

[jrummell, 2015-01-28 21:08:09]

Updated.

https://codereview.chromium.org/881853002/diff/1/media/blink/webencryptedmedi...
File media/blink/webencryptedmediaclient_impl.h (right):

https://codereview.chromium.org/881853002/diff/1/media/blink/webencryptedmedi...
media/blink/webencryptedmediaclient_impl.h:41: Reporter* LocateReporter(const
std::string& key_system);
On 2015/01/27 19:04:07, sandersd wrote:
> GetReporter()?

Done.

[sandersd (OOO until July 31), 2015-01-29 01:23:10]

Ideally we can remove the copies of these strings from key_systems.cc once the
prefixed API is gone. That will probably require moving this code to live with
the promise UMA code.

lgtm

[xhwang, 2015-01-29 05:23:26]

https://codereview.chromium.org/881853002/diff/20001/media/blink/webencrypted...
File media/blink/webencryptedmediaclient_impl.cc (right):

https://codereview.chromium.org/881853002/diff/20001/media/blink/webencrypted...
media/blink/webencryptedmediaclient_impl.cc:26: const char
kWidevineKeySystemNameForUMA[] = "Widevine";
Can you use GetKeySystemNameForUMA(const std::string& key_system) in
key_systems.h? Usually we don't know anything about "Widevine" in media/ code.
(Android code is an exception, sadly...)

https://codereview.chromium.org/881853002/diff/20001/media/blink/webencrypted...
media/blink/webencryptedmediaclient_impl.cc:146: // renderer process.
hmm, this is not true. WebEncryptedMediaClientImpl is created once per
RenderFrame(Impl). There could be multiple render frames (which belong to the
same tab, or different tabs) in one render process.

https://codereview.chromium.org/881853002/diff/20001/media/blink/webencrypted...
media/blink/webencryptedmediaclient_impl.cc:147: class
WebEncryptedMediaClientImpl::Reporter {
KeySystemsSupportUMA::Reporter is currently using 4 OneTimeReporter. If we move
WebEncryptedMediaClientImpl::Reporter to a standalone file/class, then
KeySystemsSupportUMA::Reporter can use it and it'll only need two Reporters. But
we probably don't want to touch the prefixed code....

https://codereview.chromium.org/881853002/diff/20001/media/blink/webencrypted...
media/blink/webencryptedmediaclient_impl.cc:152: KEY_SYSTEM_COUNT
KEY_SYSTEM_COUNT is misleading, it's really KEY_SYSTEM_SUPPORT_STATUS_COUNT

https://codereview.chromium.org/881853002/diff/20001/media/blink/webencrypted...
media/blink/webencryptedmediaclient_impl.cc:158: is_supported_reported_(false)
{}
s/requested/request
s/supported/support

https://codereview.chromium.org/881853002/diff/20001/media/blink/webencrypted...
media/blink/webencryptedmediaclient_impl.cc:168: void ReportSupported() {
DCHECK(is_requested_reported_) to make sure this is never called without calling
ReportRequested() first.

https://codereview.chromium.org/881853002/diff/20001/media/blink/webencrypted...
media/blink/webencryptedmediaclient_impl.cc:196:
AddReporter(kWidevineKeySystemNameForUMA);
See comment below about lazy adding these.

https://codereview.chromium.org/881853002/diff/20001/media/blink/webencrypted...
media/blink/webencryptedmediaclient_impl.cc:241: reporter->ReportSupported();
nit: report first, then resolve the request.

https://codereview.chromium.org/881853002/diff/20001/media/blink/webencrypted...
media/blink/webencryptedmediaclient_impl.cc:258: reporter->ReportSupported();
ditto.

https://codereview.chromium.org/881853002/diff/20001/media/blink/webencrypted...
media/blink/webencryptedmediaclient_impl.cc:269: reporters_.set(key_system,
scoped_ptr<Reporter>(new Reporter(key_system)));
nit: You can use make_scoped_ptr.

https://codereview.chromium.org/881853002/diff/20001/media/blink/webencrypted...
media/blink/webencryptedmediaclient_impl.cc:281: reporter_lookup =
reporters_.find(kUnknownKeySystemNameForUMA);
If you use GetKeySystemNameForUMA(), you'll get kUnknownKeySystemNameForUMA
automatically if the key system is not recognized. So really you have only a
small set of possible key system for UMA names. With that, you can probably lazy
create reporters in GetReporter(), instead of proactively create all reporters
in the constructor.

[jrummell, 2015-01-29 22:16:07]

Updated.

https://codereview.chromium.org/881853002/diff/20001/media/blink/webencrypted...
File media/blink/webencryptedmediaclient_impl.cc (right):

https://codereview.chromium.org/881853002/diff/20001/media/blink/webencrypted...
media/blink/webencryptedmediaclient_impl.cc:26: const char
kWidevineKeySystemNameForUMA[] = "Widevine";
On 2015/01/29 05:23:26, xhwang wrote:
> Can you use GetKeySystemNameForUMA(const std::string& key_system) in
> key_systems.h? Usually we don't know anything about "Widevine" in media/ code.
> (Android code is an exception, sadly...)

My concern was that histograms.xml needs to know about them, so having a fixed
list that matches up with what is there makes sense. However, switching to lazy
create the reporters.

https://codereview.chromium.org/881853002/diff/20001/media/blink/webencrypted...
media/blink/webencryptedmediaclient_impl.cc:146: // renderer process.
On 2015/01/29 05:23:25, xhwang wrote:
> hmm, this is not true. WebEncryptedMediaClientImpl is created once per
> RenderFrame(Impl). There could be multiple render frames (which belong to the
> same tab, or different tabs) in one render process.

As discussed offline, once per render frame is OK. Changed the comment.

https://codereview.chromium.org/881853002/diff/20001/media/blink/webencrypted...
media/blink/webencryptedmediaclient_impl.cc:147: class
WebEncryptedMediaClientImpl::Reporter {
On 2015/01/29 05:23:26, xhwang wrote:
> KeySystemsSupportUMA::Reporter is currently using 4 OneTimeReporter. If we
move
> WebEncryptedMediaClientImpl::Reporter to a standalone file/class, then
> KeySystemsSupportUMA::Reporter can use it and it'll only need two Reporters.
But
> we probably don't want to touch the prefixed code....

Acknowledged.

https://codereview.chromium.org/881853002/diff/20001/media/blink/webencrypted...
media/blink/webencryptedmediaclient_impl.cc:152: KEY_SYSTEM_COUNT
On 2015/01/29 05:23:26, xhwang wrote:
> KEY_SYSTEM_COUNT is misleading, it's really KEY_SYSTEM_SUPPORT_STATUS_COUNT

Done.

https://codereview.chromium.org/881853002/diff/20001/media/blink/webencrypted...
media/blink/webencryptedmediaclient_impl.cc:158: is_supported_reported_(false)
{}
On 2015/01/29 05:23:26, xhwang wrote:
> s/requested/request
> s/supported/support

Done.

https://codereview.chromium.org/881853002/diff/20001/media/blink/webencrypted...
media/blink/webencryptedmediaclient_impl.cc:168: void ReportSupported() {
On 2015/01/29 05:23:26, xhwang wrote:
> DCHECK(is_requested_reported_) to make sure this is never called without
calling
> ReportRequested() first.

Done.

https://codereview.chromium.org/881853002/diff/20001/media/blink/webencrypted...
media/blink/webencryptedmediaclient_impl.cc:196:
AddReporter(kWidevineKeySystemNameForUMA);
On 2015/01/29 05:23:26, xhwang wrote:
> See comment below about lazy adding these.

Done.

https://codereview.chromium.org/881853002/diff/20001/media/blink/webencrypted...
media/blink/webencryptedmediaclient_impl.cc:241: reporter->ReportSupported();
On 2015/01/29 05:23:26, xhwang wrote:
> nit: report first, then resolve the request.

Done.

https://codereview.chromium.org/881853002/diff/20001/media/blink/webencrypted...
media/blink/webencryptedmediaclient_impl.cc:258: reporter->ReportSupported();
On 2015/01/29 05:23:26, xhwang wrote:
> ditto.

Done.

https://codereview.chromium.org/881853002/diff/20001/media/blink/webencrypted...
media/blink/webencryptedmediaclient_impl.cc:269: reporters_.set(key_system,
scoped_ptr<Reporter>(new Reporter(key_system)));
On 2015/01/29 05:23:26, xhwang wrote:
> nit: You can use make_scoped_ptr.

Not needed now.

https://codereview.chromium.org/881853002/diff/20001/media/blink/webencrypted...
media/blink/webencryptedmediaclient_impl.cc:281: reporter_lookup =
reporters_.find(kUnknownKeySystemNameForUMA);
On 2015/01/29 05:23:25, xhwang wrote:
> If you use GetKeySystemNameForUMA(), you'll get kUnknownKeySystemNameForUMA
> automatically if the key system is not recognized. So really you have only a
> small set of possible key system for UMA names. With that, you can probably
lazy
> create reporters in GetReporter(), instead of proactively create all reporters
> in the constructor.

Done.

[xhwang, 2015-01-29 23:40:48]

lgtm % comments. Also, please update your CL description as well.

https://codereview.chromium.org/881853002/diff/40001/media/blink/webencrypted...
File media/blink/webencryptedmediaclient_impl.cc (right):

https://codereview.chromium.org/881853002/diff/40001/media/blink/webencrypted...
media/blink/webencryptedmediaclient_impl.cc:266: Reporters::iterator
reporter_lookup = reporters_.find(uma_name);
you can just use get().

https://codereview.chromium.org/881853002/diff/40001/media/blink/webencrypted...
media/blink/webencryptedmediaclient_impl.cc:273: reporters_.set(uma_name,
reporter.Pass());
You can probably just return the return value of set(), then you don't need
|result|.

[jrummell, 2015-01-30 01:54:01]

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

[jrummell, 2015-01-30 01:54:02]

+isherman@ for histograms.xml

https://codereview.chromium.org/881853002/diff/40001/media/blink/webencrypted...
File media/blink/webencryptedmediaclient_impl.cc (right):

https://codereview.chromium.org/881853002/diff/40001/media/blink/webencrypted...
media/blink/webencryptedmediaclient_impl.cc:266: Reporters::iterator
reporter_lookup = reporters_.find(uma_name);
On 2015/01/29 23:40:48, xhwang wrote:
> you can just use get().

Done.

https://codereview.chromium.org/881853002/diff/40001/media/blink/webencrypted...
media/blink/webencryptedmediaclient_impl.cc:273: reporters_.set(uma_name,
reporter.Pass());
On 2015/01/29 23:40:48, xhwang wrote:
> You can probably just return the return value of set(), then you don't need
> |result|.

Done.

[Ilya Sherman, 2015-01-30 02:30:50]

LGTM with the spurious line removed.

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

https://codereview.chromium.org/881853002/diff/60001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:62326:
+RequestMediaKeySystemAccessStatus
Hmm, what is this line?  I'm surprised that it passes the presubmit check.

[jrummell, 2015-01-30 18:12:12]

Thanks for the reviews.

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

https://codereview.chromium.org/881853002/diff/60001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:62326:
+RequestMediaKeySystemAccessStatus
On 2015/01/30 02:30:50, Ilya Sherman wrote:
> Hmm, what is this line?  I'm surprised that it passes the presubmit check.

Removed.

[jrummell, 2015-01-30 18:12:16]

The CQ bit was checked by jrummell@chromium.org

[commit-bot: I haz the power, 2015-01-30 18:13:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/881853002/80001

[commit-bot: I haz the power, 2015-01-30 19:10:56]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-01-30 19:12:12]

Patchset 5 (id:??) landed as
https://crrev.com/de870139c90af7fc5957d42c8c2b0fbb09f95591
Cr-Commit-Position: refs/heads/master@{#313956}