Refactor CdmPromise and related classes

media/base/cdm_promise.h

Index: media/base/cdm_promise.h
diff --git a/media/base/cdm_promise.h b/media/base/cdm_promise.h
index 8e6703d0c7dd63c7074f272725781c73d0804d98..6ba3a24ba0d56d4056a6e24b09007a85c4e97b75 100644
--- a/media/base/cdm_promise.h
+++ b/media/base/cdm_promise.h
@@ -22,30 +22,12 @@ namespace media {
 // This is only the base class, as parameter to resolve() varies.
 class MEDIA_EXPORT CdmPromise {
  public:
-  // A superset of media::MediaKeys::Exception for UMA reporting.
-  enum ResultCodeForUMA {
-    SUCCESS,
-    NOT_SUPPORTED_ERROR,
-    INVALID_STATE_ERROR,
-    INVALID_ACCESS_ERROR,
-    QUOTA_EXCEEDED_ERROR,
-    UNKNOWN_ERROR,
-    CLIENT_ERROR,
-    OUTPUT_ERROR,
-    NUM_RESULT_CODES
-  };
-
   enum ResolveParameterType {
     VOID_TYPE,
     STRING_TYPE,
     KEY_IDS_VECTOR_TYPE
   };
 
-  typedef base::Callback<void(MediaKeys::Exception exception_code,
-                              uint32 system_code,
-                              const std::string& error_message)>
-      PromiseRejectedCB;
-
   virtual ~CdmPromise();
 
   // Used to indicate that the operation failed. |exception_code| must be
@@ -54,56 +36,29 @@ class MEDIA_EXPORT CdmPromise {
   // codes are not supported by the Key System. |error_message| is optional.
   virtual void reject(MediaKeys::Exception exception_code,
                       uint32 system_code,
-                      const std::string& error_message);
+                      const std::string& error_message) = 0;
 
-  ResolveParameterType GetResolveParameterType() const {
-    return parameter_type_;
-  }
+  virtual ResolveParameterType GetResolveParameterType() const = 0;

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

Since this is an interface, it'd be nice to provide a comment on this.

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

Done.

 
  protected:
-  explicit CdmPromise(ResolveParameterType parameter_type);
-  CdmPromise(ResolveParameterType parameter_type, PromiseRejectedCB reject_cb);
-
-  // If constructed with a |uma_name| (which must be the name of a
-  // CdmPromiseResult UMA), CdmPromise will report the promise result (success
-  // or rejection code).
-  CdmPromise(ResolveParameterType parameter_type,
-             PromiseRejectedCB reject_cb,
-             const std::string& uma_name);
-
-  // Called by all resolve()/reject() methods to report the UMA result if
-  // applicable, and update |is_pending_|.
-  void ReportResultToUMA(ResultCodeForUMA result);
-
-  const ResolveParameterType parameter_type_;
-  PromiseRejectedCB reject_cb_;
-
-  // Keep track of whether the promise hasn't been resolved or rejected yet.
-  bool is_pending_;
-
-  // UMA name to report result to.
-  std::string uma_name_;
+  CdmPromise();

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

nit: Since this class is an interface, nobody can construct it directly. Having
a protected ctor doesn't help us much. I think we can just make the ctor public
to make things simpler.

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

Done.

 
+ private:
   DISALLOW_COPY_AND_ASSIGN(CdmPromise);
 };
 
 template <typename T>
 class MEDIA_EXPORT CdmPromiseTemplate : public CdmPromise {

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

Comment about why we need this. Also, mention that this is another "interface".

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

Done.

  public:
-  CdmPromiseTemplate(base::Callback<void(const T&)> resolve_cb,
-                     PromiseRejectedCB rejected_cb);
-  CdmPromiseTemplate(base::Callback<void(const T&)> resolve_cb,
-                     PromiseRejectedCB rejected_cb,
-                     const std::string& uma_name);
+  virtual ~CdmPromiseTemplate();
+
   virtual void resolve(const T& result);

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

It doesn't appear that CdmPromiseTemplate<foo> is ever used as a *concrete*
class. If not, make this pure virtual and you should be able to avoid your dummy
implementation.

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

+1

Also you can put reject(...) = 0 here so it's easy to track what the complete
interface to implement is.

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

Done.

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

True. But then we'd have to clone CdmPromiseTraits<> for use in
CdmResultPromise.

+  virtual ResolveParameterType GetResolveParameterType() const OVERRIDE;

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

// CdmPromise implementation.

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

Done.

 
  protected:
-  // Allow subclasses to completely override the implementation.
   CdmPromiseTemplate();

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

ditto about protected ctor since this class is also an interface.

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

Done.

 
  private:
-  base::Callback<void(const T&)> resolve_cb_;
-
   DISALLOW_COPY_AND_ASSIGN(CdmPromiseTemplate);
 };
 
@@ -111,21 +66,66 @@ class MEDIA_EXPORT CdmPromiseTemplate : public CdmPromise {
 template <>
 class MEDIA_EXPORT CdmPromiseTemplate<void> : public CdmPromise {
  public:
-  CdmPromiseTemplate(base::Callback<void(void)> resolve_cb,
-                     PromiseRejectedCB rejected_cb);
-  CdmPromiseTemplate(base::Callback<void(void)> resolve_cb,
-                     PromiseRejectedCB rejected_cb,
-                     const std::string& uma_name);
+  virtual ~CdmPromiseTemplate();
+
   virtual void resolve();

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

ditto, pure virtual.

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

Done.

+  virtual ResolveParameterType GetResolveParameterType() const OVERRIDE;
 
  protected:
-  // Allow subclasses to completely override the implementation.
   CdmPromiseTemplate();
 
  private:
+  DISALLOW_COPY_AND_ASSIGN(CdmPromiseTemplate);
+};
+
+typedef base::Callback<void(MediaKeys::Exception exception_code,
+                            uint32 system_code,
+                            const std::string& error_message)>
+    PromiseRejectedCB;
+
+template <typename T>
+class MEDIA_EXPORT CdmCallbackPromise : public CdmPromiseTemplate<T> {

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

Does it make sense to put CdmCallbackPromise in it's own file? That way, it's
consistent with CdmResultPromise. Also, we'll keep cdm_promise.h an interface
file.

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

Done.

+ public:
+  CdmCallbackPromise(base::Callback<void(const T&)> resolve_cb,
+                     PromiseRejectedCB reject_cb);
+  virtual ~CdmCallbackPromise();
+
+  virtual void resolve(const T& result) OVERRIDE;

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

// CdmPromiseTemplate<T> implementation.

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

Done.

+  virtual void reject(MediaKeys::Exception exception_code,
+                      uint32 system_code,
+                      const std::string& error_message) OVERRIDE;
+
+ private:
+  base::Callback<void(const T&)> resolve_cb_;
+  PromiseRejectedCB reject_cb_;
+
+  // Keep track of whether the promise hasn't been resolved or rejected yet.
+  bool is_pending_;

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

Is there a reason that is_pending_ can't be a member of CdmPromiseTemplate<T>?

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

It seems strange to have it defined on the base class but then used only in
derived classes. Moved.

+
+  DISALLOW_COPY_AND_ASSIGN(CdmCallbackPromise);
+};
+
+// Specialization for no parameter to resolve().
+template <>
+class MEDIA_EXPORT CdmCallbackPromise<void> : public CdmPromiseTemplate<void> {
+ public:
+  CdmCallbackPromise(base::Callback<void(void)> resolve_cb,
+                     PromiseRejectedCB reject_cb);
+  virtual ~CdmCallbackPromise();
+
+  virtual void resolve() OVERRIDE;

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

// CdmPromiseTemplate<void> implementation.

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

Done.

+  virtual void reject(MediaKeys::Exception exception_code,
+                      uint32 system_code,
+                      const std::string& error_message) OVERRIDE;
+
+ private:
   base::Callback<void(void)> resolve_cb_;
+  PromiseRejectedCB reject_cb_;
 
-  DISALLOW_COPY_AND_ASSIGN(CdmPromiseTemplate);
+  // Keep track of whether the promise hasn't been resolved or rejected yet.
+  bool is_pending_;
+
+  DISALLOW_COPY_AND_ASSIGN(CdmCallbackPromise);
 };
 
 }  // namespace media