Extract a base class from CommandLinePrefStore

Extract a base class from CommandLinePrefStore

Currently, CommandLinePrefStore (1) holds all the {switches,
preferences} maps in chrome, (2) applys the command line switches to
their corresponding preferences for the maps it holds.

This CL extracts the logic of applying simple command line switches to the
corresponding preferences in a base class, and rename the class in
chrome/ to ChromeCommandLinePrefStore. So that
ChromeCommandLinePrefStore will call the Apply*Switches methods in its
base class instead.

We made this change because we want to be able to delegate handling
Blimp command line switch to /blimp.

BUG=647848
Committed: https://crrev.com/f00ece607fb01269e3f7015bbd3f0ae22a94470b
Cr-Commit-Position: refs/heads/master@{#425713}

[Menglin, 2016-10-14 01:11:23]

[email protected] changed reviewers:
+ [email protected], [email protected]

[Menglin, 2016-10-14 01:11:23]

[email protected]: Please review changes in 
chrome/browser/prefs/ and components/prefs/

[email protected]: Please review changes in 
chrome/browser/BUILD.gn
chrome/browser/chrome_browser_main.cc
chrome/test/BUILD.gn

Thanks!

Menglin

[Menglin, 2016-10-14 01:21:39]

The CQ bit was checked by [email protected] to run a CQ dry run

[commit-bot: I haz the power, 2016-10-14 01:22:02]

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-10-14 02:07:03]

The CQ bit was unchecked by [email protected]

[commit-bot: I haz the power, 2016-10-14 02:07:04]

Dry run: This issue passed the CQ dry run.

[Bernhard Bauer, 2016-10-14 10:06:31]

https://codereview.chromium.org/2419733005/diff/1/chrome/browser/prefs/chrome...
File chrome/browser/prefs/chrome_command_line_pref_store.h (right):

https://codereview.chromium.org/2419733005/diff/1/chrome/browser/prefs/chrome...
chrome/browser/prefs/chrome_command_line_pref_store.h:1: // Copyright (c) 2012
The Chromium Authors. All rights reserved.
Update the copyright? Technically this is a new file.

https://codereview.chromium.org/2419733005/diff/1/chrome/browser/prefs/chrome...
chrome/browser/prefs/chrome_command_line_pref_store.h:24: bool
ValidateProxySwitches();
Not really your fault, but it's a bit weird to make this protected, just so that
a test can subclass this class *only to call a protected method on it*. And then
on top of that, the subclass is made a (private) friend as well :) We could
probably just make this method public and remove the subclass from the test.
(Also, we shouldn't really log warnings from a test; the production could should
do that if desired.)

If you don't feel like doing that cleanup, could you add a TODO to do it?
Thanks!

https://codereview.chromium.org/2419733005/diff/1/components/prefs/command_li...
File components/prefs/command_line_pref_store.cc (right):

https://codereview.chromium.org/2419733005/diff/1/components/prefs/command_li...
components/prefs/command_line_pref_store.cc:49: const
CommandLinePrefStore::SwitchToPreferenceMapEntry integer_switch[],
Nit: Indented incorrectly.

https://codereview.chromium.org/2419733005/diff/1/components/prefs/command_li...
File components/prefs/command_line_pref_store.h (right):

https://codereview.chromium.org/2419733005/diff/1/components/prefs/command_li...
components/prefs/command_line_pref_store.h:14: // switches to their
corresponding preferences.
Nit: Maybe something like, "Base class for a PrefStore that maps command line
switches to preferences. Subclasses should call the Apply...Switches() methods
with their own maps."?

https://codereview.chromium.org/2419733005/diff/1/components/prefs/command_li...
components/prefs/command_line_pref_store.h:17: struct SwitchToPreferenceMapEntry
{
Can these become protected as well?

[sky, 2016-10-14 15:54:04]

On 2016/10/14 01:11:23, Menglin wrote:
> mailto:[email protected]: Please review changes in 
> chrome/browser/prefs/ and components/prefs/
> 
> mailto:[email protected]: Please review changes in 
> chrome/browser/BUILD.gn
> chrome/browser/chrome_browser_main.cc
> chrome/test/BUILD.gn

Both the gn files are reviewable by anyone, I looked at them though as well as
chrome_browser_main.cc LGTM
> 
> Thanks!
> 
> Menglin

[Menglin, 2016-10-14 17:12:04]

new patch uploaded

https://codereview.chromium.org/2419733005/diff/1/chrome/browser/prefs/chrome...
File chrome/browser/prefs/chrome_command_line_pref_store.h (right):

https://codereview.chromium.org/2419733005/diff/1/chrome/browser/prefs/chrome...
chrome/browser/prefs/chrome_command_line_pref_store.h:1: // Copyright (c) 2012
The Chromium Authors. All rights reserved.
On 2016/10/14 10:06:31, Bernhard Bauer wrote:
> Update the copyright? Technically this is a new file.

Done.

https://codereview.chromium.org/2419733005/diff/1/chrome/browser/prefs/chrome...
chrome/browser/prefs/chrome_command_line_pref_store.h:24: bool
ValidateProxySwitches();
On 2016/10/14 10:06:31, Bernhard Bauer wrote:
> Not really your fault, but it's a bit weird to make this protected, just so
that
> a test can subclass this class *only to call a protected method on it*. And
then
> on top of that, the subclass is made a (private) friend as well :) We could
> probably just make this method public and remove the subclass from the test.
> (Also, we shouldn't really log warnings from a test; the production could
should
> do that if desired.)
> 
> If you don't feel like doing that cleanup, could you add a TODO to do it?
> Thanks!

ok. for now I add a TODO assigned to you :)

