Make CrOS proxy configuration write prefs directly.

Make CrOS proxy configuration write prefs directly.

Change the CrOS ProxyConfigServiceImpl code to write to local state
prefs instead of implementing ProxyConfigService. This has the advantage
of getting recommended policy to work out of the box and also lets us
reuse the IO thread switching dance from PrefProxyConfigService.

BUG=chromium-os:11262, chromium-os:11530
TEST=compiles and passes tests, recommended proxy policy is honored by in CrOS

[Mattias Nissler (ping if slow), 2011-02-21 18:12:57]

Please review: kuan for the CrOS proxy service modifications, danno for the
pref-related part. Others FYI and please let me know if you see any issues.

Side notes:

1/ I found that we have 3 different key-based proxy configuration formats:
regular PrefService prefs (used in chrome), pref keys for the proxy UI (used for
CrOS) and another DictionaryValue based format for serialization and storing to
SignedSettings (in ProxyConfigServiceImpl). Can we simplify this?

2/ I think eventually we could also probably get rid of the remaining parts of
ProxyConfigServiceImpl and hook up the UI support code in
ProxyCrosSettingsProvider with the local_state prefs directly.

Let me know what you think, since I might not be aware of all aspects in play
here. That work would be part of follow-up CLs anyway.

[danno, 2011-02-21 21:45:20]

I like it, seem to clean things up considerably.

a few questions...

http://codereview.chromium.org/6549007/diff/3001/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/login_utils.cc (right):

http://codereview.chromium.org/6549007/diff/3001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/login_utils.cc:241: #if 0
I assume this is because the CL is still a work in progress?

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

http://codereview.chromium.org/6549007/diff/3001/chrome/browser/net/pref_prox...
chrome/browser/net/pref_proxy_config_service.cc:114: !pref->HasUserSetting()) {
We discussed a strategy offline that was more like this:
1) Use user managed policy
2) if not set, use device managed policy
3) if not set, use user extension policy 
4) if not set, use what user has selected in the setting page (local state)
5) if not set, use user recommended policy (or maybe not?)
6) if not set, use device recommended policy
... or something to that effect. This code seems to implement something
different, more like:
1) Use user managed policy
2) if not set use user extension policy
3) if not set, use what user has selected in the setting page (local state)
4) use use recommended policy
can you explain why? will the other changes come in a later CL?

[Mattias Nissler (ping if slow), 2011-02-22 09:45:34]

Answer Danno's comments.

http://codereview.chromium.org/6549007/diff/3001/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/login_utils.cc (right):

http://codereview.chromium.org/6549007/diff/3001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/login_utils.cc:241: #if 0
On 2011/02/21 21:45:20, danno wrote:
> I assume this is because the CL is still a work in progress?

Ah, yes. I think I should remove this block. This will leave stuff that goes
through the default request context unaffected by user policy. Since we have
device policy now that affects everything, I think this is the way to go.

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

http://codereview.chromium.org/6549007/diff/3001/chrome/browser/net/pref_prox...
chrome/browser/net/pref_proxy_config_service.cc:114: !pref->HasUserSetting()) {
On 2011/02/21 21:45:20, danno wrote:
> We discussed a strategy offline that was more like this:
> 1) Use user managed policy
> 2) if not set, use device managed policy
> 3) if not set, use user extension policy 
> 4) if not set, use what user has selected in the setting page (local state)
> 5) if not set, use user recommended policy (or maybe not?)
> 6) if not set, use device recommended policy
> ... or something to that effect. This code seems to implement something
> different, more like:
> 1) Use user managed policy
> 2) if not set use user extension policy
> 3) if not set, use what user has selected in the setting page (local state)
> 4) use use recommended policy
> can you explain why? will the other changes come in a later CL?

I pondered this for quite a while. I came to the conclusion that for policy,
extensions, etc. it's probably better to not have device override user or vice
versa in any way. That is so these stakeholders get more flexibility on how the
overriding should work. In the case of policy, we want to make that decision at
the policy provider level (i.e. fall back to device proxy policy if there's no
user proxy policy or the like).

Things are a bit different for user settings though. We could decide not to do
the fallback here, but that would mean that when writing proxy prefs, we'd have
to put them not only into local state but the PrefServices of all Profiles we
know of. Even worse: If a profile is created later, we'd have to seed the proxy
settings there. That seemed a bad design choice to me, so I decided to go with
the idea of defaulting proxy user prefs (and only that layer in the pref store
hierarchy) to what's local state. This is what the code implements.

