Cleanup template in CdmPromise

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"
@@ skipping 49 matching lines @@
 
  protected:
   CdmPromise();
   CdmPromise(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(PromiseRejectedCB reject_cb, const std::string& uma_name);
 
+  // Called by all resolve() methods to do the check on |is_pending_|

[ddorwin, 2014/09/19 20:56:46]

... to report UMA, if applicable, and update |is_pending_|.

^ Start with the important thing (and the reason for its name); the check is a
DCHECK, so focus on the unconditional behavior.

[jrummell, 2014/09/22 19:21:10]

Done.

+  // and call the UMA success if required.
+  void ReportResolution();

[ddorwin, 2014/09/19 20:56:46]

nit: "Resolution" has many meanings. How about ReportResolved?

[xhwang, 2014/09/20 00:34:50]

ReportXxxToUMA() to be explicit?

[jrummell, 2014/09/22 19:21:10]

Done.

[jrummell, 2014/09/22 19:21:10]

Done.

+
   PromiseRejectedCB reject_cb_;
 
   // Keep track of whether the promise hasn't been resolved or rejected yet.
   bool is_pending_;
 
   // UMA to report result to.

[xhwang, 2014/09/20 00:34:50]

s/UMA/UMA name/

Can you give an example here? Sometimes I see a UMA on the server, but it's hard
to find where it's reported.

[jrummell, 2014/09/22 19:21:10]

Comment changed. As for an example, the names are generated by whoever creates
the promise. The names include the component, so they are something like
"Media.EME.ClearKey.CreateSession". However, this class doesn't care about what
names are used.

But I do agree with you -- it would be nice to simply search on the UMA name and
find where it comes from. Maybe the names should be included near the callers.

   std::string uma_name_;
 
   DISALLOW_COPY_AND_ASSIGN(CdmPromise);
 };
 
-// Specialization for no parameter to resolve().
-template <>
-class MEDIA_EXPORT CdmPromiseTemplate<void> : public CdmPromise {
+template <typename T>
+class MEDIA_EXPORT CdmPromiseTemplate : public CdmPromise {
  public:
-  CdmPromiseTemplate(base::Callback<void(void)> resolve_cb,
+  CdmPromiseTemplate(base::Callback<void(const T&)> resolve_cb,
                      PromiseRejectedCB rejected_cb);
-  CdmPromiseTemplate(base::Callback<void(void)> resolve_cb,
+  CdmPromiseTemplate(base::Callback<void(const T&)> resolve_cb,
                      PromiseRejectedCB rejected_cb,
                      const std::string& uma_name);
   virtual ~CdmPromiseTemplate();
-  virtual void resolve();
+  virtual void resolve(const T& result);
   virtual ResolveParameterType GetResolveParameterType() const OVERRIDE;

[ddorwin, 2014/09/19 20:56:46]

Rather than overriding this, can we make the base class non-virtual and have its
constructor take the type? We can use the same traits call as in the override in
this class's constructor.

[jrummell, 2014/09/22 19:21:10]

Done.

 
  protected:
   // Allow subclasses to completely override the implementation.

[ddorwin, 2014/09/19 20:56:46]

Is this still appropriate? I don't think it was use in the base either. Ditto
below.

[jrummell, 2014/09/22 19:21:10]

It is used by SessionUpdatedPromise and SessionLoadedPromise (in
ppapi_decryptor.cc). Although they will go away, I expect the new overrides in
WebContentDecryptionModuleSessionImpl will need them.

[ddorwin, 2014/09/22 20:46:30]

Maybe a TODO to consider removing these in October (timelines are desirable; if
there is a relevant bug, you can use that instead). Actually, the TODO should be
at new line 64, which can also be removed and would cause the others to be
removed too.

[jrummell, 2014/09/22 23:57:32]

I'll wait until the new overrides are implemented.

   CdmPromiseTemplate();
 
  private:
+  base::Callback<void(const T&)> resolve_cb_;
+
+  DISALLOW_COPY_AND_ASSIGN(CdmPromiseTemplate);
+};
+
+// Specialization for no parameter to resolve().
+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();
+  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);
 };
-
-template <>
-class MEDIA_EXPORT CdmPromiseTemplate<std::string> : public CdmPromise {
- public:
-  CdmPromiseTemplate(base::Callback<void(const std::string&)> resolve_cb,
-                     PromiseRejectedCB rejected_cb);
-  CdmPromiseTemplate(base::Callback<void(const std::string&)> resolve_cb,
-                     PromiseRejectedCB rejected_cb,
-                     const std::string& uma_name);
-  virtual ~CdmPromiseTemplate();
-  virtual void resolve(const std::string& result);
-  virtual ResolveParameterType GetResolveParameterType() const OVERRIDE;
-
- protected:
-  // Allow subclasses to completely override the implementation.
-  // TODO(jrummell): Remove when derived class SessionLoadedPromise
-  // (in ppapi_decryptor.cc) is no longer needed.
-  CdmPromiseTemplate();
-
- private:
-  base::Callback<void(const std::string&)> resolve_cb_;
-
-  DISALLOW_COPY_AND_ASSIGN(CdmPromiseTemplate);
-};
-
-template <>
-class MEDIA_EXPORT CdmPromiseTemplate<KeyIdsVector> : public CdmPromise {
- public:
-  CdmPromiseTemplate(base::Callback<void(const KeyIdsVector&)> resolve_cb,
-                     PromiseRejectedCB rejected_cb);
-  CdmPromiseTemplate(base::Callback<void(const KeyIdsVector&)> resolve_cb,
-                     PromiseRejectedCB rejected_cb,
-                     const std::string& uma_name);
-  virtual ~CdmPromiseTemplate();
-  virtual void resolve(const KeyIdsVector& result);
-  virtual ResolveParameterType GetResolveParameterType() const OVERRIDE;
-
- private:
-  base::Callback<void(const KeyIdsVector&)> resolve_cb_;
-
-  DISALLOW_COPY_AND_ASSIGN(CdmPromiseTemplate);
-};
 
 }  // namespace media
 
 #endif  // MEDIA_BASE_CDM_PROMISE_H_