Add UMA reporting to CdmPromise.

media/base/cdm_promise.cc

Index: media/base/cdm_promise.cc
diff --git a/media/base/cdm_promise.cc b/media/base/cdm_promise.cc
index ec5e913dbb8aae93c90aa30a24835aaa1a3cc3e8..717b3522205bdba501475a513f9341d505cbb87f 100644
--- a/media/base/cdm_promise.cc
+++ b/media/base/cdm_promise.cc
@@ -6,9 +6,13 @@
 
 #include "base/bind.h"
 #include "base/logging.h"
+#include "base/metrics/histogram.h"
 
 namespace media {
 
+COMPILE_ASSERT(CdmPromise::NUM_RESULT_CODES == MediaKeys::NUM_EXCEPTIONS + 1,
+               ResultCodeForUMA_not_synced);
+
 CdmPromise::CdmPromise() : is_pending_(true) {
 }
 
@@ -17,15 +21,47 @@ CdmPromise::CdmPromise(PromiseRejectedCB reject_cb)
   DCHECK(!reject_cb_.is_null());
 }
 
+CdmPromise::CdmPromise(PromiseRejectedCB reject_cb, const std::string& uma_name)
+    : reject_cb_(reject_cb), is_pending_(true), uma_name_(uma_name) {
+  DCHECK(!reject_cb_.is_null());
+}
+
 CdmPromise::~CdmPromise() {
   DCHECK(!is_pending_);
 }
 
+static CdmPromise::ResultCodeForUMA ConvertExceptionToUMAResult(
+    MediaKeys::Exception exception_code) {
+  switch (exception_code) {
+    case MediaKeys::NOT_SUPPORTED_ERROR:
+      return CdmPromise::NOT_SUPPORTED_ERROR;
+    case MediaKeys::INVALID_STATE_ERROR:
+      return CdmPromise::INVALID_STATE_ERROR;
+    case MediaKeys::INVALID_ACCESS_ERROR:
+      return CdmPromise::INVALID_ACCESS_ERROR;
+    case MediaKeys::QUOTA_EXCEEDED_ERROR:
+      return CdmPromise::QUOTA_EXCEEDED_ERROR;
+    case MediaKeys::UNKNOWN_ERROR:
+      return CdmPromise::UNKNOWN_ERROR;
+    case MediaKeys::CLIENT_ERROR:
+      return CdmPromise::CLIENT_ERROR;
+    case MediaKeys::OUTPUT_ERROR:
+      return CdmPromise::OUTPUT_ERROR;
+    default:

[Ilya Sherman, 2014/08/11 21:53:48]

Please omit the default case, so that the compiler can detect a missing case.  I
bet this would also allow you to drop the COMPILE_ASSERT above, if you wanted
to.

[sandersd (OOO until July 31), 2014/08/12 16:05:51]

Done.

+      NOTREACHED();
+      return CdmPromise::UNKNOWN_ERROR;

[Ilya Sherman, 2014/08/11 21:53:48]

Note that you'll still need this default return stmt outside of the switch stmt
for those compilers (notable, the Windows compiler) that aren't smart enough to
realize that all cases are covered.

[sandersd (OOO until July 31), 2014/08/12 16:05:51]

Done.

+  }
+}
+
 void CdmPromise::reject(MediaKeys::Exception exception_code,
                         uint32 system_code,
                         const std::string& error_message) {
   DCHECK(is_pending_);
   is_pending_ = false;
+  if (!uma_name_.empty()) {
+    ResultCodeForUMA result_code = ConvertExceptionToUMAResult(exception_code);
+    UMA_HISTOGRAM_ENUMERATION(uma_name_, result_code, NUM_RESULT_CODES);

[Ilya Sherman, 2014/08/11 21:53:48]

The name passed to any of the UMA_HISTOGRAM_ macros needs to be a runtime
constant.  It looks like that isn't guaranteed here.

[sandersd (OOO until July 31), 2014/08/12 16:05:51]

What do you mean by this?

[Ilya Sherman, 2014/08/12 18:00:33]

The UMA_HISTOGRAM_ENUMERATION macro expands out to something like

static base::HistogramBase* histogram = GetHistogram(name, ...);

The important part is the 'static' keyword: It means that the histogram is only
looked up by name the first time the code path is reached during a run of the
program.  Hence, if you pass "Media.EME.ClearKey.CreateSession" as the histogram
name one time, and "Media.EME.Unknown.CreateSession" as the name another time,
the data will actually be recorded to "Media.EME.ClearKey.CreateSession" both
times.

In short, the name passed to an of the UMA_HISTOGRAM_ macros should be a
constant.  Strictly speaking, it only needs to be constant during the run of the
program.  However, it's easiest if it's actually a compile-time constant.  Since
you're logging to three different histograms, this means that you should have
three separate UMA_HISTOGRAM_ENUMERATION macro invocations.  Alternately, if
you're pretty sure you know what you'e doing, you could forgo the macro, and
call the histograms code directly.

[sandersd (OOO until July 31), 2014/08/13 17:27:06]

Since this is not currently performance sensitive, I've removed the macros.

+  }
   reject_cb_.Run(exception_code, system_code, error_message);
 }
 
