Web MIDI: fix a regression of r430234

Web MIDI: fix a regression of r430234

This issue depends on platform on that how devices are initialized,
but at least on macOS, this causes problems that devices
get not responsive.

BUG=663501
Committed: https://crrev.com/ac5d7642d517d225a02d66804a26a49a9bb4567e
Cr-Commit-Position: refs/heads/master@{#431851}

[Takashi Toyoshima, 2016-11-09 06:50:21]

The CQ bit was checked by toyoshim@chromium.org to run a CQ dry run

[Takashi Toyoshima, 2016-11-09 06:50:33]

Description was changed from

==========
Web MIDI: fix a regression of r430234

This issue depends on platform that how to initialize device
information, but at least on macOS, this causes problems that devices
are not available.

BUG=663501
==========

to

==========
Web MIDI: fix a regression of r430234

This issue depends on platform that how to initialize device
information, but at least on macOS, this causes problems that devices
are not available.

BUG=663501
==========

[Takashi Toyoshima, 2016-11-09 06:50:33]

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

[commit-bot: I haz the power, 2016-11-09 06:50:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Takashi Toyoshima, 2016-11-09 06:51:07]

yhirano: sorry, there is one issue in my last change. can you take a look at
this fix?

[Takashi Toyoshima, 2016-11-09 06:51:53]

Description was changed from

==========
Web MIDI: fix a regression of r430234

This issue depends on platform that how to initialize device
information, but at least on macOS, this causes problems that devices
are not available.

BUG=663501
==========

to

==========
Web MIDI: fix a regression of r430234

This issue depends on platform on that how devices are initialized,
but at least on macOS, this causes problems that devices
are not available.

BUG=663501
==========

[Takashi Toyoshima, 2016-11-09 06:52:18]

Description was changed from

==========
Web MIDI: fix a regression of r430234

This issue depends on platform on that how devices are initialized,
but at least on macOS, this causes problems that devices
are not available.

BUG=663501
==========

to

==========
Web MIDI: fix a regression of r430234

This issue depends on platform on that how devices are initialized,
but at least on macOS, this causes problems that devices
get not responsible.

BUG=663501
==========

[yhirano, 2016-11-09 07:01:51]

description: s/responsible/responsive/, maybe?

Can we have a test?

[Takashi Toyoshima, 2016-11-09 07:17:36]

Description was changed from

==========
Web MIDI: fix a regression of r430234

This issue depends on platform on that how devices are initialized,
but at least on macOS, this causes problems that devices
get not responsible.

BUG=663501
==========

to

==========
Web MIDI: fix a regression of r430234

This issue depends on platform on that how devices are initialized,
but at least on macOS, this causes problems that devices
get not responsive.

BUG=663501
==========

[commit-bot: I haz the power, 2016-11-09 08:09:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-09 08:09:05]

Dry run: This issue passed the CQ dry run.

[Takashi Toyoshima, 2016-11-09 12:35:33]

The CQ bit was checked by toyoshim@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-09 12:35:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Takashi Toyoshima, 2016-11-09 12:38:11]

Modified Web MIDI mock, then add one layout test.

[commit-bot: I haz the power, 2016-11-09 13:40:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-09 13:40:44]

Dry run: This issue passed the CQ dry run.

[Takashi Toyoshima, 2016-11-11 11:58:06]

friendly ping.
without this fix, Web MIDI is completely unavailable on Canary/Dev now.

[yhirano, 2016-11-14 04:32:55]

Sorry for the delay.

https://codereview.chromium.org/2487113002/diff/20001/components/test_runner/...
File components/test_runner/mock_web_midi_accessor.cc (right):

https://codereview.chromium.org/2487113002/diff/20001/components/test_runner/...
components/test_runner/mock_web_midi_accessor.cc:25: const unsigned char
kSysexHeader[] = {0xf0, 0x00, 0x02, 0x0d, 0x7f};
constexpr

ditto below

https://codereview.chromium.org/2487113002/diff/20001/components/test_runner/...
components/test_runner/mock_web_midi_accessor.cc:67: if (port_index <=
max_output_port_index_)
Shouldn't this '<=' be '<'?

https://codereview.chromium.org/2487113002/diff/20001/components/test_runner/...
components/test_runner/mock_web_midi_accessor.cc:67: if (port_index <=
max_output_port_index_)
Is there any chance this condition doesn't hold?

https://codereview.chromium.org/2487113002/diff/20001/components/test_runner/...
components/test_runner/mock_web_midi_accessor.cc:94: break;
default: break;

https://codereview.chromium.org/2487113002/diff/20001/components/test_runner/...
File components/test_runner/mock_web_midi_accessor.h (right):

