Allow ProxyConfigService to report "no configuration set"

Allow ProxyConfigService to report "no configuration set"

Introduce a ConfigAvailability enum such that ProxyConfigService is able to return configuration status at a finer granularity level. This allows to fall back to default values (potentially configured through policy) if the system service doesn't have configuration.

BUG=none
TEST=unit tests, recommended proxy policy works on CrOS.

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

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

Eric,

here is a stab at the CL I chatted with you about last week. Can you please take
a look?

I went down the path of implementing a SystemProxyConfigService interface and
using that only for the platform-specific implementations of ProxyConfigService
in order to keep the additional complexity out of net/proxy_service.cc. However,
it seems that separating the "no value set" logic out into a separate class
seems not to buy us much (see the CL) so I'm wondering if we're better off after
all with just making the change in ProxyConfigService?

Note that I also changed the win and linux proxy config service implementations
to return "unset" when there's an error fetching the actual configuration.
Before, they had their own defaulting to "direct", with the new code we'll fall
back to auto configuration. Would that be OK with you? Is there some specific
reasons for the "direct" default-on-error that's implemented now?

[eroman, 2011-03-04 05:25:08]

Hi Mattias,

Thanks for experimenting with that interface change per our earlier discussion.

On seeing the code, I agree with your assessment that it doesn't really work
well.

I am in favor of just baking the functionality directly into ProxyConfigService
interface per your earlier suggestion.

My main comment on the interface, is to keep the |const ProxyConfig&| as part of
OnProxyConfigChanged(). I think this is more readable than the implicit contract
that consumers can call GetLatestProxyConfig(), and it is guaranteed to not
return CONFIG_PENDING.

So you might just need to add the ConfigAvailability parameter to both
locations:

// Availability is guaranteed to not be CONFIG_PENDING. config is not applicable
in the case of CONFIG_UNSET.
ProxyConfigService::OnProxyConfigChanged(const ProxyConfig& config,
ConfigAvailability availability) = 0;

Alsi I suggest keeping the name ProxyConfigService::GetLatestConfig(), but just
changing the bool to ConfigAvailability.

Sorry to make you do the extra work!

By the way, I am on vacation Friday and Monday. I will try to look at updates on
this code review, but I may not be as responsive (although it is not like I am
super responsive even when not on vacation :)

Cheers.

Also: For non-trivial changes to the ChromeOS proxy config service, please cc
kuan.

[Mattias Nissler (ping if slow), 2011-03-10 19:57:00]

Eric, thanks for your input. I'm sorry to make progress on this only slowly
(busy with other stuff). I agree with your comments and I've uploaded an updated
version that has the changes you propose. Please take another look.

Kuan: Can you have a look at the changes to ProxyConfigServiceImpl? Thanks.

On 2011/03/04 05:25:08, eroman wrote:
> Hi Mattias,
> 
> Thanks for experimenting with that interface change per our earlier
discussion.
> 
> On seeing the code, I agree with your assessment that it doesn't really work
> well.
> 
> I am in favor of just baking the functionality directly into
ProxyConfigService
> interface per your earlier suggestion.
> 
> My main comment on the interface, is to keep the |const ProxyConfig&| as part
of
> OnProxyConfigChanged(). I think this is more readable than the implicit
contract
> that consumers can call GetLatestProxyConfig(), and it is guaranteed to not
> return CONFIG_PENDING.
> 
> So you might just need to add the ConfigAvailability parameter to both
> locations:
> 
> // Availability is guaranteed to not be CONFIG_PENDING. config is not
applicable
> in the case of CONFIG_UNSET.
> ProxyConfigService::OnProxyConfigChanged(const ProxyConfig& config,
> ConfigAvailability availability) = 0;
> 
> Alsi I suggest keeping the name ProxyConfigService::GetLatestConfig(), but
just
> changing the bool to ConfigAvailability.
> 
> Sorry to make you do the extra work!
> 
> By the way, I am on vacation Friday and Monday. I will try to look at updates
on
> this code review, but I may not be as responsive (although it is not like I am
> super responsive even when not on vacation :)
> 
> Cheers.
> 
> Also: For non-trivial changes to the ChromeOS proxy config service, please cc
> kuan.

[kuan, 2011-03-10 22:59:04]

i looked at the chromeos/proxy_config*.* files; just 1 nit, so lgtm.

