Web MIDI: implement implicit open() on send() and setOnmidimessage()

Web MIDI: implement implicit open() on send() and setOnmidimessage()

For compatibility and usability, the spec defines that send() and
setting a onmidimessage handler invoke implcit open() operation.

This patch implemnts the implcit open() for both, and add new Layout
tests to verify state transitions.

BUG=462183
TEST=LayoutTests/webmidi

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

[Takashi Toyoshima, 2015-04-01 16:25:43]

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

[Takashi Toyoshima, 2015-04-01 16:27:15]

For LayoutTests, many changes are just renaming from foo_bar.baz to foo-bar.baz.
But, a common function is factored out from open-close.html to
state-check-utils.js, and implicit-open.html is added.

[Takashi Toyoshima, 2015-04-01 16:31:30]

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

[Takashi Toyoshima, 2015-04-01 16:32:34]

+koouhei for DECLARE_FORWARDING_ATTRIBUTE_EVENT_LISTENER macro.

https://codereview.chromium.org/1051903002/diff/1/Source/modules/webmidi/MIDI...
File Source/modules/webmidi/MIDIInput.h (right):

https://codereview.chromium.org/1051903002/diff/1/Source/modules/webmidi/MIDI...
Source/modules/webmidi/MIDIInput.h:48:
DECLARE_FORWARDING_ATTRIBUTE_EVENT_LISTENER(, midimessage);
It looks there were no users of this macro, and I'm not sure if this is expected
that the first argument is not used at all in the macro.

[yhirano, 2015-04-02 02:15:10]

https://codereview.chromium.org/1051903002/diff/1/LayoutTests/webmidi/state-c...
File LayoutTests/webmidi/state-check-utils.js (right):

