Make WebMIDI use blink Promise.

Source/modules/webmidi/MIDIAccess.cpp

Index: Source/modules/webmidi/MIDIAccess.cpp
diff --git a/Source/modules/webmidi/MIDIAccess.cpp b/Source/modules/webmidi/MIDIAccess.cpp
index 5a572fb276a6af4272f4e980dc12a8416964d21f..5f56494d6288b1a6737ce0b7d1679c735ff95cd0 100644
--- a/Source/modules/webmidi/MIDIAccess.cpp
+++ b/Source/modules/webmidi/MIDIAccess.cpp
@@ -31,33 +31,39 @@
 #include "config.h"
 #include "modules/webmidi/MIDIAccess.h"
 
+#include "bindings/v8/DOMRequestState.h"
+#include "bindings/v8/DOMWrapperWorld.h"
+#include "bindings/v8/ScriptPromise.h"
+#include "bindings/v8/ScriptPromiseResolver.h"
 #include "core/dom/DOMError.h"
 #include "core/dom/Document.h"
 #include "core/loader/DocumentLoadTiming.h"
 #include "core/loader/DocumentLoader.h"
-#include "modules/webmidi/MIDIAccessPromise.h"
 #include "modules/webmidi/MIDIConnectionEvent.h"
 #include "modules/webmidi/MIDIController.h"
+#include "modules/webmidi/MIDIOptions.h"
 #include "modules/webmidi/MIDIPort.h"