http://codereview.chromium.org/6597070/diff/19002/chrome/browser/chromeos/pro...
File chrome/browser/chromeos/proxy_config_service_impl.cc (right):

http://codereview.chromium.org/6597070/diff/19002/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.cc:423: config_availability_ =
net::ProxyConfigService::CONFIG_VALID;
why not initialize this in initializer list?

[Mattias Nissler (ping if slow), 2011-03-15 12:38:32]

Eric: Ping

http://codereview.chromium.org/6597070/diff/19002/chrome/browser/chromeos/pro...
File chrome/browser/chromeos/proxy_config_service_impl.cc (right):

http://codereview.chromium.org/6597070/diff/19002/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.cc:423: config_availability_ =
net::ProxyConfigService::CONFIG_VALID;
On 2011/03/10 22:59:05, kuan wrote:
> why not initialize this in initializer list?

Done.

[eroman, 2011-03-16 01:29:59]

Hey I'm back :)

Sorry for the delay, been getting p0wned by top crashes on M11.

http://codereview.chromium.org/6597070/diff/26001/net/proxy/polling_proxy_con...
File net/proxy/polling_proxy_config_service.cc (right):

http://codereview.chromium.org/6597070/diff/26001/net/proxy/polling_proxy_con...
net/proxy/polling_proxy_config_service.cc:100:
ProxyConfigService::ConfigAvailability available = func(&config);
Please add a DCHECK/CHECK that |available| != CONFIG_PENDING.

http://codereview.chromium.org/6597070/diff/26001/net/proxy/polling_proxy_con...
File net/proxy/polling_proxy_config_service.h (right):

http://codereview.chromium.org/6597070/diff/26001/net/proxy/polling_proxy_con...
net/proxy/polling_proxy_config_service.h:32: typedef
ProxyConfigService::ConfigAvailability
I don't think we should change this. We don't support anything except
synchronous success from this anyway (just used by tests to substitute the
windows calls).

http://codereview.chromium.org/6597070/diff/26001/net/proxy/proxy_config_serv...
File net/proxy/proxy_config_service.h (right):

http://codereview.chromium.org/6597070/diff/26001/net/proxy/proxy_config_serv...
net/proxy/proxy_config_service.h:22: // No configuration is set, use default.
nit: I suggest removing the "use default" comment (it is up to the caller to
decide what they want to do).

http://codereview.chromium.org/6597070/diff/26001/net/proxy/proxy_config_serv...
net/proxy/proxy_config_service.h:30: virtual void OnProxyConfigChanged(const
ProxyConfig& config,
Could you add a comment explaining that |availability| will NEVER be
CONFIG_PENDING here.

And moreover, that |config| is only interesting when availability ==
CONFIG_VALID.

http://codereview.chromium.org/6597070/diff/26001/net/proxy/proxy_config_serv...
net/proxy/proxy_config_service.h:41: // Gets the most recent value of the proxy
configuration. Returns false if
Please update these comments regarding the return value.

http://codereview.chromium.org/6597070/diff/26001/net/proxy/proxy_config_serv...
File net/proxy/proxy_config_service_linux.cc (left):

http://codereview.chromium.org/6597070/diff/26001/net/proxy/proxy_config_serv...
net/proxy/proxy_config_service_linux.cc:963: return false;
This seems like it will change the behavior (before it would return false, now
you will return true on invalid URLs). Are you sure about this?

If you would like to change how this works please propose that in a separate
changelist.

http://codereview.chromium.org/6597070/diff/26001/net/proxy/proxy_config_serv...
File net/proxy/proxy_config_service_linux.cc (right):

http://codereview.chromium.org/6597070/diff/26001/net/proxy/proxy_config_serv...
net/proxy/proxy_config_service_linux.cc:1113: // cached_config_, where
GetLastestProxyConfig() running on the IO thread
typo: please restore earlier version.

http://codereview.chromium.org/6597070/diff/26001/net/proxy/proxy_config_serv...
net/proxy/proxy_config_service_linux.cc:1188: return CONFIG_UNSET;
I don't think the system config services should ever return CONFIG_UNSET.

Makes more sense for me that they return DIRECT, since that is the implicit
fallback scheme they assume.

http://codereview.chromium.org/6597070/diff/26001/net/proxy/proxy_config_serv...
net/proxy/proxy_config_service_linux.cc:1227:
ProxyConfigService::CONFIG_UNSET));
See earlier comment.

