Make HTMLMediaElement.setMediaKeys() asynchronous.

Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp

Index: Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp
diff --git a/Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp b/Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp
index 1a3969e9b1750239c11716657ba29156140c10f2..acf290e76e8fb0cc940a2fa27e4103feca2c34df 100644
--- a/Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp
+++ b/Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp
@@ -6,12 +6,18 @@
 #include "modules/encryptedmedia/HTMLMediaElementEncryptedMedia.h"
 
 #include "bindings/core/v8/ExceptionState.h"
+#include "bindings/core/v8/ScriptPromise.h"
+#include "bindings/core/v8/ScriptPromiseResolver.h"
+#include "bindings/core/v8/ScriptState.h"
+#include "core/dom/DOMException.h"
 #include "core/dom/ExceptionCode.h"
 #include "core/html/HTMLMediaElement.h"
 #include "core/html/MediaKeyError.h"
 #include "core/html/MediaKeyEvent.h"
 #include "modules/encryptedmedia/MediaKeyNeededEvent.h"
 #include "modules/encryptedmedia/MediaKeys.h"
+#include "modules/encryptedmedia/SimpleContentDecryptionModuleResult.h"
+#include "platform/ContentDecryptionModuleResult.h"
 #include "platform/Logging.h"
 #include "platform/RuntimeEnabledFeatures.h"
 #include "wtf/Uint8Array.h"
@@ -38,6 +44,191 @@ static void throwExceptionIfMediaKeyExceptionOccurred(const String& keySystem, c
     return;
 }
 
+// This class allows MediaKeys to be set asynchronously.
+class SetMediaKeysHandler : public ScriptPromiseResolver {
+    WTF_MAKE_NONCOPYABLE(SetMediaKeysHandler);
+
+public:
+    static ScriptPromise create(ScriptState*, HTMLMediaElement&, MediaKeys*);
+    virtual ~SetMediaKeysHandler();
+
+    void completeWithDOMException(ExceptionCode, const String& errorMessage);

[ddorwin, 2014/08/08 02:50:51]

Is completeWithDOMException the terminology used everywhere. Why not
rejectWith...?

[jrummell, 2014/08/08 23:15:12]

ContentDecryptionModuleResult uses completeWith..., based on WebCryptoResult.
ScriptPromise uses rejectWithDOMException. Gone now.

+    void timerFired(Timer<SetMediaKeysHandler>*);
+    void proceedToNextStage();
+
+private:
+    enum Stage {
+        Starting,
+        Clearing,
+        Setting
+    };
+    SetMediaKeysHandler(ScriptState*, HTMLMediaElement&, MediaKeys*);
+
+    RawPtrWillBeMember<HTMLMediaElement> m_element;
+    Persistent<MediaKeys> m_mediaKeys;
+
+    Stage m_stage;
+    Timer<SetMediaKeysHandler> m_timer;
+};
+
+// Represents the result used when setContentDecryptionModule() is called.

[ddorwin, 2014/08/08 02:50:52]

It seems there must be a cleaner way of handling this. Maybe an object that
calls a callback (proceedToNextStage) provided at construction time and rejects
the promise on failure. For the final call, can the result just be provided
directly without using this class?

[jrummell, 2014/08/08 23:15:12]

How about this?

+// Needed as SetMediaKeysHandler may need to call setContentDecryptionModule()
+// multiple times, and not resolve the promise until the very end. Any errors

[ddorwin, 2014/08/08 02:50:51]

What does until the very end mean? Each setMK should be resolved when it
completes.

[jrummell, 2014/08/08 23:15:13]

Removed.

+// that happen will reject the promise immediately.

[ddorwin, 2014/08/08 02:50:51]

But not if a worker is not provided. I think this is a problem with this design.

[jrummell, 2014/08/08 23:15:12]

Acknowledged.

+//
+// This class can be used without specifying a worker to call. Useful when

[ddorwin, 2014/08/08 02:50:51]

Replace "worker".

[jrummell, 2014/08/08 23:15:12]

Done.

+// calling setContentDecryptionModule() while tearing down a player.

[ddorwin, 2014/08/08 02:50:51]

