Intorduce a separate preference for 'proxy server mode'

Intorduce a separate preference for 'proxy server mode'

The new preference is kProxyServerMode, which supersedes kProxyAutoDetect and kNoProxyServer. The point of this change is to represent 'use system proxy settings' in a more robust way. The proxy extension API is also adjusted to the preference system.

BUG=65732, 66023
TEST=ProxyPrefsTest.*, andalso covered by ExtensionApiTest.Porxy*,PrefProxyConfigServiceTest.*

[gfeher, 2010-12-13 19:45:13]

Please review.

Note that I have disabled a test that will be easy to fix after proxy prefs are
moved into their own dictionary.

[battre (please use the other), 2010-12-14 09:45:43]

I think it would be nicer to expose the proxy modes as string constants rather
than integers. I don't know whether there are any conventions to prefer
integers.

http://codereview.chromium.org/5701003/diff/8001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_proxy_api.cc (right):

http://codereview.chromium.org/5701003/diff/8001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_proxy_api.cc:57:
proxy_config->GetInteger("mode", &proxy_mode);
I think we should expose this as string constants instead of magic numbers to
the JavaScript interface.
If not: maybe initialize to -1 to check whether value has been specified and
return false in ApplyMode if not.
Or: we should default to System Proxies

http://codereview.chromium.org/5701003/diff/8001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_proxy_api.h (right):

http://codereview.chromium.org/5701003/diff/8001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_proxy_api.h:36: bool ApplyMode(int mode);
Insert reference to mode definitions? Should we make this a string rather than
int?

http://codereview.chromium.org/5701003/diff/8001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_proxy_apitest.cc (right):

http://codereview.chromium.org/5701003/diff/8001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_proxy_apitest.cc:64: "socks=http://9.9.9.9",
proxy_server);
Nit: Indentation broken

http://codereview.chromium.org/5701003/diff/8001/chrome/browser/net/pref_prox...
File chrome/browser/net/pref_proxy_config_service.cc (right):

http://codereview.chromium.org/5701003/diff/8001/chrome/browser/net/pref_prox...
chrome/browser/net/pref_proxy_config_service.cc:90:
pref_service_->GetInteger(prefs::kProxyServerMode));
Use IntToMode and check return value?

[danno, 2010-12-14 12:48:16]

Please also note that AutoDetect doesn't mask the bypass list AFAIK, so be
careful with your logic about masking out other proxy preferences based on the
mode.

http://codereview.chromium.org/5701003/diff/8001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_proxy_api.cc (right):

http://codereview.chromium.org/5701003/diff/8001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_proxy_api.cc:57:
proxy_config->GetInteger("mode", &proxy_mode);
How about a string value externally to the extension (that's what we'd like to
do for policy anyway), but the enum you define later internally?
On 2010/12/14 09:45:44, battre wrote:
> I think we should expose this as string constants instead of magic numbers to
> the JavaScript interface.
> If not: maybe initialize to -1 to check whether value has been specified and
> return false in ApplyMode if not.
> Or: we should default to System Proxies

http://codereview.chromium.org/5701003/diff/8001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_proxy_api.h (right):

http://codereview.chromium.org/5701003/diff/8001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_proxy_api.h:36: bool ApplyMode(int mode);
I think an enum would be better internally. 

On 2010/12/14 09:45:44, battre wrote:
> Insert reference to mode definitions? Should we make this a string rather than
> int?

http://codereview.chromium.org/5701003/diff/8001/chrome/browser/net/pref_prox...
File chrome/browser/net/pref_proxy_config_service.h (right):

http://codereview.chromium.org/5701003/diff/8001/chrome/browser/net/pref_prox...
chrome/browser/net/pref_proxy_config_service.h:117: static bool IntToMode(int
in_value, ProxyServerMode* out_value);
This could be ModeStringToEnum.

http://codereview.chromium.org/5701003/diff/8001/chrome/browser/policy/config...
File chrome/browser/policy/configuration_policy_pref_store.cc (right):

http://codereview.chromium.org/5701003/diff/8001/chrome/browser/policy/config...
chrome/browser/policy/configuration_policy_pref_store.cc:451: 
Add scoped_ptr<Value> for value here.

http://codereview.chromium.org/5701003/diff/8001/chrome/browser/policy/config...
File chrome/browser/policy/configuration_policy_pref_store_unittest.cc (right):

http://codereview.chromium.org/5701003/diff/8001/chrome/browser/policy/config...
chrome/browser/policy/configuration_policy_pref_store_unittest.cc:13: 
You changed quite a bit in the logic to apply the proxy policy, but there are no
new test cases here. Unless you know that your code is thoroughly covered by
these tests (I doubt it), then you should add new ones to make sure you are
getting what you expect.

http://codereview.chromium.org/5701003/diff/8001/chrome/browser/prefs/command...
File chrome/browser/prefs/command_line_pref_store.cc (right):

http://codereview.chromium.org/5701003/diff/8001/chrome/browser/prefs/command...
chrome/browser/prefs/command_line_pref_store.cc:87: }
validation and warning of conflicting switches since you now have more stringent
assumptions about their interaction?

http://codereview.chromium.org/5701003/diff/8001/chrome/browser/prefs/command...
File chrome/browser/prefs/command_line_pref_store_unittest.cc (right):

http://codereview.chromium.org/5701003/diff/8001/chrome/browser/prefs/command...
chrome/browser/prefs/command_line_pref_store_unittest.cc:89:
EXPECT_EQ(PrefProxyConfigService::AUTO_DETECT, int_result);
You should also have a test for MANUAL, I don't see it covered here.

