Web MIDI: add open() and close() to MIDIPort

Web MIDI: add open() and close() to MIDIPort

The latest editor's draft introduces open() and close() interfaces
to MIDIPort. This patch implements open() and close() interfaces, and
realizes proper MIDIPortState transition on open() and close().

See, http://webaudio.github.io/web-midi-api/#idl-def-MIDIPort, and
http://webaudio.github.io/web-midi-api/#idl-def-MIDIPortState.

For now, implicit open that should happen on send() and handler set
are not implemented yet. But, MIDIPort works even on "connected" state.
So, this does not break compatibility.
These things will be fixed at the next patch.

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

[Takashi Toyoshima, 2015-02-26 12:11:37]

toyoshim@chromium.org changed reviewers:
+ kouhei@chromium.org, yhirano@chromium.org

[Takashi Toyoshima, 2015-02-26 12:13:36]

ptal

[yhirano, 2015-02-26 13:00:49]

Shouldn't we introduce "closed" state?

[Takashi Toyoshima, 2015-02-26 13:34:35]

There is a discussion on MIDIPortState.
There, we agreed that "connected" means "closed" state.

https://github.com/WebAudio/web-midi-api/issues/105
https://github.com/WebAudio/web-midi-api/issues/75

In the former thread, bome proposed three state transition. IIRC there were no
objection against this idea.

[yhirano, 2015-02-26 13:38:32]

On 2015/02/26 13:34:35, Takashi Toyoshima (chromium) wrote:
> There is a discussion on MIDIPortState.
> There, we agreed that "connected" means "closed" state.
> 
> https://github.com/WebAudio/web-midi-api/issues/105
> https://github.com/WebAudio/web-midi-api/issues/75
> 
> In the former thread, bome proposed three state transition. IIRC there were no
> objection against this idea.

You mean the following statement in 
http://webaudio.github.io/web-midi-api/#widl-MIDIPort-close-Promise-MIDIPort
will be fixed, right?

success: Change the state attribute of the MIDIPort to "closed", and enqueue a
new MIDIConnectionEvent to the statechange handler of the MIDIAccess and to the
statechange handler of the MIDIPort.

[Takashi Toyoshima, 2015-02-26 14:09:50]

Oh, yes. That should be a typo. We didn't define "closed" state in
MIDIPortState,
http://webaudio.github.io/web-midi-api/#idl-def-MIDIPortState.disconnected
I'll send a pull request to fix it.

[Takashi Toyoshima, 2015-02-26 14:15:00]

sent: https://github.com/WebAudio/web-midi-api/pull/121

[yhirano, 2015-02-27 04:04:41]

https://codereview.chromium.org/962523005/diff/20001/Source/modules/webmidi/M...
File Source/modules/webmidi/MIDIPort.cpp (right):

