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..8ae0979625e4e0375f54bba8687dfbe45d743ad7 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,94 +27,97 @@ static blink::WebContentDecryptionModuleException ConvertException(
       return blink::WebContentDecryptionModuleExceptionClientError;
     case media::MediaKeys::OUTPUT_ERROR:
       return blink::WebContentDecryptionModuleExceptionOutputError;
-    default:
-      NOTREACHED();
-      return blink::WebContentDecryptionModuleExceptionUnknownError;
   }
+  NOTREACHED();
+  return blink::WebContentDecryptionModuleExceptionUnknownError;
+}
+
+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)
-    : media::CdmPromiseTemplate<T>(
-          base::Bind(&CdmResultPromise::OnResolve, base::Unretained(this)),
-          base::Bind(&CdmResultPromise::OnReject, base::Unretained(this))),
-      web_cdm_result_(result) {
+static void GenerateUMA(std::string uma_name, ResultCodeForUMA result) {

[xhwang, 2014/10/03 07:34:21]

s/Generate/Report

[jrummell, 2014/10/03 18:58:29]

Done.

+  if (uma_name.empty())
+    return;
+
+  base::LinearHistogram::FactoryGet(
+      uma_name,
+      1,
+      NUM_RESULT_CODES,
+      NUM_RESULT_CODES + 1,
+      base::HistogramBase::kUmaTargetedHistogramFlag)->Add(result);
 }
 
-template <typename T>
-CdmResultPromise<T>::CdmResultPromise(
+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...>(),

[xhwang, 2014/10/03 07:34:21]

Is this necessary? 

[jrummell, 2014/10/03 18:58:29]

Doesn't look like it. Removed.

+      web_cdm_result_(result),
+      uma_name_(uma_name) {
+}
+
+template <typename... T>
+CdmResultPromise<T...>::~CdmResultPromise() {
 }
 
-template <typename T>
-CdmResultPromise<T>::~CdmResultPromise() {
+template <typename... T>
+void CdmResultPromise<T...>::resolve(const T&... result) {
+  // As there are specializations for std::string and media::KeyIdsVector below,
+  // this should only be used for the 0 argument version.

[xhwang, 2014/10/03 07:34:21]

In this case, change this to

template<>
void CdmResultPromise<> or void CdmResultPromise<void>?

Not sure which version is the correct one...

[jrummell, 2014/10/03 18:58:29]

Done.

+  DCHECK(sizeof...(T) == 0);

[xhwang, 2014/10/03 07:34:21]

DCHECK_EQ

[jrummell, 2014/10/03 18:58:29]

Removed now that this is specific for CdmResultPromise<>.

+  this->PromiseSettled();
+  GenerateUMA(uma_name_, SUCCESS);
+  web_cdm_result_.complete();
 }
 
 template <>
-void CdmResultPromise<std::string>::OnResolve(const std::string& result) {
-  // This must be overridden in a subclass.
-  NOTREACHED();
+void CdmResultPromise<std::string>::resolve(const std::string& result) {
+  // This must be overridden in a subclass. However, this method must be called
+  // from any overrides to mark the promise as settled and report to UMA.
+  this->PromiseSettled();
+  GenerateUMA(uma_name_, SUCCESS);

[xhwang, 2014/10/03 07:34:21]

Comment why we don't call resolve here. It's hard to understand without looking
at the derived class.

[jrummell, 2014/10/03 18:58:30]

Done.

 }
 
 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.");
-}
-
-template <typename T>
-void CdmResultPromise<T>::OnReject(media::MediaKeys::Exception exception_code,
-                                   uint32 system_code,
-                                   const std::string& error_message) {
-  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) {
-}
-
-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) {
-}
-
-CdmResultPromise<void>::~CdmResultPromise() {
-}
-
-void CdmResultPromise<void>::OnResolve() {
-  web_cdm_result_.complete();
+  reject(media::MediaKeys::NOT_SUPPORTED_ERROR, 0, "Not implemented.");
 }
 
-void CdmResultPromise<void>::OnReject(
-    media::MediaKeys::Exception exception_code,
-    uint32 system_code,
-    const std::string& error_message) {
+template <typename... T>
+void CdmResultPromise<T...>::reject(media::MediaKeys::Exception exception_code,
+                                    uint32 system_code,
+                                    const std::string& error_message) {
+  this->PromiseSettled();
+  GenerateUMA(uma_name_, ConvertExceptionToUMAResult(exception_code));
   web_cdm_result_.completeWithError(ConvertException(exception_code),
                                     system_code,
                                     blink::WebString::fromUTF8(error_message));
 }
 
 // Explicit template instantiation for the templates needed.
+template class CdmResultPromise<>;
 template class CdmResultPromise<std::string>;
 template class CdmResultPromise<media::KeyIdsVector>;