https://codereview.chromium.org/1051903002/diff/1/LayoutTests/webmidi/state-c...
LayoutTests/webmidi/state-check-utils.js:27: if (options.method ==
"midimessage") {
[optional] How about naming this as "setonmidimessage"?

https://codereview.chromium.org/1051903002/diff/1/Source/modules/webmidi/MIDI...
File Source/modules/webmidi/MIDIPort.cpp (right):

https://codereview.chromium.org/1051903002/diff/1/Source/modules/webmidi/MIDI...
Source/modules/webmidi/MIDIPort.cpp:100: ScriptPromise
MIDIPort::open(ScriptState* scriptState)
Why do you need this function? Isn't it enough to drop [CallWith=ScriptState]
from the IDL?

https://codereview.chromium.org/1051903002/diff/1/Source/modules/webmidi/MIDI...
File Source/modules/webmidi/MIDIPort.h (right):

https://codereview.chromium.org/1051903002/diff/1/Source/modules/webmidi/MIDI...
Source/modules/webmidi/MIDIPort.h:86: ConnectionState getConnection() const {
return m_connection; }
I think this function can be public.

[Takashi Toyoshima, 2015-04-02 04:03:21]

Can I get a comment on Source/modules/webmidi/MIDIPort.cpp:100 again?

https://codereview.chromium.org/1051903002/diff/1/LayoutTests/webmidi/state-c...
File LayoutTests/webmidi/state-check-utils.js (right):

https://codereview.chromium.org/1051903002/diff/1/LayoutTests/webmidi/state-c...
LayoutTests/webmidi/state-check-utils.js:27: if (options.method ==
"midimessage") {
On 2015/04/02 02:15:10, yhirano wrote:
> [optional] How about naming this as "setonmidimessage"?

Done.

https://codereview.chromium.org/1051903002/diff/1/Source/modules/webmidi/MIDI...
File Source/modules/webmidi/MIDIPort.cpp (right):

https://codereview.chromium.org/1051903002/diff/1/Source/modules/webmidi/MIDI...
Source/modules/webmidi/MIDIPort.cpp:100: ScriptPromise
MIDIPort::open(ScriptState* scriptState)
ScriptState is still needed to return Promise at line 103.

https://codereview.chromium.org/1051903002/diff/1/Source/modules/webmidi/MIDI...
File Source/modules/webmidi/MIDIPort.h (right):

https://codereview.chromium.org/1051903002/diff/1/Source/modules/webmidi/MIDI...
Source/modules/webmidi/MIDIPort.h:86: ConnectionState getConnection() const {
return m_connection; }
On 2015/04/02 02:15:10, yhirano wrote:
> I think this function can be public.

Done.

[yhirano, 2015-04-02 04:13:00]

https://codereview.chromium.org/1051903002/diff/1/Source/modules/webmidi/MIDI...
File Source/modules/webmidi/MIDIPort.cpp (right):

https://codereview.chromium.org/1051903002/diff/1/Source/modules/webmidi/MIDI...
Source/modules/webmidi/MIDIPort.cpp:100: ScriptPromise
MIDIPort::open(ScriptState* scriptState)
On 2015/04/02 04:03:21, Takashi Toyoshima (chromium) wrote:
> ScriptState is still needed to return Promise at line 103.

Sorry, I overlooked that.

[yhirano, 2015-04-02 04:19:32]

lgtm, deferring to kouhei@.

[kouhei (in TOK), 2015-04-02 05:10:52]

https://codereview.chromium.org/1051903002/diff/20001/Source/modules/webmidi/...
File Source/modules/webmidi/MIDIInput.cpp (right):

https://codereview.chromium.org/1051903002/diff/20001/Source/modules/webmidi/...
Source/modules/webmidi/MIDIInput.cpp:60: // Implicit open. It does nothing if
the port is already opened.
Please add a link to the spec for "spec" behaviours.

https://codereview.chromium.org/1051903002/diff/20001/Source/modules/webmidi/...
File Source/modules/webmidi/MIDIInput.h (right):

https://codereview.chromium.org/1051903002/diff/20001/Source/modules/webmidi/...
Source/modules/webmidi/MIDIInput.h:47: 
IIUC, DECLARE_FORWARDING_ATTRIBUTE_EVENT_LISTENER is meant to be used in pair
with DEFINE_FORWARDING_ATTRIBUTE_EVENT_LISTENER.

I'd prefer to expand the macro here. e.g.)
EventListener* onmidimessage();
void setOnmidimessage(PassRefPtr<EventListener>);

[Takashi Toyoshima, 2015-04-02 05:25:41]

https://codereview.chromium.org/1051903002/diff/20001/Source/modules/webmidi/...
File Source/modules/webmidi/MIDIInput.cpp (right):

https://codereview.chromium.org/1051903002/diff/20001/Source/modules/webmidi/...
Source/modules/webmidi/MIDIInput.cpp:60: // Implicit open. It does nothing if
the port is already opened.
On 2015/04/02 05:10:52, kouhei wrote:
> Please add a link to the spec for "spec" behaviours.

Done.

https://codereview.chromium.org/1051903002/diff/20001/Source/modules/webmidi/...
File Source/modules/webmidi/MIDIInput.h (right):

https://codereview.chromium.org/1051903002/diff/20001/Source/modules/webmidi/...
Source/modules/webmidi/MIDIInput.h:47: 
On 2015/04/02 05:10:52, kouhei wrote:
> IIUC, DECLARE_FORWARDING_ATTRIBUTE_EVENT_LISTENER is meant to be used in pair
> with DEFINE_FORWARDING_ATTRIBUTE_EVENT_LISTENER.
> 
> I'd prefer to expand the macro here. e.g.)
> EventListener* onmidimessage();
> void setOnmidimessage(PassRefPtr<EventListener>);

Done.

[kouhei (in TOK), 2015-04-02 05:26:47]

lgtm

[Takashi Toyoshima, 2015-04-02 05:29:44]

The CQ bit was checked by toyoshim@chromium.org

[Takashi Toyoshima, 2015-04-02 05:29:44]

The patchset sent to the CQ was uploaded after l-g-t-m from yhirano@chromium.org
Link to the patchset: https://codereview.chromium.org/1051903002/#ps40001
(title: "kouhei review")

[commit-bot: I haz the power, 2015-04-02 05:29:59]

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

[commit-bot: I haz the power, 2015-04-02 06:38:12]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=193002