Introduce a separate preference for 'proxy server mode'

Introduce 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. 

This is a continuation of gfeher's patch from issue 5701003.

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

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=70042

[battre, 2010-12-20 12:55:59]

This is a continuation of http://codereview.chromium.org/5701003/. I have taken
over as Gabor is on vacation.

I have addressed Pam's comments.

[battre, 2010-12-21 14:21:01]

Changes for http://codereview.chromium.org/5701003/

[Mattias Nissler (ping if slow), 2010-12-21 15:54:28]

A couple of comments.

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

http://codereview.chromium.org/6004003/diff/5002/chrome/browser/policy/config...
chrome/browser/policy/configuration_policy_pref_store.cc:450: // correct and
apply them in that case.
Why can't we just write to the real dictionary instead of the containers and
sanitize everything later?

http://codereview.chromium.org/6004003/diff/5002/chrome/browser/policy/config...
chrome/browser/policy/configuration_policy_pref_store.cc:477: LOG(ERROR) <<
"Ignoring proxy policies because a policy was defined twice.";
This is not what we want. The idea is that you can call Apply() from within
Provide() as often as you want. The state should only be examined and
error-checked once Provide() returns.

http://codereview.chromium.org/6004003/diff/5002/chrome/browser/policy/config...
chrome/browser/policy/configuration_policy_pref_store.cc:486: void
ConfigurationPolicyPrefStore::Completed() {
Note that for default search, we just call FinalizeDefaultSearchSettings() after
the provide call. I think that's the better solution, since clients should not
need to worry about calling Completed() at the right times.

http://codereview.chromium.org/6004003/diff/5002/chrome/browser/policy/config...
File chrome/browser/policy/configuration_policy_store_interface.h (right):

http://codereview.chromium.org/6004003/diff/5002/chrome/browser/policy/config...
chrome/browser/policy/configuration_policy_store_interface.h:72: //
ProxyPrefs::ProxyMode for legacy reasons.
You should also explain what these legacy reasons are.

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

http://codereview.chromium.org/6004003/diff/5002/chrome/browser/prefs/command...
chrome/browser/prefs/command_line_pref_store_unittest.cc:32:
EXPECT_EQ(expected_value, int_result);
You can also write the previous 3 lines as

EXPECT_TRUE(FundamentalValue(expected_value).Equals(actual));

http://codereview.chromium.org/6004003/diff/5002/chrome/browser/prefs/command...
chrome/browser/prefs/command_line_pref_store_unittest.cc:65:
EXPECT_EQ(ProxyPrefs::DISABLED, result);
same here, or use the function you introduced.

http://codereview.chromium.org/6004003/diff/5002/chrome/browser/prefs/proxy_p...
File chrome/browser/prefs/proxy_prefs.h (right):

http://codereview.chromium.org/6004003/diff/5002/chrome/browser/prefs/proxy_p...
chrome/browser/prefs/proxy_prefs.h:18: //
configuration_policy_store_interface.h.
The last 3 lines of the comment feel like they really belong in
configuration_policy_store_interface and not here.

http://codereview.chromium.org/6004003/diff/5002/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

http://codereview.chromium.org/6004003/diff/5002/chrome/common/pref_names.cc#...
chrome/common/pref_names.cc:1130: const char kProxyMode[] = "proxy.mode";
Do we need conversion code for migrating legacy prefs?

[danno, 2010-12-21 16:16:38]

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

http://codereview.chromium.org/6004003/diff/5002/chrome/browser/policy/config...
chrome/browser/policy/configuration_policy_pref_store.cc:453: case
kPolicyProxyMode:
Indenting is not right here:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Loops_and_Swit...

http://codereview.chromium.org/6004003/diff/5002/chrome/browser/policy/config...
chrome/browser/policy/configuration_policy_pref_store.cc:486: void
ConfigurationPolicyPrefStore::Completed() {
I am not sure I agree. I like that there's a method the provider must call when
it's done providing to finish. FinalizeDefaultSearchPolicySettings should
actually be called from in here.

On 2010/12/21 15:54:29, Mattias Nissler wrote:
> Note that for default search, we just call FinalizeDefaultSearchSettings()
after
> the provide call. I think that's the better solution, since clients should not
> need to worry about calling Completed() at the right times.

http://codereview.chromium.org/6004003/diff/5002/chrome/browser/policy/config...
File chrome/browser/policy/configuration_policy_pref_store.h (right):

http://codereview.chromium.org/6004003/diff/5002/chrome/browser/policy/config...
chrome/browser/policy/configuration_policy_pref_store.h:43: virtual void
Completed();
Where are the tests to make sure that completed actually gets called by all of
the providers that have been implemented?

http://codereview.chromium.org/6004003/diff/5002/chrome/browser/policy/config...
File chrome/browser/policy/configuration_policy_provider.cc (right):

http://codereview.chromium.org/6004003/diff/5002/chrome/browser/policy/config...
chrome/browser/policy/configuration_policy_provider.cc:40: store->Completed();
I don't know if this is the right place to put this. It is currently called by
many of the providers, but I am not sure we should rely on it/hard wire it in
here.

[Mattias Nissler (ping if slow), 2010-12-21 16:22:41]

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

http://codereview.chromium.org/6004003/diff/5002/chrome/browser/policy/config...
chrome/browser/policy/configuration_policy_pref_store.cc:486: void
ConfigurationPolicyPrefStore::Completed() {
My rationale for opposing the idea is this: We'd add a function to an interface
declaration for the sole benefit of one implementor, namely
ConfigurationPolicyPrefStore, even if this implementor technically doesn't
require the extended interface. Thus, we'd make life harder for clients for no
good reason. I don't like the sprinkling of Completed() over the code much,
either.

On 2010/12/21 16:16:38, danno wrote:
> I am not sure I agree. I like that there's a method the provider must call
when
> it's done providing to finish. FinalizeDefaultSearchPolicySettings should
> actually be called from in here.
> 
> On 2010/12/21 15:54:29, Mattias Nissler wrote:
> > Note that for default search, we just call FinalizeDefaultSearchSettings()
> after
> > the provide call. I think that's the better solution, since clients should
not
> > need to worry about calling Completed() at the right times.
>

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

Addressed the comments. Thank you.

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

http://codereview.chromium.org/6004003/diff/5002/chrome/browser/policy/config...
chrome/browser/policy/configuration_policy_pref_store.cc:450: // correct and
apply them in that case.
On 2010/12/21 15:54:29, Mattias Nissler wrote:
> Why can't we just write to the real dictionary instead of the containers and
> sanitize everything later?

This would mean that we need some rollback mechanism. Besides this, we want an
atomic commit soon anyway (upcoming CL). There we want to replace the
configuration as one dictionary at once.

http://codereview.chromium.org/6004003/diff/5002/chrome/browser/policy/config...
chrome/browser/policy/configuration_policy_pref_store.cc:453: case
kPolicyProxyMode:
On 2010/12/21 16:16:38, danno wrote:
> Indenting is not right here:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Loops_and_Swit...

Done.

http://codereview.chromium.org/6004003/diff/5002/chrome/browser/policy/config...
chrome/browser/policy/configuration_policy_pref_store.cc:477: LOG(ERROR) <<
"Ignoring proxy policies because a policy was defined twice.";
On 2010/12/21 15:54:29, Mattias Nissler wrote:
> This is not what we want. The idea is that you can call Apply() from within
> Provide() as often as you want. The state should only be examined and
> error-checked once Provide() returns.

Done.

http://codereview.chromium.org/6004003/diff/5002/chrome/browser/policy/config...
chrome/browser/policy/configuration_policy_pref_store.cc:486: void
ConfigurationPolicyPrefStore::Completed() {
On 2010/12/21 16:22:41, Mattias Nissler wrote:
> My rationale for opposing the idea is this: We'd add a function to an
interface
> declaration for the sole benefit of one implementor, namely
> ConfigurationPolicyPrefStore, even if this implementor technically doesn't
> require the extended interface. Thus, we'd make life harder for clients for no
> good reason. I don't like the sprinkling of Completed() over the code much,
> either.
> 
> On 2010/12/21 16:16:38, danno wrote:
> > I am not sure I agree. I like that there's a method the provider must call
> when
> > it's done providing to finish. FinalizeDefaultSearchPolicySettings should
> > actually be called from in here.
> > 
> > On 2010/12/21 15:54:29, Mattias Nissler wrote:
> > > Note that for default search, we just call FinalizeDefaultSearchSettings()
> > after
> > > the provide call. I think that's the better solution, since clients should
> not
> > > need to worry about calling Completed() at the right times.
> > 
> 

I checked the code again and agree with Mattias. His proposal is quite elegant
and ConfigurationPolicyPrefStore::FinalizeDefaultSearchPolicySettings does
pretty much the same thing.

Done.

http://codereview.chromium.org/6004003/diff/5002/chrome/browser/policy/config...
File chrome/browser/policy/configuration_policy_pref_store.h (right):

http://codereview.chromium.org/6004003/diff/5002/chrome/browser/policy/config...
chrome/browser/policy/configuration_policy_pref_store.h:43: virtual void
Completed();
On 2010/12/21 16:16:38, danno wrote:
> Where are the tests to make sure that completed actually gets called by all of
> the providers that have been implemented?

I have deleted this.

http://codereview.chromium.org/6004003/diff/5002/chrome/browser/policy/config...
File chrome/browser/policy/configuration_policy_provider.cc (right):

http://codereview.chromium.org/6004003/diff/5002/chrome/browser/policy/config...
chrome/browser/policy/configuration_policy_provider.cc:40: store->Completed();
On 2010/12/21 16:16:38, danno wrote:
> I don't know if this is the right place to put this. It is currently called by
> many of the providers, but I am not sure we should rely on it/hard wire it in
> here.

Gone.

http://codereview.chromium.org/6004003/diff/5002/chrome/browser/policy/config...
File chrome/browser/policy/configuration_policy_store_interface.h (right):

http://codereview.chromium.org/6004003/diff/5002/chrome/browser/policy/config...
chrome/browser/policy/configuration_policy_store_interface.h:72: //
ProxyPrefs::ProxyMode for legacy reasons.
On 2010/12/21 15:54:29, Mattias Nissler wrote:
> You should also explain what these legacy reasons are.

Done.

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

http://codereview.chromium.org/6004003/diff/5002/chrome/browser/prefs/command...
chrome/browser/prefs/command_line_pref_store_unittest.cc:32:
EXPECT_EQ(expected_value, int_result);
On 2010/12/21 15:54:29, Mattias Nissler wrote:
> You can also write the previous 3 lines as
> 
> EXPECT_TRUE(FundamentalValue(expected_value).Equals(actual));

Done.

http://codereview.chromium.org/6004003/diff/5002/chrome/browser/prefs/command...
chrome/browser/prefs/command_line_pref_store_unittest.cc:65:
EXPECT_EQ(ProxyPrefs::DISABLED, result);
On 2010/12/21 15:54:29, Mattias Nissler wrote:
> same here, or use the function you introduced.

Done.

http://codereview.chromium.org/6004003/diff/5002/chrome/browser/prefs/proxy_p...
File chrome/browser/prefs/proxy_prefs.h (right):

http://codereview.chromium.org/6004003/diff/5002/chrome/browser/prefs/proxy_p...
chrome/browser/prefs/proxy_prefs.h:18: //
configuration_policy_store_interface.h.
On 2010/12/21 15:54:29, Mattias Nissler wrote:
> The last 3 lines of the comment feel like they really belong in
> configuration_policy_store_interface and not here.

They both cross reference each other. I think it is good having this information
in both places.

http://codereview.chromium.org/6004003/diff/5002/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

http://codereview.chromium.org/6004003/diff/5002/chrome/common/pref_names.cc#...
chrome/common/pref_names.cc:1130: const char kProxyMode[] = "proxy.mode";
On 2010/12/21 15:54:29, Mattias Nissler wrote:
> Do we need conversion code for migrating legacy prefs?

We have this code in configuration_policy_pref_store.cc. There is no simple 1:1
mapping. The new format is not yet supported by policies. "proxy.mode" is only
used in the user preferences file and in extension pref stores.

[eroman, 2010-12-21 22:55:20]

I did high level review, will defer to the other reviewers on the
preferences/policy details of this change.

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

http://codereview.chromium.org/6004003/diff/5002/chrome/browser/net/pref_prox...
chrome/browser/net/pref_proxy_config_service.cc:110:
config->set_pac_url(GURL(proxy_pac));
You may want to check that the URL was valid as well, in-case it is something
weird.

http://codereview.chromium.org/6004003/diff/5002/chrome/browser/net/pref_prox...
chrome/browser/net/pref_proxy_config_service.cc:112: } else {
nit: omit "else" when previous clause returns.

http://codereview.chromium.org/6004003/diff/5002/chrome/browser/net/pref_prox...
chrome/browser/net/pref_proxy_config_service.cc:121:
config->proxy_rules().ParseFromString(proxy_server);
I don't think it is a good idea to rely on the internal string format (which is
not guaranteed stable).

How about splitting these out into separate preferences? i.e. have a
kProxyForHttpUrls, kProxyForFtpUrls, kProxyForHttpsUrls, kProxyForAllSchemes
(this latter is where you would insert socks proxies).

Similarly, the extension API should be specifying these in a structured format
rather than as a string (it could just pass a dictionary from scheme --> proxy
server).

http://codereview.chromium.org/6004003/diff/5002/chrome/browser/net/pref_prox...
chrome/browser/net/pref_proxy_config_service.cc:130:
pref_service_->GetString(prefs::kProxyBypassList);
Can this be stored as a ValueList instead?

http://codereview.chromium.org/6004003/diff/5002/chrome/browser/prefs/proxy_p...
File chrome/browser/prefs/proxy_prefs.cc (right):

http://codereview.chromium.org/6004003/diff/5002/chrome/browser/prefs/proxy_p...
chrome/browser/prefs/proxy_prefs.cc:11: // These names are exposed to the proxy
extension API. They must be in sync
nit: don't indent within the namespace.

http://codereview.chromium.org/6004003/diff/5002/chrome/browser/prefs/proxy_p...
File chrome/browser/prefs/proxy_prefs.h (right):

http://codereview.chromium.org/6004003/diff/5002/chrome/browser/prefs/proxy_p...
chrome/browser/prefs/proxy_prefs.h:19: enum ProxyMode {
nit: I suggest prefixing all of these constants by MODE_*

http://codereview.chromium.org/6004003/diff/5002/chrome/browser/prefs/proxy_p...
chrome/browser/prefs/proxy_prefs.h:21: DISABLED = 0,
I suggest calling this DIRECT rather than DISABLED.

http://codereview.chromium.org/6004003/diff/5002/chrome/browser/prefs/proxy_p...
chrome/browser/prefs/proxy_prefs.h:32: FIXED_SERVERS = 3,
See my comment about storing the proxy servers for each scheme as a separate
preference value.

If you do that, then it would be natural to split this into two modes:

  (1) A mode for using a single proxy for everything (would look to
kProxyForAllSchemes).

  (2) A mode to use specific proxy server based on URL. Would use either
kProxyForHttpUrl, kProxyForHttpsUrl, kProxyForFtpUrl, or kProxyForAllSchemes.
(The latter would be used as a "fallback" when the more appropriate url-specific
proxy preference was not specified, and would be the way for specifying SOCKS
proxies).

This maps pretty directly to how the preferences are stored internally by
net::ProxyConfig

[battre, 2010-12-22 10:01:27]

I have addressed Eric's comments. Thanks!

I have created two cleanup bugs because they are somewhat orthogonal to this CL
and require bigger changes:
http://code.google.com/p/chromium/issues/detail?id=67778
http://code.google.com/p/chromium/issues/detail?id=67779

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

http://codereview.chromium.org/6004003/diff/5002/chrome/browser/net/pref_prox...
chrome/browser/net/pref_proxy_config_service.cc:110:
config->set_pac_url(GURL(proxy_pac));
On 2010/12/21 22:55:21, eroman wrote:
> You may want to check that the URL was valid as well, in-case it is something
> weird.

Done.

http://codereview.chromium.org/6004003/diff/5002/chrome/browser/net/pref_prox...
chrome/browser/net/pref_proxy_config_service.cc:112: } else {
On 2010/12/21 22:55:21, eroman wrote:
> nit: omit "else" when previous clause returns.

Done.

http://codereview.chromium.org/6004003/diff/5002/chrome/browser/net/pref_prox...
chrome/browser/net/pref_proxy_config_service.cc:121:
config->proxy_rules().ParseFromString(proxy_server);
On 2010/12/21 22:55:21, eroman wrote:
> I don't think it is a good idea to rely on the internal string format (which
is
> not guaranteed stable).
> 
> How about splitting these out into separate preferences? i.e. have a
> kProxyForHttpUrls, kProxyForFtpUrls, kProxyForHttpsUrls, kProxyForAllSchemes
> (this latter is where you would insert socks proxies).
> 
> Similarly, the extension API should be specifying these in a structured format
> rather than as a string (it could just pass a dictionary from scheme --> proxy
> server).

Created http://crbug.com/67779

http://codereview.chromium.org/6004003/diff/5002/chrome/browser/net/pref_prox...
chrome/browser/net/pref_proxy_config_service.cc:130:
pref_service_->GetString(prefs::kProxyBypassList);
On 2010/12/21 22:55:21, eroman wrote:
> Can this be stored as a ValueList instead?

Created http://crbug.com/67778

http://codereview.chromium.org/6004003/diff/5002/chrome/browser/prefs/proxy_p...
File chrome/browser/prefs/proxy_prefs.cc (right):

http://codereview.chromium.org/6004003/diff/5002/chrome/browser/prefs/proxy_p...
chrome/browser/prefs/proxy_prefs.cc:11: // These names are exposed to the proxy
extension API. They must be in sync
On 2010/12/21 22:55:21, eroman wrote:
> nit: don't indent within the namespace.

Done.

http://codereview.chromium.org/6004003/diff/5002/chrome/browser/prefs/proxy_p...
File chrome/browser/prefs/proxy_prefs.h (right):

http://codereview.chromium.org/6004003/diff/5002/chrome/browser/prefs/proxy_p...
chrome/browser/prefs/proxy_prefs.h:19: enum ProxyMode {
On 2010/12/21 22:55:21, eroman wrote:
> nit: I suggest prefixing all of these constants by MODE_*

Done.

http://codereview.chromium.org/6004003/diff/5002/chrome/browser/prefs/proxy_p...
chrome/browser/prefs/proxy_prefs.h:21: DISABLED = 0,
On 2010/12/21 22:55:21, eroman wrote:
> I suggest calling this DIRECT rather than DISABLED.

Done.

http://codereview.chromium.org/6004003/diff/5002/chrome/browser/prefs/proxy_p...
chrome/browser/prefs/proxy_prefs.h:32: FIXED_SERVERS = 3,
On 2010/12/21 22:55:21, eroman wrote:
> See my comment about storing the proxy servers for each scheme as a separate
> preference value.
> 
> If you do that, then it would be natural to split this into two modes:
> 
>   (1) A mode for using a single proxy for everything (would look to
> kProxyForAllSchemes).
> 
>   (2) A mode to use specific proxy server based on URL. Would use either
> kProxyForHttpUrl, kProxyForHttpsUrl, kProxyForFtpUrl, or kProxyForAllSchemes.
> (The latter would be used as a "fallback" when the more appropriate
url-specific
> proxy preference was not specified, and would be the way for specifying SOCKS
> proxies).
> 
> This maps pretty directly to how the preferences are stored internally by
> net::ProxyConfig

Noted in http://crbug.com/67779

[Mattias Nissler (ping if slow), 2010-12-22 10:22:11]

Some more comments. Sorry they are sprinkled over the last three revisions you
uploaded, but I thought I'd switch to the most recent version once I got my
hands on it :)

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

http://codereview.chromium.org/6004003/diff/5002/chrome/browser/policy/config...
chrome/browser/policy/configuration_policy_pref_store.cc:450: // correct and
apply them in that case.
On 2010/12/21 20:14:09, battre (please use the other) wrote:
> On 2010/12/21 15:54:29, Mattias Nissler wrote:
> > Why can't we just write to the real dictionary instead of the containers and
> > sanitize everything later?
> 
> This would mean that we need some rollback mechanism. Besides this, we want an
> atomic commit soon anyway (upcoming CL). There we want to replace the
> configuration as one dictionary at once.

The rollback mechanism is delete-all-proxy-prefs-from-the-map. This is what's
already done for default search providers in
FinalizeDefaultSearchProviderSettings() and I feel we should be consistent, no?

http://codereview.chromium.org/6004003/diff/5002/chrome/browser/policy/config...
chrome/browser/policy/configuration_policy_pref_store.cc:477: LOG(ERROR) <<
"Ignoring proxy policies because a policy was defined twice.";
On 2010/12/21 20:14:09, battre (please use the other) wrote:
> On 2010/12/21 15:54:29, Mattias Nissler wrote:
> > This is not what we want. The idea is that you can call Apply() from within
> > Provide() as often as you want. The state should only be examined and
> > error-checked once Provide() returns.
> 
> Done.

Not done?

http://codereview.chromium.org/6004003/diff/5002/chrome/browser/prefs/proxy_p...
File chrome/browser/prefs/proxy_prefs.h (right):

http://codereview.chromium.org/6004003/diff/5002/chrome/browser/prefs/proxy_p...
chrome/browser/prefs/proxy_prefs.h:18: //
configuration_policy_store_interface.h.
On 2010/12/21 20:14:09, battre (please use the other) wrote:
> On 2010/12/21 15:54:29, Mattias Nissler wrote:
> > The last 3 lines of the comment feel like they really belong in
> > configuration_policy_store_interface and not here.
> 
> They both cross reference each other. I think it is good having this
information
> in both places.

Yeah, but the pref layer should not depend on what policy is doing, so I feel
the comment doesn't belong here. I don't have strong feelings about this though.

http://codereview.chromium.org/6004003/diff/5002/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

http://codereview.chromium.org/6004003/diff/5002/chrome/common/pref_names.cc#...
chrome/common/pref_names.cc:1130: const char kProxyMode[] = "proxy.mode";
On 2010/12/21 20:14:09, battre (please use the other) wrote:
> On 2010/12/21 15:54:29, Mattias Nissler wrote:
> > Do we need conversion code for migrating legacy prefs?
> 
> We have this code in configuration_policy_pref_store.cc. There is no simple
1:1
> mapping. The new format is not yet supported by policies. "proxy.mode" is only
> used in the user preferences file and in extension pref stores.

What I meant is whether we need to care about possible existing Preferences
files that users may have. If somebody has set up their proxy by putting
appropriate configuration in the prefs file, we break them. The question is
whether that's acceptable or not, in which case we need to provide migration
code.

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

http://codereview.chromium.org/6004003/diff/36002/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_service.cc:116: }
Can't we just do the return true after the conditional?

http://codereview.chromium.org/6004003/diff/36002/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_service.cc:135: NOTREACHED() << "Unknown
proxy mode, falling back to system settings.";
Drop the default branch and move the code after the switch. That way, the
compiler warns if somebody adds an enum value but forgets to modify the code
here.

http://codereview.chromium.org/6004003/diff/36002/chrome/browser/net/pref_pro...
File chrome/browser/net/pref_proxy_config_service_unittest.cc (right):

http://codereview.chromium.org/6004003/diff/36002/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_service_unittest.cc:370: "Pac URL",
I think when reading the code, we came to the conclusion that the bypass list is
ignored if you set a PAC URL, right? The old code here suggests they are not
ignored, can you verify please?

http://codereview.chromium.org/6004003/diff/65001/chrome/browser/policy/confi...
File chrome/browser/policy/configuration_policy_store_interface.h (right):

http://codereview.chromium.org/6004003/diff/65001/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_store_interface.h:88: NUM_PROXY_MODES
why use a CAPS_CONSTANT if all others use kCameCaseConstants?

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

http://codereview.chromium.org/6004003/diff/65001/chrome/browser/prefs/proxy_...
chrome/browser/prefs/proxy_prefs.h:39: NUM_MODES
Shouldn't this also be prefixed by MODE_? Like MODE_COUNT or something? Style
guide says we should use kConstantNaming for new enums.

http://codereview.chromium.org/6004003/diff/65001/chrome/common/extensions/do...
File chrome/common/extensions/docs/experimental.proxy.html (right):

http://codereview.chromium.org/6004003/diff/65001/chrome/common/extensions/do...
chrome/common/extensions/docs/experimental.proxy.html:1403: <dd>'disabled' =
Never use a proxy<br>'auto_detect' = Auto detect proxy settings<br>'pac_script'
= Use specified PAC script<br>'fixed_servers' = Manually specify proxy
servers<br>'system' = Use system proxy settings</dd>
should we rename the strings here as well, i.e. disabled->direct so they agree
with what we have in the code after you made the naming changes eroman
suggested?

http://codereview.chromium.org/6004003/diff/65001/chrome/common/policy_consta...
File chrome/common/policy_constants.cc (right):

http://codereview.chromium.org/6004003/diff/65001/chrome/common/policy_consta...
chrome/common/policy_constants.cc:36: // not change it after publishing the
policy API.
It is not the API, but the naming in the win registry and policy config files.

[battre, 2010-12-22 14:41:16]

Thank you for the comments. I have addressed them in the new CL.

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

http://codereview.chromium.org/6004003/diff/5002/chrome/browser/policy/config...
chrome/browser/policy/configuration_policy_pref_store.cc:450: // correct and
apply them in that case.
On 2010/12/22 10:22:11, Mattias Nissler wrote:
> On 2010/12/21 20:14:09, battre (please use the other) wrote:
> > On 2010/12/21 15:54:29, Mattias Nissler wrote:
> > > Why can't we just write to the real dictionary instead of the containers
and
> > > sanitize everything later?
> > 
> > This would mean that we need some rollback mechanism. Besides this, we want
an
> > atomic commit soon anyway (upcoming CL). There we want to replace the
> > configuration as one dictionary at once.
> 
> The rollback mechanism is delete-all-proxy-prefs-from-the-map. This is what's
> already done for default search providers in
> FinalizeDefaultSearchProviderSettings() and I feel we should be consistent,
no?

Well, the point is that eventually we want a single
prefs_->Set("proxy", proxy_dictionary)-alike call to set all proxy settings
atomically. I feel that the current code is closer to this than a
delete-all-proxy-prefs-from-the-map.

http://codereview.chromium.org/6004003/diff/5002/chrome/browser/policy/config...
chrome/browser/policy/configuration_policy_pref_store.cc:477: LOG(ERROR) <<
"Ignoring proxy policies because a policy was defined twice.";
On 2010/12/22 10:22:11, Mattias Nissler wrote:
> On 2010/12/21 20:14:09, battre (please use the other) wrote:
> > On 2010/12/21 15:54:29, Mattias Nissler wrote:
> > > This is not what we want. The idea is that you can call Apply() from
within
> > > Provide() as often as you want. The state should only be examined and
> > > error-checked once Provide() returns.
> > 
> > Done.
> 
> Not done?

Sorry. I deleted some other code related to this but forgot the main location.
Not it is really done. :-)

http://codereview.chromium.org/6004003/diff/5002/chrome/browser/prefs/proxy_p...
File chrome/browser/prefs/proxy_prefs.h (right):

http://codereview.chromium.org/6004003/diff/5002/chrome/browser/prefs/proxy_p...
chrome/browser/prefs/proxy_prefs.h:18: //
configuration_policy_store_interface.h.
On 2010/12/22 10:22:11, Mattias Nissler wrote:
> On 2010/12/21 20:14:09, battre (please use the other) wrote:
> > On 2010/12/21 15:54:29, Mattias Nissler wrote:
> > > The last 3 lines of the comment feel like they really belong in
> > > configuration_policy_store_interface and not here.
> > 
> > They both cross reference each other. I think it is good having this
> information
> > in both places.
> 
> Yeah, but the pref layer should not depend on what policy is doing, so I feel
> the comment doesn't belong here. I don't have strong feelings about this
though.

I agree.

Done.

http://codereview.chromium.org/6004003/diff/5002/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

http://codereview.chromium.org/6004003/diff/5002/chrome/common/pref_names.cc#...
chrome/common/pref_names.cc:1130: const char kProxyMode[] = "proxy.mode";
On 2010/12/22 10:22:11, Mattias Nissler wrote:
> On 2010/12/21 20:14:09, battre (please use the other) wrote:
> > On 2010/12/21 15:54:29, Mattias Nissler wrote:
> > > Do we need conversion code for migrating legacy prefs?
> > 
> > We have this code in configuration_policy_pref_store.cc. There is no simple
> 1:1
> > mapping. The new format is not yet supported by policies. "proxy.mode" is
only
> > used in the user preferences file and in extension pref stores.
> 
> What I meant is whether we need to care about possible existing Preferences
> files that users may have. If somebody has set up their proxy by putting
> appropriate configuration in the prefs file, we break them. The question is
> whether that's acceptable or not, in which case we need to provide migration
> code.

If a user has set no kProxyMode constant because it was not introduced before,
Chrome falls back to system proxy prefs (registered default). I think this is
reasonable, given that this feature has not seen much exposure, yet.

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

http://codereview.chromium.org/6004003/diff/36002/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_service.cc:116: }
On 2010/12/22 10:22:12, Mattias Nissler wrote:
> Can't we just do the return true after the conditional?

I added a new condition (pac_url valid) and changed this to be more linear (no
if/else) in order to reduce the nesting.

So it is not applicable any more.

http://codereview.chromium.org/6004003/diff/36002/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_service.cc:135: NOTREACHED() << "Unknown
proxy mode, falling back to system settings.";
On 2010/12/22 10:22:12, Mattias Nissler wrote:
> Drop the default branch and move the code after the switch. That way, the
> compiler warns if somebody adds an enum value but forgets to modify the code
> here.

Done.

http://codereview.chromium.org/6004003/diff/36002/chrome/browser/net/pref_pro...
File chrome/browser/net/pref_proxy_config_service_unittest.cc (right):

http://codereview.chromium.org/6004003/diff/36002/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_service_unittest.cc:370: "Pac URL",
On 2010/12/22 10:22:12, Mattias Nissler wrote:
> I think when reading the code, we came to the conclusion that the bypass list
is
> ignored if you set a PAC URL, right? The old code here suggests they are not
> ignored, can you verify please?

Yes, verified. See TEST(ProxyServiceTest, BypassDoesntApplyToPac) in
proxy_service_unittest.cc

http://codereview.chromium.org/6004003/diff/65001/chrome/browser/policy/confi...
File chrome/browser/policy/configuration_policy_store_interface.h (right):

http://codereview.chromium.org/6004003/diff/65001/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_store_interface.h:88: NUM_PROXY_MODES
On 2010/12/22 10:22:12, Mattias Nissler wrote:
> why use a CAPS_CONSTANT if all others use kCameCaseConstants?

Done.

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

http://codereview.chromium.org/6004003/diff/65001/chrome/browser/prefs/proxy_...
chrome/browser/prefs/proxy_prefs.h:39: NUM_MODES
On 2010/12/22 10:22:12, Mattias Nissler wrote:
> Shouldn't this also be prefixed by MODE_? Like MODE_COUNT or something? Style
> guide says we should use kConstantNaming for new enums.

Done.

http://codereview.chromium.org/6004003/diff/65001/chrome/common/extensions/do...
File chrome/common/extensions/docs/experimental.proxy.html (right):

http://codereview.chromium.org/6004003/diff/65001/chrome/common/extensions/do...
chrome/common/extensions/docs/experimental.proxy.html:1403: <dd>'disabled' =
Never use a proxy<br>'auto_detect' = Auto detect proxy settings<br>'pac_script'
= Use specified PAC script<br>'fixed_servers' = Manually specify proxy
servers<br>'system' = Use system proxy settings</dd>
On 2010/12/22 10:22:12, Mattias Nissler wrote:
> should we rename the strings here as well, i.e. disabled->direct so they agree
> with what we have in the code after you made the naming changes eroman
> suggested?

Done.

http://codereview.chromium.org/6004003/diff/65001/chrome/common/policy_consta...
File chrome/common/policy_constants.cc (right):

http://codereview.chromium.org/6004003/diff/65001/chrome/common/policy_consta...
chrome/common/policy_constants.cc:36: // not change it after publishing the
policy API.
On 2010/12/22 10:22:12, Mattias Nissler wrote:
> It is not the API, but the naming in the win registry and policy config files.

Done.

[Mattias Nissler (ping if slow), 2010-12-22 15:26:14]

getting closer.

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

http://codereview.chromium.org/6004003/diff/5002/chrome/browser/policy/config...
chrome/browser/policy/configuration_policy_pref_store.cc:450: // correct and
apply them in that case.
OK, but if you want to keep the evaluate-later approach, please use a
std::map<ConfigurationPolicyType, Value*> for caching the values instead of the
tedious switching here.

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

http://codereview.chromium.org/6004003/diff/58002/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_service.cc:140: // Fall through to
NOTREACHED().
Indentation, no need for the branch curlies.

[battre, 2010-12-22 16:15:12]

Thanks.

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

http://codereview.chromium.org/6004003/diff/5002/chrome/browser/policy/config...
chrome/browser/policy/configuration_policy_pref_store.cc:450: // correct and
apply them in that case.
On 2010/12/22 15:26:15, Mattias Nissler wrote:
> OK, but if you want to keep the evaluate-later approach, please use a
> std::map<ConfigurationPolicyType, Value*> for caching the values instead of
the
> tedious switching here.

Done.

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

http://codereview.chromium.org/6004003/diff/58002/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_service.cc:140: // Fall through to
NOTREACHED().
On 2010/12/22 15:26:15, Mattias Nissler wrote:
> Indentation, no need for the branch curlies.

Fixed indentation.

We need the curlies. Without them, we get the following compiler error:
chrome/browser/net/pref_proxy_config_service.cc: In member function ‘bool
PrefProxyConfigTracker::ReadPrefConfig(net::ProxyConfig*)’:
chrome/browser/net/pref_proxy_config_service.cc:141: error: expected
primary-expression before ‘}’ token
chrome/browser/net/pref_proxy_config_service.cc:141: error: expected ‘;’ before
‘}’ token

