Web MIDI: adding backend to support dynamic instantiation on Windows

Web MIDI: adding backend to support dynamic instantiation on Windows

This patch implements new backend that supports dynamic instantiation
on Windows. This does not cover all functionalities, but just allow
to enumerate devices and hotplug.

BUG=497573, 617086, 672793
Review-Url: https://codereview.chromium.org/2704643002
Cr-Commit-Position: refs/heads/master@{#451652}
Committed: https://chromium.googlesource.com/chromium/src/+/be814a03409b305b3ddbe54b481de71998d6aad3

[Takashi Toyoshima, 2017-02-17 09:27:21]

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

[Takashi Toyoshima, 2017-02-17 09:27:30]

Description was changed from

==========
Web MIDI: adding backend to support dynamic instantiation on Windows

This patch implements new backend that supports dynamic instantiation
on Windows. This does not cover all functionalities, but just allow
to enumerate devices and hotplug.

BUG=497573, 617086, 672793
==========

to

==========
Web MIDI: adding backend to support dynamic instantiation on Windows

This patch implements new backend that supports dynamic instantiation
on Windows. This does not cover all functionalities, but just allow
to enumerate devices and hotplug.

BUG=497573, 617086, 672793
==========

[Takashi Toyoshima, 2017-02-17 09:27:30]

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

[commit-bot: I haz the power, 2017-02-17 09:27:37]

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

[Takashi Toyoshima, 2017-02-17 09:34:27]

PTAL

https://codereview.chromium.org/2704643002/diff/1/media/midi/dynamically_init...
File media/midi/dynamically_initialized_midi_manager_win.cc (right):

https://codereview.chromium.org/2704643002/diff/1/media/midi/dynamically_init...
media/midi/dynamically_initialized_midi_manager_win.cc:29: // Global variables
to identify MidiManager instance.
Concept is same with ALSA's dynamic instantiation support

https://codereview.chromium.org/2704643002/diff/1/media/midi/dynamically_init...
media/midi/dynamically_initialized_midi_manager_win.cc:286: void
DynamicallyInitializedMidiManagerWin::UpdateDeviceListOnTaskRunner() {
main logic of this patch.
100+ lines, but latter part for output ports is almost a duplication of one for
input ports actually.

[yhirano, 2017-02-17 10:27:49]

https://codereview.chromium.org/2704643002/diff/1/media/midi/dynamically_init...
File media/midi/dynamically_initialized_midi_manager_win.cc (right):

https://codereview.chromium.org/2704643002/diff/1/media/midi/dynamically_init...
media/midi/dynamically_initialized_midi_manager_win.cc:37: static base::Lock*
lock = new base::Lock;
Looks racy; If there is a guarantee that the first call will not be done
simultaneously, please write so (with reason).

https://codereview.chromium.org/2704643002/diff/1/media/midi/dynamically_init...
media/midi/dynamically_initialized_midi_manager_win.cc:91: 
+virtual ~Port() {}

https://codereview.chromium.org/2704643002/diff/1/media/midi/dynamically_init...
media/midi/dynamically_initialized_midi_manager_win.cc:203: const base::Closure&
task) {
base::AutoLock lock(*GetInstanceIdLock());

https://codereview.chromium.org/2704643002/diff/1/media/midi/dynamically_init...
media/midi/dynamically_initialized_midi_manager_win.cc:263: base::AutoLock
lock(*GetInstanceIdLock());
I believe these two lines should be in PostTask. PostTask touches instance_id_
so it should need the instance id lock (some are missing in this patch). As
g_active_instance_id is modified only on the IO thread, expecting
g_active_instance_id is equal to instance_id_ is safe on the thread.

https://codereview.chromium.org/2704643002/diff/1/media/midi/dynamically_init...
media/midi/dynamically_initialized_midi_manager_win.cc:273: void
DynamicallyInitializedMidiManagerWin::PostTask(const base::Closure& task) {
DCHECK(thread_runner_->BelongsToCurrentThread()); or CHECK?

https://codereview.chromium.org/2704643002/diff/1/media/midi/dynamically_init...
media/midi/dynamically_initialized_midi_manager_win.cc:286: void
DynamicallyInitializedMidiManagerWin::UpdateDeviceListOnTaskRunner() {
Splitting this method into two (one for input and one for output) would be good.

[commit-bot: I haz the power, 2017-02-17 11:01:20]

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

[commit-bot: I haz the power, 2017-02-17 11:01:21]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Takashi Toyoshima, 2017-02-17 11:46:36]

https://codereview.chromium.org/2704643002/diff/1/media/midi/dynamically_init...
File media/midi/dynamically_initialized_midi_manager_win.cc (right):

https://codereview.chromium.org/2704643002/diff/1/media/midi/dynamically_init...
media/midi/dynamically_initialized_midi_manager_win.cc:37: static base::Lock*
lock = new base::Lock;
This looks a quite new idiom that is replacing LazyInstance.
Now LazyInstance is not permitted under media

https://codereview.chromium.org/2704643002/diff/1/media/midi/dynamically_init...
media/midi/dynamically_initialized_midi_manager_win.cc:91: 
On 2017/02/17 10:27:49, yhirano wrote:
> +virtual ~Port() {}

Done.

https://codereview.chromium.org/2704643002/diff/1/media/midi/dynamically_init...
media/midi/dynamically_initialized_midi_manager_win.cc:203: const base::Closure&
task) {
instance id lock is used only for global variables that are related to instance
id.
|instance_id_| is a member variable, that are usually
 - read-only on task runner threads, and protected by task lock (at RunTask)
 - not protected on the I/O thread there read and write happen but on the same
thread

https://codereview.chromium.org/2704643002/diff/1/media/midi/dynamically_init...
media/midi/dynamically_initialized_midi_manager_win.cc:263: base::AutoLock
lock(*GetInstanceIdLock());
For missing lock for instance_id_, see my previous comment.

My original intention of this check is to ensure that we unregister the instance
from the system monitor observer list currently. But in such case, we can not
access "this" correctly. So this check does not make sense.

I will remove this check.

https://codereview.chromium.org/2704643002/diff/1/media/midi/dynamically_init...
media/midi/dynamically_initialized_midi_manager_win.cc:273: void
DynamicallyInitializedMidiManagerWin::PostTask(const base::Closure& task) {
I will use this even from task runner.
I expect this method is still thread safe.
(service()->GetTaskRunner() is thread safe)

https://codereview.chromium.org/2704643002/diff/1/media/midi/dynamically_init...
media/midi/dynamically_initialized_midi_manager_win.cc:286: void
DynamicallyInitializedMidiManagerWin::UpdateDeviceListOnTaskRunner() {
will try rewriting this as we discussed

[yhirano, 2017-02-17 11:51:43]

https://codereview.chromium.org/2704643002/diff/1/media/midi/dynamically_init...
File media/midi/dynamically_initialized_midi_manager_win.cc (right):

https://codereview.chromium.org/2704643002/diff/1/media/midi/dynamically_init...
media/midi/dynamically_initialized_midi_manager_win.cc:203: const base::Closure&
task) {
On 2017/02/17 11:46:36, Takashi Toyoshima wrote:
> instance id lock is used only for global variables that are related to
instance
> id.
> |instance_id_| is a member variable, that are usually
>  - read-only on task runner threads, and protected by task lock (at RunTask)
>  - not protected on the I/O thread there read and write happen but on the same
> thread

Thank you! I overlooked that.
Is it possible to initialize instance_id_ in the constructor, and make it const?
That will make it clear that accessing instance_id_ is safe. What I'm suggesting
is, create IssueNextInstanceId(), call it in the constructor, and update
g_active_instance_id in StartInitialization.

[Takashi Toyoshima, 2017-02-17 17:30:08]

Patchset #3 (id:40001) has been deleted

[Takashi Toyoshima, 2017-02-17 17:42:29]

ptal ps4!

https://codereview.chromium.org/2704643002/diff/1/media/midi/dynamically_init...
File media/midi/dynamically_initialized_midi_manager_win.cc (right):

https://codereview.chromium.org/2704643002/diff/1/media/midi/dynamically_init...
media/midi/dynamically_initialized_midi_manager_win.cc:147: InPort(int
instance_id, UINT device_id, const MIDIINCAPS2W& caps)
this first argument wasn't used yet in this first cl

https://codereview.chromium.org/2704643002/diff/1/media/midi/dynamically_init...
File media/midi/dynamically_initialized_midi_manager_win.h (right):

https://codereview.chromium.org/2704643002/diff/1/media/midi/dynamically_init...
media/midi/dynamically_initialized_midi_manager_win.h:61: // Sends MIDI data on
TaskRunner.
This wasn't used yet in this first cl.

https://codereview.chromium.org/2704643002/diff/80001/media/midi/dynamically_...
File media/midi/dynamically_initialized_midi_manager_win.cc (right):

https://codereview.chromium.org/2704643002/diff/80001/media/midi/dynamically_...
media/midi/dynamically_initialized_midi_manager_win.cc:393: 
NotifyPortStateSet() or NotifyPortAdded() are called from caller sides.
They could be hidden behind Connect() and Disconnect() methods, but it requires
that port instance has a manager's pointer, new bool flag to call a right
NotifyX method, and dirty logic to handle blacklisted output port correctly.

https://codereview.chromium.org/2704643002/diff/80001/media/midi/dynamically_...
File media/midi/dynamically_initialized_midi_manager_win.h (right):

https://codereview.chromium.org/2704643002/diff/80001/media/midi/dynamically_...
media/midi/dynamically_initialized_midi_manager_win.h:13: #include
"base/memory/ref_counted.h"
restore the line I mistakenly removed from the original.

[yhirano, 2017-02-20 08:02:07]

lgtm

https://codereview.chromium.org/2704643002/diff/80001/media/midi/dynamically_...
File media/midi/dynamically_initialized_midi_manager_win.cc (right):

https://codereview.chromium.org/2704643002/diff/80001/media/midi/dynamically_...
media/midi/dynamically_initialized_midi_manager_win.cc:140: virtual void
NotifyPortAdded(
These two functions need not to be virtual.

https://codereview.chromium.org/2704643002/diff/80001/media/midi/dynamically_...
File media/midi/dynamically_initialized_midi_manager_win.h (right):

https://codereview.chromium.org/2704643002/diff/80001/media/midi/dynamically_...
media/midi/dynamically_initialized_midi_manager_win.h:65: T* known_ports,
std::vector<T>* is easier to understand, I think.

https://codereview.chromium.org/2704643002/diff/80001/media/midi/dynamically_...
media/midi/dynamically_initialized_midi_manager_win.h:66: T* active_ports);
const std::vector<T>& is easier to understand, I think.

[Takashi Toyoshima, 2017-02-20 16:51:59]

https://codereview.chromium.org/2704643002/diff/80001/media/midi/dynamically_...
File media/midi/dynamically_initialized_midi_manager_win.cc (right):

https://codereview.chromium.org/2704643002/diff/80001/media/midi/dynamically_...
media/midi/dynamically_initialized_midi_manager_win.cc:140: virtual void
NotifyPortAdded(
Ah, right.
I did so for calling them from Port class, but now the latest code does not need
it any more.

Done.

https://codereview.chromium.org/2704643002/diff/80001/media/midi/dynamically_...
File media/midi/dynamically_initialized_midi_manager_win.h (right):

https://codereview.chromium.org/2704643002/diff/80001/media/midi/dynamically_...
media/midi/dynamically_initialized_midi_manager_win.h:65: T* known_ports,
On 2017/02/20 08:02:06, yhirano wrote:
> std::vector<T>* is easier to understand, I think.

Done.

https://codereview.chromium.org/2704643002/diff/80001/media/midi/dynamically_...
media/midi/dynamically_initialized_midi_manager_win.h:66: T* active_ports);
I may misunderstand something, but if I use "const", I can not call
push_back(std::move(port)) that internally requires non-const method calls.

Let me fix this in the next patch set if I can.

[Takashi Toyoshima, 2017-02-20 17:23:09]

The CQ bit was checked by toyoshim@chromium.org

[Takashi Toyoshima, 2017-02-20 17:23:09]

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/2704643002/#ps100001
(title: "review #13")

[commit-bot: I haz the power, 2017-02-20 17:23:27]

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, 2017-02-20 19:07:42]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1487611389426250,
"parent_rev": "8467839dc15f6e3d4027406ba436c9a64344418d", "commit_rev":
"be814a03409b305b3ddbe54b481de71998d6aad3"}

[commit-bot: I haz the power, 2017-02-20 19:08:21]

Description was changed from

==========
Web MIDI: adding backend to support dynamic instantiation on Windows

This patch implements new backend that supports dynamic instantiation
on Windows. This does not cover all functionalities, but just allow
to enumerate devices and hotplug.

BUG=497573, 617086, 672793
==========

to

==========
Web MIDI: adding backend to support dynamic instantiation on Windows

This patch implements new backend that supports dynamic instantiation
on Windows. This does not cover all functionalities, but just allow
to enumerate devices and hotplug.

BUG=497573, 617086, 672793

Review-Url: https://codereview.chromium.org/2704643002
Cr-Commit-Position: refs/heads/master@{#451652}
Committed:
https://chromium.googlesource.com/chromium/src/+/be814a03409b305b3ddbe54b481d...
==========

[commit-bot: I haz the power, 2017-02-20 19:08:23]

Committed patchset #5 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/be814a03409b305b3ddbe54b481d...

[yhirano, 2017-02-21 03:03:48]

https://codereview.chromium.org/2704643002/diff/80001/media/midi/dynamically_...
File media/midi/dynamically_initialized_midi_manager_win.h (right):

https://codereview.chromium.org/2704643002/diff/80001/media/midi/dynamically_...
media/midi/dynamically_initialized_midi_manager_win.h:66: T* active_ports);
On 2017/02/20 16:51:59, Takashi Toyoshima wrote:
> I may misunderstand something, but if I use "const", I can not call
> push_back(std::move(port)) that internally requires non-const method calls.
> 
> Let me fix this in the next patch set if I can.

Ah, I was wrong, Sorry.