Web MIDI: add dynamic MidiManager instantiation support for Linux

Web MIDI: add dynamic MidiManager instantiation support for Linux

With this change, MidiService starts supporting dynamic MidiManager
instantiation. First, this enables it for Linux for a trial.

BUG=672793
Review-Url: https://codereview.chromium.org/2673423002
Cr-Commit-Position: refs/heads/master@{#449574}
Committed: https://chromium.googlesource.com/chromium/src/+/0f1c3f34031ae08400afbe43c70fd71267a870b1

[Takashi Toyoshima, 2017-02-07 11:28:58]

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

[commit-bot: I haz the power, 2017-02-07 11:29:12]

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-07 11:40:41]

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

[commit-bot: I haz the power, 2017-02-07 11:40:51]

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-07 11:41:51]

Description was changed from

==========
Web MIDI: add dynamic MidiManager instantiation support for Linux

With this change, MidiService starts supporting dynamic MidiManager
instantiation. First, this enables it for Linux for a trial.

BUG=672793
==========

to

==========
Web MIDI: add dynamic MidiManager instantiation support for Linux

With this change, MidiService starts supporting dynamic MidiManager
instantiation. First, this enables it for Linux for a trial.

BUG=672793
==========

[Takashi Toyoshima, 2017-02-07 11:41:52]

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

[Takashi Toyoshima, 2017-02-07 11:50:23]

Adam, can you review this change?
Background of this change was explained in the crbug.

Note to understand this change.
Finalize() ran on I/O thread, but it happened only when the browser going to
shutdown. In the browser finalization step, we can call Thread::Stop or
Thread::~Thread since it runs inside ScopedAlloowIO instance. But now we need to
run Finalize() on I/O thread outside the ScopedAllowIO. To make MidiManager free
from the browser main thread, we need to manage worker threads inside
MidiService that can outlive, and reuse same threads for all MidiManager
instances until the browser goes to shutdown.

We could make Finalize() asynchronous method, but it would introduce additional
complexity to platform dependent implementations and MidiService.

+yhirano for the final review, and cc:tzik who is experts on PostTask and
threading.

[commit-bot: I haz the power, 2017-02-07 12:16:50]

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

[commit-bot: I haz the power, 2017-02-07 12:16:51]

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-07 13:16:12]

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

[commit-bot: I haz the power, 2017-02-07 13:16:20]

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

[Adam Goode, 2017-02-07 13:31:14]

