MidiManagerWinrt as base::FeatureList feature

MidiManagerWinrt as base::FeatureList feature

Now MidiManagerWinrt is available as a base::FeatureList feature instead of command-line switch.

BUG=645403
Committed: https://crrev.com/64ee320eb5f9adb0281de97c0d4a17317327ad10
Cr-Commit-Position: refs/heads/master@{#420165}

[Shao-Chuan Lee, 2016-09-20 08:45:07]

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

[Shao-Chuan Lee, 2016-09-20 08:45:07]

Maybe we don't want to merge this immediately.
How about collecting usage statistics on Windows 10 first, or can we distinguish
Windows 10 users from currently available UMA metrics?

[Takashi Toyoshima, 2016-09-20 09:12:04]

https://codereview.chromium.org/2352013002/diff/1/media/midi/midi_features.cc
File media/midi/midi_features.cc (right):

https://codereview.chromium.org/2352013002/diff/1/media/midi/midi_features.cc...
media/midi/midi_features.cc:11: base::FEATURE_ENABLED_BY_DEFAULT};
The Feature API is used for a field trial in general, and there is a clear
guideline, go/finch101.
Generally speaking, flipping this to ENABLED_BY_DEFAULT is the last step of the
feature launch process, and need to start from DISABLED_BY_DEFAULT, and enable
it via server side configurations so that we can shutdown the feature safely.

I don't think this change need a launch review, but it's better to follow the
guideline just in case so to avoid unnecessary troubles.

https://codereview.chromium.org/2352013002/diff/1/media/midi/midi_features.h
File media/midi/midi_features.h (right):

https://codereview.chromium.org/2352013002/diff/1/media/midi/midi_features.h#...
media/midi/midi_features.h:14: MIDI_EXPORT extern const base::Feature
kUseWinrtMidiApi;
We do not use 'Use' or 'Enable' like prefix for the Feature API naming.
Probably, kWinrtMidiManager would be fine.

https://codereview.chromium.org/2352013002/diff/1/media/midi/midi_features.h#...
media/midi/midi_features.h:19: #endif  // MEDIA_MIDI_MIDI_FEATURES_H_
Can we reuse midi_switches as media_switches does.

[Shao-Chuan Lee, 2016-09-20 09:45:31]

Description was changed from

==========
UseWinrtMidiApi as base::FeatureList feature, enabled by default

Now UseWinrtMidiApi is available as a feature instead of command-line switch,
enabled by default on Windows 10 and can be explicitly disabled by user.

BUG=645403
==========

to

==========
UseWinrtMidiApi as base::FeatureList feature

Now UseWinrtMidiApi is available as a base::FeatureList feature instead of
command-line switch. 

BUG=645403
==========

[Shao-Chuan Lee, 2016-09-20 09:57:27]

https://codereview.chromium.org/2352013002/diff/1/media/midi/midi_features.cc
File media/midi/midi_features.cc (right):

https://codereview.chromium.org/2352013002/diff/1/media/midi/midi_features.cc...
media/midi/midi_features.cc:11: base::FEATURE_ENABLED_BY_DEFAULT};
On 2016/09/20 09:12:04, toyoshim wrote:
> The Feature API is used for a field trial in general, and there is a clear
> guideline, go/finch101.
> Generally speaking, flipping this to ENABLED_BY_DEFAULT is the last step of
the
> feature launch process, and need to start from DISABLED_BY_DEFAULT, and enable
> it via server side configurations so that we can shutdown the feature safely.
> 
> I don't think this change need a launch review, but it's better to follow the
> guideline just in case so to avoid unnecessary troubles.

Done.

https://codereview.chromium.org/2352013002/diff/1/media/midi/midi_features.h
File media/midi/midi_features.h (right):

https://codereview.chromium.org/2352013002/diff/1/media/midi/midi_features.h#...
media/midi/midi_features.h:14: MIDI_EXPORT extern const base::Feature
kUseWinrtMidiApi;
On 2016/09/20 09:12:04, toyoshim wrote:
> We do not use 'Use' or 'Enable' like prefix for the Feature API naming.
> Probably, kWinrtMidiManager would be fine.

Now using kMidiManagerWinrt to stick with the class name.