*Not specifying is* useful when...  ?
Do we even need this class in that case? Is there a base class we could use
instead?

[jrummell, 2014/08/08 23:15:12]

Removed.

+class SetContentDecryptionModuleResult FINAL : public ContentDecryptionModuleResult {
+public:
+    explicit SetContentDecryptionModuleResult(SetMediaKeysHandler* worker)
+        : m_worker(worker)

[ddorwin, 2014/08/08 02:50:51]

change var names too

[jrummell, 2014/08/08 23:15:12]

Done.

+    {
+    }
+
+    // ContentDecryptionModuleResult implementation.
+    virtual void complete() OVERRIDE
+    {
+        if (m_worker)
+            m_worker->proceedToNextStage();
+    }
+
+    virtual void completeWithSession(blink::WebContentDecryptionModuleResult::SessionStatus status) OVERRIDE
+    {
+        ASSERT_NOT_REACHED();
+        if (m_worker)
+            m_worker->completeWithDOMException(InvalidStateError, "Unexpected completion.");
+    }
+
+    virtual void completeWithError(blink::WebContentDecryptionModuleException code, unsigned long systemCode, const blink::WebString& message) OVERRIDE
+    {
+        if (m_worker)
+            m_worker->completeWithDOMException(WebCdmExceptionToExceptionCode(code), message);
+    }
+
+private:
+    SetMediaKeysHandler* m_worker;
+};
+
+ScriptPromise SetMediaKeysHandler::create(ScriptState* scriptState, HTMLMediaElement& element, MediaKeys* mediaKeys)
+{
+    RefPtr<SetMediaKeysHandler> worker = adoptRef(new SetMediaKeysHandler(scriptState, element, mediaKeys));
+    worker->suspendIfNeeded();
+    worker->keepAliveWhilePending();
+    return worker->promise();
+}
+
+SetMediaKeysHandler::SetMediaKeysHandler(ScriptState* scriptState, HTMLMediaElement& element, MediaKeys* mediaKeys)
+    : ScriptPromiseResolver(scriptState)
+    , m_element(element)
+    , m_mediaKeys(mediaKeys)
+    , m_stage(Starting)
+    , m_timer(this, &SetMediaKeysHandler::timerFired)
+{
+    WTF_LOG(Media, "SetMediaKeysHandler::SetMediaKeysHandler");
+
+    // 3. Complete setting this asynchronously.

[ddorwin, 2014/08/08 02:50:51]

nit: Run the remaining steps asynchronously?

[jrummell, 2014/08/08 23:15:12]

Done.

+    m_timer.startOneShot(0, FROM_HERE);
+}
+
+SetMediaKeysHandler::~SetMediaKeysHandler()
+{
+}
+
+void SetMediaKeysHandler::completeWithDOMException(ExceptionCode code, const String& errorMessage)
+{
+    reject(DOMException::create(code, errorMessage));
+}
+
+void SetMediaKeysHandler::timerFired(Timer<SetMediaKeysHandler>*)
+{
+    ASSERT(m_stage == Starting);
+    proceedToNextStage();

[ddorwin, 2014/08/08 02:50:51]

I assume this can only be called in Starting from this location. Should that
just be a separate function instead of a stage? That might simplify things. I
still need to understand the other stages and whether we could then eliminate
them

[jrummell, 2014/08/08 23:15:12]

Done.

+}
+
+void SetMediaKeysHandler::proceedToNextStage()
+{
+    WTF_LOG(Media, "SetMediaKeysHandler::proceedToNextStage(%d)", m_stage);
+    HTMLMediaElementEncryptedMedia& thisElement = HTMLMediaElementEncryptedMedia::from(*m_element);
+
+    switch (m_stage) {
+    case Starting:
+        // 3.1 If mediaKeys is not null, it is already in use by another media
+        //     element, and the user agent is unable to use it with this element,
+        //     reject promise with a new DOMException whose name is
+        //     "QuotaExceededError". (Currently no restrictions on using the same

[ddorwin, 2014/08/08 02:50:52]

What is the purpose of the statement in ()?

[jrummell, 2014/08/08 23:15:12]

Just to note why there is no code for this step.

+        //     MediaKeys with multiple media elements.)
+        // 3.2 If the mediaKeys attribute is not null, run the following steps:
+        m_stage = Clearing;
+        if (thisElement.m_mediaKeys) {
+            // 3.2.1 If the user agent or CDM do not support removing the
+            //       association, return a promise rejected with a new DOMException
+            //       whose name is "NotSupportedError".
+            // 3.2.2 If the association cannot currently be removed (i.e. during
+            //       playback), return a promise rejected with a new DOMException
+            //       whose name is "InvalidStateError".
+            if (m_element->isPlaying()) {

[ddorwin, 2014/08/08 02:50:51]

Note that "playback" could also mean load has occurred. If !playing is enough
for us, greeat.

[jrummell, 2014/08/08 23:15:12]

isPlaying() is the only function I could find. However, if the load has
occurred, then it's possible the pipeline has been initialized, so checking load
status would be better. However, load status is private. Not sure if there is a
better test.

+                completeWithDOMException(InvalidStateError, "The existing MediaKeys object cannot be removed at this time.");

[ddorwin, 2014/08/08 02:50:51]

... removed while playing?

[jrummell, 2014/08/08 23:15:12]

Sure. I think the error message is from the older spec where the messages were
specified.

+                return;
+            }
+
+            // 3.2.3 Stop using the CDM instance represented by the mediaKeys
+            //       attribute to decrypt media data and remove the association
+            //       with the media element.
+            // 3.2.4 If the preceding step failed, reject promise with a new
+            //       DOMException whose name is the appropriate error name and
+            //       that has an appropriate message.
+            if (m_element->webMediaPlayer()) {
+                ContentDecryptionModuleResult* result = new SetContentDecryptionModuleResult(this);
+                m_element->webMediaPlayer()->setContentDecryptionModule(nullptr, result->result());
+
+                // Don't do anything more until SetContentDecryptionModuleResult

[ddorwin, 2014/08/08 02:50:51]

...until |result| is resolved?

[jrummell, 2014/08/08 23:15:12]

Done.

+                // finishes (and calls back to proceedToNextStage()).
+                return;
+            }
+        }
+
+        // MediaKeys not currently set or no player connected. Continue to next
+        // stage.
+
+    case Clearing:
+        // Successfully cleared MediaKeys, so drop reference to previous value.
+        thisElement.m_mediaKeys.clear();

[ddorwin, 2014/08/08 02:50:51]

Technically, the JS attribute is not supposed to be cleared yet. We're just
supposed to run line 224 in all cases except failing (l203).
I'm open to changing that.

[jrummell, 2014/08/08 23:15:11]

Done this way in case setContentDecryptionModule() call below fails. Fixed.

+
+        // 3.3 If mediaKeys is not null, run the following steps:
+        m_stage = Setting;
+        if (m_mediaKeys) {

[ddorwin, 2014/08/08 02:50:51]

This name is confusing (see 195). Maybe m_newMediaKeys. 

[jrummell, 2014/08/08 23:15:12]

Done.

+            // 3.3.1 Associate the CDM instance represented by mediaKeys with the

[ddorwin, 2014/08/08 02:50:51]

These are large blocks, which would be good in functions. OTOH, it's also nice
to have all the steps together in order.
Getting rid of Stage would probably be motivation enough, though, and we can
still have the steps in order in different functions.

[jrummell, 2014/08/08 23:15:12]

Acknowledged.

+            //       media element for decrypting media data.
+            // 3.3.2 If the preceding step failed, run the following steps:
+            // 3.3.2.1 Set the mediaKeys attribute to null.

[ddorwin, 2014/08/08 02:50:52]

This is handled above. We should note that. But maybe we should do this in the
promise instead (see also the comment at l195). Of course, the failure at 182
would need to not clear the attribute. It would be nice if we could catch/wrap
the failures and put the failure text and code there.

[jrummell, 2014/08/08 23:15:12]

Acknowledged.

+            // 3.3.2.2 Reject promise with a new DOMException whose name is the
+            //         appropriate error name and that has an appropriate message.
+            // 3.3.3 Run the Attempt to Resume Playback If Necessary algorithm on
+            //       the media element. The user agent may choose to skip this
+            //       step if it knows resuming will fail (i.e. mediaKeys has no
+            //       sessions).
+            if (m_element->webMediaPlayer()) {
+                ContentDecryptionModuleResult* result = new SetContentDecryptionModuleResult(this);
+                m_element->webMediaPlayer()->setContentDecryptionModule(m_mediaKeys->contentDecryptionModule(), result->result());
+
+                // Don't do anything more until SetContentDecryptionModuleResult

[ddorwin, 2014/08/08 02:50:52]

ditto

[jrummell, 2014/08/08 23:15:12]

Done.

+                // finishes (and calls back to proceedToNextStage()).
+                return;
+            }
+        }
+        // MediaKeys doesn't need to be set on the player, so continue on to
+        // next stage.
+
+    case Setting:
+        // 3.4 Set the mediaKeys attribute to mediaKeys.
+        thisElement.m_mediaKeys = m_mediaKeys;
+
+        // 3.5 Resolve promise with undefined.
+        resolve();
+        return;
+    }
+}
+
 HTMLMediaElementEncryptedMedia::HTMLMediaElementEncryptedMedia()
     : m_emeMode(EmeModeNotSelected)
 {
@@ -60,12 +251,11 @@ HTMLMediaElementEncryptedMedia& HTMLMediaElementEncryptedMedia::from(HTMLMediaEl
     return *supplement;
 }
 
-bool HTMLMediaElementEncryptedMedia::setEmeMode(EmeMode emeMode, ExceptionState& exceptionState)
+bool HTMLMediaElementEncryptedMedia::setEmeMode(EmeMode emeMode)
 {
-    if (m_emeMode != EmeModeNotSelected && m_emeMode != emeMode) {
-        exceptionState.throwDOMException(InvalidStateError, "Mixed use of EME prefixed and unprefixed API not allowed.");
+    if (m_emeMode != EmeModeNotSelected && m_emeMode != emeMode)
         return false;
-    }
+
     m_emeMode = emeMode;
     return true;
 }
@@ -81,28 +271,21 @@ MediaKeys* HTMLMediaElementEncryptedMedia::mediaKeys(HTMLMediaElement& element)
     return thisElement.m_mediaKeys.get();
 }
 