http://codereview.chromium.org/5701003/diff/8001/chrome/common/extensions/api...
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/5701003/diff/8001/chrome/common/extensions/api...
chrome/common/extensions/api/extension_api.json:3640: "mode": {"type":
"integer", "description": "0 = Never use a proxy<br>1 = Auto detect proxy
settings<br>2 = Manually specify proxy settings<br>3 = Use system proxy
settings"}
this definitely should be a string that gets translated internally to an enum.

[danno, 2010-12-14 12:54:48]

http://codereview.chromium.org/5701003/diff/8001/chrome/browser/policy/config...
File chrome/browser/policy/configuration_policy_pref_store.cc (right):

http://codereview.chromium.org/5701003/diff/8001/chrome/browser/policy/config...
chrome/browser/policy/configuration_policy_pref_store.cc:451: 
On 2010/12/14 12:48:17, danno wrote:
> Add scoped_ptr<Value> for value here.

Nevermind (ignore).

[gfeher, 2010-12-16 10:42:04]

Please take another look.

http://codereview.chromium.org/5701003/diff/8001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_proxy_api.cc (right):

http://codereview.chromium.org/5701003/diff/8001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_proxy_api.cc:57:
proxy_config->GetInteger("mode", &proxy_mode);
On 2010/12/14 12:48:17, danno wrote:
> How about a string value externally to the extension (that's what we'd like to
> do for policy anyway), but the enum you define later internally?
> On 2010/12/14 09:45:44, battre wrote:
> > I think we should expose this as string constants instead of magic numbers
to
> > the JavaScript interface.
> > If not: maybe initialize to -1 to check whether value has been specified and
> > return false in ApplyMode if not.
> > Or: we should default to System Proxies
> 

Done.

http://codereview.chromium.org/5701003/diff/8001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_proxy_api.h (right):

http://codereview.chromium.org/5701003/diff/8001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_proxy_api.h:36: bool ApplyMode(int mode);
I'll do the string->enum conversion inside this method, becuase other Apply*
methods tend to do similar things.

http://codereview.chromium.org/5701003/diff/8001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_proxy_apitest.cc (right):

http://codereview.chromium.org/5701003/diff/8001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_proxy_apitest.cc:64: "socks=http://9.9.9.9",
proxy_server);
On 2010/12/14 09:45:44, battre wrote:
> Nit: Indentation broken

Done.

http://codereview.chromium.org/5701003/diff/8001/chrome/browser/net/pref_prox...
File chrome/browser/net/pref_proxy_config_service.cc (right):

http://codereview.chromium.org/5701003/diff/8001/chrome/browser/net/pref_prox...
chrome/browser/net/pref_proxy_config_service.cc:90:
pref_service_->GetInteger(prefs::kProxyServerMode));
On 2010/12/14 09:45:44, battre wrote:
> Use IntToMode and check return value?

Done.

http://codereview.chromium.org/5701003/diff/8001/chrome/browser/net/pref_prox...
File chrome/browser/net/pref_proxy_config_service.h (right):

http://codereview.chromium.org/5701003/diff/8001/chrome/browser/net/pref_prox...
chrome/browser/net/pref_proxy_config_service.h:117: static bool IntToMode(int
in_value, ProxyServerMode* out_value);
I still need this to convert from policy and preference integer values.

http://codereview.chromium.org/5701003/diff/8001/chrome/browser/policy/config...
File chrome/browser/policy/configuration_policy_pref_store_unittest.cc (right):

http://codereview.chromium.org/5701003/diff/8001/chrome/browser/policy/config...
chrome/browser/policy/configuration_policy_pref_store_unittest.cc:13: 
I believe I've left the core logic unchanged. Nevertheless, I've made the
testing of it more complete.

http://codereview.chromium.org/5701003/diff/8001/chrome/browser/prefs/command...
File chrome/browser/prefs/command_line_pref_store.cc (right):

http://codereview.chromium.org/5701003/diff/8001/chrome/browser/prefs/command...
chrome/browser/prefs/command_line_pref_store.cc:87: }
This happens in ValidateProxySwitches. It looks to me that it still does the
right thing.

http://codereview.chromium.org/5701003/diff/8001/chrome/browser/prefs/command...
File chrome/browser/prefs/command_line_pref_store_unittest.cc (right):

http://codereview.chromium.org/5701003/diff/8001/chrome/browser/prefs/command...
chrome/browser/prefs/command_line_pref_store_unittest.cc:89:
EXPECT_EQ(PrefProxyConfigService::AUTO_DETECT, int_result);
On 2010/12/14 12:48:17, danno wrote:
> You should also have a test for MANUAL, I don't see it covered here.

Done.