Putting just an ; in an empty line is invalid according to the linter.

[Mattias Nissler (ping if slow), 2010-12-22 16:23:38]

One little nit left. LGTM on my side pending the fix and trybot happyness.

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

http://codereview.chromium.org/6004003/diff/58002/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_service.cc:140: // Fall through to
NOTREACHED().
On 2010/12/22 16:15:12, battre wrote:
> On 2010/12/22 15:26:15, Mattias Nissler wrote:
> > Indentation, no need for the branch curlies.
> 
> Fixed indentation.
> 
> We need the curlies. Without them, we get the following compiler error:
> chrome/browser/net/pref_proxy_config_service.cc: In member function ‘bool
> PrefProxyConfigTracker::ReadPrefConfig(net::ProxyConfig*)’:
> chrome/browser/net/pref_proxy_config_service.cc:141: error: expected
> primary-expression before ‘}’ token
> chrome/browser/net/pref_proxy_config_service.cc:141: error: expected ‘;’
before
> ‘}’ token
> 
> Putting just an ; in an empty line is invalid according to the linter.

Ah, right, we need a statement. OK then.

http://codereview.chromium.org/6004003/diff/64004/chrome/browser/policy/confi...
File chrome/browser/policy/configuration_policy_pref_store.cc (right):

http://codereview.chromium.org/6004003/diff/64004/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_pref_store.cc:569: case
kPolicyNoProxyServerMode:
fix case statement indentation.