https://codereview.chromium.org/2487113002/diff/20001/components/test_runner/...
components/test_runner/mock_web_midi_accessor.h:25: explicit
MockWebMIDIAccessor(blink::WebMIDIAccessorClient* client,
Not related to this CL but explicit is not needed.

https://codereview.chromium.org/2487113002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/webmidi/add-port.html (right):

https://codereview.chromium.org/2487113002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/webmidi/add-port.html:40: try {
No try-catch is needed: the exception should be caught in the catch function.

https://codereview.chromium.org/2487113002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/webmidi/add-port.html:73:
testFailed("requestMIDIAccess() return an error.");
It would be good to print the passed exception.

[Takashi Toyoshima, 2016-11-14 06:25:28]

https://codereview.chromium.org/2487113002/diff/20001/components/test_runner/...
File components/test_runner/mock_web_midi_accessor.cc (right):

https://codereview.chromium.org/2487113002/diff/20001/components/test_runner/...
components/test_runner/mock_web_midi_accessor.cc:25: const unsigned char
kSysexHeader[] = {0xf0, 0x00, 0x02, 0x0d, 0x7f};
On 2016/11/14 04:32:54, yhirano wrote:
> constexpr
> 
> ditto below

Done.

https://codereview.chromium.org/2487113002/diff/20001/components/test_runner/...
components/test_runner/mock_web_midi_accessor.cc:67: if (port_index <=
max_output_port_index_)
If it's only for this CL, I can change sysex commands to add both input and
output ports together so that we can omit this index check and send messages to
a relevant loop-back port safely, but I'd have different tests later with this
testing infra. E.g., I can test hot-plug functionalities in layout tests.

Anyway, this variable name is bad. I'd change it as next_xxxx_port_index_.

https://codereview.chromium.org/2487113002/diff/20001/components/test_runner/...
components/test_runner/mock_web_midi_accessor.cc:94: break;
On 2016/11/14 04:32:54, yhirano wrote:
> default: break;

Done.

https://codereview.chromium.org/2487113002/diff/20001/components/test_runner/...
File components/test_runner/mock_web_midi_accessor.h (right):

https://codereview.chromium.org/2487113002/diff/20001/components/test_runner/...
components/test_runner/mock_web_midi_accessor.h:25: explicit
MockWebMIDIAccessor(blink::WebMIDIAccessorClient* client,
On 2016/11/14 04:32:54, yhirano wrote:
> Not related to this CL but explicit is not needed.

Done.

https://codereview.chromium.org/2487113002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/webmidi/add-port.html (right):

https://codereview.chromium.org/2487113002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/webmidi/add-port.html:40: try {
I just want to have a different catch for requestMIDIAccess() and function
running in then(). But showing exception's contents below looks fine.

https://codereview.chromium.org/2487113002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/webmidi/add-port.html:73:
testFailed("requestMIDIAccess() return an error.");
On 2016/11/14 04:32:55, yhirano wrote:
> It would be good to print the passed exception.

Done.

[Takashi Toyoshima, 2016-11-14 06:26:12]

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

[Takashi Toyoshima, 2016-11-14 06:26:40]

+tkent from components/test_runner/OWNERS

[tkent, 2016-11-14 06:34:02]

https://codereview.chromium.org/2487113002/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/webmidi/add-port.html (right):

https://codereview.chromium.org/2487113002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/webmidi/add-port.html:4: <script
src="../resources/js-test.js"></script>
Do you have any reason to avoid testharness.js ?

https://codereview.chromium.org/2487113002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/webmidi/add-port.html:39: }).then(function(a) {
Indentation is inconsistent.  This |then| line and the |catch| line below should
be at the same level.

[Takashi Toyoshima, 2016-11-14 07:53:43]

ok, now the test is in testharness style.
ptal.

https://codereview.chromium.org/2487113002/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/webmidi/add-port.html (right):

https://codereview.chromium.org/2487113002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/webmidi/add-port.html:4: <script
src="../resources/js-test.js"></script>
On 2016/11/14 06:34:02, tkent wrote:
> Do you have any reason to avoid testharness.js ?

Done.

https://codereview.chromium.org/2487113002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/webmidi/add-port.html:39: }).then(function(a) {
On 2016/11/14 06:34:02, tkent wrote:
> Indentation is inconsistent.  This |then| line and the |catch| line below
should
> be at the same level.

Done.

[tkent, 2016-11-14 08:02:37]

lgtm

[Takashi Toyoshima, 2016-11-14 09:12:30]

https://codereview.chromium.org/2487113002/diff/100001/components/test_runner...
File components/test_runner/mock_web_midi_accessor.cc (right):

https://codereview.chromium.org/2487113002/diff/100001/components/test_runner...
components/test_runner/mock_web_midi_accessor.cc:68: if (port_index <
next_input_port_index_)
fixed

[yhirano, 2016-11-14 09:15:51]

lgtm

[Takashi Toyoshima, 2016-11-14 09:25:39]

The CQ bit was checked by toyoshim@chromium.org

[Takashi Toyoshima, 2016-11-14 09:25:39]

The patchset sent to the CQ was uploaded after l-g-t-m from tkent@chromium.org
Link to the patchset: https://codereview.chromium.org/2487113002/#ps100001
(title: "fix a bug that was discussed offline")

[commit-bot: I haz the power, 2016-11-14 09:25:50]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-14 10:37:17]

Description was changed from

==========
Web MIDI: fix a regression of r430234

This issue depends on platform on that how devices are initialized,
but at least on macOS, this causes problems that devices
get not responsive.

BUG=663501
==========

to

==========
Web MIDI: fix a regression of r430234

This issue depends on platform on that how devices are initialized,
but at least on macOS, this causes problems that devices
get not responsive.

BUG=663501
==========

[commit-bot: I haz the power, 2016-11-14 10:37:18]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-11-14 10:39:35]

Description was changed from

==========
Web MIDI: fix a regression of r430234

This issue depends on platform on that how devices are initialized,
but at least on macOS, this causes problems that devices
get not responsive.

BUG=663501
==========

to

==========
Web MIDI: fix a regression of r430234

This issue depends on platform on that how devices are initialized,
but at least on macOS, this causes problems that devices
get not responsive.

BUG=663501

Committed: https://crrev.com/ac5d7642d517d225a02d66804a26a49a9bb4567e
Cr-Commit-Position: refs/heads/master@{#431851}
==========

[commit-bot: I haz the power, 2016-11-14 10:39:36]

Patchset 6 (id:??) landed as
https://crrev.com/ac5d7642d517d225a02d66804a26a49a9bb4567e
Cr-Commit-Position: refs/heads/master@{#431851}