http://codereview.chromium.org/6597070/diff/26001/net/proxy/proxy_config_serv...
File net/proxy/proxy_config_service_win.cc (right):

http://codereview.chromium.org/6597070/diff/26001/net/proxy/proxy_config_serv...
net/proxy/proxy_config_service_win.cc:163: return CONFIG_UNSET;
See my previous comment -- i don't think we should ever return UNSET, but should
simply fall-back to direct here. The "system" always has settings, they just
might be weird.

http://codereview.chromium.org/6597070/diff/26001/net/proxy/proxy_service.cc
File net/proxy/proxy_service.cc (right):

http://codereview.chromium.org/6597070/diff/26001/net/proxy/proxy_service.cc#...
net/proxy/proxy_service.cc:835: case ProxyConfigService::CONFIG_PENDING:
I don't think this case makes sense. We should instead DCHECK() that
|availability| != CONFIG_PENDING.

http://codereview.chromium.org/6597070/diff/26001/net/proxy/proxy_service.cc#...
net/proxy/proxy_service.cc:842: effective_config =
ProxyConfig::CreateAutoDetect();
I think we should default to Direct() rather than AutoDetect(). That said, I
wander if it should be valid to reach CONFIG_UNSET here.

CONFIG_UNSET seems like a helper value for creating a chain of services. I don't
think it should not be valid for the leaf nodes to emit this.

(That way the proxyconfigservice is fully in control of the configuration
fallback chain, rather than having the ProxyService decide to use AutoDetect or
something here).

(Also note that the "defaults" are somewhat platform-specific, which is why it
is better to let the system ProxyConfigService make this call. For instance on
Windows it is very common to default to auto-detect so that default would make
sense. However on Linux that would be weird, and you would want to default to
direct).

[Mattias Nissler (ping if slow), 2011-03-16 17:40:00]

Here's an updated version with Eric's comments addressed.

http://codereview.chromium.org/6597070/diff/26001/net/proxy/polling_proxy_con...
File net/proxy/polling_proxy_config_service.cc (right):

http://codereview.chromium.org/6597070/diff/26001/net/proxy/polling_proxy_con...
net/proxy/polling_proxy_config_service.cc:100:
ProxyConfigService::ConfigAvailability available = func(&config);
On 2011/03/16 01:30:00, eroman wrote:
> Please add a DCHECK/CHECK that |available| != CONFIG_PENDING.

Done.

http://codereview.chromium.org/6597070/diff/26001/net/proxy/polling_proxy_con...
File net/proxy/polling_proxy_config_service.h (right):

http://codereview.chromium.org/6597070/diff/26001/net/proxy/polling_proxy_con...
net/proxy/polling_proxy_config_service.h:32: typedef
ProxyConfigService::ConfigAvailability
On 2011/03/16 01:30:00, eroman wrote:
> I don't think we should change this. We don't support anything except
> synchronous success from this anyway (just used by tests to substitute the
> windows calls).

Done.

http://codereview.chromium.org/6597070/diff/26001/net/proxy/proxy_config_serv...
File net/proxy/proxy_config_service.h (right):

http://codereview.chromium.org/6597070/diff/26001/net/proxy/proxy_config_serv...
net/proxy/proxy_config_service.h:22: // No configuration is set, use default.
On 2011/03/16 01:30:00, eroman wrote:
> nit: I suggest removing the "use default" comment (it is up to the caller to
> decide what they want to do).

Done.