[battre, 2010-12-22 16:32:02]

Done, thanks.

http://codereview.chromium.org/6004003/diff/64004/chrome/browser/policy/confi...
File chrome/browser/policy/configuration_policy_pref_store.cc (right):

http://codereview.chromium.org/6004003/diff/64004/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_pref_store.cc:569: case
kPolicyNoProxyServerMode:
On 2010/12/22 16:23:39, Mattias Nissler wrote:
> fix case statement indentation.

Done.

[eroman, 2010-12-23 01:06:14]

LGTM

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

http://codereview.chromium.org/6004003/diff/102001/chrome/browser/net/pref_pr...
chrome/browser/net/pref_proxy_config_service_unittest.cc:194: // The above
switches the mode the default (ProxyPrefs::SYSTEM), so the next
nit: I think this sentence is missing "to".   "switches the mode to the default"

http://codereview.chromium.org/6004003/diff/102001/chrome/browser/policy/conf...
File chrome/browser/policy/configuration_policy_pref_store.h (right):

http://codereview.chromium.org/6004003/diff/102001/chrome/browser/policy/conf...
chrome/browser/policy/configuration_policy_pref_store.h:78: // Temporary cache
that store values until FinalizeProxyPolicySettings()
nit: store --> "stores".

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

http://codereview.chromium.org/6004003/diff/102001/chrome/browser/prefs/proxy...
chrome/browser/prefs/proxy_prefs.h:19: kModeDirect = 0,
Note that Chrome's style is to use UPPER_CASE for enums. (even though this is
different from the official google style guide).