-void HTMLMediaElementEncryptedMedia::setMediaKeysInternal(HTMLMediaElement& element, MediaKeys* mediaKeys)
-{
-    if (m_mediaKeys == mediaKeys)
-        return;
-
-    ASSERT(m_emeMode == EmeModeUnprefixed);
-    m_mediaKeys = mediaKeys;
-
-    // If a player is connected, tell it that the CDM has changed.
-    if (element.webMediaPlayer())
-        element.webMediaPlayer()->setContentDecryptionModule(contentDecryptionModule());
-}
-
-void HTMLMediaElementEncryptedMedia::setMediaKeys(HTMLMediaElement& element, MediaKeys* mediaKeys, ExceptionState& exceptionState)
+ScriptPromise HTMLMediaElementEncryptedMedia::setMediaKeys(ScriptState* scriptState, HTMLMediaElement& element, MediaKeys* mediaKeys)
 {
-    WTF_LOG(Media, "HTMLMediaElementEncryptedMedia::setMediaKeys");
     HTMLMediaElementEncryptedMedia& thisElement = HTMLMediaElementEncryptedMedia::from(element);
+    WTF_LOG(Media, "HTMLMediaElementEncryptedMedia::setMediaKeys current(%p), new(%p)", thisElement.m_mediaKeys.get(), mediaKeys);
 
-    if (!thisElement.setEmeMode(EmeModeUnprefixed, exceptionState))
-        return;
+    if (!thisElement.setEmeMode(EmeModeUnprefixed))
+        return ScriptPromise::rejectWithDOMException(scriptState, DOMException::create(InvalidStateError, "Mixed use of EME prefixed and unprefixed API not allowed."));
+
+    // 1. If mediaKeys and the mediaKeys attribute are the same object, return
+    //    a promise resolved with undefined.
+    if (thisElement.m_mediaKeys == mediaKeys)
+        return ScriptPromise::cast(scriptState, V8ValueTraits<V8UndefinedType>::toV8Value(V8UndefinedType(), scriptState->context()->Global(), scriptState->isolate()));
 
-    thisElement.setMediaKeysInternal(element, mediaKeys);
+    // 2. Let promise be a new promise. Remaining steps done in worker.

[ddorwin, 2014/08/08 02:50:52]

handler

[jrummell, 2014/08/08 23:15:12]

Done.

+    return SetMediaKeysHandler::create(scriptState, element, mediaKeys);
 }
 
 // Create a MediaKeyNeededEvent for WD EME.
