Support base::Feature entries in chrome://flags.

Support base::Feature entries in chrome://flags.

Since base::Features already support toggling via --enable-features=
and --disable-features= flags, integrate with those.

Changes some if statements on FeatureEntry.type to be switch
statements so that future changes to the enum without updating them
result in compile errors.

BUG=544158
Committed: https://crrev.com/03007d04e150b30cfe62ab26a7ed47f71c2a79c2
Cr-Commit-Position: refs/heads/master@{#355419}

[Alexei Svitkine (slow), 2015-10-20 18:42:19]

Patchset #2 (id:20001) has been deleted

[Alexei Svitkine (slow), 2015-10-20 18:42:24]

Patchset #1 (id:1) has been deleted

[Alexei Svitkine (slow), 2015-10-20 18:42:31]

Patchset #3 (id:80001) has been deleted

[Alexei Svitkine (slow), 2015-10-20 18:42:35]

Patchset #2 (id:60001) has been deleted

[Alexei Svitkine (slow), 2015-10-20 18:42:41]

Patchset #1 (id:40001) has been deleted

[Alexei Svitkine (slow), 2015-10-20 18:42:48]

Patchset #1 (id:100001) has been deleted

[Alexei Svitkine (slow), 2015-10-20 18:45:13]

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

[Alexei Svitkine (slow), 2015-10-20 18:45:13]

Hey Nico,

Please take a look. Sorry, this change is a little bit on the larger side, but
hopefully you're ok reviewing it. Let me know if you would prefer if someone
else would review it first before I send it to you for owners review.

I'm separately working on making a chromium.org version of the design doc, which
I'm hoping to also finish this week. Once both this change is in and there's a
public design doc, I'm planning to send an email to chromium-dev@ announcing the
new support.

[Nico, 2015-10-21 20:45:35]

lgtm, seems reasonable

I don't know much about FeatureList though.

https://codereview.chromium.org/1408783002/diff/160001/chrome/browser/about_f...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/1408783002/diff/160001/chrome/browser/about_f...
chrome/browser/about_flags.cc:87: // Macros to simplify specifying the type.
That's a lot of macros by now. Maybe this should have a short "Which one do you
want?" section to help people decide which one to use?

https://codereview.chromium.org/1408783002/diff/160001/chrome/browser/about_f...
chrome/browser/about_flags.cc:107: #define FEATURE_VALUE_TYPE(feature)          
                     \
Do you have some feature that you want to add to this CL, as demo for the macro?

https://codereview.chromium.org/1408783002/diff/160001/chrome/browser/about_f...
chrome/browser/about_flags.cc:2174: bool feature_state;
Will this need to be a tri-state (force on, force off, use default which might
be set via experiment) in the future? No, because the UI shows yes / no /
default and this file then maps that to --enable / --disable / no flag, right?

https://codereview.chromium.org/1408783002/diff/160001/chrome/browser/about_f...
chrome/browser/about_flags.cc:2797: const auto& existing_value =
(*switch_list)[switch_name];
nit: i'd use std::string instead of auto in the previous 3 lines, as the type of
the rhs isn't obvious

https://codereview.chromium.org/1408783002/diff/160001/chrome/browser/about_f...
chrome/browser/about_flags.cc:2929: // practice n should be very small.
how many base::Feature entries are there today? (i think this is fine as-is,
just curious)

[Alexei Svitkine (slow), 2015-10-21 21:37:00]

Thanks!

https://codereview.chromium.org/1408783002/diff/160001/chrome/browser/about_f...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/1408783002/diff/160001/chrome/browser/about_f...
chrome/browser/about_flags.cc:87: // Macros to simplify specifying the type.
On 2015/10/21 20:45:35, Nico wrote:
> That's a lot of macros by now. Maybe this should have a short "Which one do
you
> want?" section to help people decide which one to use?

The comments in the header file on the enum are actually pretty good in this
regard, so I've added a note here pointing at those, to help people.

https://codereview.chromium.org/1408783002/diff/160001/chrome/browser/about_f...
chrome/browser/about_flags.cc:107: #define FEATURE_VALUE_TYPE(feature)          
                     \
On 2015/10/21 20:45:35, Nico wrote:
> Do you have some feature that you want to add to this CL, as demo for the
macro?

Not yet, but I expect a few to be added soon once I announce this API on
chromium-dev@ (planning to do so later this week after this CL lands).

https://codereview.chromium.org/1408783002/diff/160001/chrome/browser/about_f...
chrome/browser/about_flags.cc:2174: bool feature_state;
On 2015/10/21 20:45:35, Nico wrote:
> Will this need to be a tri-state (force on, force off, use default which might
> be set via experiment) in the future? No, because the UI shows yes / no /
> default and this file then maps that to --enable / --disable / no flag, right?

Correct, it doesn't need to be tri-state. See line 2702 in this patch set about
what the default maps to (i.e. it doesn't set the feature name).

https://codereview.chromium.org/1408783002/diff/160001/chrome/browser/about_f...
chrome/browser/about_flags.cc:2797: const auto& existing_value =
(*switch_list)[switch_name];
On 2015/10/21 20:45:35, Nico wrote:
> nit: i'd use std::string instead of auto in the previous 3 lines, as the type
of
> the rhs isn't obvious

It's different between platforms. The actual type is
base::CommandLine::StringType which is typedef'd to either string16 or
std::string. 

Instead of expanding that long type which is not meaningful unless the person is
familiar with CommandLine impl details, I instead now added a comment about what
it maps to, depending on platform.

https://codereview.chromium.org/1408783002/diff/160001/chrome/browser/about_f...
chrome/browser/about_flags.cc:2929: // practice n should be very small.
On 2015/10/21 20:45:35, Nico wrote:
> how many base::Feature entries are there today? (i think this is fine as-is,
> just curious)

Currently, none - as I haven't announced the base::FeatureList API yet (it's
new). Was waiting for this CL to land before I do so, since integration with
about_flags is one of the things people would expect to exist before they start
using it.

[Alexei Svitkine (slow), 2015-10-21 21:37:06]

The CQ bit was checked by asvitkine@chromium.org

[Alexei Svitkine (slow), 2015-10-21 21:37:06]

The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/1408783002/#ps180001
(title: "Address comments.")

[commit-bot: I haz the power, 2015-10-21 21:37:49]

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

[commit-bot: I haz the power, 2015-10-21 22:50:15]

Committed patchset #4 (id:180001)

[commit-bot: I haz the power, 2015-10-21 22:51:08]

Patchset 4 (id:??) landed as
https://crrev.com/03007d04e150b30cfe62ab26a7ed47f71c2a79c2
Cr-Commit-Position: refs/heads/master@{#355419}