Refactor CdmPromise and related classes

content/renderer/media/cdm_result_promise.cc

Index: content/renderer/media/cdm_result_promise.cc
diff --git a/content/renderer/media/cdm_result_promise.cc b/content/renderer/media/cdm_result_promise.cc
index ad591677b1dde5e859fec400d162629741487e13..8726201eb66cccf36b6d83738016cb735af7dae9 100644
--- a/content/renderer/media/cdm_result_promise.cc
+++ b/content/renderer/media/cdm_result_promise.cc
@@ -4,8 +4,8 @@
 
 #include "content/renderer/media/cdm_result_promise.h"
 
-#include "base/bind.h"
 #include "base/logging.h"
+#include "base/metrics/histogram.h"
 #include "third_party/WebKit/public/platform/WebString.h"
 
 namespace content {
@@ -27,93 +27,126 @@ static blink::WebContentDecryptionModuleException ConvertException(
       return blink::WebContentDecryptionModuleExceptionClientError;
     case media::MediaKeys::OUTPUT_ERROR:
       return blink::WebContentDecryptionModuleExceptionOutputError;
-    default:
-      NOTREACHED();
-      return blink::WebContentDecryptionModuleExceptionUnknownError;
   }
+  NOTREACHED();
+  return blink::WebContentDecryptionModuleExceptionUnknownError;
 }
 