@@ -139,8 +322,10 @@ void HTMLMediaElementEncryptedMedia::generateKeyRequest(blink::WebMediaPlayer* w
 {
     WTF_LOG(Media, "HTMLMediaElementEncryptedMedia::webkitGenerateKeyRequest");
 
-    if (!setEmeMode(EmeModePrefixed, exceptionState))
+    if (!setEmeMode(EmeModePrefixed)) {
+        exceptionState.throwDOMException(InvalidStateError, "Mixed use of EME prefixed and unprefixed API not allowed.");
         return;
+    }
 
     if (keySystem.isEmpty()) {
         exceptionState.throwDOMException(SyntaxError, "The key system provided is empty.");
@@ -177,8 +362,10 @@ void HTMLMediaElementEncryptedMedia::addKey(blink::WebMediaPlayer* webMediaPlaye
 {
     WTF_LOG(Media, "HTMLMediaElementEncryptedMedia::webkitAddKey");
 
-    if (!setEmeMode(EmeModePrefixed, exceptionState))
+    if (!setEmeMode(EmeModePrefixed)) {
+        exceptionState.throwDOMException(InvalidStateError, "Mixed use of EME prefixed and unprefixed API not allowed.");
         return;
+    }
 
     if (keySystem.isEmpty()) {
         exceptionState.throwDOMException(SyntaxError, "The key system provided is empty.");
@@ -225,8 +412,10 @@ void HTMLMediaElementEncryptedMedia::cancelKeyRequest(blink::WebMediaPlayer* web
 {
     WTF_LOG(Media, "HTMLMediaElementEncryptedMedia::webkitCancelKeyRequest");
 
-    if (!setEmeMode(EmeModePrefixed, exceptionState))
+    if (!setEmeMode(EmeModePrefixed)) {
+        exceptionState.throwDOMException(InvalidStateError, "Mixed use of EME prefixed and unprefixed API not allowed.");
         return;
+    }
 
     if (keySystem.isEmpty()) {
         exceptionState.throwDOMException(SyntaxError, "The key system provided is empty.");
@@ -341,7 +530,17 @@ void HTMLMediaElementEncryptedMedia::playerDestroyed(HTMLMediaElement& element)
 #endif
 
     HTMLMediaElementEncryptedMedia& thisElement = HTMLMediaElementEncryptedMedia::from(element);
-    thisElement.setMediaKeysInternal(element, 0);
+    if (!thisElement.m_mediaKeys)
+        return;
+
+    ASSERT(thisElement.m_emeMode == EmeModeUnprefixed);
+    thisElement.m_mediaKeys.clear();
+
+    // If a player is connected, tell it that the CDM has changed.
+    if (element.webMediaPlayer()) {

[ddorwin, 2014/08/08 02:50:51]

Won't this always be false in this function? (I see this was copied from
setMediaKeysInternal(), but it may not have applied here.)

[jrummell, 2014/08/08 23:15:11]

Converted to an ASSERT, which doesn't fire in any of the tests.

+        SetContentDecryptionModuleResult* result = new SetContentDecryptionModuleResult(0);
+        element.webMediaPlayer()->setContentDecryptionModule(thisElement.contentDecryptionModule(), result->result());
+    }
 }
 
 blink::WebContentDecryptionModule* HTMLMediaElementEncryptedMedia::contentDecryptionModule(HTMLMediaElement& element)