Refactor CdmPromise and related classes

Refactor CdmPromise and related classes

Move the implementation from CdmPromiseTemplate into a new templated
class CdmCallbackPromise class. CdmResultPromise can now override
resolve() and reject() without having to be concerned with the
callbacks.

Also convert the templates into variadic templates to avoid the
specialization for <void> (now just <>).

BUG=358271
TEST=existing EME tests still pass

Committed: https://crrev.com/60b669f6768594714aca8b72d1530ed2672c126b
Cr-Commit-Position: refs/heads/master@{#298554}

[jrummell, 2014-09-26 21:05:55]

jrummell@chromium.org changed reviewers:
+ ddorwin@chromium.org, sandersd@chromium.org, xhwang@chromium.org

[jrummell, 2014-09-26 21:05:56]

PTAL.

Question: UMA is only used by CreateSession(), currently. I could simplify
CdmResultPromise by moving all the UMA stuff to the override in
webcontentdecryptionmodulesession_impl.cc. WDYT?

Secondly, I had to create instances of CdmPromiseTemplate in cdm_promise (and
thus resolve() has to be defined) so that libcontent.so (which contains
CdmResultPromise) doesn't get link errors trying to find methods needed (e.g.
GetResolveParameterType()).

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

xhwang, please review.

I skimmed a few files. I think we should keep the uma part generic - we may want
to add more. I think the resolve() issue might be solvable by making it pure
virtual (see comments).

https://codereview.chromium.org/604283003/diff/1/content/renderer/media/cdm_r...
File content/renderer/media/cdm_result_promise.cc (right):

