Refactor CdmPromise and related classes

media/base/cdm_promise.h

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef MEDIA_BASE_CDM_PROMISE_H_
 #define MEDIA_BASE_CDM_PROMISE_H_
 
 #include <string>
 
 #include "base/basictypes.h"
 #include "base/callback.h"
 #include "media/base/media_export.h"
 #include "media/base/media_keys.h"
 

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

As we are one of the few pioneers using Promise in chromium code. It's likely
that in the future people will copy our work to implement their promises.
Actually, our CdmPromise can be easily generalized by adding another variadic
template parameter for reject(). We don't need to do it now because YAGNI. But
we should add a comment here warning people that if they plan to copy and modify
this class, they should talk to us about generalizing this class...

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

Done.

 namespace media {
 
 // Interface for promises being resolved/rejected in response to various
 // session actions. These may be called synchronously or asynchronously.
 // The promise must be resolved or rejected exactly once. It is expected that
 // the caller free the promise once it is resolved/rejected.

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

This doc needs to be updated. For example, we don't have "resolve" here... It
might be better to explain why we need CdmPromise here etc...

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

Done.

 //
 // 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
   // specified. |system_code| is a Key System-specific value for the error
   // that occurred, or 0 if there is no associated status code or such status
   // 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);
+  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.

 
-  // 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_;
-
+ 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);
 };
 
 // Specialization for no parameter to resolve().

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

FYI, now "Variadic Templates" (a c++11 feature) is allowed, with which we don't
need to make a special case for "void" (no parameter) any more. I think it
should work for our case and will make our code cleaner. But feel free to
investigate this later...

http://en.wikipedia.org/wiki/Variadic_template

 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:
-  base::Callback<void(void)> resolve_cb_;
-
   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_;
+
+  // Keep track of whether the promise hasn't been resolved or rejected yet.
+  bool is_pending_;
+
+  DISALLOW_COPY_AND_ASSIGN(CdmCallbackPromise);
+};
+
 }  // namespace media
 
 #endif  // MEDIA_BASE_CDM_PROMISE_H_