https://codereview.chromium.org/2419733005/diff/1/components/prefs/command_li...
File components/prefs/command_line_pref_store.cc (right):

https://codereview.chromium.org/2419733005/diff/1/components/prefs/command_li...
components/prefs/command_line_pref_store.cc:49: const
CommandLinePrefStore::SwitchToPreferenceMapEntry integer_switch[],
On 2016/10/14 10:06:31, Bernhard Bauer wrote:
> Nit: Indented incorrectly.

Done.

https://codereview.chromium.org/2419733005/diff/1/components/prefs/command_li...
File components/prefs/command_line_pref_store.h (right):

https://codereview.chromium.org/2419733005/diff/1/components/prefs/command_li...
components/prefs/command_line_pref_store.h:14: // switches to their
corresponding preferences.
On 2016/10/14 10:06:31, Bernhard Bauer wrote:
> Nit: Maybe something like, "Base class for a PrefStore that maps command line
> switches to preferences. Subclasses should call the Apply...Switches() methods
> with their own maps."?

Done.

https://codereview.chromium.org/2419733005/diff/1/components/prefs/command_li...
components/prefs/command_line_pref_store.h:17: struct SwitchToPreferenceMapEntry
{
On 2016/10/14 10:06:31, Bernhard Bauer wrote:
> Can these become protected as well? 

hmm I think if in blimp, we have 
static
BlimpClientContext::RegisterCommandLinePrefOverrides(CommandLinePrefStore*
store) {
  store->Apply*Switches(blimp_switch_map_);
}
we need to make both the struct and the methods public in order to make that
call ? wdyt?

[Menglin, 2016-10-14 20:36:19]

The CQ bit was checked by [email protected] to run a CQ dry run

[commit-bot: I haz the power, 2016-10-14 20:36:56]

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-10-14 22:08:07]

The CQ bit was unchecked by [email protected]

[commit-bot: I haz the power, 2016-10-14 22:08:07]

Dry run: This issue passed the CQ dry run.

[Bernhard Bauer, 2016-10-17 14:46:38]

LGTM with one comment nit:

https://codereview.chromium.org/2419733005/diff/20001/components/prefs/comman...
File components/prefs/command_line_pref_store.h (right):

https://codereview.chromium.org/2419733005/diff/20001/components/prefs/comman...
components/prefs/command_line_pref_store.h:14: // Subclasses should call the
Apply...Switches() methods with their own maps.
OK, sorry. Now this comment isn't quite accurate anymore, because the
Apply...Switches() methods can in fact be called from other places than
subclasses... maybe add a parenthetical comment "(or delegate to other code)"?

[Menglin, 2016-10-17 16:27:52]

https://codereview.chromium.org/2419733005/diff/20001/components/prefs/comman...
File components/prefs/command_line_pref_store.h (right):

https://codereview.chromium.org/2419733005/diff/20001/components/prefs/comman...
components/prefs/command_line_pref_store.h:14: // Subclasses should call the
Apply...Switches() methods with their own maps.
On 2016/10/17 14:46:38, Bernhard Bauer wrote:
> OK, sorry. Now this comment isn't quite accurate anymore, because the
> Apply...Switches() methods can in fact be called from other places than
> subclasses... maybe add a parenthetical comment "(or delegate to other code)"?

yeah i should've changed this in the last upload.

[Menglin, 2016-10-17 16:28:13]

The CQ bit was checked by [email protected] to run a CQ dry run

[commit-bot: I haz the power, 2016-10-17 16:28:29]

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

[Bernhard Bauer, 2016-10-17 16:52:51]

Ship it :)

[commit-bot: I haz the power, 2016-10-17 17:23:51]

The CQ bit was unchecked by [email protected]

[commit-bot: I haz the power, 2016-10-17 17:23:52]

Dry run: This issue passed the CQ dry run.

[Menglin, 2016-10-17 17:24:06]

The CQ bit was checked by [email protected]

[Menglin, 2016-10-17 17:24:06]

The patchset sent to the CQ was uploaded after l-g-t-m from [email protected],
[email protected]
Link to the patchset: https://codereview.chromium.org/2419733005/#ps40001
(title: "nit in comment")

[commit-bot: I haz the power, 2016-10-17 17:24: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, 2016-10-17 17:29:31]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-10-17 17:31:09]

Description was changed from

==========
Extract a base class from CommandLinePrefStore

Currently, CommandLinePrefStore (1) holds all the {switches,
preferences} maps in chrome, (2) applys the command line switches to
their corresponding preferences for the maps it holds.

This CL extracts the logic of applying simple command line switches to the
corresponding preferences in a base class, and rename the class in
chrome/ to ChromeCommandLinePrefStore. So that
ChromeCommandLinePrefStore will call the Apply*Switches methods in its
base class instead.

We made this change because we want to be able to delegate handling
Blimp command line switch to /blimp.

BUG=647848
==========

to

==========
Extract a base class from CommandLinePrefStore

Currently, CommandLinePrefStore (1) holds all the {switches,
preferences} maps in chrome, (2) applys the command line switches to
their corresponding preferences for the maps it holds.

This CL extracts the logic of applying simple command line switches to the
corresponding preferences in a base class, and rename the class in
chrome/ to ChromeCommandLinePrefStore. So that
ChromeCommandLinePrefStore will call the Apply*Switches methods in its
base class instead.

We made this change because we want to be able to delegate handling
Blimp command line switch to /blimp.

BUG=647848

Committed: https://crrev.com/f00ece607fb01269e3f7015bbd3f0ae22a94470b
Cr-Commit-Position: refs/heads/master@{#425713}
==========

[commit-bot: I haz the power, 2016-10-17 17:31:10]

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