http://codereview.chromium.org/6597070/diff/26001/net/proxy/proxy_config_serv...
net/proxy/proxy_config_service.h:30: virtual void OnProxyConfigChanged(const
ProxyConfig& config,
On 2011/03/16 01:30:00, eroman wrote:
> Could you add a comment explaining that |availability| will NEVER be
> CONFIG_PENDING here.
> 
> And moreover, that |config| is only interesting when availability ==
> CONFIG_VALID.

Done.

http://codereview.chromium.org/6597070/diff/26001/net/proxy/proxy_config_serv...
net/proxy/proxy_config_service.h:41: // Gets the most recent value of the proxy
configuration. Returns false if
On 2011/03/16 01:30:00, eroman wrote:
> Please update these comments regarding the return value.

Absolutely! Sorry for missing it.
Done.

http://codereview.chromium.org/6597070/diff/26001/net/proxy/proxy_config_serv...
File net/proxy/proxy_config_service_linux.cc (left):

http://codereview.chromium.org/6597070/diff/26001/net/proxy/proxy_config_serv...
net/proxy/proxy_config_service_linux.cc:963: return false;
On 2011/03/16 01:30:00, eroman wrote:
> This seems like it will change the behavior (before it would return false, now
> you will return true on invalid URLs). Are you sure about this?
> 
> If you would like to change how this works please propose that in a separate
> changelist.

I agree it's not a good idea to change behavior of the system services in this
CL.
Done.

http://codereview.chromium.org/6597070/diff/26001/net/proxy/proxy_config_serv...
File net/proxy/proxy_config_service_linux.cc (right):

http://codereview.chromium.org/6597070/diff/26001/net/proxy/proxy_config_serv...
net/proxy/proxy_config_service_linux.cc:1113: // cached_config_, where
GetLastestProxyConfig() running on the IO thread
On 2011/03/16 01:30:00, eroman wrote:
> typo: please restore earlier version.

Done.

http://codereview.chromium.org/6597070/diff/26001/net/proxy/proxy_config_serv...
net/proxy/proxy_config_service_linux.cc:1188: return CONFIG_UNSET;
On 2011/03/16 01:30:00, eroman wrote:
> I don't think the system config services should ever return CONFIG_UNSET.
> 
> Makes more sense for me that they return DIRECT, since that is the implicit
> fallback scheme they assume.

I did this change for consolidating the defaulting into ProxyConfigService.
However, I don't feel strong about this, we can always change this later if the
need arises, so I reverted this change.
Done.

http://codereview.chromium.org/6597070/diff/26001/net/proxy/proxy_config_serv...
net/proxy/proxy_config_service_linux.cc:1227:
ProxyConfigService::CONFIG_UNSET));
On 2011/03/16 01:30:00, eroman wrote:
> See earlier comment.

Done.

http://codereview.chromium.org/6597070/diff/26001/net/proxy/proxy_config_serv...
File net/proxy/proxy_config_service_win.cc (right):

http://codereview.chromium.org/6597070/diff/26001/net/proxy/proxy_config_serv...
net/proxy/proxy_config_service_win.cc:163: return CONFIG_UNSET;
On 2011/03/16 01:30:00, eroman wrote:
> See my previous comment -- i don't think we should ever return UNSET, but
should
> simply fall-back to direct here. The "system" always has settings, they just
> might be weird.

Done.

http://codereview.chromium.org/6597070/diff/26001/net/proxy/proxy_service.cc
File net/proxy/proxy_service.cc (right):

http://codereview.chromium.org/6597070/diff/26001/net/proxy/proxy_service.cc#...
net/proxy/proxy_service.cc:835: case ProxyConfigService::CONFIG_PENDING:
On 2011/03/16 01:30:00, eroman wrote:
> I don't think this case makes sense. We should instead DCHECK() that
> |availability| != CONFIG_PENDING.

You're right in that it doesn't make sense to send the notification in the first
place. However, there's no harm in handling that condition gracefully. Put a
NOTREACHED().

http://codereview.chromium.org/6597070/diff/26001/net/proxy/proxy_service.cc#...
net/proxy/proxy_service.cc:842: effective_config =
ProxyConfig::CreateAutoDetect();
OK, I agree that for now we should make the system services not return
CONFIG_UNSET. I still think that it makes sense to put a reasonable default
here, since there is no harm in having it and ProxyConfigService implementors
can still decide what's best for them.

[Mattias Nissler (ping if slow), 2011-03-22 17:29:36]

Eric: Ping