https://codereview.chromium.org/604283003/diff/1/content/renderer/media/cdm_r...
content/renderer/media/cdm_result_promise.cc:101:
base::LinearHistogram::FactoryGet(
Can we make this a local function that takes a name and result? I get nervous
with UMA calls in multiple places.

https://codereview.chromium.org/604283003/diff/1/content/renderer/media/cdm_r...
File content/renderer/media/cdm_result_promise.h (right):

https://codereview.chromium.org/604283003/diff/1/content/renderer/media/cdm_r...
content/renderer/media/cdm_result_promise.h:50: // Called by all
resolve()/reject() methods to report the UMA result if
// All implementations must call this method in resolve() and reject() to...

https://codereview.chromium.org/604283003/diff/1/content/renderer/media/cdm_r...
content/renderer/media/cdm_result_promise.h:80: // Called by all
resolve()/reject() methods to report the UMA result if
ditto

https://codereview.chromium.org/604283003/diff/1/content/renderer/media/cdm_r...
content/renderer/media/cdm_result_promise.h:86: // UMA name to report result to.
Kind of sad that we have to implement these members twice. :(

https://codereview.chromium.org/604283003/diff/1/content/renderer/media/crypt...
File content/renderer/media/crypto/ppapi_decryptor.cc (right):

https://codereview.chromium.org/604283003/diff/1/content/renderer/media/crypt...
content/renderer/media/crypto/ppapi_decryptor.cc:31: class SessionUpdatedPromise
: public media::CdmPromiseTemplate<void> {
Would CdmCallbackPromise be a better fit (even if reject CB is null)?
ditto below.

https://codereview.chromium.org/604283003/diff/1/content/renderer/media/crypt...
content/renderer/media/crypto/ppapi_decryptor.cc:38: virtual void resolve()
OVERRIDE {
Note that there is currently no is_pending_ check for this class because the
parent does not define it.

https://codereview.chromium.org/604283003/diff/1/media/base/cdm_promise.h
File media/base/cdm_promise.h (right):

https://codereview.chromium.org/604283003/diff/1/media/base/cdm_promise.h#new...
media/base/cdm_promise.h:55: virtual void resolve(const T& result);
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.

https://codereview.chromium.org/604283003/diff/1/media/base/cdm_promise.h#new...
media/base/cdm_promise.h:103: bool is_pending_;
Is there a reason that is_pending_ can't be a member of CdmPromiseTemplate<T>?

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

Thanks for cleaning this up! Here're my first round of comments.

https://codereview.chromium.org/604283003/diff/1/content/renderer/media/cdm_r...
File content/renderer/media/cdm_result_promise.cc (right):

https://codereview.chromium.org/604283003/diff/1/content/renderer/media/cdm_r...
content/renderer/media/cdm_result_promise.cc:100: if (!uma_name_.empty()) {
Return early:

if (uma_name_.empty())
  return;

// report uma...

https://codereview.chromium.org/604283003/diff/1/content/renderer/media/cdm_r...
content/renderer/media/cdm_result_promise.cc:140: if (!uma_name_.empty()) {
ditto

https://codereview.chromium.org/604283003/diff/1/content/renderer/media/cdm_r...
File content/renderer/media/cdm_result_promise.h (right):

https://codereview.chromium.org/604283003/diff/1/content/renderer/media/cdm_r...
content/renderer/media/cdm_result_promise.h:18: SUCCESS,
Assign numbers to these values to make sure they match 

https://code.google.com/p/chromium/codesearch#chromium/src/tools/metrics/hist...

Also add a comment that never change the value of these enums since they are
used for UMA.

https://codereview.chromium.org/604283003/diff/1/content/renderer/media/cdm_r...
content/renderer/media/cdm_result_promise.h:35: // CdmPromiseResult UMA),
CdmResultPromise will report the promise result
What is "CdmPromiseResult UMA"?

https://codereview.chromium.org/604283003/diff/1/content/renderer/media/cdm_r...
content/renderer/media/cdm_result_promise.h:36: // (success or rejection code).
s/CdmPromise/CdmPromiseTemplate throughout this comment. e.g. we don't have
CdmPromise::resolve()...

https://codereview.chromium.org/604283003/diff/1/content/renderer/media/cdm_r...
content/renderer/media/cdm_result_promise.h:44: virtual void resolve(const T&
result) OVERRIDE;
// CdmPromiseTemplate<T> implementation.

https://codereview.chromium.org/604283003/diff/1/content/renderer/media/cdm_r...
content/renderer/media/cdm_result_promise.h:74: virtual void resolve() OVERRIDE;
// CdmPromiseTemplate<void> implementation.

https://codereview.chromium.org/604283003/diff/1/content/renderer/media/crypt...
File content/renderer/media/crypto/ppapi_decryptor.cc (right):

https://codereview.chromium.org/604283003/diff/1/content/renderer/media/crypt...
content/renderer/media/crypto/ppapi_decryptor.cc:58: class SessionLoadedPromise
: public media::CdmPromiseTemplate<std::string> {
Can we merge SessionUpdatedPromise and SessionLoadedPromise into something like
CdmAdditionalCallbackPromise?

https://codereview.chromium.org/604283003/diff/1/media/base/cdm_promise.cc
File media/base/cdm_promise.cc (right):

https://codereview.chromium.org/604283003/diff/1/media/base/cdm_promise.cc#ne...
media/base/cdm_promise.cc:28: };
You can put CdmPromiseTraits in an anonymous namespace.

https://codereview.chromium.org/604283003/diff/1/media/base/cdm_promise.cc#ne...
media/base/cdm_promise.cc:39: }
nit: will clang-format make this in one line?

https://codereview.chromium.org/604283003/diff/1/media/base/cdm_promise.cc#ne...
media/base/cdm_promise.cc:49: }
If resolve is pure virtual we don't need this :)

https://codereview.chromium.org/604283003/diff/1/media/base/cdm_promise.cc#ne...
media/base/cdm_promise.cc:66: }
ditto

https://codereview.chromium.org/604283003/diff/1/media/base/cdm_promise.h
File media/base/cdm_promise.h (right):

https://codereview.chromium.org/604283003/diff/1/media/base/cdm_promise.h#new...
media/base/cdm_promise.h:14: 
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...

https://codereview.chromium.org/604283003/diff/1/media/base/cdm_promise.h#new...
media/base/cdm_promise.h:20: // the caller free the promise once it is
resolved/rejected.
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...

https://codereview.chromium.org/604283003/diff/1/media/base/cdm_promise.h#new...
media/base/cdm_promise.h:41: virtual ResolveParameterType
GetResolveParameterType() const = 0;
Since this is an interface, it'd be nice to provide a comment on this.

https://codereview.chromium.org/604283003/diff/1/media/base/cdm_promise.h#new...
media/base/cdm_promise.h:44: CdmPromise();
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.

https://codereview.chromium.org/604283003/diff/1/media/base/cdm_promise.h#new...
media/base/cdm_promise.h:51: class MEDIA_EXPORT CdmPromiseTemplate : public
CdmPromise {
Comment about why we need this. Also, mention that this is another "interface".

https://codereview.chromium.org/604283003/diff/1/media/base/cdm_promise.h#new...
media/base/cdm_promise.h:55: virtual void resolve(const T& result);
On 2014/09/26 22:40:00, ddorwin wrote:
> 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.

+1

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

https://codereview.chromium.org/604283003/diff/1/media/base/cdm_promise.h#new...
media/base/cdm_promise.h:56: virtual ResolveParameterType
GetResolveParameterType() const OVERRIDE;
// CdmPromise implementation.

https://codereview.chromium.org/604283003/diff/1/media/base/cdm_promise.h#new...
media/base/cdm_promise.h:59: CdmPromiseTemplate();
ditto about protected ctor since this class is also an interface.

https://codereview.chromium.org/604283003/diff/1/media/base/cdm_promise.h#new...
media/base/cdm_promise.h:65: // Specialization for no parameter to resolve().
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

https://codereview.chromium.org/604283003/diff/1/media/base/cdm_promise.h#new...
media/base/cdm_promise.h:71: virtual void resolve();
ditto, pure virtual.

https://codereview.chromium.org/604283003/diff/1/media/base/cdm_promise.h#new...
media/base/cdm_promise.h:87: class MEDIA_EXPORT CdmCallbackPromise : public
CdmPromiseTemplate<T> {
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.

https://codereview.chromium.org/604283003/diff/1/media/base/cdm_promise.h#new...
media/base/cdm_promise.h:93: virtual void resolve(const T& result) OVERRIDE;
// CdmPromiseTemplate<T> implementation.

https://codereview.chromium.org/604283003/diff/1/media/base/cdm_promise.h#new...
media/base/cdm_promise.h:116: virtual void resolve() OVERRIDE;
// CdmPromiseTemplate<void> implementation.

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

PS2 is the changes, PS3 is the conversion to variadic templates (removed the
need for specializing the <void> instance and removes duplicated methods).

https://codereview.chromium.org/604283003/diff/1/content/renderer/media/cdm_r...
File content/renderer/media/cdm_result_promise.cc (right):

https://codereview.chromium.org/604283003/diff/1/content/renderer/media/cdm_r...
content/renderer/media/cdm_result_promise.cc:100: if (!uma_name_.empty()) {
On 2014/09/29 18:32:58, xhwang wrote:
> Return early:
> 
> if (uma_name_.empty())
>   return;
> 
> // report uma...

Done.

https://codereview.chromium.org/604283003/diff/1/content/renderer/media/cdm_r...
content/renderer/media/cdm_result_promise.cc:101:
base::LinearHistogram::FactoryGet(
On 2014/09/26 22:40:00, ddorwin wrote:
> Can we make this a local function that takes a name and result? I get nervous
> with UMA calls in multiple places.

Done.

https://codereview.chromium.org/604283003/diff/1/content/renderer/media/cdm_r...
content/renderer/media/cdm_result_promise.cc:140: if (!uma_name_.empty()) {
On 2014/09/29 18:32:58, xhwang wrote:
> ditto

Gone.

https://codereview.chromium.org/604283003/diff/1/content/renderer/media/cdm_r...
File content/renderer/media/cdm_result_promise.h (right):

https://codereview.chromium.org/604283003/diff/1/content/renderer/media/cdm_r...
content/renderer/media/cdm_result_promise.h:18: SUCCESS,
On 2014/09/29 18:32:58, xhwang wrote:
> Assign numbers to these values to make sure they match 
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/tools/metrics/hist...
> 
> Also add a comment that never change the value of these enums since they are
> used for UMA.

Done.

https://codereview.chromium.org/604283003/diff/1/content/renderer/media/cdm_r...
content/renderer/media/cdm_result_promise.h:35: // CdmPromiseResult UMA),
CdmResultPromise will report the promise result
On 2014/09/29 18:32:58, xhwang wrote:
> What is "CdmPromiseResult UMA"?

Removed.

https://codereview.chromium.org/604283003/diff/1/content/renderer/media/cdm_r...
content/renderer/media/cdm_result_promise.h:36: // (success or rejection code).
On 2014/09/29 18:32:58, xhwang wrote:
> s/CdmPromise/CdmPromiseTemplate throughout this comment. e.g. we don't have
> CdmPromise::resolve()...

Done.

https://codereview.chromium.org/604283003/diff/1/content/renderer/media/cdm_r...
content/renderer/media/cdm_result_promise.h:44: virtual void resolve(const T&
result) OVERRIDE;
On 2014/09/29 18:32:58, xhwang wrote:
> // CdmPromiseTemplate<T> implementation.

Done.

https://codereview.chromium.org/604283003/diff/1/content/renderer/media/cdm_r...
content/renderer/media/cdm_result_promise.h:50: // Called by all
resolve()/reject() methods to report the UMA result if
On 2014/09/26 22:40:00, ddorwin wrote:
> // All implementations must call this method in resolve() and reject() to...

Removed.

https://codereview.chromium.org/604283003/diff/1/content/renderer/media/cdm_r...
content/renderer/media/cdm_result_promise.h:74: virtual void resolve() OVERRIDE;
On 2014/09/29 18:32:58, xhwang wrote:
> // CdmPromiseTemplate<void> implementation.

Done.

https://codereview.chromium.org/604283003/diff/1/content/renderer/media/cdm_r...
content/renderer/media/cdm_result_promise.h:80: // Called by all
resolve()/reject() methods to report the UMA result if
On 2014/09/26 22:40:00, ddorwin wrote:
> ditto

Done.

https://codereview.chromium.org/604283003/diff/1/content/renderer/media/cdm_r...
content/renderer/media/cdm_result_promise.h:86: // UMA name to report result to.
On 2014/09/26 22:40:00, ddorwin wrote:
> Kind of sad that we have to implement these members twice. :(

Acknowledged.

https://codereview.chromium.org/604283003/diff/1/content/renderer/media/crypt...
File content/renderer/media/crypto/ppapi_decryptor.cc (right):

https://codereview.chromium.org/604283003/diff/1/content/renderer/media/crypt...
content/renderer/media/crypto/ppapi_decryptor.cc:31: class SessionUpdatedPromise
: public media::CdmPromiseTemplate<void> {
On 2014/09/26 22:40:00, ddorwin wrote:
> Would CdmCallbackPromise be a better fit (even if reject CB is null)?
> ditto below.

Nope. It needs to call resolve/reject on the passed in CdmPromise as well as the
additional cb on resolve().

https://codereview.chromium.org/604283003/diff/1/content/renderer/media/crypt...
content/renderer/media/crypto/ppapi_decryptor.cc:38: virtual void resolve()
OVERRIDE {
On 2014/09/26 22:40:00, ddorwin wrote:
> Note that there is currently no is_pending_ check for this class because the
> parent does not define it.

Done.

https://codereview.chromium.org/604283003/diff/1/content/renderer/media/crypt...
content/renderer/media/crypto/ppapi_decryptor.cc:58: class SessionLoadedPromise
: public media::CdmPromiseTemplate<std::string> {
On 2014/09/29 18:32:58, xhwang wrote:
> Can we merge SessionUpdatedPromise and SessionLoadedPromise into something
like
> CdmAdditionalCallbackPromise?

Could be done, although it would be no less code. These should be removed soon
(once WV starts generating the KeysChanged event), so it's not worth the effort.

https://codereview.chromium.org/604283003/diff/1/media/base/cdm_promise.cc
File media/base/cdm_promise.cc (right):

https://codereview.chromium.org/604283003/diff/1/media/base/cdm_promise.cc#ne...
media/base/cdm_promise.cc:28: };
On 2014/09/29 18:32:58, xhwang wrote:
> You can put CdmPromiseTraits in an anonymous namespace.

Done.

https://codereview.chromium.org/604283003/diff/1/media/base/cdm_promise.cc#ne...
media/base/cdm_promise.cc:39: }
On 2014/09/29 18:32:58, xhwang wrote:
> nit: will clang-format make this in one line?

File has been run through clang-format.

https://codereview.chromium.org/604283003/diff/1/media/base/cdm_promise.cc#ne...
media/base/cdm_promise.cc:49: }
On 2014/09/29 18:32:58, xhwang wrote:
> If resolve is pure virtual we don't need this :)

Done.

https://codereview.chromium.org/604283003/diff/1/media/base/cdm_promise.cc#ne...
media/base/cdm_promise.cc:66: }
On 2014/09/29 18:32:58, xhwang wrote:
> ditto

Done.

https://codereview.chromium.org/604283003/diff/1/media/base/cdm_promise.h
File media/base/cdm_promise.h (right):

https://codereview.chromium.org/604283003/diff/1/media/base/cdm_promise.h#new...
media/base/cdm_promise.h:14: 
On 2014/09/29 18:32:59, xhwang wrote:
> 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...

Done.

https://codereview.chromium.org/604283003/diff/1/media/base/cdm_promise.h#new...
media/base/cdm_promise.h:20: // the caller free the promise once it is
resolved/rejected.
On 2014/09/29 18:32:59, xhwang wrote:
> 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...

Done.

https://codereview.chromium.org/604283003/diff/1/media/base/cdm_promise.h#new...
media/base/cdm_promise.h:41: virtual ResolveParameterType
GetResolveParameterType() const = 0;
On 2014/09/29 18:32:59, xhwang wrote:
> Since this is an interface, it'd be nice to provide a comment on this.

Done.

https://codereview.chromium.org/604283003/diff/1/media/base/cdm_promise.h#new...
media/base/cdm_promise.h:44: CdmPromise();
On 2014/09/29 18:32:59, xhwang wrote:
> 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.

Done.

https://codereview.chromium.org/604283003/diff/1/media/base/cdm_promise.h#new...
media/base/cdm_promise.h:51: class MEDIA_EXPORT CdmPromiseTemplate : public
CdmPromise {
On 2014/09/29 18:32:58, xhwang wrote:
> Comment about why we need this. Also, mention that this is another
"interface".

Done.

https://codereview.chromium.org/604283003/diff/1/media/base/cdm_promise.h#new...
media/base/cdm_promise.h:55: virtual void resolve(const T& result);
On 2014/09/26 22:40:00, ddorwin wrote:
> 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.

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

https://codereview.chromium.org/604283003/diff/1/media/base/cdm_promise.h#new...
media/base/cdm_promise.h:55: virtual void resolve(const T& result);
On 2014/09/29 18:32:59, xhwang wrote:
> On 2014/09/26 22:40:00, ddorwin wrote:
> > 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.
> 
> +1
> 
> Also you can put reject(...) = 0 here so it's easy to track what the complete
> interface to implement is.

Done.

https://codereview.chromium.org/604283003/diff/1/media/base/cdm_promise.h#new...
media/base/cdm_promise.h:56: virtual ResolveParameterType
GetResolveParameterType() const OVERRIDE;
On 2014/09/29 18:32:59, xhwang wrote:
> // CdmPromise implementation.

Done.

https://codereview.chromium.org/604283003/diff/1/media/base/cdm_promise.h#new...
media/base/cdm_promise.h:59: CdmPromiseTemplate();
On 2014/09/29 18:32:59, xhwang wrote:
> ditto about protected ctor since this class is also an interface.

Done.

https://codereview.chromium.org/604283003/diff/1/media/base/cdm_promise.h#new...
media/base/cdm_promise.h:71: virtual void resolve();
On 2014/09/29 18:32:59, xhwang wrote:
> ditto, pure virtual.

Done.

https://codereview.chromium.org/604283003/diff/1/media/base/cdm_promise.h#new...
media/base/cdm_promise.h:87: class MEDIA_EXPORT CdmCallbackPromise : public
CdmPromiseTemplate<T> {
On 2014/09/29 18:32:59, xhwang wrote:
> 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.

Done.

https://codereview.chromium.org/604283003/diff/1/media/base/cdm_promise.h#new...
media/base/cdm_promise.h:93: virtual void resolve(const T& result) OVERRIDE;
On 2014/09/29 18:32:59, xhwang wrote:
> // CdmPromiseTemplate<T> implementation.

Done.

https://codereview.chromium.org/604283003/diff/1/media/base/cdm_promise.h#new...
media/base/cdm_promise.h:103: bool is_pending_;
On 2014/09/26 22:40:00, ddorwin wrote:
> Is there a reason that is_pending_ can't be a member of CdmPromiseTemplate<T>?

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

https://codereview.chromium.org/604283003/diff/1/media/base/cdm_promise.h#new...
media/base/cdm_promise.h:116: virtual void resolve() OVERRIDE;
On 2014/09/29 18:32:59, xhwang wrote:
> // CdmPromiseTemplate<void> implementation.

Done.

[xhwang, 2014-10-03 07:34:22]

Thanks, I like Variadic templates! Some trivial comments...

https://codereview.chromium.org/604283003/diff/40001/content/renderer/media/c...
File content/renderer/media/cdm_result_promise.cc (right):

https://codereview.chromium.org/604283003/diff/40001/content/renderer/media/c...
content/renderer/media/cdm_result_promise.cc:57: static void
GenerateUMA(std::string uma_name, ResultCodeForUMA result) {
s/Generate/Report

https://codereview.chromium.org/604283003/diff/40001/content/renderer/media/c...
content/renderer/media/cdm_result_promise.cc:73: :
media::CdmPromiseTemplate<T...>(),
Is this necessary?

https://codereview.chromium.org/604283003/diff/40001/content/renderer/media/c...
content/renderer/media/cdm_result_promise.cc:85: // this should only be used for
the 0 argument version.
In this case, change this to

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

Not sure which version is the correct one...

https://codereview.chromium.org/604283003/diff/40001/content/renderer/media/c...
content/renderer/media/cdm_result_promise.cc:86: DCHECK(sizeof...(T) == 0);
DCHECK_EQ

https://codereview.chromium.org/604283003/diff/40001/content/renderer/media/c...
content/renderer/media/cdm_result_promise.cc:97: GenerateUMA(uma_name_,
SUCCESS);
Comment why we don't call resolve here. It's hard to understand without looking
at the derived class.

https://codereview.chromium.org/604283003/diff/40001/content/renderer/media/c...
File content/renderer/media/cdm_result_promise.h (right):

https://codereview.chromium.org/604283003/diff/40001/content/renderer/media/c...
content/renderer/media/cdm_result_promise.h:39: class CdmResultPromise : public
media::CdmPromiseTemplate<T...> {
Cool!

https://codereview.chromium.org/604283003/diff/40001/content/renderer/media/c...
content/renderer/media/cdm_result_promise.h:54: private:
You can have this here:

using media::CdmPromiseTemplate<T...>::PromiseSettled;

so that you don't need the "this->" part in this->PromiseSettled.

https://codereview.chromium.org/604283003/diff/40001/content/renderer/media/w...
File content/renderer/media/webcontentdecryptionmodulesession_impl.cc (right):

https://codereview.chromium.org/604283003/diff/40001/content/renderer/media/w...
content/renderer/media/webcontentdecryptionmodulesession_impl.cc:37:
web_cdm_result_.completeWithSession(status);
Hmm, this is more confusing than I thought. Is this going away? If so, add a
TODO to remove it...

https://codereview.chromium.org/604283003/diff/40001/media/base/cdm_callback_...
File media/base/cdm_callback_promise.cc (right):

https://codereview.chromium.org/604283003/diff/40001/media/base/cdm_callback_...
media/base/cdm_callback_promise.cc:15: : CdmPromiseTemplate<T...>(),
is this necessary?

https://codereview.chromium.org/604283003/diff/40001/media/base/cdm_callback_...
media/base/cdm_callback_promise.cc:28: this->PromiseSettled();
ditto

https://codereview.chromium.org/604283003/diff/40001/media/base/cdm_callback_...
File media/base/cdm_callback_promise.h (right):

https://codereview.chromium.org/604283003/diff/40001/media/base/cdm_callback_...
media/base/cdm_callback_promise.h:27: PromiseRejectedCB reject_cb);
pass callback by const-ref

https://codereview.chromium.org/604283003/diff/40001/media/base/cdm_promise.cc
File media/base/cdm_promise.cc (right):

https://codereview.chromium.org/604283003/diff/40001/media/base/cdm_promise.c...
media/base/cdm_promise.cc:9: namespace {
Move this anonymous namespace into the media namespace and you don't need all
the "media::" part.

https://codereview.chromium.org/604283003/diff/40001/media/base/cdm_promise.h
File media/base/cdm_promise.h (right):

https://codereview.chromium.org/604283003/diff/40001/media/base/cdm_promise.h...
media/base/cdm_promise.h:50: // saved CdmPromise objects can be cast to the
corrent templated version.
s/corrent/correct

https://codereview.chromium.org/604283003/diff/40001/media/base/cdm_promise.h...
media/base/cdm_promise.h:76: void PromiseSettled();
Use a verb? SettlePromise() or SetPromiseSettled()?

https://codereview.chromium.org/604283003/diff/40001/media/base/cdm_promise.h...
media/base/cdm_promise.h:80: bool is_pending_;
is_settled_?

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

Updated.

https://codereview.chromium.org/604283003/diff/40001/content/renderer/media/c...
File content/renderer/media/cdm_result_promise.cc (right):

https://codereview.chromium.org/604283003/diff/40001/content/renderer/media/c...
content/renderer/media/cdm_result_promise.cc:57: static void
GenerateUMA(std::string uma_name, ResultCodeForUMA result) {
On 2014/10/03 07:34:21, xhwang wrote:
> s/Generate/Report

Done.

https://codereview.chromium.org/604283003/diff/40001/content/renderer/media/c...
content/renderer/media/cdm_result_promise.cc:73: :
media::CdmPromiseTemplate<T...>(),
On 2014/10/03 07:34:21, xhwang wrote:
> Is this necessary? 

Doesn't look like it. Removed.

https://codereview.chromium.org/604283003/diff/40001/content/renderer/media/c...
content/renderer/media/cdm_result_promise.cc:85: // this should only be used for
the 0 argument version.
On 2014/10/03 07:34:21, xhwang wrote:
> In this case, change this to
> 
> template<>
> void CdmResultPromise<> or void CdmResultPromise<void>?
> 
> Not sure which version is the correct one...

Done.

https://codereview.chromium.org/604283003/diff/40001/content/renderer/media/c...
content/renderer/media/cdm_result_promise.cc:86: DCHECK(sizeof...(T) == 0);
On 2014/10/03 07:34:21, xhwang wrote:
> DCHECK_EQ

Removed now that this is specific for CdmResultPromise<>.

https://codereview.chromium.org/604283003/diff/40001/content/renderer/media/c...
content/renderer/media/cdm_result_promise.cc:97: GenerateUMA(uma_name_,
SUCCESS);
On 2014/10/03 07:34:21, xhwang wrote:
> Comment why we don't call resolve here. It's hard to understand without
looking
> at the derived class.

Done.

https://codereview.chromium.org/604283003/diff/40001/content/renderer/media/c...
File content/renderer/media/cdm_result_promise.h (right):

https://codereview.chromium.org/604283003/diff/40001/content/renderer/media/c...
content/renderer/media/cdm_result_promise.h:39: class CdmResultPromise : public
media::CdmPromiseTemplate<T...> {
On 2014/10/03 07:34:21, xhwang wrote:
> Cool!

Acknowledged. I like it too.

https://codereview.chromium.org/604283003/diff/40001/content/renderer/media/c...
content/renderer/media/cdm_result_promise.h:54: private:
On 2014/10/03 07:34:21, xhwang wrote:
> You can have this here:
> 
> using media::CdmPromiseTemplate<T...>::PromiseSettled;
> 
> so that you don't need the "this->" part in this->PromiseSettled.

Done.

https://codereview.chromium.org/604283003/diff/40001/content/renderer/media/w...
File content/renderer/media/webcontentdecryptionmodulesession_impl.cc (right):

https://codereview.chromium.org/604283003/diff/40001/content/renderer/media/w...
content/renderer/media/webcontentdecryptionmodulesession_impl.cc:37:
web_cdm_result_.completeWithSession(status);
On 2014/10/03 07:34:21, xhwang wrote:
> Hmm, this is more confusing than I thought. Is this going away? If so, add a
> TODO to remove it...

This won't be going away, even if we pass |web_session_id| to blink. Registering
the session with cdm_adapter is still needed.

https://codereview.chromium.org/604283003/diff/40001/media/base/cdm_callback_...
File media/base/cdm_callback_promise.cc (right):

https://codereview.chromium.org/604283003/diff/40001/media/base/cdm_callback_...
media/base/cdm_callback_promise.cc:15: : CdmPromiseTemplate<T...>(),
On 2014/10/03 07:34:21, xhwang wrote:
> is this necessary?

Removed.

https://codereview.chromium.org/604283003/diff/40001/media/base/cdm_callback_...
media/base/cdm_callback_promise.cc:28: this->PromiseSettled();
On 2014/10/03 07:34:21, xhwang wrote:
> ditto

Done.

https://codereview.chromium.org/604283003/diff/40001/media/base/cdm_callback_...
File media/base/cdm_callback_promise.h (right):

https://codereview.chromium.org/604283003/diff/40001/media/base/cdm_callback_...
media/base/cdm_callback_promise.h:27: PromiseRejectedCB reject_cb);
On 2014/10/03 07:34:21, xhwang wrote:
> pass callback by const-ref

Done.

https://codereview.chromium.org/604283003/diff/40001/media/base/cdm_promise.cc
File media/base/cdm_promise.cc (right):

https://codereview.chromium.org/604283003/diff/40001/media/base/cdm_promise.c...
media/base/cdm_promise.cc:9: namespace {
On 2014/10/03 07:34:21, xhwang wrote:
> Move this anonymous namespace into the media namespace and you don't need all
> the "media::" part.

Done.

https://codereview.chromium.org/604283003/diff/40001/media/base/cdm_promise.h
File media/base/cdm_promise.h (right):

https://codereview.chromium.org/604283003/diff/40001/media/base/cdm_promise.h...
media/base/cdm_promise.h:50: // saved CdmPromise objects can be cast to the
corrent templated version.
On 2014/10/03 07:34:21, xhwang wrote:
> s/corrent/correct

Done.

https://codereview.chromium.org/604283003/diff/40001/media/base/cdm_promise.h...
media/base/cdm_promise.h:76: void PromiseSettled();
On 2014/10/03 07:34:22, xhwang wrote:
> Use a verb? SettlePromise() or SetPromiseSettled()?

MarkPromiseSettled() since it doesn't actually settle the promise ... just
indicates that it is settled.

https://codereview.chromium.org/604283003/diff/40001/media/base/cdm_promise.h...
media/base/cdm_promise.h:80: bool is_pending_;
On 2014/10/03 07:34:22, xhwang wrote:
> is_settled_?

Done.

[xhwang, 2014-10-03 19:25:16]

lgtm % comments

https://codereview.chromium.org/604283003/diff/40001/content/renderer/media/w...
File content/renderer/media/webcontentdecryptionmodulesession_impl.cc (right):

https://codereview.chromium.org/604283003/diff/40001/content/renderer/media/w...
content/renderer/media/webcontentdecryptionmodulesession_impl.cc:37:
web_cdm_result_.completeWithSession(status);
On 2014/10/03 18:58:30, jrummell wrote:
> On 2014/10/03 07:34:21, xhwang wrote:
> > Hmm, this is more confusing than I thought. Is this going away? If so, add a
> > TODO to remove it...
> 
> This won't be going away, even if we pass |web_session_id| to blink.
Registering
> the session with cdm_adapter is still needed.

I see. This is a bit hacky. Imagine what if we have another type of promise that
really takes a string and calls something like 
completeWithString() in blink::WebContentDecryptionModuleResult.

Maybe inherit from CdmPromiseTemplate<std::string> directly would be cleaner in
that case.

For now feel free to add a TODO so that we don't forget about this.

https://codereview.chromium.org/604283003/diff/60001/media/base/cdm_promise.cc
File media/base/cdm_promise.cc (right):

https://codereview.chromium.org/604283003/diff/60001/media/base/cdm_promise.c...
media/base/cdm_promise.cc:10: 
Sorry, I meant move the anonymous namespace "into" media namespace, i.e.

namespace media {

namespace {

CdmPromiseTraits....

}

}

[jrummell, 2014-10-03 21:41:05]

Updated.

https://codereview.chromium.org/604283003/diff/40001/content/renderer/media/w...
File content/renderer/media/webcontentdecryptionmodulesession_impl.cc (right):

https://codereview.chromium.org/604283003/diff/40001/content/renderer/media/w...
content/renderer/media/webcontentdecryptionmodulesession_impl.cc:37:
web_cdm_result_.completeWithSession(status);
On 2014/10/03 19:25:16, xhwang wrote:
> On 2014/10/03 18:58:30, jrummell wrote:
> > On 2014/10/03 07:34:21, xhwang wrote:
> > > Hmm, this is more confusing than I thought. Is this going away? If so, add
a
> > > TODO to remove it...
> > 
> > This won't be going away, even if we pass |web_session_id| to blink.
> Registering
> > the session with cdm_adapter is still needed.
> 
> I see. This is a bit hacky. Imagine what if we have another type of promise
that
> really takes a string and calls something like 
> completeWithString() in blink::WebContentDecryptionModuleResult.
> 
> Maybe inherit from CdmPromiseTemplate<std::string> directly would be cleaner
in
> that case.
> 
> For now feel free to add a TODO so that we don't forget about this.

Moved to cdm_result_promise as a specialization.

https://codereview.chromium.org/604283003/diff/60001/media/base/cdm_promise.cc
File media/base/cdm_promise.cc (right):

https://codereview.chromium.org/604283003/diff/60001/media/base/cdm_promise.c...
media/base/cdm_promise.cc:10: 
On 2014/10/03 19:25:16, xhwang wrote:
> Sorry, I meant move the anonymous namespace "into" media namespace, i.e.
> 
> namespace media {
> 
> namespace {
> 
> CdmPromiseTraits....
> 
> }
> 
> }

Done.

[jrummell, 2014-10-03 23:03:55]

The CQ bit was checked by jrummell@chromium.org

[commit-bot: I haz the power, 2014-10-03 23:05:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/604283003/80001

[xhwang, 2014-10-03 23:13:57]

Looks much better, thanks!

lgtm % tiny nits

You can address them later :)

https://codereview.chromium.org/604283003/diff/80001/content/renderer/media/c...
File content/renderer/media/cdm_result_promise.cc (right):

https://codereview.chromium.org/604283003/diff/80001/content/renderer/media/c...
content/renderer/media/cdm_result_promise.cc:28: }  // namespace
nit: Move this whole block into content namespace. Then you can also move the
static methods into the anonymous namespace.

https://codereview.chromium.org/604283003/diff/80001/content/renderer/media/c...
File content/renderer/media/cdm_result_promise.h (right):

https://codereview.chromium.org/604283003/diff/80001/content/renderer/media/c...
content/renderer/media/cdm_result_promise.h:50: // Specialization for resolving
a new session promise. Resolving a new session
This is a special class, but not a template specialization.

[jrummell, 2014-10-03 23:19:40]

The CQ bit was unchecked by jrummell@chromium.org

[jrummell, 2014-10-03 23:32:07]

Thanks for the reviews.

https://codereview.chromium.org/604283003/diff/80001/content/renderer/media/c...
File content/renderer/media/cdm_result_promise.cc (right):

https://codereview.chromium.org/604283003/diff/80001/content/renderer/media/c...
content/renderer/media/cdm_result_promise.cc:28: }  // namespace
On 2014/10/03 23:13:57, xhwang wrote:
> nit: Move this whole block into content namespace. Then you can also move the
> static methods into the anonymous namespace.

Done.

https://codereview.chromium.org/604283003/diff/80001/content/renderer/media/c...
File content/renderer/media/cdm_result_promise.h (right):

https://codereview.chromium.org/604283003/diff/80001/content/renderer/media/c...
content/renderer/media/cdm_result_promise.h:50: // Specialization for resolving
a new session promise. Resolving a new session
On 2014/10/03 23:13:57, xhwang wrote:
> This is a special class, but not a template specialization.

Done.

[jrummell, 2014-10-03 23:32:53]

The CQ bit was checked by jrummell@chromium.org

[commit-bot: I haz the power, 2014-10-03 23:33:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/604283003/100001

[jrummell, 2014-10-03 23:50:40]

The CQ bit was unchecked by jrummell@chromium.org

[jrummell, 2014-10-04 00:18:31]

The CQ bit was checked by jrummell@chromium.org

[commit-bot: I haz the power, 2014-10-04 00:19:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/604283003/120001

[commit-bot: I haz the power, 2014-10-04 01:38:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-04 01:38:21]

Try jobs failed on following builders:
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[jrummell, 2014-10-07 18:48:56]

The CQ bit was checked by jrummell@chromium.org

[commit-bot: I haz the power, 2014-10-07 18:50:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/604283003/160001

[commit-bot: I haz the power, 2014-10-07 20:09:42]

Committed patchset #9 (id:160001) as 4d206647bec629c63c4bf387bc3e97c74dade3c4

[commit-bot: I haz the power, 2014-10-07 20:10:37]

Patchset 9 (id:??) landed as
https://crrev.com/60b669f6768594714aca8b72d1530ed2672c126b
Cr-Commit-Position: refs/heads/master@{#298554}