+#include "modules/webmidi/NavigatorWebMIDI.h"
 
 namespace WebCore {
 
-PassRefPtr<MIDIAccess> MIDIAccess::create(ExecutionContext* context, MIDIAccessPromise* promise)
+PassRefPtr<MIDIAccess> MIDIAccess::create(const MIDIOptions& options, ExecutionContext* context, NavigatorWebMIDI* navigator)
 {
-    RefPtr<MIDIAccess> midiAccess(adoptRef(new MIDIAccess(context, promise)));
+    RefPtr<MIDIAccess> midiAccess(adoptRef(new MIDIAccess(options, context, navigator)));
     midiAccess->suspendIfNeeded();
-    midiAccess->startRequest();
     return midiAccess.release();
 }
 
 MIDIAccess::~MIDIAccess()
 {
     stop();
+    ASSERT(!m_navigator);
 }
 
-MIDIAccess::MIDIAccess(ExecutionContext* context, MIDIAccessPromise* promise)
+MIDIAccess::MIDIAccess(const MIDIOptions& options, ExecutionContext* context, NavigatorWebMIDI* navigator)
     : ActiveDOMObject(context)
-    , m_promise(promise)
+    , m_navigator(navigator)
+    , m_options(options)
     , m_hasAccess(false)
     , m_sysExEnabled(false)
     , m_requesting(false)
@@ -68,12 +74,12 @@ MIDIAccess::MIDIAccess(ExecutionContext* context, MIDIAccessPromise* promise)
 
 void MIDIAccess::setSysExEnabled(bool enable)
 {
-    m_requesting = false;
     m_sysExEnabled = enable;
     if (enable)
         m_accessor->startSession();
     else
-        permissionDenied();
+        reject(DOMError::create("SecurityError"));
+    // |this| can be deleted here.

[haraken, 2013/12/06 00:42:29]

This sounds ugly. How about putting 'RefPtr<MIDIAccess> protect(this)' at the
beginning of this method?

[yhirano, 2014/02/21 10:52:30]

Done.

 }
 
 void MIDIAccess::didAddInputPort(const String& id, const String& manufacturer, const String& name, const String& version)
@@ -94,12 +100,13 @@ void MIDIAccess::didAddOutputPort(const String& id, const String& manufacturer,
 void MIDIAccess::didStartSession(bool success)
 {
     ASSERT(isMainThread());
-
-    m_hasAccess = success;
+    if (!m_requesting)
+        return;
     if (success)
-        m_promise->fulfill();
+        resolve();
     else
-        m_promise->reject(DOMError::create("InvalidStateError"));
+        reject(DOMError::create("InvalidStateError"));
+    // |this| can be deleted here.

[haraken, 2013/12/06 00:42:29]

Ditto. The same comment for other unprotected |this| objects.

[yhirano, 2014/02/21 10:52:30]

Done.

 }
 
 void MIDIAccess::didReceiveMIDIData(unsigned portIndex, const unsigned char* data, size_t length, double timeStamp)
@@ -144,9 +151,12 @@ void MIDIAccess::sendMIDIData(unsigned portIndex, const unsigned char* data, siz
 void MIDIAccess::stop()
 {
     m_hasAccess = false;
-    if (!m_requesting)
+    if (!m_requesting) {
+        // Since MIDIAccess is not pending, this object cannot be registered
+        // in |m_navigator|.
+        m_navigator = 0;
         return;
-    m_requesting = false;
+    }
     Document* document = toDocument(executionContext());
     ASSERT(document);
     MIDIController* controller = MIDIController::from(document->page());
@@ -154,31 +164,76 @@ void MIDIAccess::stop()
     controller->cancelSysExPermissionRequest(this);
 
     m_accessor.clear();
+    reject(DOMError::create("AbortError"));
+    // |this| can be deleted here.
 }
 
-void MIDIAccess::startRequest()
+void MIDIAccess::permissionDenied()
+{
+    ASSERT(isMainThread());
+    reject(DOMError::create("SecurityError"));
+    // |this| can be deleted here.
+}
+
+ScriptPromise MIDIAccess::startRequest()
 {
-    if (!m_promise->options()->sysex) {
+    ScriptPromise promise = ScriptPromise::createPending();
+    m_resolver = ScriptPromiseResolver::create(promise, executionContext());
+    m_world = RefPtr<DOMWrapperWorld>(DOMWrapperWorld::current());
+    m_requesting = true;
+    ASSERT(m_navigator);
+    m_navigator->registerPending(this);
+    if (!m_options.sysex) {
         m_accessor->startSession();
-        return;
+        return promise;
     }
     Document* document = toDocument(executionContext());
     ASSERT(document);
     MIDIController* controller = MIDIController::from(document->page());
-    if (controller) {
-        m_requesting = true;
+    if (controller)
         controller->requestSysExPermission(this);
-    } else {
-        permissionDenied();
-    }
+    else
+        reject(DOMError::create("SecurityError"));
+    // |this| can be deleted here.
+    return promise;
 }
 
-void MIDIAccess::permissionDenied()
+void MIDIAccess::resolve()
 {
-    ASSERT(isMainThread());
+    if (!m_requesting)
+        return;
+    ASSERT(m_world.get());
+    DOMRequestState state(executionContext(), m_world);

[haraken, 2013/12/06 00:42:29]

I don't fully understand why you need to store m_world in startRequest() and
pass the m_world explicitly to DOMRequestState here.

Is it possible that resolve() is called from a different isolated world that
called startRequest()? In other words, is it possible that m_world is different
from DOMWrapperWorld::current()?

[yhirano, 2014/02/21 10:52:30]

This function can be called from IPC handlers. In such a case,
DOMWrapperWorld::current() returns null.

+    DOMRequestState::Scope scope(state);
+    m_world.clear();
+    m_hasAccess = true;
+    m_requesting = false;
+    // FIXME: Care about suspend / stop.
+    m_resolver->resolve(this, executionContext());
+    NavigatorWebMIDI* navigator = m_navigator;
+    m_navigator = 0;
+    if (navigator)
+        navigator->unregisterPending(this);
+    // |this| can be deleted here.
+}
 
+void MIDIAccess::reject(PassRefPtr<DOMError> error)
+{
+    if (!m_requesting)
+        return;
+    ASSERT(m_world.get());
+    DOMRequestState state(executionContext(), m_world);
+    DOMRequestState::Scope scope(state);
+    m_world.clear();
     m_hasAccess = false;
-    m_promise->reject(DOMError::create("SecurityError"));
+    m_requesting = false;
+    // FIXME: Care about suspend / stop.
+    m_resolver->reject(error, executionContext());
+    NavigatorWebMIDI* navigator = m_navigator;
+    m_navigator = 0;
+    if (navigator)
+        navigator->unregisterPending(this);

[haraken, 2013/12/06 00:42:29]

The lifetime management of m_navigator looks fragile. Can you use wtf/WeakPtr?

[yhirano, 2014/02/21 10:52:30]

This class now uses [un]setPendingActivity for the purpose.

+    // |this| can be deleted here.
 }
 
 } // namespace WebCore