https://codereview.chromium.org/2352013002/diff/1/media/midi/midi_features.h#...
media/midi/midi_features.h:19: #endif  // MEDIA_MIDI_MIDI_FEATURES_H_
On 2016/09/20 09:12:04, toyoshim wrote:
> Can we reuse midi_switches as media_switches does.

As discussed offline, now moved to midi_switches under namespace
media::midi::features.

[Takashi Toyoshima, 2016-09-20 10:11:47]

lgtm for midi. Probably, it's better to ask a field trial team member.
Let me add asvitkine@ to hte reviewer list.

[Takashi Toyoshima, 2016-09-20 10:12:23]

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

[Takashi Toyoshima, 2016-09-20 10:18:16]

asvitkine@, we want to enable an experimental implementation for Canary and Dev
to check stability.

Our plan is
1. convert an existing flag to a field trial flag (this CL)
2. enable the field trial for 50% users in Canary and Dev
3. monitor stability for a couple of weeks
4. enable it by default (should we have a launch review for this kind of
change?)

[Alexei Svitkine (slow), 2016-09-20 14:48:19]

LGTM

[Shao-Chuan Lee, 2016-09-20 14:50:43]

Description was changed from

==========
UseWinrtMidiApi as base::FeatureList feature

Now UseWinrtMidiApi is available as a base::FeatureList feature instead of
command-line switch. 

BUG=645403
==========

to

==========
MidiManagerWinrt as base::FeatureList feature

Now MidiManagerWinrt is available as a base::FeatureList feature instead of
command-line switch.

BUG=645403
==========

[Shao-Chuan Lee, 2016-09-20 14:51:53]

The CQ bit was checked by shaochuan@chromium.org

[commit-bot: I haz the power, 2016-09-20 14:52:14]

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-09-20 16:11:03]

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

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

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Shao-Chuan Lee, 2016-09-21 02:32:58]

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

[Shao-Chuan Lee, 2016-09-21 02:32:58]

isherman@: PTAL at histograms.xml. Should I remove the previous entry for
"use-winrt-midi-api"?

[Shao-Chuan Lee, 2016-09-21 02:34:16]

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

[commit-bot: I haz the power, 2016-09-21 02:35:00]

Dry run: 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-09-21 03:55:16]

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

[commit-bot: I haz the power, 2016-09-21 03:55:17]

Dry run: This issue passed the CQ dry run.

[Ilya Sherman, 2016-09-21 21:17:08]

On 2016/09/21 02:32:58, Shao-Chuan Lee wrote:
> isherman@: PTAL at histograms.xml. Should I remove the previous entry for
> "use-winrt-midi-api"?

histograms.xml lgtm (and Alexei has already given you approval for this file). 
There's no need to remove previous entries.

[Ilya Sherman, 2016-09-21 21:17:23]

The CQ bit was checked by isherman@chromium.org

[Ilya Sherman, 2016-09-21 21:17:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
toyoshim@chromium.org, asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/2352013002/#ps40001
(title: "fix histograms.xml")

[commit-bot: I haz the power, 2016-09-21 21:17:57]

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-09-21 21:26:22]

Description was changed from

==========
MidiManagerWinrt as base::FeatureList feature

Now MidiManagerWinrt is available as a base::FeatureList feature instead of
command-line switch.

BUG=645403
==========

to

==========
MidiManagerWinrt as base::FeatureList feature

Now MidiManagerWinrt is available as a base::FeatureList feature instead of
command-line switch.

BUG=645403
==========

[commit-bot: I haz the power, 2016-09-21 21:26:24]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-09-21 21:28:03]

Description was changed from

==========
MidiManagerWinrt as base::FeatureList feature

Now MidiManagerWinrt is available as a base::FeatureList feature instead of
command-line switch.

BUG=645403
==========

to

==========
MidiManagerWinrt as base::FeatureList feature

Now MidiManagerWinrt is available as a base::FeatureList feature instead of
command-line switch.

BUG=645403

Committed: https://crrev.com/64ee320eb5f9adb0281de97c0d4a17317327ad10
Cr-Commit-Position: refs/heads/master@{#420165}
==========

[commit-bot: I haz the power, 2016-09-21 21:28:04]

Patchset 3 (id:??) landed as
https://crrev.com/64ee320eb5f9adb0281de97c0d4a17317327ad10
Cr-Commit-Position: refs/heads/master@{#420165}