Simplify use of ContentDecryptionModuleResult with promises

Simplify use of ContentDecryptionModuleResult with promises

Since ContentDecryptionModuleResult objects are often used to return
promises, add ContentDecryptionModuleResultPromise as a base class
that includes the promise and allow existing classes to only override
the complete() method that they expect to get called.

BUG=358271
TEST=existing EME tests pass

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=185168

[jrummell, 2014-11-08 01:29:34]

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

[jrummell, 2014-11-08 01:29:35]

PTAL.

[ddorwin, 2014-11-11 22:44:09]

https://codereview.chromium.org/687533004/diff/1/Source/modules/encryptedmedi...
File Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.h
(right):

https://codereview.chromium.org/687533004/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.h:17: //
ContentDecryptionModuleResult objects, and is to be derived from to resolve
nit: s/derived from/subclassed/
That reads better.

https://codereview.chromium.org/687533004/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.h:18: //
promises as needed. The default implementations of complete(),
I'm not sure what "resolve promises as needed" means, though. Maybe you mean to
say they should override one of the complete methods.

https://codereview.chromium.org/687533004/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.h:20: class
ContentDecryptionModuleResultPromise : public ContentDecryptionModuleResult {
Why is this a "...ResultPromise"? It doesn't look like a promise and it actually
contains a promise.

https://codereview.chromium.org/687533004/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.h:22:
explicit ContentDecryptionModuleResultPromise(ScriptState*);
protected? Should this really be instantiated?

https://codereview.chromium.org/687533004/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.h:27: virtual
void completeWithSession(WebContentDecryptionModuleResult::SessionStatus)
override;
Why does this "base class" have this specialized completion method? Why doesn't
it contain the others?

https://codereview.chromium.org/687533004/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.h:36: //
Called when the promise is to be resolved by derived classes.
Resolves the promise with |value|. Used by subclasses to....

https://codereview.chromium.org/687533004/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.h:44: //
Reject the promise with a DOMException.
nit: Rejects

https://codereview.chromium.org/687533004/diff/1/Source/modules/encryptedmedi...
File Source/modules/encryptedmedia/MediaKeySession.cpp (right):

https://codereview.chromium.org/687533004/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeySession.cpp:205: class NewSessionResult :
public ContentDecryptionModuleResultPromise {
nit: If the base and Simple* are "...ResultPromise", these two should be as
well.

https://codereview.chromium.org/687533004/diff/1/Source/modules/encryptedmedi...
File Source/modules/encryptedmedia/SimpleContentDecryptionModuleResultPromise.h
(right):

https://codereview.chromium.org/687533004/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/SimpleContentDecryptionModuleResultPromise.h:14:
// methods will reject the promise.
nit: Perhaps better to say that they should not be used. This _could_ be
interpreted as saying they should be called to reject.

https://codereview.chromium.org/687533004/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/SimpleContentDecryptionModuleResultPromise.h:20:
// ContentDecryptionModuleResult implementation.
ContentDecryptionModuleResultPromise?

[jrummell, 2014-11-11 23:30:11]

Updated.

https://codereview.chromium.org/687533004/diff/1/Source/modules/encryptedmedi...
File Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.h
(right):

https://codereview.chromium.org/687533004/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.h:17: //
ContentDecryptionModuleResult objects, and is to be derived from to resolve
On 2014/11/11 22:44:09, ddorwin wrote:
> nit: s/derived from/subclassed/
> That reads better.

Done.

https://codereview.chromium.org/687533004/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.h:18: //
promises as needed. The default implementations of complete(),
On 2014/11/11 22:44:09, ddorwin wrote:
> I'm not sure what "resolve promises as needed" means, though. Maybe you mean
to
> say they should override one of the complete methods.

Done.

https://codereview.chromium.org/687533004/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.h:20: class
ContentDecryptionModuleResultPromise : public ContentDecryptionModuleResult {
On 2014/11/11 22:44:09, ddorwin wrote:
> Why is this a "...ResultPromise"? It doesn't look like a promise and it
actually
> contains a promise.

I guess ...ResultWithPromise would be better. However, the name is getting
longer and longer.

https://codereview.chromium.org/687533004/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.h:22:
explicit ContentDecryptionModuleResultPromise(ScriptState*);
On 2014/11/11 22:44:09, ddorwin wrote:
> protected? Should this really be instantiated?

Done.

https://codereview.chromium.org/687533004/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.h:27: virtual
void completeWithSession(WebContentDecryptionModuleResult::SessionStatus)
override;
On 2014/11/11 22:44:09, ddorwin wrote:
> Why does this "base class" have this specialized completion method? Why
doesn't
> it contain the others?

ContentDecryptionModuleResult has 3 complete methods, and all are specified
here. The goal is to provide default implementations (that cause the promise to
be rejected) so that subclasses only need to override the complete method they
expect to get called.

https://codereview.chromium.org/687533004/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.h:36: //
Called when the promise is to be resolved by derived classes.
On 2014/11/11 22:44:09, ddorwin wrote:
> Resolves the promise with |value|. Used by subclasses to....

Done.

https://codereview.chromium.org/687533004/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/ContentDecryptionModuleResultPromise.h:44: //
Reject the promise with a DOMException.
On 2014/11/11 22:44:09, ddorwin wrote:
> nit: Rejects

Done.

https://codereview.chromium.org/687533004/diff/1/Source/modules/encryptedmedi...
File Source/modules/encryptedmedia/MediaKeySession.cpp (right):

https://codereview.chromium.org/687533004/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeySession.cpp:205: class NewSessionResult :
public ContentDecryptionModuleResultPromise {
On 2014/11/11 22:44:09, ddorwin wrote:
> nit: If the base and Simple* are "...ResultPromise", these two should be as
> well.

Done.

https://codereview.chromium.org/687533004/diff/1/Source/modules/encryptedmedi...
File Source/modules/encryptedmedia/SimpleContentDecryptionModuleResultPromise.h
(right):

https://codereview.chromium.org/687533004/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/SimpleContentDecryptionModuleResultPromise.h:14:
// methods will reject the promise.
On 2014/11/11 22:44:09, ddorwin wrote:
> nit: Perhaps better to say that they should not be used. This _could_ be
> interpreted as saying they should be called to reject.

Done.

https://codereview.chromium.org/687533004/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/SimpleContentDecryptionModuleResultPromise.h:20:
// ContentDecryptionModuleResult implementation.
On 2014/11/11 22:44:09, ddorwin wrote:
> ContentDecryptionModuleResultPromise?

Done.

[ddorwin, 2014-11-12 01:14:33]

lgtm

Thanks.

[jrummell, 2014-11-12 01:22:33]

The CQ bit was checked by jrummell@chromium.org

[commit-bot: I haz the power, 2014-11-12 01:22:51]

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

[commit-bot: I haz the power, 2014-11-12 02:26:10]

Committed patchset #2 (id:20001) as 185168