On 2011/03/16 17:40:00, Mattias Nissler wrote:
> Here's an updated version with Eric's comments addressed.
> 
>
http://codereview.chromium.org/6597070/diff/26001/net/proxy/polling_proxy_con...
> File net/proxy/polling_proxy_config_service.cc (right):
> 
>
http://codereview.chromium.org/6597070/diff/26001/net/proxy/polling_proxy_con...
> net/proxy/polling_proxy_config_service.cc:100:
> ProxyConfigService::ConfigAvailability available = func(&config);
> On 2011/03/16 01:30:00, eroman wrote:
> > Please add a DCHECK/CHECK that |available| != CONFIG_PENDING.
> 
> Done.
> 
>
http://codereview.chromium.org/6597070/diff/26001/net/proxy/polling_proxy_con...
> File net/proxy/polling_proxy_config_service.h (right):
> 
>
http://codereview.chromium.org/6597070/diff/26001/net/proxy/polling_proxy_con...
> net/proxy/polling_proxy_config_service.h:32: typedef
> ProxyConfigService::ConfigAvailability
> On 2011/03/16 01:30:00, eroman wrote:
> > I don't think we should change this. We don't support anything except
> > synchronous success from this anyway (just used by tests to substitute the
> > windows calls).
> 
> Done.
> 
>
http://codereview.chromium.org/6597070/diff/26001/net/proxy/proxy_config_serv...
> File net/proxy/proxy_config_service.h (right):
> 
>
http://codereview.chromium.org/6597070/diff/26001/net/proxy/proxy_config_serv...
> net/proxy/proxy_config_service.h:22: // No configuration is set, use default.
> On 2011/03/16 01:30:00, eroman wrote:
> > nit: I suggest removing the "use default" comment (it is up to the caller to
> > decide what they want to do).
> 
> Done.
> 
>
http://codereview.chromium.org/6597070/diff/26001/net/proxy/proxy_config_serv...
> net/proxy/proxy_config_service.h:30: virtual void OnProxyConfigChanged(const
> ProxyConfig& config,
> On 2011/03/16 01:30:00, eroman wrote:
> > Could you add a comment explaining that |availability| will NEVER be
> > CONFIG_PENDING here.
> > 
> > And moreover, that |config| is only interesting when availability ==
> > CONFIG_VALID.
> 
> Done.
> 
>
http://codereview.chromium.org/6597070/diff/26001/net/proxy/proxy_config_serv...
> net/proxy/proxy_config_service.h:41: // Gets the most recent value of the
proxy
> configuration. Returns false if
> On 2011/03/16 01:30:00, eroman wrote:
> > Please update these comments regarding the return value.
> 
> Absolutely! Sorry for missing it.
> Done.
> 
>
http://codereview.chromium.org/6597070/diff/26001/net/proxy/proxy_config_serv...
> File net/proxy/proxy_config_service_linux.cc (left):
> 
>
http://codereview.chromium.org/6597070/diff/26001/net/proxy/proxy_config_serv...
> net/proxy/proxy_config_service_linux.cc:963: return false;
> On 2011/03/16 01:30:00, eroman wrote:
> > This seems like it will change the behavior (before it would return false,
now
> > you will return true on invalid URLs). Are you sure about this?
> > 
> > If you would like to change how this works please propose that in a separate
> > changelist.
> 
> I agree it's not a good idea to change behavior of the system services in this
> CL.
> Done.
> 
>
http://codereview.chromium.org/6597070/diff/26001/net/proxy/proxy_config_serv...
> File net/proxy/proxy_config_service_linux.cc (right):
> 
>
http://codereview.chromium.org/6597070/diff/26001/net/proxy/proxy_config_serv...
> net/proxy/proxy_config_service_linux.cc:1113: // cached_config_, where
> GetLastestProxyConfig() running on the IO thread
> On 2011/03/16 01:30:00, eroman wrote:
> > typo: please restore earlier version.
> 
> Done.
> 
>
http://codereview.chromium.org/6597070/diff/26001/net/proxy/proxy_config_serv...
> net/proxy/proxy_config_service_linux.cc:1188: return CONFIG_UNSET;
> On 2011/03/16 01:30:00, eroman wrote:
> > I don't think the system config services should ever return CONFIG_UNSET.
> > 
> > Makes more sense for me that they return DIRECT, since that is the implicit
> > fallback scheme they assume.
> 
> I did this change for consolidating the defaulting into ProxyConfigService.
> However, I don't feel strong about this, we can always change this later if
the
> need arises, so I reverted this change.
> Done.
> 
>
http://codereview.chromium.org/6597070/diff/26001/net/proxy/proxy_config_serv...
> net/proxy/proxy_config_service_linux.cc:1227:
> ProxyConfigService::CONFIG_UNSET));
> On 2011/03/16 01:30:00, eroman wrote:
> > See earlier comment.
> 
> Done.
> 
>
http://codereview.chromium.org/6597070/diff/26001/net/proxy/proxy_config_serv...
> File net/proxy/proxy_config_service_win.cc (right):
> 
>
http://codereview.chromium.org/6597070/diff/26001/net/proxy/proxy_config_serv...
> net/proxy/proxy_config_service_win.cc:163: return CONFIG_UNSET;
> On 2011/03/16 01:30:00, eroman wrote:
> > See my previous comment -- i don't think we should ever return UNSET, but
> should
> > simply fall-back to direct here. The "system" always has settings, they just
> > might be weird.
> 
> Done.
> 
> http://codereview.chromium.org/6597070/diff/26001/net/proxy/proxy_service.cc
> File net/proxy/proxy_service.cc (right):
> 
>
http://codereview.chromium.org/6597070/diff/26001/net/proxy/proxy_service.cc#...
> net/proxy/proxy_service.cc:835: case ProxyConfigService::CONFIG_PENDING:
> On 2011/03/16 01:30:00, eroman wrote:
> > I don't think this case makes sense. We should instead DCHECK() that
> > |availability| != CONFIG_PENDING.
> 
> You're right in that it doesn't make sense to send the notification in the
first
> place. However, there's no harm in handling that condition gracefully. Put a
> NOTREACHED().
> 
>
http://codereview.chromium.org/6597070/diff/26001/net/proxy/proxy_service.cc#...
> net/proxy/proxy_service.cc:842: effective_config =
> ProxyConfig::CreateAutoDetect();
> OK, I agree that for now we should make the system services not return
> CONFIG_UNSET. I still think that it makes sense to put a reasonable default
> here, since there is no harm in having it and ProxyConfigService implementors
> can still decide what's best for them.

