Web MIDI: add blink APIs to notify device connection events

Web MIDI: add blink APIs to notify device connection events

Add different versions of APIs of didAddInputPort() and didAddOutputPort()
that have a active flag. Old versions will be deprecated once Chromium
side is changed to use the new APIs.

Also, add didSetInputPortState() and didSetOutputPortState() to notify
device status changes.

These updates are needed to manage device connections and disconnections
in consistent between Chromium and Blink.

BUG=279097
TEST=LayoutTets and manual tests

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

[Takashi Toyoshima, 2014-10-18 20:37:34]

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

[Takashi Toyoshima, 2014-10-18 20:39:59]

Hey Web MIDI owners,
Can you take a look? Once you are fine with this patch, I'll ask API owners,
maybe tkent.

[kouhei (in TOK), 2014-10-20 00:46:42]

lgtm

[kouhei (in TOK), 2014-10-20 00:47:06]

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

[kouhei (in TOK), 2014-10-20 00:47:06]

+haraken for platform/

[haraken, 2014-10-20 00:55:11]

Do we want to have a test for inactive states?

(I don't have an owner of public/.)

[yhirano, 2014-10-20 02:14:11]

https://codereview.chromium.org/649683006/diff/1/Source/modules/webmidi/MIDIA...
File Source/modules/webmidi/MIDIAccess.h (right):

https://codereview.chromium.org/649683006/diff/1/Source/modules/webmidi/MIDIA...
Source/modules/webmidi/MIDIAccess.h:80: virtual void didAddInputPort(const
String& id, const String& manufacturer, const String& name, const String&
version, bool active) override;
[optional] Is enum more preferable?

https://codereview.chromium.org/649683006/diff/1/Source/modules/webmidi/MIDIA...
File Source/modules/webmidi/MIDIAccessInitializer.cpp (right):

https://codereview.chromium.org/649683006/diff/1/Source/modules/webmidi/MIDIA...
Source/modules/webmidi/MIDIAccessInitializer.cpp:72: ASSERT_NOT_REACHED();
Is this correct? IIUC the browser can fire this notification anytime, right?

https://codereview.chromium.org/649683006/diff/1/Source/modules/webmidi/MIDIA...
Source/modules/webmidi/MIDIAccessInitializer.cpp:77: ASSERT_NOT_REACHED();
ditto

https://codereview.chromium.org/649683006/diff/1/Source/modules/webmidi/MIDIA...
File Source/modules/webmidi/MIDIAccessor.h (right):

https://codereview.chromium.org/649683006/diff/1/Source/modules/webmidi/MIDIA...
Source/modules/webmidi/MIDIAccessor.h:57: virtual void didAddInputPort(const
WebString& id, const WebString& manufacturer, const WebString& name, const
WebString& version) override;
Can you write FIXME?

[Takashi Toyoshima, 2014-10-20 02:40:25]

I will address other points, but here is one question.

https://codereview.chromium.org/649683006/diff/1/Source/modules/webmidi/MIDIA...
File Source/modules/webmidi/MIDIAccessInitializer.cpp (right):

https://codereview.chromium.org/649683006/diff/1/Source/modules/webmidi/MIDIA...
Source/modules/webmidi/MIDIAccessInitializer.cpp:72: ASSERT_NOT_REACHED();
Embedder (Chromium) sends, didAddInputPort(), didAddOutputPort(), and
didStartSession() in this order. After that, didSetOutputPort() and
didSetInputPort() are used when a device online state is changed.

IIRC, once didStartSession() is called, these client methods are handled by
another class directly, right?

[Takashi Toyoshima, 2014-10-20 03:12:56]

FYI, here is chromium side CL; https://codereview.chromium.org/664853003/.
It depends on other CLs, so you may not be able to confirm that the caller
follows the rule I mentioned.
Anyway, at least, I should describe this API convention in
public/platform/WebMIDIAccessorClient.h.

Also on the active flag, if I'm willing to use enum for the active flag, I need
to define similar enum in Blink, Blink API, and Chromium, and use meaningless
static_cast between them. I feel it it's overkill. WDYT? Maybe I can improve
readability by using a variable from caller side. That could be something like,

```
bool online_flag = true;
didSetInputPort(portIndex, onlone_flag);

// or just
didSetInputPort(portIndex, online_flags_[portIndex]);

// or,
void fooBar(unsigned portIndex, bool active) {
  didSetInputPort(portIndex, active);
}
```

[Takashi Toyoshima, 2014-10-20 04:51:04]

haraken:

For testing, yes, I'll write it. But we have a mock in chromium side. So having
Layout tests will be after this API change is submitted.
FYI, src/content/shell/renderer/test_runner/mock_web_midi_accessor.h is the
mock.

I'll add some easter egg for testing, e.g. sendMIDIData() with a special
sequence makes a device offline, or something like that.

[Takashi Toyoshima, 2014-10-20 05:39:51]

https://codereview.chromium.org/649683006/diff/1/Source/modules/webmidi/MIDIA...
File Source/modules/webmidi/MIDIAccess.h (right):

https://codereview.chromium.org/649683006/diff/1/Source/modules/webmidi/MIDIA...
Source/modules/webmidi/MIDIAccess.h:80: virtual void didAddInputPort(const
String& id, const String& manufacturer, const String& name, const String&
version, bool active) override;
See https://codereview.chromium.org/649683006/#msg9

https://codereview.chromium.org/649683006/diff/1/Source/modules/webmidi/MIDIA...
File Source/modules/webmidi/MIDIAccessInitializer.cpp (right):

https://codereview.chromium.org/649683006/diff/1/Source/modules/webmidi/MIDIA...
Source/modules/webmidi/MIDIAccessInitializer.cpp:72: ASSERT_NOT_REACHED();
I added comments here too.

https://codereview.chromium.org/649683006/diff/1/Source/modules/webmidi/MIDIA...
Source/modules/webmidi/MIDIAccessInitializer.cpp:77: ASSERT_NOT_REACHED();
ditto

https://codereview.chromium.org/649683006/diff/1/Source/modules/webmidi/MIDIA...
File Source/modules/webmidi/MIDIAccessInitializer.h (right):

https://codereview.chromium.org/649683006/diff/1/Source/modules/webmidi/MIDIA...
Source/modules/webmidi/MIDIAccessInitializer.h:18: class MIDIAccess;
Changes in this file isn't mine.
Sorry, I work on a different machine, so it causes unexpected rebase.

https://codereview.chromium.org/649683006/diff/1/Source/modules/webmidi/MIDIA...
File Source/modules/webmidi/MIDIAccessor.h (right):

https://codereview.chromium.org/649683006/diff/1/Source/modules/webmidi/MIDIA...
Source/modules/webmidi/MIDIAccessor.h:57: virtual void didAddInputPort(const
WebString& id, const WebString& manufacturer, const WebString& name, const
WebString& version) override;
On 2014/10/20 02:14:10, yhirano wrote:
> Can you write FIXME?

Done.

[Takashi Toyoshima, 2014-10-20 05:43:13]

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

[Takashi Toyoshima, 2014-10-20 05:43:13]

tkent@chromium.org: Please review changes in 

API changes for Web MIDI in platform/

[Takashi Toyoshima, 2014-10-20 05:43:28]

tkent@chromium.org: Please review changes in 

API changes for Web MIDI in platform/

[haraken, 2014-10-20 05:46:58]

On 2014/10/20 04:51:04, Takashi Toyoshima (chromium) wrote:
> haraken:
> 
> For testing, yes, I'll write it. But we have a mock in chromium side. So
having
> Layout tests will be after this API change is submitted.
> FYI, src/content/shell/renderer/test_runner/mock_web_midi_accessor.h is the
> mock.
> 
> I'll add some easter egg for testing, e.g. sendMIDIData() with a special
> sequence makes a device offline, or something like that.

Thanks, makes sense.

[tkent, 2014-10-20 05:54:07]

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

https://codereview.chromium.org/649683006/diff/20001/Source/modules/webmidi/M...
Source/modules/webmidi/MIDIAccess.cpp:73: size_t inactive = 0;
The name |inactive| looks weird.  inactiveCount or something?

https://codereview.chromium.org/649683006/diff/20001/Source/modules/webmidi/M...
Source/modules/webmidi/MIDIAccess.cpp:91: size_t inactive = 0;
ditto.

https://codereview.chromium.org/649683006/diff/20001/Source/modules/webmidi/M...
Source/modules/webmidi/MIDIAccess.cpp:122: if (portIndex >= m_inputs.size())
nit:

if (portIndex < m_inputs.size())
    m_inputs[portIndex]->setActiveState(active);

is simpler.

https://codereview.chromium.org/649683006/diff/20001/Source/modules/webmidi/M...
Source/modules/webmidi/MIDIAccess.cpp:130: if (portIndex >= m_outputs.size())
Ditto.

https://codereview.chromium.org/649683006/diff/20001/Source/modules/webmidi/M...
File Source/modules/webmidi/MIDIAccessInitializer.h (right):

https://codereview.chromium.org/649683006/diff/20001/Source/modules/webmidi/M...
Source/modules/webmidi/MIDIAccessInitializer.h:29: bool active;
should be |isActive|

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

https://codereview.chromium.org/649683006/diff/20001/Source/modules/webmidi/M...
Source/modules/webmidi/MIDIPort.h:81: bool m_active;
should be |m_isActive|

https://codereview.chromium.org/649683006/diff/20001/public/platform/WebMIDIA...
File public/platform/WebMIDIAccessorClient.h (right):

https://codereview.chromium.org/649683006/diff/20001/public/platform/WebMIDIA...
public/platform/WebMIDIAccessorClient.h:44: // didAddInputPort() and
didAddOutputPort() can be called before and after didStartSession() is called.
But |id| should be unique in each function.
nit: I recommend to wrap code comments in 80 columns.

[Takashi Toyoshima, 2014-10-20 06:24:18]

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

https://codereview.chromium.org/649683006/diff/20001/Source/modules/webmidi/M...
Source/modules/webmidi/MIDIAccess.cpp:73: size_t inactive = 0;
On 2014/10/20 05:54:07, tkent wrote:
> The name |inactive| looks weird.  inactiveCount or something?

Done.

https://codereview.chromium.org/649683006/diff/20001/Source/modules/webmidi/M...
Source/modules/webmidi/MIDIAccess.cpp:91: size_t inactive = 0;
On 2014/10/20 05:54:07, tkent wrote:
> ditto.

Done.

https://codereview.chromium.org/649683006/diff/20001/Source/modules/webmidi/M...
Source/modules/webmidi/MIDIAccess.cpp:122: if (portIndex >= m_inputs.size())
On 2014/10/20 05:54:07, tkent wrote:
> nit:
> 
> if (portIndex < m_inputs.size())
>     m_inputs[portIndex]->setActiveState(active);
> 
> is simpler.

Done.

https://codereview.chromium.org/649683006/diff/20001/Source/modules/webmidi/M...
Source/modules/webmidi/MIDIAccess.cpp:130: if (portIndex >= m_outputs.size())
On 2014/10/20 05:54:07, tkent wrote:
> Ditto.

Done.

https://codereview.chromium.org/649683006/diff/20001/Source/modules/webmidi/M...
File Source/modules/webmidi/MIDIAccessInitializer.h (right):

https://codereview.chromium.org/649683006/diff/20001/Source/modules/webmidi/M...
Source/modules/webmidi/MIDIAccessInitializer.h:29: bool active;
On 2014/10/20 05:54:07, tkent wrote:
> should be |isActive|

Done.

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

https://codereview.chromium.org/649683006/diff/20001/Source/modules/webmidi/M...
Source/modules/webmidi/MIDIPort.h:81: bool m_active;
On 2014/10/20 05:54:07, tkent wrote:
> should be |m_isActive|

Done.

https://codereview.chromium.org/649683006/diff/20001/public/platform/WebMIDIA...
File public/platform/WebMIDIAccessorClient.h (right):

https://codereview.chromium.org/649683006/diff/20001/public/platform/WebMIDIA...
public/platform/WebMIDIAccessorClient.h:44: // didAddInputPort() and
didAddOutputPort() can be called before and after didStartSession() is called.
But |id| should be unique in each function.
On 2014/10/20 05:54:07, tkent wrote:
> nit: I recommend to wrap code comments in 80 columns.

Done.

[Takashi Toyoshima, 2014-10-20 06:42:39]

Build passed. PTAL again.

[tkent, 2014-10-20 06:51:17]

lgtm

[Takashi Toyoshima, 2014-10-20 06:55:51]

Thanks.
I'll wait yhirano confirmi my comments at
https://codereview.chromium.org/649683006/#msg8 before submitting this.

[yhirano, 2014-10-20 08:58:47]

Thanks, lgtm

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

https://codereview.chromium.org/649683006/diff/40001/Source/modules/webmidi/M...
Source/modules/webmidi/MIDIAccessInitializer.cpp:72: // didSetInputPortState()
is not allowed to call before didStartSession() is called.
Please wrap the comment in 80 columns.

[Takashi Toyoshima, 2014-10-20 10:54:41]

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

https://codereview.chromium.org/649683006/diff/40001/Source/modules/webmidi/M...
Source/modules/webmidi/MIDIAccessInitializer.cpp:72: // didSetInputPortState()
is not allowed to call before didStartSession() is called.
On 2014/10/20 08:58:47, yhirano wrote:
> Please wrap the comment in 80 columns.

Done.

[Takashi Toyoshima, 2014-10-20 10:55:00]

The CQ bit was checked by toyoshim@chromium.org

[commit-bot: I haz the power, 2014-10-20 10:55:36]

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

[commit-bot: I haz the power, 2014-10-20 12:00:47]

Committed patchset #4 (id:60001) as 183974