@@ -38,6 +74,15 @@ CdmPromiseTemplate<T>::CdmPromiseTemplate(
 }
 
 template <typename T>
+CdmPromiseTemplate<T>::CdmPromiseTemplate(
+    base::Callback<void(const T&)> resolve_cb,
+    PromiseRejectedCB reject_cb,
+    const std::string& uma_name)
+    : CdmPromise(reject_cb, uma_name), resolve_cb_(resolve_cb) {
+  DCHECK(!resolve_cb_.is_null());
+}
+
+template <typename T>
 CdmPromiseTemplate<T>::~CdmPromiseTemplate() {
   DCHECK(!is_pending_);
 }
@@ -46,6 +91,8 @@ template <typename T>
 void CdmPromiseTemplate<T>::resolve(const T& result) {
   DCHECK(is_pending_);
   is_pending_ = false;
+  if (!uma_name_.empty())
+    UMA_HISTOGRAM_ENUMERATION(uma_name_, SUCCESS, NUM_RESULT_CODES);

[Ilya Sherman, 2014/08/11 21:53:48]

Ditto.

[sandersd (OOO until July 31), 2014/08/13 17:27:06]

Done.

   resolve_cb_.Run(result);
 }
 
@@ -55,6 +102,14 @@ CdmPromiseTemplate<void>::CdmPromiseTemplate(base::Callback<void()> resolve_cb,
   DCHECK(!resolve_cb_.is_null());
 }
 
+CdmPromiseTemplate<void>::CdmPromiseTemplate(base::Callback<void()> resolve_cb,
+                                             PromiseRejectedCB reject_cb,
+                                             const std::string& uma_name)
+    : CdmPromise(reject_cb, uma_name), resolve_cb_(resolve_cb) {
+  DCHECK(!resolve_cb_.is_null());
+  DCHECK(!uma_name_.empty());
+}
+
 CdmPromiseTemplate<void>::CdmPromiseTemplate() {
 }
 
@@ -65,6 +120,8 @@ CdmPromiseTemplate<void>::~CdmPromiseTemplate() {
 void CdmPromiseTemplate<void>::resolve() {
   DCHECK(is_pending_);
   is_pending_ = false;
+  if (!uma_name_.empty())
+    UMA_HISTOGRAM_ENUMERATION(uma_name_, SUCCESS, NUM_RESULT_CODES);

[Ilya Sherman, 2014/08/11 21:53:48]

Ditto.

[sandersd (OOO until July 31), 2014/08/13 17:27:06]

Done.

   resolve_cb_.Run();
 }