https://codereview.chromium.org/2673423002/diff/60001/chrome/browser/about_fl...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/2673423002/diff/60001/chrome/browser/about_fl...
chrome/browser/about_flags.cc:2202:
{"enablee-midi-manager-dynamic-instantiation",
"enable-midi-manager-dynamic-instantiation" ?

https://codereview.chromium.org/2673423002/diff/60001/media/midi/midi_manager...
File media/midi/midi_manager_alsa.cc (right):

https://codereview.chromium.org/2673423002/diff/60001/media/midi/midi_manager...
media/midi/midi_manager_alsa.cc:919: base::AutoLock
lock(g_event_task_runner_lock.Get());
Can you add a comment here (or above) explaining the logic?

It looks like we only want to run a single instance at any time, and need to
protect against the case where we are still shutting down one instance and then
starting another?

[commit-bot: I haz the power, 2017-02-07 14:17:22]

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

[commit-bot: I haz the power, 2017-02-07 14:17:23]

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

[yhirano, 2017-02-08 03:50:24]

https://codereview.chromium.org/2673423002/diff/80001/media/midi/midi_service.cc
File media/midi/midi_service.cc (right):

https://codereview.chromium.org/2673423002/diff/80001/media/midi/midi_service...
media/midi/midi_service.cc:103: return
g_midi_service->GetTaskRunnerImpl(runner_id);
Is it possible to store a MidiService pointer in MidiManagerAlsa (assuming
MidiService always outlives MidiManager)? With that we don't need g_midi_service
which I don't like.

https://codereview.chromium.org/2673423002/diff/80001/media/midi/midi_service.h
File media/midi/midi_service.h (right):

https://codereview.chromium.org/2673423002/diff/80001/media/midi/midi_service...
media/midi/midi_service.h:50: protected:
Why is this protected? Even if you place the friend declaration in protected
area, the friend class can access private members.

https://codereview.chromium.org/2673423002/diff/80001/media/midi/midi_service...
media/midi/midi_service.h:60: static scoped_refptr<base::SingleThreadTaskRunner>
GetTaskRunner(
> each task should be bound with WeakPtr<>

Can you tell me what this means? Does that mean each bound |this| should be
weak? Is so, are Unretained(this) in MidiManagerAlsa problematic?

https://codereview.chromium.org/2673423002/diff/80001/media/midi/midi_service...
media/midi/midi_service.h:74: bool is_dynamic_instantiation_enabled_;
I think you don't need to protect this member, right? Making this variable
constant would be helpful.

[Takashi Toyoshima, 2017-02-08 09:07:39]

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

[commit-bot: I haz the power, 2017-02-08 09:07:51]

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-08 09:08:01]

https://codereview.chromium.org/2673423002/diff/60001/chrome/browser/about_fl...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/2673423002/diff/60001/chrome/browser/about_fl...
chrome/browser/about_flags.cc:2202:
{"enablee-midi-manager-dynamic-instantiation",
On 2017/02/07 13:31:14, Adam Goode wrote:
> "enable-midi-manager-dynamic-instantiation" ?

Done.

https://codereview.chromium.org/2673423002/diff/60001/media/midi/midi_manager...
File media/midi/midi_manager_alsa.cc (right):

https://codereview.chromium.org/2673423002/diff/60001/media/midi/midi_manager...
media/midi/midi_manager_alsa.cc:919: base::AutoLock
lock(g_event_task_runner_lock.Get());
On 2017/02/07 13:31:14, Adam Goode wrote:
> Can you add a comment here (or above) explaining the logic?
> 
> It looks like we only want to run a single instance at any time, and need to
> protect against the case where we are still shutting down one instance and
then
> starting another?

Done.

https://codereview.chromium.org/2673423002/diff/80001/media/midi/midi_service.h
File media/midi/midi_service.h (right):

https://codereview.chromium.org/2673423002/diff/80001/media/midi/midi_service...
media/midi/midi_service.h:50: protected:
On 2017/02/08 03:50:24, yhirano wrote:
> Why is this protected? Even if you place the friend declaration in protected
> area, the friend class can access private members.

Done.

https://codereview.chromium.org/2673423002/diff/80001/media/midi/midi_service...
media/midi/midi_service.h:60: static scoped_refptr<base::SingleThreadTaskRunner>
GetTaskRunner(
Oops, later part of this comment was stale (explained about one of my prototype)

https://codereview.chromium.org/2673423002/diff/80001/media/midi/midi_service...
media/midi/midi_service.h:74: bool is_dynamic_instantiation_enabled_;
On 2017/02/08 03:50:24, yhirano wrote:
> I think you don't need to protect this member, right? Making this variable
> constant would be helpful.

Done.

[commit-bot: I haz the power, 2017-02-08 09:17:11]

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

[commit-bot: I haz the power, 2017-02-08 09:17:12]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[yhirano, 2017-02-08 10:04:37]

https://codereview.chromium.org/2673423002/diff/120001/media/midi/midi_manager.h
File media/midi/midi_manager.h (right):

https://codereview.chromium.org/2673423002/diff/120001/media/midi/midi_manage...
media/midi/midi_manager.h:84: MidiManager(MidiService* service);
+explicit

https://codereview.chromium.org/2673423002/diff/120001/media/midi/midi_manage...
media/midi/midi_manager.h:93: // *** Note ***: If dynamic instantiation feature
is enabled, MidiService call
call"s"

https://codereview.chromium.org/2673423002/diff/120001/media/midi/midi_manage...
File media/midi/midi_manager_alsa.cc (right):

https://codereview.chromium.org/2673423002/diff/120001/media/midi/midi_manage...
media/midi/midi_manager_alsa.cc:894: base::AutoLock
lock(g_send_task_lock.Get());
I think moving this below the instance id check is better.

https://codereview.chromium.org/2673423002/diff/120001/media/midi/midi_manage...
media/midi/midi_manager_alsa.cc:929: base::AutoLock
lock(g_event_task_lock.Get());
I think moving this below the instance id check is better.

https://codereview.chromium.org/2673423002/diff/120001/media/midi/midi_manage...
File media/midi/midi_manager_alsa.h (right):

https://codereview.chromium.org/2673423002/diff/120001/media/midi/midi_manage...
media/midi/midi_manager_alsa.h:34: MidiManagerAlsa(MidiService* service);
+explicit

https://codereview.chromium.org/2673423002/diff/120001/media/midi/midi_manage...
File media/midi/midi_manager_mac.h (right):

https://codereview.chromium.org/2673423002/diff/120001/media/midi/midi_manage...
media/midi/midi_manager_mac.h:28: MidiManagerMac(MidiService* service);
+explicit

https://codereview.chromium.org/2673423002/diff/120001/media/midi/midi_manage...
File media/midi/midi_manager_usb.h (right):

https://codereview.chromium.org/2673423002/diff/120001/media/midi/midi_manage...
media/midi/midi_manager_usb.h:40: explicit MidiManagerUsb(
-explicit

https://codereview.chromium.org/2673423002/diff/120001/media/midi/midi_manage...
File media/midi/midi_manager_win.h (right):

https://codereview.chromium.org/2673423002/diff/120001/media/midi/midi_manage...
media/midi/midi_manager_win.h:51: MidiManagerWin(MidiService* service);
+explicit

https://codereview.chromium.org/2673423002/diff/120001/media/midi/midi_manage...
File media/midi/midi_manager_winrt.h (right):

https://codereview.chromium.org/2673423002/diff/120001/media/midi/midi_manage...
media/midi/midi_manager_winrt.h:25: MidiManagerWinrt(MidiService* service);
+explicit

https://codereview.chromium.org/2673423002/diff/120001/media/midi/midi_servic...
File media/midi/midi_service.cc (right):

https://codereview.chromium.org/2673423002/diff/120001/media/midi/midi_servic...
media/midi/midi_service.cc:33: active_clients_ = 0u;
Can you move this initialization to the member initialization list (or the
header)?

https://codereview.chromium.org/2673423002/diff/120001/media/midi/midi_servic...
media/midi/midi_service.cc:42: active_clients_ = 0u;
ditto

[Takashi Toyoshima, 2017-02-08 11:08:44]

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

[commit-bot: I haz the power, 2017-02-08 11:08:54]

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-08 11:08:56]

https://codereview.chromium.org/2673423002/diff/120001/media/midi/midi_manager.h
File media/midi/midi_manager.h (right):

https://codereview.chromium.org/2673423002/diff/120001/media/midi/midi_manage...
media/midi/midi_manager.h:84: MidiManager(MidiService* service);
On 2017/02/08 10:04:36, yhirano wrote:
> +explicit

Done.

https://codereview.chromium.org/2673423002/diff/120001/media/midi/midi_manage...
media/midi/midi_manager.h:93: // *** Note ***: If dynamic instantiation feature
is enabled, MidiService call
On 2017/02/08 10:04:36, yhirano wrote:
> call"s"

Done.

https://codereview.chromium.org/2673423002/diff/120001/media/midi/midi_manage...
File media/midi/midi_manager_alsa.cc (right):

https://codereview.chromium.org/2673423002/diff/120001/media/midi/midi_manage...
media/midi/midi_manager_alsa.cc:894: base::AutoLock
lock(g_send_task_lock.Get());
If this lock is created after the instance id check, we would overlook a race
here.

1. Send: ID check pass
2. I/O : Finalize() is running in parallel
3. I/O : Finalize() finishes
4. Send: g_send_task_lock is obtained

If we keep g_send_task_lock obtained as this patch, we can keep Finalize()
unfinished until this task finishes.

https://codereview.chromium.org/2673423002/diff/120001/media/midi/midi_manage...
media/midi/midi_manager_alsa.cc:929: base::AutoLock
lock(g_event_task_lock.Get());
same

https://codereview.chromium.org/2673423002/diff/120001/media/midi/midi_manage...
File media/midi/midi_manager_alsa.h (right):

https://codereview.chromium.org/2673423002/diff/120001/media/midi/midi_manage...
media/midi/midi_manager_alsa.h:34: MidiManagerAlsa(MidiService* service);
On 2017/02/08 10:04:37, yhirano wrote:
> +explicit

Done.

https://codereview.chromium.org/2673423002/diff/120001/media/midi/midi_manage...
File media/midi/midi_manager_mac.h (right):

https://codereview.chromium.org/2673423002/diff/120001/media/midi/midi_manage...
media/midi/midi_manager_mac.h:28: MidiManagerMac(MidiService* service);
On 2017/02/08 10:04:37, yhirano wrote:
> +explicit

Done.

https://codereview.chromium.org/2673423002/diff/120001/media/midi/midi_manage...
File media/midi/midi_manager_usb.h (right):

https://codereview.chromium.org/2673423002/diff/120001/media/midi/midi_manage...
media/midi/midi_manager_usb.h:40: explicit MidiManagerUsb(
On 2017/02/08 10:04:37, yhirano wrote:
> -explicit

Done.

https://codereview.chromium.org/2673423002/diff/120001/media/midi/midi_manage...
File media/midi/midi_manager_win.h (right):

https://codereview.chromium.org/2673423002/diff/120001/media/midi/midi_manage...
media/midi/midi_manager_win.h:51: MidiManagerWin(MidiService* service);
On 2017/02/08 10:04:37, yhirano wrote:
> +explicit

Done.

https://codereview.chromium.org/2673423002/diff/120001/media/midi/midi_manage...
File media/midi/midi_manager_winrt.h (right):

https://codereview.chromium.org/2673423002/diff/120001/media/midi/midi_manage...
media/midi/midi_manager_winrt.h:25: MidiManagerWinrt(MidiService* service);
On 2017/02/08 10:04:37, yhirano wrote:
> +explicit

Done.

https://codereview.chromium.org/2673423002/diff/120001/media/midi/midi_servic...
File media/midi/midi_service.cc (right):

https://codereview.chromium.org/2673423002/diff/120001/media/midi/midi_servic...
media/midi/midi_service.cc:33: active_clients_ = 0u;
On 2017/02/08 10:04:37, yhirano wrote:
> Can you move this initialization to the member initialization list (or the
> header)?

Done.

https://codereview.chromium.org/2673423002/diff/120001/media/midi/midi_servic...
media/midi/midi_service.cc:42: active_clients_ = 0u;
On 2017/02/08 10:04:37, yhirano wrote:
> ditto

Done.

[commit-bot: I haz the power, 2017-02-08 11:58:30]

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

[commit-bot: I haz the power, 2017-02-08 11:58:31]

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

[Takashi Toyoshima, 2017-02-08 12:54:22]

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

[commit-bot: I haz the power, 2017-02-08 12:54: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, 2017-02-08 12:56:13]

https://codereview.chromium.org/2673423002/diff/200001/media/midi/midi_manage...
File media/midi/midi_manager_alsa.cc (right):

https://codereview.chromium.org/2673423002/diff/200001/media/midi/midi_manage...
media/midi/midi_manager_alsa.cc:193: {
Since Finalize() is called only when StartInitialization() is called, I need to
set g_active_instance_id here so that CHECK_EQ at line 189 can be fine.

[commit-bot: I haz the power, 2017-02-08 14:31:01]

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

[commit-bot: I haz the power, 2017-02-08 14:31:02]

Dry run: This issue passed the CQ dry run.

[Adam Goode, 2017-02-08 15:10:37]

lgtm

I tested on Linux. Looks good! I confirm that Chrome now disconnects from ALSA
when navigating away from the page and reconnects cleanly again when returning.

[Takashi Toyoshima, 2017-02-09 05:26:07]

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

[Takashi Toyoshima, 2017-02-09 05:26:43]

+Alexei for tools/metrics

[yhirano, 2017-02-09 09:31:26]

lgtm

[Alexei Svitkine (slow), 2017-02-09 16:27:27]

lgtm

[Takashi Toyoshima, 2017-02-10 01:14:07]

The CQ bit was checked by toyoshim@chromium.org

[commit-bot: I haz the power, 2017-02-10 01:14:33]

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-10 01:34:49]

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

[commit-bot: I haz the power, 2017-02-10 01:34:50]

Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[Takashi Toyoshima, 2017-02-10 05:17:20]

The CQ bit was checked by toyoshim@chromium.org

[Takashi Toyoshima, 2017-02-10 05:17:20]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org, agoode@chromium.org, yhirano@chromium.org
Link to the patchset: https://codereview.chromium.org/2673423002/#ps220001
(title: "rebase")

[commit-bot: I haz the power, 2017-02-10 05:17:43]

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-10 05:35:57]

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

[commit-bot: I haz the power, 2017-02-10 05:35:58]

Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)

[Takashi Toyoshima, 2017-02-10 06:57:02]

The CQ bit was checked by toyoshim@chromium.org

[commit-bot: I haz the power, 2017-02-10 06:57:25]

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-10 07:13:34]

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

[commit-bot: I haz the power, 2017-02-10 07:13:35]

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

[Takashi Toyoshima, 2017-02-10 08:05:41]

The CQ bit was checked by toyoshim@chromium.org

[commit-bot: I haz the power, 2017-02-10 08:06:08]

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

[Takashi Toyoshima, 2017-02-10 08:08:37]

The CQ bit was checked by toyoshim@chromium.org

[Takashi Toyoshima, 2017-02-10 08:08:38]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org, agoode@chromium.org, yhirano@chromium.org
Link to the patchset: https://codereview.chromium.org/2673423002/#ps240001
(title: "rebase again")

[commit-bot: I haz the power, 2017-02-10 08:09:00]

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-10 10:03:37]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1486714117646330,
"parent_rev": "e777c2aebf305f494ca8186328b03532a78f35d8", "commit_rev":
"0f1c3f34031ae08400afbe43c70fd71267a870b1"}

[commit-bot: I haz the power, 2017-02-10 10:04:52]

Description was changed from

==========
Web MIDI: add dynamic MidiManager instantiation support for Linux

With this change, MidiService starts supporting dynamic MidiManager
instantiation. First, this enables it for Linux for a trial.

BUG=672793
==========

to

==========
Web MIDI: add dynamic MidiManager instantiation support for Linux

With this change, MidiService starts supporting dynamic MidiManager
instantiation. First, this enables it for Linux for a trial.

BUG=672793

Review-Url: https://codereview.chromium.org/2673423002
Cr-Commit-Position: refs/heads/master@{#449574}
Committed:
https://chromium.googlesource.com/chromium/src/+/0f1c3f34031ae08400afbe43c70f...
==========

[commit-bot: I haz the power, 2017-02-10 10:04:54]

Committed patchset #13 (id:240001) as
https://chromium.googlesource.com/chromium/src/+/0f1c3f34031ae08400afbe43c70f...