https://codereview.chromium.org/962523005/diff/20001/Source/modules/webmidi/M...
Source/modules/webmidi/MIDIPort.cpp:45: class MIDIPortStateTransition : public
ScriptPromiseResolver {
I don't know about the future design, but at this moment using a simple
ScriptPromiseResolver seems enough.

https://codereview.chromium.org/962523005/diff/20001/Source/modules/webmidi/M...
Source/modules/webmidi/MIDIPort.cpp:124: // FIXME: Remove following code once
blink API has a real open and close operations.
Please wrap the comment in 80 columns.

[Takashi Toyoshima, 2015-02-27 13:15:00]

https://codereview.chromium.org/962523005/diff/20001/Source/modules/webmidi/M...
File Source/modules/webmidi/MIDIPort.cpp (right):

https://codereview.chromium.org/962523005/diff/20001/Source/modules/webmidi/M...
Source/modules/webmidi/MIDIPort.cpp:45: class MIDIPortStateTransition : public
ScriptPromiseResolver {
On 2015/02/27 04:04:40, yhirano wrote:
> I don't know about the future design, but at this moment using a simple
> ScriptPromiseResolver seems enough.
> 

Done.

https://codereview.chromium.org/962523005/diff/20001/Source/modules/webmidi/M...
Source/modules/webmidi/MIDIPort.cpp:124: // FIXME: Remove following code once
blink API has a real open and close operations.
On 2015/02/27 04:04:41, yhirano wrote:
> Please wrap the comment in 80 columns.

Done.

[Takashi Toyoshima, 2015-02-27 13:20:12]

https://codereview.chromium.org/962523005/diff/40001/Source/modules/webmidi/M...
File Source/modules/webmidi/MIDIPort.cpp (right):

https://codereview.chromium.org/962523005/diff/40001/Source/modules/webmidi/M...
Source/modules/webmidi/MIDIPort.cpp:145:
RefPtrWillBeRawPtr<ScriptPromiseResolver> resolver =
ScriptPromiseResolver::create(scriptState);
Oh, I just notice that cast() is what I want to use here now.
I'll update this CL again.

[Takashi Toyoshima, 2015-02-27 14:09:03]

ptal Patch Set 4

[yhirano, 2015-03-02 05:34:32]

https://codereview.chromium.org/962523005/diff/60001/LayoutTests/webmidi/open...
File LayoutTests/webmidi/open_close.html (right):

https://codereview.chromium.org/962523005/diff/60001/LayoutTests/webmidi/open...
LayoutTests/webmidi/open_close.html:75: checkCloseOnConnected(port);
How about listing tests here?

return Promise.resolve()
    .then(checkInitialStatus.bind(undefined, port))
    .then(checkCloseOnConnected.bind(undefined, port))
    .then(checkOpenOnConnected.bind(undefined, port))
    .then(checkCloseOnOpened.bind(undefined, port)); 

checkCloseOnConnected can be defined like this:

function checkCloseOnConnected(port) {
    window.port = port;
    shouldBeEqualToString("port.state", "connected");
    port.onstatechange = openedPortHandler;
    return port.open().then(function() {
        shouldBeEqualToString("port.state", "opened");
    }, function(e) {
        testFailed("...");
        // If you want to stop the test here.
        throw e;
    });
}

https://codereview.chromium.org/962523005/diff/60001/LayoutTests/webmidi/open...
LayoutTests/webmidi/open_close.html:82:
checkMIDIPortStateTransition(access.inputs.values().next().value);
If you return a promise from checkMIDIPortStateTransition, you can write
finishJSTest here.


checkMIDIPortStateTransition(...).then(finishJSTest, finishJSTest);

[Takashi Toyoshima, 2015-03-02 15:44:44]

https://codereview.chromium.org/962523005/diff/60001/LayoutTests/webmidi/open...
File LayoutTests/webmidi/open_close.html (right):

https://codereview.chromium.org/962523005/diff/60001/LayoutTests/webmidi/open...
LayoutTests/webmidi/open_close.html:75: checkCloseOnConnected(port);
that's nice idea, and I went more aggressively in the last patch set.

https://codereview.chromium.org/962523005/diff/60001/LayoutTests/webmidi/open...
LayoutTests/webmidi/open_close.html:82:
checkMIDIPortStateTransition(access.inputs.values().next().value);
On 2015/03/02 05:34:32, yhirano wrote:
> If you return a promise from checkMIDIPortStateTransition, you can write
> finishJSTest here.
> 
> 
> checkMIDIPortStateTransition(...).then(finishJSTest, finishJSTest);

Done.

[yhirano, 2015-03-03 11:18:31]

https://codereview.chromium.org/962523005/diff/80001/LayoutTests/webmidi/open...
File LayoutTests/webmidi/open_close.html (right):

https://codereview.chromium.org/962523005/diff/80001/LayoutTests/webmidi/open...
LayoutTests/webmidi/open_close.html:16: port.onstatechange = function(e) {
Throwing from this function has no effect: You can write as follows:

var callbackPromise = new Promise(resolve, reject) {
    port.onstatechange = function(e) {
        ...
        if (...) {
            reject(options);
            return;
        }
        ...
        resolve();
    };
});


return port[options.method]().then(function(p) {
    ...
    return callbackPromise;
}, ...);
};

[Takashi Toyoshima, 2015-03-03 13:18:39]

https://codereview.chromium.org/962523005/diff/80001/LayoutTests/webmidi/open...
File LayoutTests/webmidi/open_close.html (right):

https://codereview.chromium.org/962523005/diff/80001/LayoutTests/webmidi/open...
LayoutTests/webmidi/open_close.html:16: port.onstatechange = function(e) {
Oh, nice catch.
But, I guess your suggesting approach still have a problem because onstatechange
may not be called if the state is not changed, and that results in hangup.

I think, testFailed can catch unexpected calls as a expectation mismatch.
So just removing "throw options" works fine.

[yhirano, 2015-03-04 04:12:47]

lgtm

[Takashi Toyoshima, 2015-03-04 04:48:07]

The CQ bit was checked by toyoshim@chromium.org

[commit-bot: I haz the power, 2015-03-04 04:49:00]

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

[commit-bot: I haz the power, 2015-03-04 07:34:04]

Committed patchset #6 (id:100001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=191259