[eroman, 2011-03-22 18:40:53]

sorry, was out sick past few days, will take another look today.

[eroman, 2011-03-26 00:50:30]

Did you upload the changes? I am still seeing the same content as before.
Thanks.

http://codereview.chromium.org/6597070/diff/26001/net/proxy/proxy_config_serv...
File net/proxy/proxy_config_service.h (right):

http://codereview.chromium.org/6597070/diff/26001/net/proxy/proxy_config_serv...
net/proxy/proxy_config_service.h:41: // Gets the most recent value of the proxy
configuration. Returns false if
On 2011/03/16 17:40:00, Mattias Nissler wrote:
> On 2011/03/16 01:30:00, eroman wrote:
> > Please update these comments regarding the return value.
> 
> Absolutely! Sorry for missing it.
> Done.

I don't see this change in the latest pathset...

http://codereview.chromium.org/6597070/diff/26001/net/proxy/proxy_config_serv...
File net/proxy/proxy_config_service_linux.cc (left):

http://codereview.chromium.org/6597070/diff/26001/net/proxy/proxy_config_serv...
net/proxy/proxy_config_service_linux.cc:963: return false;
On 2011/03/16 17:40:00, Mattias Nissler wrote:
> On 2011/03/16 01:30:00, eroman wrote:
> > This seems like it will change the behavior (before it would return false,
now
> > you will return true on invalid URLs). Are you sure about this?
> > 
> > If you would like to change how this works please propose that in a separate
> > changelist.
> 
> I agree it's not a good idea to change behavior of the system services in this
> CL.
> Done.

I still see this change in the latest patchset...

http://codereview.chromium.org/6597070/diff/26001/net/proxy/proxy_config_serv...
File net/proxy/proxy_config_service_linux.cc (right):

http://codereview.chromium.org/6597070/diff/26001/net/proxy/proxy_config_serv...
net/proxy/proxy_config_service_linux.cc:1113: // cached_config_, where
GetLastestProxyConfig() running on the IO thread
On 2011/03/16 17:40:00, Mattias Nissler wrote:
> On 2011/03/16 01:30:00, eroman wrote:
> > typo: please restore earlier version.
> 
> Done.

I don't see this change int he latest patchset...

http://codereview.chromium.org/6597070/diff/24026/chrome/browser/chromeos/pro...
File chrome/browser/chromeos/proxy_config_service_impl.cc (right):

http://codereview.chromium.org/6597070/diff/24026/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.cc:549: } else {
nit: remove the "else" if you are going to return above.

[Mattias Nissler (ping if slow), 2011-03-30 18:23:30]

Sorry, it seems I have accidentally clobbered the changes I've made in my Linux
tree when pulling in the windows fixes. I've now updated the CL (also including
a bugfix for the fallback logic and a corresponding test case). Please take
another look. Thanks!