In a later CL, I indeed plan to modify our cloud policy providers so device
proxy policy and user policy are merged for the profile proxy configuration.

[battre, 2011-02-22 10:32:40]

A few minor comments.

http://codereview.chromium.org/6549007/diff/3001/chrome/browser/chromeos/prox...
File chrome/browser/chromeos/proxy_config_service_impl.cc (right):

http://codereview.chromium.org/6549007/diff/3001/chrome/browser/chromeos/prox...
chrome/browser/chromeos/proxy_config_service_impl.cc:453: void
ProxyConfigServiceImpl::UIGetProxyConfig(ProxyConfig* config) {
Any reason for removing the checks? The method is public.

http://codereview.chromium.org/6549007/diff/3001/chrome/browser/chromeos/prox...
File chrome/browser/chromeos/proxy_config_service_impl.h (right):

http://codereview.chromium.org/6549007/diff/3001/chrome/browser/chromeos/prox...
chrome/browser/chromeos/proxy_config_service_impl.h:33: class
ProxyConfigServiceImpl
should this be called ProxyConfigService now?

http://codereview.chromium.org/6549007/diff/3001/chrome/browser/chromeos/prox...
chrome/browser/chromeos/proxy_config_service_impl.h:40: // as a scoped_refptr
(because it's RefCountedThreadSafe).
ProxyConfigServiceImpl is not RefCountedThreadSafe anymore

http://codereview.chromium.org/6549007/diff/3001/chrome/browser/chromeos/prox...
chrome/browser/chromeos/proxy_config_service_impl.h:107: // system. Ownership is
transferred to the caller
nit: . at the end.

http://codereview.chromium.org/6549007/diff/3001/chrome/browser/chromeos/prox...
chrome/browser/chromeos/proxy_config_service_impl.h:159: // Helper function for
building up the manual proxy spec. A declration of
nit: declaration

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

http://codereview.chromium.org/6549007/diff/3001/chrome/browser/net/pref_prox...
chrome/browser/net/pref_proxy_config_service.cc:27: // Also install an observer
on |fallback|, if appropriate.
nit: |fallback_pref_service_|

[Mattias Nissler (ping if slow), 2011-02-22 11:08:02]

Answer comments.

http://codereview.chromium.org/6549007/diff/3001/chrome/browser/chromeos/prox...
File chrome/browser/chromeos/proxy_config_service_impl.cc (right):

http://codereview.chromium.org/6549007/diff/3001/chrome/browser/chromeos/prox...
chrome/browser/chromeos/proxy_config_service_impl.cc:453: void
ProxyConfigServiceImpl::UIGetProxyConfig(ProxyConfig* config) {
On 2011/02/22 10:32:40, battre wrote:
> Any reason for removing the checks? The method is public.

The class doesn't claim to be thread safe any longer, so the default applies:
you cannot expected it to be thread safe. Classes that are not purpose-build for
the Chrome threading infrastructure should be assumed to be non-thread safe and
usable on any (single) thread you want them on, so the checks don't make sense
any longer.

http://codereview.chromium.org/6549007/diff/3001/chrome/browser/chromeos/prox...
File chrome/browser/chromeos/proxy_config_service_impl.h (right):

http://codereview.chromium.org/6549007/diff/3001/chrome/browser/chromeos/prox...
chrome/browser/chromeos/proxy_config_service_impl.h:33: class
ProxyConfigServiceImpl
On 2011/02/22 10:32:40, battre wrote:
> should this be called ProxyConfigService now.

We could change the name. Maybe ChromeOSProxyConfigService? WDYT?

http://codereview.chromium.org/6549007/diff/3001/chrome/browser/chromeos/prox...
chrome/browser/chromeos/proxy_config_service_impl.h:40: // as a scoped_refptr
(because it's RefCountedThreadSafe).
On 2011/02/22 10:32:40, battre wrote:
> ProxyConfigServiceImpl is not RefCountedThreadSafe anymore

Done.

http://codereview.chromium.org/6549007/diff/3001/chrome/browser/chromeos/prox...
chrome/browser/chromeos/proxy_config_service_impl.h:107: // system. Ownership is
transferred to the caller
On 2011/02/22 10:32:40, battre wrote:
> nit: . at the end.

Done.

http://codereview.chromium.org/6549007/diff/3001/chrome/browser/chromeos/prox...
chrome/browser/chromeos/proxy_config_service_impl.h:159: // Helper function for
building up the manual proxy spec. A declration of
On 2011/02/22 10:32:40, battre wrote:
> nit: declaration

Done.

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

http://codereview.chromium.org/6549007/diff/3001/chrome/browser/net/pref_prox...
chrome/browser/net/pref_proxy_config_service.cc:27: // Also install an observer
on |fallback|, if appropriate.
On 2011/02/22 10:32:40, battre wrote:
> nit: |fallback_pref_service_|

Done.

[kuan, 2011-02-22 22:20:25]

http://codereview.chromium.org/6549007/diff/4020/chrome/browser/chromeos/prox...
File chrome/browser/chromeos/proxy_config_service_impl.h (right):

http://codereview.chromium.org/6549007/diff/4020/chrome/browser/chromeos/prox...
chrome/browser/chromeos/proxy_config_service_impl.h:38: //
Profile::GetChromeOSProxyConfigServiceImpl. It is accessed from
i'm sure u're aware of battre's cl that places ProxyConfigServiceImpl in browser
process, so this comment will need to change after.

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

http://codereview.chromium.org/6549007/diff/4020/chrome/browser/net/pref_prox...
chrome/browser/net/pref_proxy_config_service.h:91: // A fallback PrefSerice that
is checked for user preferences if
nit: typo: PrefService

http://codereview.chromium.org/6549007/diff/4020/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_io_data.cc (right):

http://codereview.chromium.org/6549007/diff/4020/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_io_data.cc:278: #endif  // defined(OS_CHROMEOS)
chromeos has no base_service? this would mean that the very initial config used
for PrefProxyConfigService must be same as that retrieved in
ProxyCrosSettingsProvider::GetConfigService.  the former uses net::ProxyConfig
in the prefs, whose default is DIRECT connection, but the latter uses
AUTO_DETECT for default.  will this cause the ui and backend to differ? 

nit: if it turns out i'm wrong, the comment "// defined" shld be "// !defined"

[Mattias Nissler (ping if slow), 2011-03-01 15:19:20]

Here's my answers for the record. Note that for various reasons (mostly
initialization order), I'll probably go for a different approach, which is in
http://codereview.chromium.org/6597070/, so closing this issue.

http://codereview.chromium.org/6549007/diff/4020/chrome/browser/chromeos/prox...
File chrome/browser/chromeos/proxy_config_service_impl.h (right):

http://codereview.chromium.org/6549007/diff/4020/chrome/browser/chromeos/prox...
chrome/browser/chromeos/proxy_config_service_impl.h:38: //
Profile::GetChromeOSProxyConfigServiceImpl. It is accessed from
On 2011/02/22 22:20:25, kuan wrote:
> i'm sure u're aware of battre's cl that places ProxyConfigServiceImpl in
browser
> process, so this comment will need to change after.

Yes, he's actually sitting 2 desks away from me, and we've already synced.

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

http://codereview.chromium.org/6549007/diff/4020/chrome/browser/net/pref_prox...
chrome/browser/net/pref_proxy_config_service.h:91: // A fallback PrefSerice that
is checked for user preferences if
On 2011/02/22 22:20:25, kuan wrote:
> nit: typo: PrefService

Done.

http://codereview.chromium.org/6549007/diff/4020/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_io_data.cc (right):

http://codereview.chromium.org/6549007/diff/4020/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_io_data.cc:278: #endif  // defined(OS_CHROMEOS)
On 2011/02/22 22:20:25, kuan wrote:
> chromeos has no base_service? this would mean that the very initial config
used
> for PrefProxyConfigService must be same as that retrieved in
> ProxyCrosSettingsProvider::GetConfigService.  the former uses net::ProxyConfig
> in the prefs, whose default is DIRECT connection, but the latter uses
> AUTO_DETECT for default.  will this cause the ui and backend to differ? 
> 
> nit: if it turns out i'm wrong, the comment "// defined" shld be "// !defined"

It's intentional that ChromeOS now doesn't have base service any longer, since
it'll feed the configuration directly via local_state prefs.

That wrong default in PrefProxyConfigService was really a mistake on my side, I
originally intended to put the gold default, which is auto configuration.