See:

http://www.chromium.org/developers/coding-style

http://codereview.chromium.org/6004003/diff/102001/chrome/browser/prefs/proxy...
chrome/browser/prefs/proxy_prefs.h:43: }  // namespace
nit: "// namespace ProxyPrefs"

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

http://codereview.chromium.org/6004003/diff/102001/chrome/browser/prefs/proxy...
chrome/browser/prefs/proxy_prefs_unittest.cc:13: class ProxyPrefsTest : public
testing::Test {
I don't think it is necessary to define a test fixture here. I believe you can
delete this class.

And then replace the TEST_F() in this file with TEST().

[battre, 2010-12-23 09:41:42]

Thanks a lot for the reviews.

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

http://codereview.chromium.org/6004003/diff/102001/chrome/browser/net/pref_pr...
chrome/browser/net/pref_proxy_config_service_unittest.cc:194: // The above
switches the mode the default (ProxyPrefs::SYSTEM), so the next
On 2010/12/23 01:06:15, eroman wrote:
> nit: I think this sentence is missing "to".   "switches the mode to the
default"

Done.

http://codereview.chromium.org/6004003/diff/102001/chrome/browser/policy/conf...
File chrome/browser/policy/configuration_policy_pref_store.h (right):

http://codereview.chromium.org/6004003/diff/102001/chrome/browser/policy/conf...
chrome/browser/policy/configuration_policy_pref_store.h:78: // Temporary cache
that store values until FinalizeProxyPolicySettings()
On 2010/12/23 01:06:15, eroman wrote:
> nit: store --> "stores".

Done.

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

http://codereview.chromium.org/6004003/diff/102001/chrome/browser/prefs/proxy...
chrome/browser/prefs/proxy_prefs.h:19: kModeDirect = 0,
On 2010/12/23 01:06:15, eroman wrote:
> Note that Chrome's style is to use UPPER_CASE for enums. (even though this is
> different from the official google style guide).
> 
> See:
> 
> http://www.chromium.org/developers/coding-style

Done.

http://codereview.chromium.org/6004003/diff/102001/chrome/browser/prefs/proxy...
chrome/browser/prefs/proxy_prefs.h:43: }  // namespace
On 2010/12/23 01:06:15, eroman wrote:
> nit: "// namespace ProxyPrefs"

Done.

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

http://codereview.chromium.org/6004003/diff/102001/chrome/browser/prefs/proxy...
chrome/browser/prefs/proxy_prefs_unittest.cc:13: class ProxyPrefsTest : public
testing::Test {
On 2010/12/23 01:06:15, eroman wrote:
> I don't think it is necessary to define a test fixture here. I believe you can
> delete this class.
> 
> And then replace the TEST_F() in this file with TEST().

Done.

[danno, 2010-12-23 09:46:48]

lgtm with one nit

http://codereview.chromium.org/6004003/diff/102001/chrome/browser/prefs/pref_...
File chrome/browser/prefs/pref_service_unittest.cc (right):

http://codereview.chromium.org/6004003/diff/102001/chrome/browser/prefs/pref_...
chrome/browser/prefs/pref_service_unittest.cc:14: #include
"chrome/browser/prefs/proxy_prefs.h"
nit: alphabetize