-template <typename T>
-CdmResultPromise<T>::CdmResultPromise(
-    const blink::WebContentDecryptionModuleResult& result)
-    : media::CdmPromiseTemplate<T>(
-          base::Bind(&CdmResultPromise::OnResolve, base::Unretained(this)),
-          base::Bind(&CdmResultPromise::OnReject, base::Unretained(this))),
-      web_cdm_result_(result) {
+static ResultCodeForUMA ConvertExceptionToUMAResult(
+    media::MediaKeys::Exception exception_code) {
+  switch (exception_code) {
+    case media::MediaKeys::NOT_SUPPORTED_ERROR:
+      return NOT_SUPPORTED_ERROR;
+    case media::MediaKeys::INVALID_STATE_ERROR:
+      return INVALID_STATE_ERROR;
+    case media::MediaKeys::INVALID_ACCESS_ERROR:
+      return INVALID_ACCESS_ERROR;
+    case media::MediaKeys::QUOTA_EXCEEDED_ERROR:
+      return QUOTA_EXCEEDED_ERROR;
+    case media::MediaKeys::UNKNOWN_ERROR:
+      return UNKNOWN_ERROR;
+    case media::MediaKeys::CLIENT_ERROR:
+      return CLIENT_ERROR;
+    case media::MediaKeys::OUTPUT_ERROR:
+      return OUTPUT_ERROR;
+  }
+  NOTREACHED();
+  return UNKNOWN_ERROR;
 }
 
 template <typename T>
 CdmResultPromise<T>::CdmResultPromise(
     const blink::WebContentDecryptionModuleResult& result,
     const std::string& uma_name)
-    : media::CdmPromiseTemplate<T>(
-          base::Bind(&CdmResultPromise::OnResolve, base::Unretained(this)),
-          base::Bind(&CdmResultPromise::OnReject, base::Unretained(this)),
-          uma_name),
-      web_cdm_result_(result) {
+    : media::CdmPromiseTemplate<T>(),
+      web_cdm_result_(result),
+      uma_name_(uma_name),
+      is_pending_(true) {
 }
 
 template <typename T>
 CdmResultPromise<T>::~CdmResultPromise() {
+  DCHECK(!is_pending_);
 }
 
 template <>
-void CdmResultPromise<std::string>::OnResolve(const std::string& result) {
+void CdmResultPromise<std::string>::resolve(const std::string& result) {
   // This must be overridden in a subclass.
   NOTREACHED();
 }
 
 template <>
-void CdmResultPromise<media::KeyIdsVector>::OnResolve(
+void CdmResultPromise<media::KeyIdsVector>::resolve(
     const media::KeyIdsVector& result) {
   // TODO(jrummell): Update blink::WebContentDecryptionModuleResult to
   // handle the set of keys.
-  OnReject(media::MediaKeys::NOT_SUPPORTED_ERROR, 0, "Not implemented.");
+  reject(media::MediaKeys::NOT_SUPPORTED_ERROR, 0, "Not implemented.");
 }
 
 template <typename T>
-void CdmResultPromise<T>::OnReject(media::MediaKeys::Exception exception_code,
-                                   uint32 system_code,
-                                   const std::string& error_message) {
+void CdmResultPromise<T>::reject(media::MediaKeys::Exception exception_code,
+                                 uint32 system_code,
+                                 const std::string& error_message) {
+  ReportResultToUMA(ConvertExceptionToUMAResult(exception_code));
   web_cdm_result_.completeWithError(ConvertException(exception_code),
                                     system_code,
                                     blink::WebString::fromUTF8(error_message));
 }
 
-CdmResultPromise<void>::CdmResultPromise(
-    const blink::WebContentDecryptionModuleResult& result)
-    : media::CdmPromiseTemplate<void>(
-          base::Bind(&CdmResultPromise::OnResolve, base::Unretained(this)),
-          base::Bind(&CdmResultPromise::OnReject, base::Unretained(this))),
-      web_cdm_result_(result) {
+template <typename T>
+void CdmResultPromise<T>::ReportResultToUMA(ResultCodeForUMA result) {
+  DCHECK(is_pending_);
+  is_pending_ = false;
+  if (!uma_name_.empty()) {

[xhwang, 2014/09/29 18:32:58]

Return early:

if (uma_name_.empty())
  return;

// report uma...

[jrummell, 2014/09/29 22:08:38]

Done.

+    base::LinearHistogram::FactoryGet(

[ddorwin, 2014/09/26 22:40:00]

Can we make this a local function that takes a name and result? I get nervous
with UMA calls in multiple places.

[jrummell, 2014/09/29 22:08:38]

Done.

+        uma_name_,
+        1,
+        NUM_RESULT_CODES,
+        NUM_RESULT_CODES + 1,
+        base::HistogramBase::kUmaTargetedHistogramFlag)->Add(result);
+  }
 }
 
 CdmResultPromise<void>::CdmResultPromise(
     const blink::WebContentDecryptionModuleResult& result,
     const std::string& uma_name)
-    : media::CdmPromiseTemplate<void>(
-          base::Bind(&CdmResultPromise::OnResolve, base::Unretained(this)),
-          base::Bind(&CdmResultPromise::OnReject, base::Unretained(this)),
-          uma_name),
-      web_cdm_result_(result) {
+    : media::CdmPromiseTemplate<void>(),
+      web_cdm_result_(result),
+      uma_name_(uma_name),
+      is_pending_(true) {
 }
 
 CdmResultPromise<void>::~CdmResultPromise() {
+  DCHECK(!is_pending_);
 }
 
-void CdmResultPromise<void>::OnResolve() {
+void CdmResultPromise<void>::resolve() {
+  ReportResultToUMA(SUCCESS);
   web_cdm_result_.complete();
 }
 
-void CdmResultPromise<void>::OnReject(
-    media::MediaKeys::Exception exception_code,
-    uint32 system_code,
-    const std::string& error_message) {
+void CdmResultPromise<void>::reject(media::MediaKeys::Exception exception_code,
+                                    uint32 system_code,
+                                    const std::string& error_message) {
+  ReportResultToUMA(ConvertExceptionToUMAResult(exception_code));
   web_cdm_result_.completeWithError(ConvertException(exception_code),
                                     system_code,
                                     blink::WebString::fromUTF8(error_message));
 }
 
+void CdmResultPromise<void>::ReportResultToUMA(ResultCodeForUMA result) {
+  DCHECK(is_pending_);
+  is_pending_ = false;
+  if (!uma_name_.empty()) {

[xhwang, 2014/09/29 18:32:58]

ditto

[jrummell, 2014/09/29 22:08:38]

Gone.

+    base::LinearHistogram::FactoryGet(
+        uma_name_,
+        1,
+        NUM_RESULT_CODES,
+        NUM_RESULT_CODES + 1,
+        base::HistogramBase::kUmaTargetedHistogramFlag)->Add(result);
+  }
+}
+
 // Explicit template instantiation for the templates needed.
 template class CdmResultPromise<std::string>;
 template class CdmResultPromise<media::KeyIdsVector>;