Fixed minor atomicity issue in PrefProxyConfigService::GetLatestProxyConfig.

Fixed minor atomicity issue in PrefProxyConfigService::GetLatestProxyConfig and remaining comments of 5005002.

See http://codereview.chromium.org/5005002.

BUG=48930
TEST=none

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

[eroman, 2010-12-02 19:31:22]

Thanks for addressing those comments.

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

http://codereview.chromium.org/5537002/diff/9001/chrome/browser/net/pref_prox...
chrome/browser/net/pref_proxy_config_service.cc:73: if (valid && (!valid_ ||
!pref_config_.Equals(config))) {
I don't think this is right, since it looks like we will never be able to return
to "use system settings (valid=false)".

I think what you want here instead is something like:

if (valid_ != valid || (valid && pref_config_.Equals(config))

Basically the only restriction we add is to avoid comparing config to
pref_config_ when !valid.

[eroman, 2010-12-02 19:32:40]

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

http://codereview.chromium.org/5537002/diff/9001/chrome/browser/net/pref_prox...
chrome/browser/net/pref_proxy_config_service.cc:73: if (valid && (!valid_ ||
!pref_config_.Equals(config))) {
On 2010/12/02 19:31:22, eroman wrote:
> I don't think this is right, since it looks like we will never be able to
return
> to "use system settings (valid=false)".
> 
> I think what you want here instead is something like:
> 
> if (valid_ != valid || (valid && pref_config_.Equals(config))
> 
> Basically the only restriction we add is to avoid comparing config to
> pref_config_ when !valid.

Oops my pseudo-code wasn't right either :-)
Let me try again:

if (valid_ != valid || (valid && !pref_config_.Equals(config))

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

I agree, thanks for pointing this out.

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

http://codereview.chromium.org/5537002/diff/9001/chrome/browser/net/pref_prox...
chrome/browser/net/pref_proxy_config_service.cc:73: if (valid && (!valid_ ||
!pref_config_.Equals(config))) {
On 2010/12/02 19:32:40, eroman wrote:
> On 2010/12/02 19:31:22, eroman wrote:
> > I don't think this is right, since it looks like we will never be able to
> return
> > to "use system settings (valid=false)".
> > 
> > I think what you want here instead is something like:
> > 
> > if (valid_ != valid || (valid && pref_config_.Equals(config))
> > 
> > Basically the only restriction we add is to avoid comparing config to
> > pref_config_ when !valid.
> 
> Oops my pseudo-code wasn't right either :-)
> Let me try again:
> 
> if (valid_ != valid || (valid && !pref_config_.Equals(config))

Done.

[Mattias Nissler (ping if slow), 2010-12-03 09:40:03]

Dominic, thanks for fixing this. LGTM.

[eroman, 2010-12-03 20:02:43]

LGTM