Componentize internal class FlagsState in flags_ui component.

Componentize internal class FlagsState in flags_ui component.

Move FlagsState class definition to components/flags_ui so that it can
be shared with iOS but leave the global FeatureEntry list definition
in the embedder.

Slightly change FlagsState interface to take the name of embedder
specific switches (switches::kEnableFeatures, ...) and use dummy values
for unit testing.

Split the unit tests moving all those that have no dependency on the
global FeatureEntry to flags_state_unittest.cc. Remove helper testing
function that are no longer useful (overridding global state).

BUG=551954
Committed: https://crrev.com/0e56634f4731afdbec1dbbd0a230c5a27f0296ec
Cr-Commit-Position: refs/heads/master@{#361295}

[sdefresne, 2015-11-06 16:45:39]

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

[sdefresne, 2015-11-06 16:45:40]

Second step to componentize about_flags.{cc,h}. Here I componentize FlagsState
and the corresponding unit tests. The next patch will upstream iOS forks to
FlagsState (mostly adding a kOSIOS value).

Could you take a look Alexei?

[sdefresne, 2015-11-06 17:51:19]

sdefresne@chromium.org changed reviewers:
+ nkostylev@chromium.org, sky@chromium.org

[sdefresne, 2015-11-06 17:51:19]

+nkostylev@chromium.org: as OWNERS of chrome/browser/chromeos &
chrome/browser/ui
+sky@chromium.org: as OWNERS of the rest of chrome/browser, chrome/common &
chrome/test

[sky, 2015-11-06 21:17:48]

sky@chromium.org changed reviewers:
+ thakis@chromium.org
- sky@chromium.org

[sky, 2015-11-06 21:17:48]

thakis is the original author of about_flags, and he's an owner. So, sky->thakis

[Alexei Svitkine (slow), 2015-11-06 21:22:28]

Is there a way for to have the diff for the new files to look like they were
branched from about_flags.cc/_unittest.cc so that it's easier to review?

https://codereview.chromium.org/1411453004/diff/40001/chrome/browser/about_fl...
File chrome/browser/about_flags.cc (left):

https://codereview.chromium.org/1411453004/diff/40001/chrome/browser/about_fl...
chrome/browser/about_flags.cc:2515: default:
I landed https://codereview.chromium.org/1406023021/ earlier today, can you sync
to that change? Thanks.

[Alexei Svitkine (slow), 2015-11-06 21:26:01]

https://codereview.chromium.org/1411453004/diff/40001/components/flags_ui/fla...
File components/flags_ui/flags_state.h (right):

https://codereview.chromium.org/1411453004/diff/40001/components/flags_ui/fla...
components/flags_ui/flags_state.h:140: int GetCurrentPlatform();
It's kind of weird to have free-standing utility methods in a header file that
primarily declares a class. Perhaps these could be statics on the class?

[sdefresne, 2015-11-12 11:37:25]

PTAL

https://codereview.chromium.org/1411453004/diff/40001/chrome/browser/about_fl...
File chrome/browser/about_flags.cc (left):

https://codereview.chromium.org/1411453004/diff/40001/chrome/browser/about_fl...
chrome/browser/about_flags.cc:2515: default:
On 2015/11/06 at 21:22:28, Alexei Svitkine (slow) wrote:
> I landed https://codereview.chromium.org/1406023021/ earlier today, can you
sync to that change? Thanks.

Done.

https://codereview.chromium.org/1411453004/diff/40001/components/flags_ui/fla...
File components/flags_ui/flags_state.h (right):

https://codereview.chromium.org/1411453004/diff/40001/components/flags_ui/fla...
components/flags_ui/flags_state.h:140: int GetCurrentPlatform();
On 2015/11/06 at 21:26:01, Alexei Svitkine (slow) wrote:
> It's kind of weird to have free-standing utility methods in a header file that
primarily declares a class. Perhaps these could be statics on the class?

Done.

[sdefresne, 2015-11-12 11:47:36]

Patchset #4 (id:60001) has been deleted

[sdefresne, 2015-11-12 11:54:34]

Patchset #4 (id:80001) has been deleted

[sdefresne, 2015-11-16 20:53:17]

Please take a look.

[Nico, 2015-11-16 21:20:08]

(my working hypothesis here is that this is waiting on asvitkine for the main
review?)

[Alexei Svitkine (slow), 2015-11-16 21:21:20]

Woops, will take a look now.

[Alexei Svitkine (slow), 2015-11-16 21:52:54]

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

https://codereview.chromium.org/1411453004/diff/120001/chrome/browser/about_f...
chrome/browser/about_flags.cc:2085: bool
SkipConditionalFeatureEntry(version_info::Channel channel,
Nit: Seems like it's not really needed to add a channel param here, since this
function is staying in about_flags.cc (at least in this CL). So it can still
query the channel on its own to reduce the diff.

https://codereview.chromium.org/1411453004/diff/120001/chrome/browser/about_f...
chrome/browser/about_flags.cc:2166: const char*
extra_flag_sentinel_begin_flag_name =
Add a comment about this logic.

Nit: Also, how about:

const char* extra_flag_sentinel_begin_flag_name = nullptr;
const char* extra_flag_sentinel_end_flag_name = nullptr;
#if defined(OS_CHROMEOS)
extra_flag_sentinel_begin_flag_name = chromeos::switches::kPolicySwitchesBegin;
extra_flag_sentinel_end_flag_name = chromeos::switches::kPolicySwitchesEnd;
#endif

https://codereview.chromium.org/1411453004/diff/120001/chrome/browser/chromeo...
File chrome/browser/chromeos/settings/owner_flags_storage.cc (right):

https://codereview.chromium.org/1411453004/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/settings/owner_flags_storage.cc:60:
::flags_ui::kNoSentinels);
Nit: No need for leading ::, since there's no longer a conflict here.

https://codereview.chromium.org/1411453004/diff/120001/components/flags_ui.gypi
File components/flags_ui.gypi (right):

https://codereview.chromium.org/1411453004/diff/120001/components/flags_ui.gy...
components/flags_ui.gypi:52: ['OS=="win" and target_arch=="ia32"', {
Why is this needed? Could use a comment, otherwise it looks very strange.

https://codereview.chromium.org/1411453004/diff/120001/components/flags_ui/BU...
File components/flags_ui/BUILD.gn (right):

https://codereview.chromium.org/1411453004/diff/120001/components/flags_ui/BU...
components/flags_ui/BUILD.gn:30: source_set("switches") {
Why is this a separate source set? If there's a good reason, can you add a
comment? Otherwise, I'd just make it part of flags_ui.

https://codereview.chromium.org/1411453004/diff/120001/components/flags_ui/fl...
File components/flags_ui/flags_state.cc (right):

https://codereview.chromium.org/1411453004/diff/120001/components/flags_ui/fl...
components/flags_ui/flags_state.cc:24: const struct {
Nit: I'd define it next to the function that uses it.

https://codereview.chromium.org/1411453004/diff/120001/components/flags_ui/fl...
components/flags_ui/flags_state.cc:44: // Scoops flags from a command line.
Nit: Expand the comment to mention the two new params you added.

https://codereview.chromium.org/1411453004/diff/120001/components/flags_ui/fl...
components/flags_ui/flags_state.cc:63: // Then add those between the extra
sentinels.
Nit: Move this comment above the outside if and add a blank line before.

https://codereview.chromium.org/1411453004/diff/120001/components/flags_ui/fl...
components/flags_ui/flags_state.cc:417: default:
Remove default.

https://codereview.chromium.org/1411453004/diff/120001/components/flags_ui/fl...
File components/flags_ui/flags_state.h (right):

https://codereview.chromium.org/1411453004/diff/120001/components/flags_ui/fl...
components/flags_ui/flags_state.h:89: const char*
extra_flag_sentinel_begin_flag_name,
Nit: Document the new params.

[sdefresne, 2015-11-17 16:28:57]

Addressed comments, please take another look.

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

https://codereview.chromium.org/1411453004/diff/120001/chrome/browser/about_f...
chrome/browser/about_flags.cc:2085: bool
SkipConditionalFeatureEntry(version_info::Channel channel,
On 2015/11/16 at 21:52:54, Alexei Svitkine (slow) wrote:
> Nit: Seems like it's not really needed to add a channel param here, since this
function is staying in about_flags.cc (at least in this CL). So it can still
query the channel on its own to reduce the diff.

Reverted.

https://codereview.chromium.org/1411453004/diff/120001/chrome/browser/about_f...
chrome/browser/about_flags.cc:2166: const char*
extra_flag_sentinel_begin_flag_name =
On 2015/11/16 at 21:52:54, Alexei Svitkine (slow) wrote:
> Add a comment about this logic.
> 
> Nit: Also, how about:
> 
> const char* extra_flag_sentinel_begin_flag_name = nullptr;
> const char* extra_flag_sentinel_end_flag_name = nullptr;
> #if defined(OS_CHROMEOS)
> extra_flag_sentinel_begin_flag_name =
chromeos::switches::kPolicySwitchesBegin;
> extra_flag_sentinel_end_flag_name = chromeos::switches::kPolicySwitchesEnd;
> #endif

Done.

https://codereview.chromium.org/1411453004/diff/120001/chrome/browser/chromeo...
File chrome/browser/chromeos/settings/owner_flags_storage.cc (right):

https://codereview.chromium.org/1411453004/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/settings/owner_flags_storage.cc:60:
::flags_ui::kNoSentinels);
On 2015/11/16 at 21:52:54, Alexei Svitkine (slow) wrote:
> Nit: No need for leading ::, since there's no longer a conflict here.

Done.

https://codereview.chromium.org/1411453004/diff/120001/components/flags_ui.gypi
File components/flags_ui.gypi (right):

https://codereview.chromium.org/1411453004/diff/120001/components/flags_ui.gy...
components/flags_ui.gypi:52: ['OS=="win" and target_arch=="ia32"', {
On 2015/11/16 at 21:52:54, Alexei Svitkine (slow) wrote:
> Why is this needed? Could use a comment, otherwise it looks very strange.

Done (took the comment from base_win64 target).

https://codereview.chromium.org/1411453004/diff/120001/components/flags_ui/BU...
File components/flags_ui/BUILD.gn (right):

https://codereview.chromium.org/1411453004/diff/120001/components/flags_ui/BU...
components/flags_ui/BUILD.gn:30: source_set("switches") {
On 2015/11/16 at 21:52:54, Alexei Svitkine (slow) wrote:
> Why is this a separate source set? If there's a good reason, can you add a
comment? Otherwise, I'd just make it part of flags_ui.

Done (added a comment).

https://codereview.chromium.org/1411453004/diff/120001/components/flags_ui/fl...
File components/flags_ui/flags_state.cc (right):

https://codereview.chromium.org/1411453004/diff/120001/components/flags_ui/fl...
components/flags_ui/flags_state.cc:24: const struct {
On 2015/11/16 at 21:52:54, Alexei Svitkine (slow) wrote:
> Nit: I'd define it next to the function that uses it.

Done.

https://codereview.chromium.org/1411453004/diff/120001/components/flags_ui/fl...
components/flags_ui/flags_state.cc:44: // Scoops flags from a command line.
On 2015/11/16 at 21:52:54, Alexei Svitkine (slow) wrote:
> Nit: Expand the comment to mention the two new params you added.

Done.

https://codereview.chromium.org/1411453004/diff/120001/components/flags_ui/fl...
components/flags_ui/flags_state.cc:63: // Then add those between the extra
sentinels.
On 2015/11/16 at 21:52:54, Alexei Svitkine (slow) wrote:
> Nit: Move this comment above the outside if and add a blank line before.

Done.

https://codereview.chromium.org/1411453004/diff/120001/components/flags_ui/fl...
components/flags_ui/flags_state.cc:417: default:
On 2015/11/16 at 21:52:54, Alexei Svitkine (slow) wrote:
> Remove default.

Done.

https://codereview.chromium.org/1411453004/diff/120001/components/flags_ui/fl...
File components/flags_ui/flags_state.h (right):

https://codereview.chromium.org/1411453004/diff/120001/components/flags_ui/fl...
components/flags_ui/flags_state.h:89: const char*
extra_flag_sentinel_begin_flag_name,
On 2015/11/16 at 21:52:54, Alexei Svitkine (slow) wrote:
> Nit: Document the new params.

Done.

[Alexei Svitkine (slow), 2015-11-17 16:46:48]

LGTM

[sdefresne, 2015-11-18 15:19:40]

On 2015/11/17 at 16:46:48, asvitkine wrote:
> LGTM

Ping,

nkostylev@: please review the following as OWNERS
 - chrome/browser/chromeos
 - chrome/browser/ui

thakis@:please review the following as OWNERS
 - chrome/browser
 - chrome/common
 - chrome/test

[sdefresne, 2015-11-19 11:53:48]

ping?

[Nikita (slow), 2015-11-19 12:19:42]

On 2015/11/18 15:19:40, sdefresne wrote:
> On 2015/11/17 at 16:46:48, asvitkine wrote:
> > LGTM
> 
> Ping,
> 
> nkostylev@: please review the following as OWNERS
>  - chrome/browser/chromeos
>  - chrome/browser/ui

lgtm even though I'm not working on Chrome OS anymore.
I should clean OWNERS files though.

[achuithb, 2015-11-19 22:19:26]

On 2015/11/19 12:19:42, Nikita - not on CrOS anymore wrote:
> On 2015/11/18 15:19:40, sdefresne wrote:
> > On 2015/11/17 at 16:46:48, asvitkine wrote:
> > > LGTM
> > 
> > Ping,
> > 
> > nkostylev@: please review the following as OWNERS
> >  - chrome/browser/chromeos
> >  - chrome/browser/ui
> 
> lgtm even though I'm not working on Chrome OS anymore.
> I should clean OWNERS files though.

I can take care of it, Nikita.

[sdefresne, 2015-11-22 18:54:36]

thakis: ping

[Nico, 2015-11-23 17:47:38]

lgtm

[sdefresne, 2015-11-23 17:52:50]

The CQ bit was checked by sdefresne@chromium.org

[sdefresne, 2015-11-23 17:52:51]

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

[commit-bot: I haz the power, 2015-11-23 17:53:20]

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

[commit-bot: I haz the power, 2015-11-23 21:26:50]

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

[commit-bot: I haz the power, 2015-11-23 21:26:51]

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

[sdefresne, 2015-11-24 08:19:13]

The CQ bit was checked by sdefresne@chromium.org

[sdefresne, 2015-11-24 08:19:14]

Failure looks like it is unrelated, sending back to CQ.

[commit-bot: I haz the power, 2015-11-24 08:20:42]

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

[commit-bot: I haz the power, 2015-11-24 08:55:55]

Committed patchset #10 (id:220001)

[commit-bot: I haz the power, 2015-11-24 08:56:44]

Patchset 10 (id:??) landed as
https://crrev.com/0e56634f4731afdbec1dbbd0a230c5a27f0296ec
Cr-Commit-Position: refs/heads/master@{#361295}