[WebMIDI] Add "use-android-midi-api" flag for Android M+ devices.

[WebMIDI] Add "use-android-midi-api" flag for Android M+ devices.

For experimental support for Android Midi API which is introduced in Android M,
this CL adds a chrome flag.

BUG=486584
Committed: https://crrev.com/bbea627295f45d7b9cb0167d251abb687d436e68
Cr-Commit-Position: refs/heads/master@{#349360}

[yhirano, 2015-08-11 09:12:49]

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

[yhirano, 2015-08-11 09:12:49]

PTAL.

This CL is currently private, but introducing MIDI API was already published and
so I think there is no problem to make it public. Please correct me if I'm
wrong.

I will land this CL when the implementation CL[1] gets ready.

1: https://codereview.chromium.org/1177973003/

[Takashi Toyoshima, 2015-08-12 04:39:47]

I slightly feel like CreationOptions is overdoing. WDYT?

https://codereview.chromium.org/1276233005/diff/1/content/browser/browser_mai...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/1276233005/diff/1/content/browser/browser_mai...
content/browser/browser_main_loop.cc:1164:
media::midi::MidiManager::CreationOptions options;
I prefer to use
base::CommandLine::ForCurrentProcess()->HasSwitch(switches::kUseAndroidMidiApi)
inside MidiManager(Android) so that we do not need to touch this file, and have
options for constructors.

https://codereview.chromium.org/1276233005/diff/1/media/midi/midi_manager_and...
File media/midi/midi_manager_android.cc (right):

https://codereview.chromium.org/1276233005/diff/1/media/midi/midi_manager_and...
media/midi/midi_manager_android.cc:13: return new MidiManagerUsb(
Could it be simply something like this?

if
(base::CommandLine::ForCurrentProcess()->HasSwitch(switches::kUseAndroidMidiApi))
  return new MidiManagerAndroidNative()
else
  return new MidiManagerUsb(...)

[yhirano, 2015-08-12 05:31:43]

On 2015/08/12 04:39:47, Takashi Toyoshima (chromium) wrote:
> I slightly feel like CreationOptions is overdoing. WDYT?

Ah, yes, you're right. I moved the switch definition to media_switches files.

[yhirano, 2015-08-12 05:37:06]

https://codereview.chromium.org/1276233005/diff/1/content/browser/browser_mai...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/1276233005/diff/1/content/browser/browser_mai...
content/browser/browser_main_loop.cc:1164:
media::midi::MidiManager::CreationOptions options;
On 2015/08/12 04:39:47, Takashi Toyoshima (chromium) wrote:
> I prefer to use
>
base::CommandLine::ForCurrentProcess()->HasSwitch(switches::kUseAndroidMidiApi)
> inside MidiManager(Android) so that we do not need to touch this file, and
have
> options for constructors.

Done.

https://codereview.chromium.org/1276233005/diff/1/media/midi/midi_manager_and...
File media/midi/midi_manager_android.cc (right):

https://codereview.chromium.org/1276233005/diff/1/media/midi/midi_manager_and...
media/midi/midi_manager_android.cc:13: return new MidiManagerUsb(
On 2015/08/12 04:39:47, Takashi Toyoshima (chromium) wrote:
> Could it be simply something like this?
> 
> if
>
(base::CommandLine::ForCurrentProcess()->HasSwitch(switches::kUseAndroidMidiApi))
>   return new MidiManagerAndroidNative()
> else
>   return new MidiManagerUsb(...)

Done.

[Takashi Toyoshima, 2015-08-12 05:40:24]

One concern about the media_switches on the patch set 2.
When we actually use it from media/midi, can it be built in component build
correctly?
I think we didn't build and link with media to build midi and midi_uinttests
components. I think we can easily fix the build problem by adding media/base
dependency to midi_unittests, but just in case.

If there aren't any difficulty on build, this looks good.

[yhirano, 2015-08-12 06:00:05]

On 2015/08/12 05:40:24, Takashi Toyoshima (chromium) wrote:
> One concern about the media_switches on the patch set 2.
> When we actually use it from media/midi, can it be built in component build
> correctly?
> I think we didn't build and link with media to build midi and midi_uinttests
> components. I think we can easily fix the build problem by adding media/base
> dependency to midi_unittests, but just in case.
> 
> If there aren't any difficulty on build, this looks good.

I failed to build.
Is it better to create dependency to media/? I think creating midi_switches also
works.

[Takashi Toyoshima, 2015-08-12 06:05:31]

Aha. Since now midi is almost separated from media as a component, having
midi_switches sounds slightly better to me.

[yhirano, 2015-08-17 11:29:20]

Patchset #4 (id:60001) has been deleted

[yhirano, 2015-08-17 11:29:24]

Patchset #3 (id:40001) has been deleted

[yhirano, 2015-08-17 11:30:14]

On 2015/08/12 06:05:31, Takashi Toyoshima (chromium) wrote:
> Aha. Since now midi is almost separated from media as a component, having
> midi_switches sounds slightly better to me.

Done.

[Takashi Toyoshima, 2015-08-17 12:26:29]

Ah, midi was linked only with content/common, so we need one more dependency
from chrome/browser as you added. ok, this patch set 3 lgtm.

[yhirano, 2015-08-18 14:20:09]

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

[yhirano, 2015-08-18 14:21:03]

Nico, can you take a look at chrome/ changes?

[yhirano, 2015-08-20 12:26:06]

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

[yhirano, 2015-08-20 12:28:12]

+isherman@ for histograms.xml

[Ilya Sherman, 2015-08-20 22:44:01]

histograms.xml lgtm

[yhirano, 2015-08-26 04:50:52]

thakis@: ping

[yhirano, 2015-09-15 04:50:29]

On 2015/08/26 04:50:52, yhirano wrote:
> thakis@: ping

thakis@: ping

[Nico, 2015-09-16 06:08:17]

chrome lgtm

[yhirano, 2015-09-17 04:05:03]

Thanks!

[yhirano, 2015-09-17 04:05:07]

The CQ bit was checked by yhirano@chromium.org

[yhirano, 2015-09-17 04:05:07]

The patchset sent to the CQ was uploaded after l-g-t-m from
toyoshim@chromium.org, thakis@chromium.org, isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/1276233005/#ps140001
(title: "rebase")

[commit-bot: I haz the power, 2015-09-17 04:05:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1276233005/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1276233005/140001

[commit-bot: I haz the power, 2015-09-17 07:09:11]

Committed patchset #6 (id:140001)

[commit-bot: I haz the power, 2015-09-17 07:09:52]

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