On 2011/03/26 00:50:30, eroman wrote:
> Did you upload the changes? I am still seeing the same content as before.
> Thanks.
> 
>
http://codereview.chromium.org/6597070/diff/26001/net/proxy/proxy_config_serv...
> File net/proxy/proxy_config_service.h (right):
> 
>
http://codereview.chromium.org/6597070/diff/26001/net/proxy/proxy_config_serv...
> net/proxy/proxy_config_service.h:41: // Gets the most recent value of the
proxy
> configuration. Returns false if
> On 2011/03/16 17:40:00, Mattias Nissler wrote:
> > On 2011/03/16 01:30:00, eroman wrote:
> > > Please update these comments regarding the return value.
> > 
> > Absolutely! Sorry for missing it.
> > Done.
> 
> I don't see this change in the latest pathset...
> 
>
http://codereview.chromium.org/6597070/diff/26001/net/proxy/proxy_config_serv...
> File net/proxy/proxy_config_service_linux.cc (left):
> 
>
http://codereview.chromium.org/6597070/diff/26001/net/proxy/proxy_config_serv...
> net/proxy/proxy_config_service_linux.cc:963: return false;
> On 2011/03/16 17:40:00, Mattias Nissler wrote:
> > On 2011/03/16 01:30:00, eroman wrote:
> > > This seems like it will change the behavior (before it would return false,
> now
> > > you will return true on invalid URLs). Are you sure about this?
> > > 
> > > If you would like to change how this works please propose that in a
separate
> > > changelist.
> > 
> > I agree it's not a good idea to change behavior of the system services in
this
> > CL.
> > Done.
> 
> I still see this change in the latest patchset...
> 
>
http://codereview.chromium.org/6597070/diff/26001/net/proxy/proxy_config_serv...
> File net/proxy/proxy_config_service_linux.cc (right):
> 
>
http://codereview.chromium.org/6597070/diff/26001/net/proxy/proxy_config_serv...
> net/proxy/proxy_config_service_linux.cc:1113: // cached_config_, where
> GetLastestProxyConfig() running on the IO thread
> On 2011/03/16 17:40:00, Mattias Nissler wrote:
> > On 2011/03/16 01:30:00, eroman wrote:
> > > typo: please restore earlier version.
> > 
> > Done.
> 
> I don't see this change int he latest patchset...
> 
>
http://codereview.chromium.org/6597070/diff/24026/chrome/browser/chromeos/pro...
> File chrome/browser/chromeos/proxy_config_service_impl.cc (right):
> 
>
http://codereview.chromium.org/6597070/diff/24026/chrome/browser/chromeos/pro...
> chrome/browser/chromeos/proxy_config_service_impl.cc:549: } else {
> nit: remove the "else" if you are going to return above.

[eroman, 2011-03-30 21:42:31]

LGTM with following comments addressed.

http://codereview.chromium.org/6597070/diff/44002/chrome/browser/chromeos/pro...
File chrome/browser/chromeos/proxy_config_service_impl.cc (right):

http://codereview.chromium.org/6597070/diff/44002/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.cc:412: config_availability_ =
net::ProxyConfigService::CONFIG_UNSET;
Per our earlier conversation, please restore the default here, of auto-detect.
(I would like for the default in proxyservice to be direct, hence this cannot
use unset).

http://codereview.chromium.org/6597070/diff/44002/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.cc:557:
IOSetProxyConfig(reference_config_, net::ProxyConfigService::CONFIG_UNSET);
Same as above, please use CONFIG_VALID with default settings (auto-detect).

http://codereview.chromium.org/6597070/diff/44002/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.cc:600: delete task;
remove the delete task, apparently BrowserThread already deletes it on failure.

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

http://codereview.chromium.org/6597070/diff/44002/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_service_unittest.cc:29: explicit
TestProxyConfigService(const net::ProxyConfig& config,
nit: remove "explicit"

http://codereview.chromium.org/6597070/diff/44002/net/proxy/proxy_service.cc
File net/proxy/proxy_service.cc (right):

http://codereview.chromium.org/6597070/diff/44002/net/proxy/proxy_service.cc#...
net/proxy/proxy_service.cc:843: effective_config =
ProxyConfig::CreateAutoDetect();
Please default to Direct() instead.

[Mattias Nissler (ping if slow), 2011-03-30 22:05:47]

Two quick replies. I agree with the other comments you made and will address
them tomorrow.

http://codereview.chromium.org/6597070/diff/44002/chrome/browser/chromeos/pro...
File chrome/browser/chromeos/proxy_config_service_impl.cc (right):

http://codereview.chromium.org/6597070/diff/44002/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.cc:412: config_availability_ =
net::ProxyConfigService::CONFIG_UNSET;
On 2011/03/30 21:42:31, eroman wrote:
> Per our earlier conversation, please restore the default here, of auto-detect.
> (I would like for the default in proxyservice to be direct, hence this cannot
> use unset).

This is intentional, since if there is no proxy configuration on CrOS, we should
fall back to what policy tells us. This is really the problem I'm trying to
solve :)

http://codereview.chromium.org/6597070/diff/44002/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.cc:557:
IOSetProxyConfig(reference_config_, net::ProxyConfigService::CONFIG_UNSET);
On 2011/03/30 21:42:31, eroman wrote:
> Same as above, please use CONFIG_VALID with default settings (auto-detect).

See my comment above.

[Mattias Nissler (ping if slow), 2011-03-31 14:04:47]

Address remaining comments. Eric, are you OK with my answers regarding
CONFIG_UNSET in the ChromeOS proxy config service?

http://codereview.chromium.org/6597070/diff/44002/chrome/browser/chromeos/pro...
File chrome/browser/chromeos/proxy_config_service_impl.cc (right):

http://codereview.chromium.org/6597070/diff/44002/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.cc:600: delete task;
On 2011/03/30 21:42:31, eroman wrote:
> remove the delete task, apparently BrowserThread already deletes it on
failure.

Done (was copying existing code, apparently absent-minded).

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

http://codereview.chromium.org/6597070/diff/44002/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_service_unittest.cc:29: explicit
TestProxyConfigService(const net::ProxyConfig& config,
On 2011/03/30 21:42:31, eroman wrote:
> nit: remove "explicit"

Done.

http://codereview.chromium.org/6597070/diff/44002/net/proxy/proxy_service.cc
File net/proxy/proxy_service.cc (right):

http://codereview.chromium.org/6597070/diff/44002/net/proxy/proxy_service.cc#...
net/proxy/proxy_service.cc:843: effective_config =
ProxyConfig::CreateAutoDetect();
On 2011/03/30 21:42:31, eroman wrote:
> Please default to Direct() instead.

Done.

[Mattias Nissler (ping if slow), 2011-04-08 15:07:01]

Ping

On 2011/03/31 14:04:47, Mattias Nissler wrote:
> Address remaining comments. Eric, are you OK with my answers regarding
> CONFIG_UNSET in the ChromeOS proxy config service?
> 
>
http://codereview.chromium.org/6597070/diff/44002/chrome/browser/chromeos/pro...
> File chrome/browser/chromeos/proxy_config_service_impl.cc (right):
> 
>
http://codereview.chromium.org/6597070/diff/44002/chrome/browser/chromeos/pro...
> chrome/browser/chromeos/proxy_config_service_impl.cc:600: delete task;
> On 2011/03/30 21:42:31, eroman wrote:
> > remove the delete task, apparently BrowserThread already deletes it on
> failure.
> 
> Done (was copying existing code, apparently absent-minded).
> 
>
http://codereview.chromium.org/6597070/diff/44002/chrome/browser/net/pref_pro...
> File chrome/browser/net/pref_proxy_config_service_unittest.cc (right):
> 
>
http://codereview.chromium.org/6597070/diff/44002/chrome/browser/net/pref_pro...
> chrome/browser/net/pref_proxy_config_service_unittest.cc:29: explicit
> TestProxyConfigService(const net::ProxyConfig& config,
> On 2011/03/30 21:42:31, eroman wrote:
> > nit: remove "explicit"
> 
> Done.
> 
> http://codereview.chromium.org/6597070/diff/44002/net/proxy/proxy_service.cc
> File net/proxy/proxy_service.cc (right):
> 
>
http://codereview.chromium.org/6597070/diff/44002/net/proxy/proxy_service.cc#...
> net/proxy/proxy_service.cc:843: effective_config =
> ProxyConfig::CreateAutoDetect();
> On 2011/03/30 21:42:31, eroman wrote:
> > Please default to Direct() instead.
> 
> Done.

[eroman, 2011-04-08 17:42:50]

Yep sounds good.

(In fact I had assumed you already checked in, sorry didn't realize you were
waiting on me!)

LGTM