http://codereview.chromium.org/5701003/diff/8001/chrome/common/extensions/api...
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/5701003/diff/8001/chrome/common/extensions/api...
chrome/common/extensions/api/extension_api.json:3640: "mode": {"type":
"integer", "description": "0 = Never use a proxy<br>1 = Auto detect proxy
settings<br>2 = Manually specify proxy settings<br>3 = Use system proxy
settings"}
On 2010/12/14 12:48:17, danno wrote:
> this definitely should be a string that gets translated internally to an enum.

Done.

[battre (please use the other), 2010-12-16 12:52:16]

http://codereview.chromium.org/5701003/diff/62001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_proxy_api.cc (right):

http://codereview.chromium.org/5701003/diff/62001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_proxy_api.cc:88: mode_enum =
ProxyPrefs::MANUAL;
This should default to system (see also comment in StringToMode) and return
false here.

http://codereview.chromium.org/5701003/diff/62001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_proxy_apitest.cc (right):

http://codereview.chromium.org/5701003/diff/62001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_proxy_apitest.cc:57:
pref_service->FindPreference(prefs::kProxyServer);
Nit: Indentation

http://codereview.chromium.org/5701003/diff/62001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_proxy_apitest.cc:125:
pref_service->FindPreference(prefs::kProxyServer);
Nit: Indentation

http://codereview.chromium.org/5701003/diff/62001/chrome/browser/net/pref_pro...
File chrome/browser/net/pref_proxy_config_service.cc (right):

http://codereview.chromium.org/5701003/diff/62001/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_service.cc:107: default:
how about
case ProxyPrefs::THIRD_OPTION:
   ...
  return true;
default:
  NOTREACHED();
to point out issues if we add another mode?

[gfeher, 2010-12-16 14:28:17]

I've addressed Dominic's comments.

I've also found that extension_proxy_api is not doing a very good job of taking
control of all the prefs. I've added a TODO for this. This will be resolved
naturally when all the proxy prefs are moved into a dictionary.

http://codereview.chromium.org/5701003/diff/62001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_proxy_api.cc (right):

http://codereview.chromium.org/5701003/diff/62001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_proxy_api.cc:88: mode_enum =
ProxyPrefs::MANUAL;
Done. But which comment do you mean in StringToMode?

http://codereview.chromium.org/5701003/diff/62001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_proxy_apitest.cc (right):

http://codereview.chromium.org/5701003/diff/62001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_proxy_apitest.cc:57:
pref_service->FindPreference(prefs::kProxyServer);
On 2010/12/16 12:52:16, battre wrote:
> Nit: Indentation

Done.

http://codereview.chromium.org/5701003/diff/62001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_proxy_apitest.cc:125:
pref_service->FindPreference(prefs::kProxyServer);
On 2010/12/16 12:52:16, battre wrote:
> Nit: Indentation

Done.

http://codereview.chromium.org/5701003/diff/62001/chrome/browser/net/pref_pro...
File chrome/browser/net/pref_proxy_config_service.cc (right):

http://codereview.chromium.org/5701003/diff/62001/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_service.cc:107: default:
Good idea. Done.

[battre (please use the other), 2010-12-16 14:31:19]

>
http://codereview.chromium.org/5701003/diff/62001/chrome/browser/extensions/e...
> chrome/browser/extensions/extension_proxy_api.cc:88: mode_enum =
> ProxyPrefs::MANUAL;
> Done. But which comment do you mean in StringToMode?

Sorry, meant to refer here:
http://codereview.chromium.org/5701003/diff/62001/chrome/browser/net/pref_pro...

[danno, 2010-12-16 16:29:29]

Here's some comments, I'll go over more later.

http://codereview.chromium.org/5701003/diff/71001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_proxy_api.cc (right):

http://codereview.chromium.org/5701003/diff/71001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_proxy_api.cc:88: mode_enum =
ProxyPrefs::SYSTEM;
Add a log warning.

http://codereview.chromium.org/5701003/diff/71001/chrome/browser/prefs/proxy_...
File chrome/browser/prefs/proxy_prefs.cc (right):

http://codereview.chromium.org/5701003/diff/71001/chrome/browser/prefs/proxy_...
chrome/browser/prefs/proxy_prefs.cc:10: bool ProxyPrefs::IntToMode(
If you use const int constants instead of an enum, you don't need.

http://codereview.chromium.org/5701003/diff/71001/chrome/browser/prefs/proxy_...
chrome/browser/prefs/proxy_prefs.cc:36: if (in_value == "disable") {
Do this with a small table?

http://codereview.chromium.org/5701003/diff/71001/chrome/browser/prefs/proxy_...
File chrome/browser/prefs/proxy_prefs.h (right):

http://codereview.chromium.org/5701003/diff/71001/chrome/browser/prefs/proxy_...
chrome/browser/prefs/proxy_prefs.h:11: class ProxyPrefs {
Put these in the prefs namespace

http://codereview.chromium.org/5701003/diff/71001/chrome/browser/prefs/proxy_...
chrome/browser/prefs/proxy_prefs.h:17: DISABLED,
In order to not pollute the namespace, prefix with PROXY_

http://codereview.chromium.org/5701003/diff/71001/chrome/browser/prefs/proxy_...
chrome/browser/prefs/proxy_prefs.h:17: DISABLED,
Maybe add specific = values here? might obviate the need for IntToMode?

http://codereview.chromium.org/5701003/diff/71001/chrome/browser/prefs/proxy_...
chrome/browser/prefs/proxy_prefs.h:31: SYSTEM
Add "PROXY_MODE_COUNT"

http://codereview.chromium.org/5701003/diff/71001/chrome/browser/prefs/proxy_...
chrome/browser/prefs/proxy_prefs.h:34: static bool IntToMode(int in_value,
ProxyServerMode* out_value);
IntToProxyMode

http://codereview.chromium.org/5701003/diff/71001/chrome/browser/prefs/proxy_...
chrome/browser/prefs/proxy_prefs.h:35: static bool StringToMode(const
std::string& in_value,
StringToProxyMode

[gfeher, 2010-12-16 23:54:28]

I've addressed/answered your comments. I've found some similar uses of enums in
other places of the code, please check my inline comments.

http://codereview.chromium.org/5701003/diff/71001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_proxy_api.cc (right):

http://codereview.chromium.org/5701003/diff/71001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_proxy_api.cc:88: mode_enum =
ProxyPrefs::SYSTEM;
On 2010/12/16 16:29:29, danno wrote:
> Add a log warning.

Done.

http://codereview.chromium.org/5701003/diff/71001/chrome/browser/prefs/proxy_...
File chrome/browser/prefs/proxy_prefs.cc (right):

http://codereview.chromium.org/5701003/diff/71001/chrome/browser/prefs/proxy_...
chrome/browser/prefs/proxy_prefs.cc:10: bool ProxyPrefs::IntToMode(
That's true. But I've managed to simplify this function while still using an
enum.

http://codereview.chromium.org/5701003/diff/71001/chrome/browser/prefs/proxy_...
chrome/browser/prefs/proxy_prefs.cc:36: if (in_value == "disable") {
On 2010/12/16 16:29:29, danno wrote:
> Do this with a small table?

Done.

http://codereview.chromium.org/5701003/diff/71001/chrome/browser/prefs/proxy_...
File chrome/browser/prefs/proxy_prefs.h (right):

http://codereview.chromium.org/5701003/diff/71001/chrome/browser/prefs/proxy_...
chrome/browser/prefs/proxy_prefs.h:11: class ProxyPrefs {
The prefs namespace currently only contains the const chars of preference paths.
I would defer messing with this to another CL.

http://codereview.chromium.org/5701003/diff/71001/chrome/browser/prefs/proxy_...
chrome/browser/prefs/proxy_prefs.h:17: DISABLED,
On 2010/12/16 16:29:29, danno wrote:
> In order to not pollute the namespace, prefix with PROXY_

extension.h and session_startup_pref.h have similar enums defined. The only
difference is that they not only have static members, but also some non-static
ones.

http://codereview.chromium.org/5701003/diff/71001/chrome/browser/prefs/proxy_...
chrome/browser/prefs/proxy_prefs.h:17: DISABLED,
On 2010/12/16 16:29:29, danno wrote:
> Maybe add specific = values here? might obviate the need for IntToMode?

Specific int values are encouraged by the style guide. However, I like IntToMode
because it puts range-checking at one place, but I would be okay with removing
it.

http://codereview.chromium.org/5701003/diff/71001/chrome/browser/prefs/proxy_...
chrome/browser/prefs/proxy_prefs.h:31: SYSTEM
On 2010/12/16 16:29:29, danno wrote:
> Add "PROXY_MODE_COUNT"

Done.

http://codereview.chromium.org/5701003/diff/71001/chrome/browser/prefs/proxy_...
chrome/browser/prefs/proxy_prefs.h:34: static bool IntToMode(int in_value,
ProxyServerMode* out_value);
On 2010/12/16 16:29:29, danno wrote:
> IntToProxyMode

Done.

http://codereview.chromium.org/5701003/diff/71001/chrome/browser/prefs/proxy_...
chrome/browser/prefs/proxy_prefs.h:35: static bool StringToMode(const
std::string& in_value,
On 2010/12/16 16:29:29, danno wrote:
> StringToProxyMode

Done.

[gfeher, 2010-12-17 10:41:43]

I've fixed a bug that was caused by the wrong order of strings in
proxy_prefs.cc:kProxyModeNames. I've also added proxy_prefs_unittest.cc to catch
such problems earlier.

[danno, 2010-12-17 14:21:11]

Double check that command-line options actually really work as expected in
chrome executable.

http://codereview.chromium.org/5701003/diff/112001/chrome/browser/net/pref_pr...
File chrome/browser/net/pref_proxy_config_service.cc (right):

http://codereview.chromium.org/5701003/diff/112001/chrome/browser/net/pref_pr...
chrome/browser/net/pref_proxy_config_service.cc:91:
pref_service_->GetInteger(prefs::kProxyServerMode),
nit: indentation is wrong.

http://codereview.chromium.org/5701003/diff/112001/chrome/browser/net/pref_pr...
File chrome/browser/net/pref_proxy_config_service_unittest.cc (right):

http://codereview.chromium.org/5701003/diff/112001/chrome/browser/net/pref_pr...
chrome/browser/net/pref_proxy_config_service_unittest.cc:150: // observer gets
fired twice, because two preferences are set.
Reactivate this test (even though it tests double notification) so that Dominic
notices when his "correct" changes breaks.

http://codereview.chromium.org/5701003/diff/112001/chrome/browser/prefs/proxy...
File chrome/browser/prefs/proxy_prefs.cc (right):

http://codereview.chromium.org/5701003/diff/112001/chrome/browser/prefs/proxy...
chrome/browser/prefs/proxy_prefs.cc:12: // with the constants of ProxyPrefs.
nit: you should also have a corresponding comment in the enum, e.g. "if you add
an enum constant, add a string to the mode names".

http://codereview.chromium.org/5701003/diff/112001/chrome/browser/prefs/proxy...
chrome/browser/prefs/proxy_prefs.cc:30: *out_value =
static_cast<ProxyPrefs::ProxyServerMode> (in_value);
nit: extra space after >

[gfeher, 2010-12-17 15:44:16]

I've double-checked it with command-line and policy settings.

http://codereview.chromium.org/5701003/diff/112001/chrome/browser/net/pref_pr...
File chrome/browser/net/pref_proxy_config_service.cc (right):

http://codereview.chromium.org/5701003/diff/112001/chrome/browser/net/pref_pr...
chrome/browser/net/pref_proxy_config_service.cc:91:
pref_service_->GetInteger(prefs::kProxyServerMode),
Done.

http://codereview.chromium.org/5701003/diff/112001/chrome/browser/net/pref_pr...
File chrome/browser/net/pref_proxy_config_service_unittest.cc (right):

http://codereview.chromium.org/5701003/diff/112001/chrome/browser/net/pref_pr...
chrome/browser/net/pref_proxy_config_service_unittest.cc:150: // observer gets
fired twice, because two preferences are set.
On 2010/12/17 14:21:11, danno wrote:
> Reactivate this test (even though it tests double notification) so that
Dominic
> notices when his "correct" changes breaks.

Done.

http://codereview.chromium.org/5701003/diff/112001/chrome/browser/prefs/proxy...
File chrome/browser/prefs/proxy_prefs.cc (right):

http://codereview.chromium.org/5701003/diff/112001/chrome/browser/prefs/proxy...
chrome/browser/prefs/proxy_prefs.cc:12: // with the constants of ProxyPrefs.
On 2010/12/17 14:21:11, danno wrote:
> nit: you should also have a corresponding comment in the enum, e.g. "if you
add
> an enum constant, add a string to the mode names".

Done.

http://codereview.chromium.org/5701003/diff/112001/chrome/browser/prefs/proxy...
chrome/browser/prefs/proxy_prefs.cc:30: *out_value =
static_cast<ProxyPrefs::ProxyServerMode> (in_value);
On 2010/12/17 14:21:11, danno wrote:
> nit: extra space after >

Done.

[danno, 2010-12-17 16:11:30]

LGTM if you fix the indenting discussed offline and get eromans LGTM.

[gfeher, 2010-12-17 16:28:32]

I'll be on vacation next week, but eroman, please do the review. battre is going
to take over this CL, because he is blocked on it.

[Pam (message me for reviews), 2010-12-18 08:29:54]

In the description, typo -> "Introduce"

http://codereview.chromium.org/5701003/diff/142001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_proxy_api.cc (right):

http://codereview.chromium.org/5701003/diff/142001/chrome/browser/extensions/...
chrome/browser/extensions/extension_proxy_api.cc:89: LOG(WARNING) << "Invalid
mode for proxy settings: " << mode << std::endl;
Is the endl needed? I don't recall seeing it in other log messages, and I'd
expect the macro to handle it.

http://codereview.chromium.org/5701003/diff/142001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_proxy_apitest.cc (right):

http://codereview.chromium.org/5701003/diff/142001/chrome/browser/extensions/...
chrome/browser/extensions/extension_proxy_apitest.cc:26:
pref_service->FindPreference(prefs::kProxyServerMode);
Not to quibble, but I don't understand the naming here. How is "use system proxy
settings" or "auto-detect settings" described by "ProxyServerMode"? It sounds
like the browser is itself acting as a proxy server. Would "kProxySettingsMode"
or just "kProxyMode" work?

Not obligatory, just something to consider.

http://codereview.chromium.org/5701003/diff/142001/chrome/browser/extensions/...
chrome/browser/extensions/extension_proxy_apitest.cc:80: // Tests setting to use
the systems's proxy settings.
typo -> system's

http://codereview.chromium.org/5701003/diff/142001/chrome/browser/prefs/comma...
File chrome/browser/prefs/command_line_pref_store.cc (right):

http://codereview.chromium.org/5701003/diff/142001/chrome/browser/prefs/comma...
chrome/browser/prefs/command_line_pref_store.cc:76: void
CommandLinePrefStore::ApplyProxyMode() {
Naming again. If the pref is kProxyServerMode, this should be
ApplyProxyServerMode. Or the other way around.

[battre (please use the other), 2010-12-20 12:57:23]

This is continued in http://codereview.chromium.org/6004003/.

http://codereview.chromium.org/5701003/diff/142001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_proxy_api.cc (right):

http://codereview.chromium.org/5701003/diff/142001/chrome/browser/extensions/...
chrome/browser/extensions/extension_proxy_api.cc:89: LOG(WARNING) << "Invalid
mode for proxy settings: " << mode << std::endl;
On 2010/12/18 08:29:55, Pam wrote:
> Is the endl needed? I don't recall seeing it in other log messages, and I'd
> expect the macro to handle it.

Done.

http://codereview.chromium.org/5701003/diff/142001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_proxy_apitest.cc (right):

http://codereview.chromium.org/5701003/diff/142001/chrome/browser/extensions/...
chrome/browser/extensions/extension_proxy_apitest.cc:26:
pref_service->FindPreference(prefs::kProxyServerMode);
On 2010/12/18 08:29:55, Pam wrote:
> Not to quibble, but I don't understand the naming here. How is "use system
proxy
> settings" or "auto-detect settings" described by "ProxyServerMode"? It sounds
> like the browser is itself acting as a proxy server. Would
"kProxySettingsMode"
> or just "kProxyMode" work?
> 
> Not obligatory, just something to consider.

Done.

I discussed this with Danno. We agree that kProxyMode sounds good. I have
replaced this and kPolicyProxySettingsMode constants. Unfortunately, we cannot
get rid of the "ProxyServerMode" literal that has been published in the policy
API. Therefore, a few occurrences of "ProxyServerMode" remain.

http://codereview.chromium.org/5701003/diff/142001/chrome/browser/extensions/...
chrome/browser/extensions/extension_proxy_apitest.cc:80: // Tests setting to use
the systems's proxy settings.
On 2010/12/18 08:29:55, Pam wrote:
> typo -> system's

Done.

http://codereview.chromium.org/5701003/diff/142001/chrome/browser/prefs/comma...
File chrome/browser/prefs/command_line_pref_store.cc (right):

http://codereview.chromium.org/5701003/diff/142001/chrome/browser/prefs/comma...
chrome/browser/prefs/command_line_pref_store.cc:76: void
CommandLinePrefStore::ApplyProxyMode() {
On 2010/12/18 08:29:55, Pam wrote:
> Naming again. If the pref is kProxyServerMode, this should be
> ApplyProxyServerMode. Or the other way around.

This remains ApplyProxyMode after renaming kProxyServerMode to kProxyMode.

[Mattias Nissler (ping if slow), 2010-12-20 13:34:02]

a bunch of comments.

http://codereview.chromium.org/5701003/diff/142001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_proxy_api.cc (right):

http://codereview.chromium.org/5701003/diff/142001/chrome/browser/extensions/...
chrome/browser/extensions/extension_proxy_api.cc:89: LOG(WARNING) << "Invalid
mode for proxy settings: " << mode << std::endl;
On 2010/12/18 08:29:55, Pam wrote:
> Is the endl needed? I don't recall seeing it in other log messages, and I'd
> expect the macro to handle it.

No, it gets appended automatically. Please remove.

http://codereview.chromium.org/5701003/diff/142001/chrome/browser/extensions/...
chrome/browser/extensions/extension_proxy_api.cc:92:
SendNotification(prefs::kProxyServerMode,
I think it's time to rename SendNotification() to ApplyPreference() or
something. It's not sending notifications for a while now after all ;)

http://codereview.chromium.org/5701003/diff/142001/chrome/browser/net/pref_pr...
File chrome/browser/net/pref_proxy_config_service.cc (right):

http://codereview.chromium.org/5701003/diff/142001/chrome/browser/net/pref_pr...
chrome/browser/net/pref_proxy_config_service.cc:123: }
Is this case branch correct? I'd expect the kProxyServer pref to get ignored if
I set the mode to auto detect. The bypass list should be applied to both though.

http://codereview.chromium.org/5701003/diff/142001/chrome/browser/net/pref_pr...
chrome/browser/net/pref_proxy_config_service.cc:126: NOTREACHED() << "Unknown
proxy mode.";
Maybe add ", falling back to system settings"?

http://codereview.chromium.org/5701003/diff/142001/chrome/browser/net/pref_pr...
File chrome/browser/net/pref_proxy_config_service_unittest.cc (right):

http://codereview.chromium.org/5701003/diff/142001/chrome/browser/net/pref_pr...
chrome/browser/net/pref_proxy_config_service_unittest.cc:170: // The above does
not trigger a notification, because PreProxyConfig still
s/PreProxyConfig/PrefProxyConfig/

http://codereview.chromium.org/5701003/diff/142001/chrome/browser/net/pref_pr...
chrome/browser/net/pref_proxy_config_service_unittest.cc:174: // completed.
s/completed/resolved/? Also below. I'm not really sure though and leave the
decision to our native speakers.

http://codereview.chromium.org/5701003/diff/142001/chrome/browser/net/pref_pr...
chrome/browser/net/pref_proxy_config_service_unittest.cc:177:
Value::CreateIntegerValue(ProxyPrefs::MANUAL));
Don't the parameters fit line after the opening parentheses?

http://codereview.chromium.org/5701003/diff/142001/chrome/browser/net/pref_pr...
chrome/browser/net/pref_proxy_config_service_unittest.cc:194: // The above
swithces the mode the default (ProxyPrefs::SYSTEM), so the next
s/swithces/switches/
s/mode the default/mode to the default/

http://codereview.chromium.org/5701003/diff/142001/chrome/browser/policy/conf...
File chrome/browser/policy/configuration_policy_pref_store.cc (right):

http://codereview.chromium.org/5701003/diff/142001/chrome/browser/policy/conf...
chrome/browser/policy/configuration_policy_pref_store.cc:460: // server mode
that lead to the conflict.
s/lead/led/

http://codereview.chromium.org/5701003/diff/142001/chrome/browser/policy/conf...
chrome/browser/policy/configuration_policy_pref_store.cc:463: std::string
preference_path;
Could use a comment saying what this is used for.

http://codereview.chromium.org/5701003/diff/142001/chrome/browser/policy/conf...
chrome/browser/policy/configuration_policy_pref_store.cc:543: }
Hm, this whole logic for detecting we have a conflict is really cumbersome and
hard to understand. Maybe a two-step approach would be better? I'm thinking
along the lines of applying whatever we get fed in Apply() and then making the
sanity checks and printing the warning after Provide() is done? This would make
it obvious that the code behaves consistently no matter what order we Apply()
policies in.

http://codereview.chromium.org/5701003/diff/142001/chrome/browser/policy/conf...
File chrome/browser/policy/configuration_policy_pref_store_unittest.cc (right):

http://codereview.chromium.org/5701003/diff/142001/chrome/browser/policy/conf...
chrome/browser/policy/configuration_policy_pref_store_unittest.cc:228: // Verify
that all the proxy prefs are set to the speicifed expected values.
s/speicifed/specified/

http://codereview.chromium.org/5701003/diff/142001/chrome/browser/policy/conf...
chrome/browser/policy/configuration_policy_pref_store_unittest.cc:229: void
VerifyEnabledProxyPrefs(
Looks like this could be static?

http://codereview.chromium.org/5701003/diff/142001/chrome/browser/policy/conf...
chrome/browser/policy/configuration_policy_pref_store_unittest.cc:252: void
VerifyDisabledProxyPrefs(
static?

http://codereview.chromium.org/5701003/diff/142001/chrome/browser/policy/conf...
chrome/browser/policy/configuration_policy_pref_store_unittest.cc:266: void
AddProxyPolicies(
static?

http://codereview.chromium.org/5701003/diff/142001/chrome/browser/policy/conf...
File chrome/browser/policy/configuration_policy_store_interface.h (left):

http://codereview.chromium.org/5701003/diff/142001/chrome/browser/policy/conf...
chrome/browser/policy/configuration_policy_store_interface.h:69: static const
int kPolicyNoProxyServerMode = 0;
Why not make these an enum?

http://codereview.chromium.org/5701003/diff/142001/chrome/browser/prefs/comma...
File chrome/browser/prefs/command_line_pref_store.cc (right):

http://codereview.chromium.org/5701003/diff/142001/chrome/browser/prefs/comma...
chrome/browser/prefs/command_line_pref_store.cc:84:
command_line_->HasSwitch(switches::kProxyPacUrl) ||
can we align these with the opening parentheses? (I didn't check the style
guide, but would prefer that alignment for readability)

http://codereview.chromium.org/5701003/diff/142001/chrome/browser/prefs/comma...
chrome/browser/prefs/command_line_pref_store.cc:89: // Mode defaults to system.
What's that comment good for? Could also be overridden by policy. No pref store
should make any assumptions on what pref values apply if no value is set, which
the comment seems to imply.

http://codereview.chromium.org/5701003/diff/142001/chrome/browser/prefs/comma...
File chrome/browser/prefs/command_line_pref_store_unittest.cc (right):

http://codereview.chromium.org/5701003/diff/142001/chrome/browser/prefs/comma...
chrome/browser/prefs/command_line_pref_store_unittest.cc:145:
store2.VerifyIntPref(prefs::kProxyServerMode, ProxyPrefs::MANUAL);
What's the difference between 1 and 2?

http://codereview.chromium.org/5701003/diff/142001/chrome/browser/prefs/proxy...
File chrome/browser/prefs/proxy_prefs.cc (right):

http://codereview.chromium.org/5701003/diff/142001/chrome/browser/prefs/proxy...
chrome/browser/prefs/proxy_prefs.cc:25: ProxyPrefs::ProxyServerMode* out_value)
{
Do these fit on the opening parentheses line?

http://codereview.chromium.org/5701003/diff/142001/chrome/browser/prefs/proxy...
chrome/browser/prefs/proxy_prefs.cc:37: ProxyPrefs::ProxyServerMode* out_value)
{
same here?

http://codereview.chromium.org/5701003/diff/142001/chrome/browser/prefs/proxy...
File chrome/browser/prefs/proxy_prefs.h (right):

http://codereview.chromium.org/5701003/diff/142001/chrome/browser/prefs/proxy...
chrome/browser/prefs/proxy_prefs.h:11: class ProxyPrefs {
Make it a namespace?

http://codereview.chromium.org/5701003/diff/142001/chrome/browser/prefs/proxy...
File chrome/browser/prefs/proxy_prefs_unittest.cc (right):

http://codereview.chromium.org/5701003/diff/142001/chrome/browser/prefs/proxy...
chrome/browser/prefs/proxy_prefs_unittest.cc:16: TEST_F(ProxyPrefsTest,
IntValuesFixed) {
Hm. Isn't this overkill?

[battre (please use the other), 2010-12-21 14:18:17]

I have addressed the issues in http://codereview.chromium.org/6004003/

http://codereview.chromium.org/5701003/diff/142001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_proxy_api.cc (right):

http://codereview.chromium.org/5701003/diff/142001/chrome/browser/extensions/...
chrome/browser/extensions/extension_proxy_api.cc:89: LOG(WARNING) << "Invalid
mode for proxy settings: " << mode << std::endl;
On 2010/12/20 13:34:03, Mattias Nissler wrote:
> On 2010/12/18 08:29:55, Pam wrote:
> > Is the endl needed? I don't recall seeing it in other log messages, and I'd
> > expect the macro to handle it.
> 
> No, it gets appended automatically. Please remove.

Done.

http://codereview.chromium.org/5701003/diff/142001/chrome/browser/extensions/...
chrome/browser/extensions/extension_proxy_api.cc:92:
SendNotification(prefs::kProxyServerMode,
On 2010/12/20 13:34:03, Mattias Nissler wrote:
> I think it's time to rename SendNotification() to ApplyPreference() or
> something. It's not sending notifications for a while now after all ;)

Done.

http://codereview.chromium.org/5701003/diff/142001/chrome/browser/net/pref_pr...
File chrome/browser/net/pref_proxy_config_service.cc (right):

http://codereview.chromium.org/5701003/diff/142001/chrome/browser/net/pref_pr...
chrome/browser/net/pref_proxy_config_service.cc:126: NOTREACHED() << "Unknown
proxy mode.";
On 2010/12/20 13:34:03, Mattias Nissler wrote:
> Maybe add ", falling back to system settings"?

Done.

http://codereview.chromium.org/5701003/diff/142001/chrome/browser/net/pref_pr...
File chrome/browser/net/pref_proxy_config_service_unittest.cc (right):

http://codereview.chromium.org/5701003/diff/142001/chrome/browser/net/pref_pr...
chrome/browser/net/pref_proxy_config_service_unittest.cc:170: // The above does
not trigger a notification, because PreProxyConfig still
On 2010/12/20 13:34:03, Mattias Nissler wrote:
> s/PreProxyConfig/PrefProxyConfig/

Done.

http://codereview.chromium.org/5701003/diff/142001/chrome/browser/net/pref_pr...
chrome/browser/net/pref_proxy_config_service_unittest.cc:174: // completed.
On 2010/12/20 13:34:03, Mattias Nissler wrote:
> s/completed/resolved/? Also below. I'm not really sure though and leave the
> decision to our native speakers.

Done.

http://codereview.chromium.org/5701003/diff/142001/chrome/browser/net/pref_pr...
chrome/browser/net/pref_proxy_config_service_unittest.cc:177:
Value::CreateIntegerValue(ProxyPrefs::MANUAL));
On 2010/12/20 13:34:03, Mattias Nissler wrote:
> Don't the parameters fit line after the opening parentheses?

No, but formated nicely, now.

http://codereview.chromium.org/5701003/diff/142001/chrome/browser/net/pref_pr...
chrome/browser/net/pref_proxy_config_service_unittest.cc:194: // The above
swithces the mode the default (ProxyPrefs::SYSTEM), so the next
On 2010/12/20 13:34:03, Mattias Nissler wrote:
> s/swithces/switches/
> s/mode the default/mode to the default/

Done.

http://codereview.chromium.org/5701003/diff/142001/chrome/browser/policy/conf...
File chrome/browser/policy/configuration_policy_pref_store.cc (right):

http://codereview.chromium.org/5701003/diff/142001/chrome/browser/policy/conf...
chrome/browser/policy/configuration_policy_pref_store.cc:460: // server mode
that lead to the conflict.
On 2010/12/20 13:34:03, Mattias Nissler wrote:
> s/lead/led/

Gone.

http://codereview.chromium.org/5701003/diff/142001/chrome/browser/policy/conf...
chrome/browser/policy/configuration_policy_pref_store.cc:463: std::string
preference_path;
On 2010/12/20 13:34:03, Mattias Nissler wrote:
> Could use a comment saying what this is used for.

Gone.

http://codereview.chromium.org/5701003/diff/142001/chrome/browser/policy/conf...
chrome/browser/policy/configuration_policy_pref_store.cc:543: }
On 2010/12/20 13:34:03, Mattias Nissler wrote:
> Hm, this whole logic for detecting we have a conflict is really cumbersome and
> hard to understand. Maybe a two-step approach would be better? I'm thinking
> along the lines of applying whatever we get fed in Apply() and then making the
> sanity checks and printing the warning after Provide() is done? This would
make
> it obvious that the code behaves consistently no matter what order we Apply()
> policies in.

I thought the same and implemented a Completed() method that is called by
Provide() at the end.

Done.

http://codereview.chromium.org/5701003/diff/142001/chrome/browser/policy/conf...
File chrome/browser/policy/configuration_policy_pref_store_unittest.cc (right):

http://codereview.chromium.org/5701003/diff/142001/chrome/browser/policy/conf...
chrome/browser/policy/configuration_policy_pref_store_unittest.cc:228: // Verify
that all the proxy prefs are set to the speicifed expected values.
On 2010/12/20 13:34:03, Mattias Nissler wrote:
> s/speicifed/specified/

Done.

http://codereview.chromium.org/5701003/diff/142001/chrome/browser/policy/conf...
chrome/browser/policy/configuration_policy_pref_store_unittest.cc:229: void
VerifyEnabledProxyPrefs(
On 2010/12/20 13:34:03, Mattias Nissler wrote:
> Looks like this could be static?

Done.

http://codereview.chromium.org/5701003/diff/142001/chrome/browser/policy/conf...
chrome/browser/policy/configuration_policy_pref_store_unittest.cc:252: void
VerifyDisabledProxyPrefs(
On 2010/12/20 13:34:03, Mattias Nissler wrote:
> static?

Done.

http://codereview.chromium.org/5701003/diff/142001/chrome/browser/policy/conf...
chrome/browser/policy/configuration_policy_pref_store_unittest.cc:266: void
AddProxyPolicies(
On 2010/12/20 13:34:03, Mattias Nissler wrote:
> static?

Done.

http://codereview.chromium.org/5701003/diff/142001/chrome/browser/policy/conf...
File chrome/browser/policy/configuration_policy_store_interface.h (left):

http://codereview.chromium.org/5701003/diff/142001/chrome/browser/policy/conf...
chrome/browser/policy/configuration_policy_store_interface.h:69: static const
int kPolicyNoProxyServerMode = 0;
I reintroduced these as an enum. As the internal representation is now more fine
grained, we need these constants again.

Done.

http://codereview.chromium.org/5701003/diff/142001/chrome/browser/prefs/comma...
File chrome/browser/prefs/command_line_pref_store.cc (right):

http://codereview.chromium.org/5701003/diff/142001/chrome/browser/prefs/comma...
chrome/browser/prefs/command_line_pref_store.cc:84:
command_line_->HasSwitch(switches::kProxyPacUrl) ||
On 2010/12/20 13:34:03, Mattias Nissler wrote:
> can we align these with the opening parentheses? (I didn't check the style
> guide, but would prefer that alignment for readability)

Yes we can.

Done.

http://codereview.chromium.org/5701003/diff/142001/chrome/browser/prefs/comma...
chrome/browser/prefs/command_line_pref_store.cc:89: // Mode defaults to system.
On 2010/12/20 13:34:03, Mattias Nissler wrote:
> What's that comment good for? Could also be overridden by policy. No pref
store
> should make any assumptions on what pref values apply if no value is set,
which
> the comment seems to imply.

I deleted it.

http://codereview.chromium.org/5701003/diff/142001/chrome/browser/prefs/comma...
File chrome/browser/prefs/command_line_pref_store_unittest.cc (right):

http://codereview.chromium.org/5701003/diff/142001/chrome/browser/prefs/comma...
chrome/browser/prefs/command_line_pref_store_unittest.cc:145:
store2.VerifyIntPref(prefs::kProxyServerMode, ProxyPrefs::MANUAL);
On 2010/12/20 13:34:03, Mattias Nissler wrote:
> What's the difference between 1 and 2?

Deleted.

http://codereview.chromium.org/5701003/diff/142001/chrome/browser/prefs/proxy...
File chrome/browser/prefs/proxy_prefs.cc (right):

http://codereview.chromium.org/5701003/diff/142001/chrome/browser/prefs/proxy...
chrome/browser/prefs/proxy_prefs.cc:25: ProxyPrefs::ProxyServerMode* out_value)
{
On 2010/12/20 13:34:03, Mattias Nissler wrote:
> Do these fit on the opening parentheses line?

Done.

http://codereview.chromium.org/5701003/diff/142001/chrome/browser/prefs/proxy...
chrome/browser/prefs/proxy_prefs.cc:37: ProxyPrefs::ProxyServerMode* out_value)
{
On 2010/12/20 13:34:03, Mattias Nissler wrote:
> same here?

Done.

http://codereview.chromium.org/5701003/diff/142001/chrome/browser/prefs/proxy...
File chrome/browser/prefs/proxy_prefs.h (right):

http://codereview.chromium.org/5701003/diff/142001/chrome/browser/prefs/proxy...
chrome/browser/prefs/proxy_prefs.h:11: class ProxyPrefs {
On 2010/12/20 13:34:03, Mattias Nissler wrote:
> Make it a namespace?

Done.

http://codereview.chromium.org/5701003/diff/142001/chrome/browser/prefs/proxy...
File chrome/browser/prefs/proxy_prefs_unittest.cc (right):

http://codereview.chromium.org/5701003/diff/142001/chrome/browser/prefs/proxy...
chrome/browser/prefs/proxy_prefs_unittest.cc:16: TEST_F(ProxyPrefsTest,
IntValuesFixed) {
On 2010/12/20 13:34:03, Mattias Nissler wrote:
> Hm. Isn't this overkill?

Made these assertions for the last tests.

[eroman, 2010-12-21 20:50:58]

Apologies for no comments earlier; was on vacation

(Unfortunatley codereview doesn't have a mechanism for vacation auto-responder).

I'll send my comments to battre's continuation of this CL.