Dynamically refresh pref-configured proxies.

Dynamically refresh pref-configured proxies.

BUG=63175, 48930
TEST=unit tests

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

[Mattias Nissler (ping if slow), 2010-11-16 22:40:35]

Please review.

[Mattias Nissler (ping if slow), 2010-11-16 22:40:35]

Please review.

[jochen (gone - plz use gerrit), 2010-11-16 22:48:32]

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

http://codereview.chromium.org/5005002/diff/2001/chrome/browser/net/pref_prox...
chrome/browser/net/pref_proxy_config_service.cc:22:
BrowserThread::DeleteOnUIThread>,
make sure to run this on the valgrind bots

http://codereview.chromium.org/5005002/diff/2001/chrome/browser/net/pref_prox...
chrome/browser/net/pref_proxy_config_service.cc:60: };
disallow copy and assign?

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

http://codereview.chromium.org/5005002/diff/2001/chrome/browser/net/pref_prox...
chrome/browser/net/pref_proxy_config_service.h:8: #include <base/basictypes.h>
"" instead of <>

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

Adressed Jochen's comments. New version is up.

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

http://codereview.chromium.org/5005002/diff/2001/chrome/browser/net/pref_prox...
chrome/browser/net/pref_proxy_config_service.cc:22:
BrowserThread::DeleteOnUIThread>,
On 2010/11/16 22:48:32, jochen wrote:
> make sure to run this on the valgrind bots

Will do.

http://codereview.chromium.org/5005002/diff/2001/chrome/browser/net/pref_prox...
chrome/browser/net/pref_proxy_config_service.cc:60: };
On 2010/11/16 22:48:32, jochen wrote:
> disallow copy and assign?

Done.

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

http://codereview.chromium.org/5005002/diff/2001/chrome/browser/net/pref_prox...
chrome/browser/net/pref_proxy_config_service.h:8: #include <base/basictypes.h>
On 2010/11/16 22:48:32, jochen wrote:
> "" instead of <>

Sigh, I make this same mistake again and again...
Done.

[eroman, 2010-11-17 02:14:49]

Overall approach looks good, however I have comments about the specific
implementation.

http://codereview.chromium.org/5005002/diff/15001/chrome/browser/net/chrome_u...
File chrome/browser/net/chrome_url_request_context.cc (right):

http://codereview.chromium.org/5005002/diff/15001/chrome/browser/net/chrome_u...
chrome/browser/net/chrome_url_request_context.cc:72: // is configured through
prefs (NB: prefs include command line and
What does "NB:" mean?

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

http://codereview.chromium.org/5005002/diff/15001/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_service.cc:22:
BrowserThread::DeleteOnUIThread>,
DeleteOnUIThread is problematic, since it can lead to leaks during shutdown.
Please avoid using this.

In particular consider what happens if ProxyConfigService is 
deleted during IOThread::Stop(): we release the final reference to the tracker,
which will in turn post a task to the UI thread to delete. However since we are
already shutting down, this task will never actually get run.

It would be better to do the teardown explicitly when the pref_service is
deleted. I guess one way would be to watch for PROFILE_DESTROYED, or move the
helper object into the profile.

http://codereview.chromium.org/5005002/diff/15001/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_service.cc:57:
base::WeakPtr<PrefProxyConfigService> pref_config_service_;
WeakPtr is not threadsafe. Is it valid for this to be deleted from the UI
thread? (Whereas PrefProxyConfigService will be deleted from the IO thread).

http://codereview.chromium.org/5005002/diff/15001/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_service.cc:96:
config->set_id(net::ProxyConfig::INVALID_ID);
Please avoid using the set_id (see my later comment). This is a historical
detail which hasn't been cleaned up and is confusing.

Rather please return a bool or an enum, or a composite structure.

Conceptually I believe this function basically wants to return two types of
values: either (a) a ProxyConfig, or (b) an indication that we should use the
system settings (i.e. base_service).

To clarify this throughout the file, I suggest introducing a helper class like
this:

class ProxyConfigFromPrefs {
 public:
  const ProxyConfig& config() const {
    DCHECK(was_specified_);
    return config_;
  }

  bool was_specified() const {
    return was_specified_;
  }

 private:
  ProxyConfig config_;
  bool was_specified_;
};

Now all the functions which wanted to return an indication of "fall back to the
base service" and were doing so by way of setting invalid ID, would just return
a ProxyConfigFromPrefs where was_specified_ = false.


Moreover in the future it seems reasonable that we would actually want a
preference that says "Use the system proxy settings", as opposed to inferring
that action from lack of any other preference.

http://codereview.chromium.org/5005002/diff/15001/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_service.cc:169: ++current_id_;
What is the use-case for this ID number? Please just leave the ID number at its
default (the ID number is an internal detail used within the ProxyService for
some book-keeping, but does not need to be set by the ProxyConfigService).

http://codereview.chromium.org/5005002/diff/15001/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_service.cc:171:
GetLatestProxyConfig(&current_config);
Important: you are disregarding the return value from this function.

If the "base_service_" has returned false, it means we don't know what the proxy
configuration is yet (i.e. it completes asynchronously). In which case it is
incorrect to notify the observers, since |current__config| could be garbage.

http://codereview.chromium.org/5005002/diff/15001/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_service.cc:188: if (pref_config.is_valid())
{
See comment about not using the IDs. If you decided to use the struct, this
function could be re-written as:

const ProxyConfigFromPrefs& pref_config(pref_config_tracker_->GetProxyConfig());

if (pref_config.was_specified()) {
  *config = pref_config.config();
  return true;
}
return base_service_->GetLatestProxyConfig(config);

http://codereview.chromium.org/5005002/diff/15001/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_service.cc:206: if
(!pref_config_tracker_->GetProxyConfig().is_valid()) {
See earlier comment, please don't use the ID field, and instead consider a
ProxyConfig structure as meaning a (valid) configuration.

http://codereview.chromium.org/5005002/diff/15001/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_service.cc:208:
new_config.set_id(++current_id_);
Shouldn't be a need for the ID.

[Mattias Nissler (ping if slow), 2010-11-17 12:55:37]

Thanks Eric for your feedback! I addressed all comments and uploaded a new
version. Please take another look.

+ gfeher (FYI: policy_templates.json changes)

http://codereview.chromium.org/5005002/diff/15001/chrome/browser/net/chrome_u...
File chrome/browser/net/chrome_url_request_context.cc (right):

http://codereview.chromium.org/5005002/diff/15001/chrome/browser/net/chrome_u...
chrome/browser/net/chrome_url_request_context.cc:72: // is configured through
prefs (NB: prefs include command line and
On 2010/11/17 02:14:49, eroman wrote:
> What does "NB:" mean?

Nota bene. Changed it to "Note".
Done.

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

http://codereview.chromium.org/5005002/diff/15001/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_service.cc:22:
BrowserThread::DeleteOnUIThread>,
On 2010/11/17 02:14:49, eroman wrote:
> DeleteOnUIThread is problematic, since it can lead to leaks during shutdown.
> Please avoid using this.
> 
> In particular consider what happens if ProxyConfigService is 
> deleted during IOThread::Stop(): we release the final reference to the
tracker,
> which will in turn post a task to the UI thread to delete. However since we
are
> already shutting down, this task will never actually get run.
> 
> It would be better to do the teardown explicitly when the pref_service is
> deleted. I guess one way would be to watch for PROFILE_DESTROYED, or move the
> helper object into the profile.

Good point, I actually realized the same thing while thinking about it a bit
more last night. I've now moved the tracker into the profile, from which we can
shut it down cleanly.

http://codereview.chromium.org/5005002/diff/15001/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_service.cc:57:
base::WeakPtr<PrefProxyConfigService> pref_config_service_;
On 2010/11/17 02:14:49, eroman wrote:
> WeakPtr is not threadsafe. Is it valid for this to be deleted from the UI
> thread? (Whereas PrefProxyConfigService will be deleted from the IO thread).

Replaced this hack with a proper observer interface in the tracker.

http://codereview.chromium.org/5005002/diff/15001/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_service.cc:96:
config->set_id(net::ProxyConfig::INVALID_ID);
On 2010/11/17 02:14:49, eroman wrote:
> Please avoid using the set_id (see my later comment). This is a historical
> detail which hasn't been cleaned up and is confusing.
> 
> Rather please return a bool or an enum, or a composite structure.
> 
> Conceptually I believe this function basically wants to return two types of
> values: either (a) a ProxyConfig, or (b) an indication that we should use the
> system settings (i.e. base_service).
> 
> To clarify this throughout the file, I suggest introducing a helper class like
> this:
> 
> class ProxyConfigFromPrefs {
>  public:
>   const ProxyConfig& config() const {
>     DCHECK(was_specified_);
>     return config_;
>   }
> 
>   bool was_specified() const {
>     return was_specified_;
>   }
> 
>  private:
>   ProxyConfig config_;
>   bool was_specified_;
> };
> 
> Now all the functions which wanted to return an indication of "fall back to
the
> base service" and were doing so by way of setting invalid ID, would just
return
> a ProxyConfigFromPrefs where was_specified_ = false.
> 
> 
> Moreover in the future it seems reasonable that we would actually want a
> preference that says "Use the system proxy settings", as opposed to inferring
> that action from lack of any other preference.

Done.

http://codereview.chromium.org/5005002/diff/15001/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_service.cc:169: ++current_id_;
On 2010/11/17 02:14:49, eroman wrote:
> What is the use-case for this ID number? Please just leave the ID number at
its
> default (the ID number is an internal detail used within the ProxyService for
> some book-keeping, but does not need to be set by the ProxyConfigService).

Done.

http://codereview.chromium.org/5005002/diff/15001/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_service.cc:171:
GetLatestProxyConfig(&current_config);
On 2010/11/17 02:14:49, eroman wrote:
> Important: you are disregarding the return value from this function.
> 
> If the "base_service_" has returned false, it means we don't know what the
proxy
> configuration is yet (i.e. it completes asynchronously). In which case it is
> incorrect to notify the observers, since |current__config| could be garbage.

Done.

http://codereview.chromium.org/5005002/diff/15001/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_service.cc:188: if (pref_config.is_valid())
{
On 2010/11/17 02:14:49, eroman wrote:
> See comment about not using the IDs. If you decided to use the struct, this
> function could be re-written as:
> 
> const ProxyConfigFromPrefs&
pref_config(pref_config_tracker_->GetProxyConfig());
> 
> if (pref_config.was_specified()) {
>   *config = pref_config.config();
>   return true;
> }
> return base_service_->GetLatestProxyConfig(config);

Done.

http://codereview.chromium.org/5005002/diff/15001/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_service.cc:206: if
(!pref_config_tracker_->GetProxyConfig().is_valid()) {
On 2010/11/17 02:14:49, eroman wrote:
> See earlier comment, please don't use the ID field, and instead consider a
> ProxyConfig structure as meaning a (valid) configuration.

Done.

http://codereview.chromium.org/5005002/diff/15001/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_service.cc:208:
new_config.set_id(++current_id_);
On 2010/11/17 02:14:49, eroman wrote:
> Shouldn't be a need for the ID.

Done.

[eroman, 2010-11-18 18:09:42]

BTW: I am out of office (vacation) yesterday and today, I will do next review
pass friday.

(Unless I find some free time today :)

[Mattias Nissler (ping if slow), 2010-11-18 18:39:03]

On Thu, Nov 18, 2010 at 7:09 PM, <eroman@chromium.org> wrote:

> BTW: I am out of office (vacation) yesterday and today, I will do next
> review
> pass friday.
>

Oh, I didn't know that :) Thanks for taking the time and writing feedback
then and enjoy your vacation! As long as I get to submit the thing some time
next week, I'm happy :)

Cheers,
Mattias

[eroman, 2010-11-20 03:19:07]

This looks good, however I would like to give it one more pass after the below
comments are addressed.

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

http://codereview.chromium.org/5005002/diff/21001/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_service.cc:32: void
PrefProxyConfigTracker::Shutdown() {
Can you add an assertion that we are on UI thread?

DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));

http://codereview.chromium.org/5005002/diff/21001/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_service.cc:39:
PrefProxyConfigTracker::Observer* observer) {
Please DCHECK that we are on IO thread.

http://codereview.chromium.org/5005002/diff/21001/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_service.cc:43: void
PrefProxyConfigTracker::RemoveObserver(
Please DCHECK that we are on IO thread.

http://codereview.chromium.org/5005002/diff/21001/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_service.cc:179: if
(!pref_config_tracker_->GetProxyConfig(&pref_config)) {
I suggest adding a comment to make this easier to follow.
Explaining that if GetProxyConfig returned true it means we are not using the
system proxy settings, so the change event dispatched by base_service_ is
disregarded.

http://codereview.chromium.org/5005002/diff/21001/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_service.cc:188: if
(GetLatestProxyConfig(&new_config)) {
I suggest adding a comment to the effect that if GetLatestProxyConfig() returned
false it means we are using base_service_ but it was not ready yet, so the
change notification will be broadcast from OnProxyConfigChanged() instead.

http://codereview.chromium.org/5005002/diff/21001/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_service.cc:189:
FOR_EACH_OBSERVER(net::ProxyConfigService::Observer, observers_,
Nit: we may broadcast a change event even though the effective new_config is
unchanged (this is also true of the other location where FOR_EACH_OBSERVER is
called). This isn't necessary to address (since current observers will just end
up re-configuring). I suppose you could avoid it by calling
ProxyConfig::Equals() to see if they are the same.

I don't actually suggest you fix this, but please add a note explaining that we
don't handle that.

Also as an aside, if you want to see the impact of this, try opening
chrome://net-internals/#events and then changing around the proxy settings. Now
look for the PROXY_CONFIG_CHANGED events that are generated.

http://codereview.chromium.org/5005002/diff/21001/chrome/browser/net/pref_pro...
File chrome/browser/net/pref_proxy_config_service.h (right):

http://codereview.chromium.org/5005002/diff/21001/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_service.h:11: #include "base/weak_ptr.h"
Can you delete this header?

http://codereview.chromium.org/5005002/diff/21001/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_service.h:72: ObserverList<Observer>
observers_;
Can you define it as:
ObserverList<Observer, true>

So we enforce that the observer list is empty when destroyed?

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

http://codereview.chromium.org/5005002/diff/21001/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_service_unittest.cc:8: #include
"chrome/browser/net/pref_proxy_config_service.h"
nit: please put this at the top of the file.

http://codereview.chromium.org/5005002/diff/21001/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_service_unittest.cc:17: using testing::_;
What is this?

http://codereview.chromium.org/5005002/diff/21001/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_service_unittest.cc:25: class
TestProxyConfigService : public net::ProxyConfigServiceFixed {
ProxyConfigServiceFixed is so small, that I don't think there is any advantage
to extending it.

I think you would be better off just creating a standalone class.

http://codereview.chromium.org/5005002/diff/21001/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_service_unittest.cc:31: void
FireObservers() {
What is a weird about this, is the config didn't actually change (we are firing
with the same value as before).

See comment above about not inheriting from ProxyConfigServiceFixed.

I propose instead naming this method:

void SetProxyConfig(const ProxyConfig& new_config);

And updating our config_ member variable, and then firing the observers.

http://codereview.chromium.org/5005002/diff/21001/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_service_unittest.cc:47:
ObserverList<net::ProxyConfigService::Observer> observers_;
I recommend adding the ", true" to template trait, to catch cases where we don't
remove observers.

http://codereview.chromium.org/5005002/diff/21001/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_service_unittest.cc:85:
scoped_ptr<TestingPrefService> pref_service_;
nit: I suggest ordering this above proxy_config_service_ (just to emphasize that
it outlives it).

http://codereview.chromium.org/5005002/diff/21001/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_service_unittest.cc:94: class
PrefProxyConfigServiceTest :
style nit: put the colon on the next line (and indent by 4 spaces).

http://codereview.chromium.org/5005002/diff/21001/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_service_unittest.cc:138:
TEST_F(PrefProxyConfigServiceTest, Observers) {
Thanks for adding extensive tests!

http://codereview.chromium.org/5005002/diff/21001/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_service_unittest.cc:143:
EXPECT_CALL(observer,
I'll give this a more careful re-read later, I must admit that I don't actually
understand gmock.

http://codereview.chromium.org/5005002/diff/21001/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_service_unittest.cc:224: for (unsigned int
i = 0; i < arraysize(GetParam().switches); i++) {
nit: I have typically seen size_t used for such loops.

http://codereview.chromium.org/5005002/diff/21001/chrome/browser/profile.h
File chrome/browser/profile.h (right):

http://codereview.chromium.org/5005002/diff/21001/chrome/browser/profile.h#ne...
chrome/browser/profile.h:482: virtual PrefProxyConfigTracker*
GetProxyConfigTracker() = 0;
comment?

http://codereview.chromium.org/5005002/diff/21001/chrome/browser/profile_impl.cc
File chrome/browser/profile_impl.cc (right):

http://codereview.chromium.org/5005002/diff/21001/chrome/browser/profile_impl...
chrome/browser/profile_impl.cc:561: pref_proxy_config_tracker_->Shutdown();
For consistency with the callers I notice above, I suggest calling the method
DestroyingProfile(), or even DestroyingPreferenceService().

http://codereview.chromium.org/5005002/diff/21001/chrome/test/testing_profile.h
File chrome/test/testing_profile.h (right):

http://codereview.chromium.org/5005002/diff/21001/chrome/test/testing_profile...
chrome/test/testing_profile.h:298: return NULL;
I wander if this is going to cause problems on any codepath (we aren't prepared
to handle a NULL PrefProxyConfigTracker inside of the proxyconfigservice).

[Mattias Nissler (ping if slow), 2010-11-21 22:49:13]

Thanks for the feedback. I just uploaded an update version that addresses the
comments.

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

http://codereview.chromium.org/5005002/diff/21001/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_service.cc:32: void
PrefProxyConfigTracker::Shutdown() {
On 2010/11/20 03:19:07, eroman wrote:
> Can you add an assertion that we are on UI thread?
> 
> DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));

Done.

http://codereview.chromium.org/5005002/diff/21001/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_service.cc:39:
PrefProxyConfigTracker::Observer* observer) {
On 2010/11/20 03:19:07, eroman wrote:
> Please DCHECK that we are on IO thread.

Done.

http://codereview.chromium.org/5005002/diff/21001/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_service.cc:43: void
PrefProxyConfigTracker::RemoveObserver(
On 2010/11/20 03:19:07, eroman wrote:
> Please DCHECK that we are on IO thread.

Done.

http://codereview.chromium.org/5005002/diff/21001/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_service.cc:179: if
(!pref_config_tracker_->GetProxyConfig(&pref_config)) {
On 2010/11/20 03:19:07, eroman wrote:
> I suggest adding a comment to make this easier to follow.
> Explaining that if GetProxyConfig returned true it means we are not using the
> system proxy settings, so the change event dispatched by base_service_ is
> disregarded.

Done.

http://codereview.chromium.org/5005002/diff/21001/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_service.cc:188: if
(GetLatestProxyConfig(&new_config)) {
On 2010/11/20 03:19:07, eroman wrote:
> I suggest adding a comment to the effect that if GetLatestProxyConfig()
returned
> false it means we are using base_service_ but it was not ready yet, so the
> change notification will be broadcast from OnProxyConfigChanged() instead.

Done.

http://codereview.chromium.org/5005002/diff/21001/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_service.cc:189:
FOR_EACH_OBSERVER(net::ProxyConfigService::Observer, observers_,
On 2010/11/20 03:19:07, eroman wrote:
> Nit: we may broadcast a change event even though the effective new_config is
> unchanged (this is also true of the other location where FOR_EACH_OBSERVER is
> called). This isn't necessary to address (since current observers will just
end
> up re-configuring). I suppose you could avoid it by calling
> ProxyConfig::Equals() to see if they are the same.
> 
> I don't actually suggest you fix this, but please add a note explaining that
we
> don't handle that.
> 
> Also as an aside, if you want to see the impact of this, try opening
> chrome://net-internals/#events and then changing around the proxy settings.
Now
> look for the PROXY_CONFIG_CHANGED events that are generated.

You are right. I added an Equals() check to the pref tracker, so the unnecessary
notifications should only show up in cases where we switch from the preference
proxy configuration to the system proxy service configuration or vice versa and
the effective proxy configuration doesn't actually change. That should be very
rare however, so fixing that by keeping another cache of the configuration and
comparing against that before sending the notification here seems overkill.
Added a note to that effect.

http://codereview.chromium.org/5005002/diff/21001/chrome/browser/net/pref_pro...
File chrome/browser/net/pref_proxy_config_service.h (right):

http://codereview.chromium.org/5005002/diff/21001/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_service.h:11: #include "base/weak_ptr.h"
On 2010/11/20 03:19:07, eroman wrote:
> Can you delete this header?

Done.

http://codereview.chromium.org/5005002/diff/21001/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_service.h:72: ObserverList<Observer>
observers_;
On 2010/11/20 03:19:07, eroman wrote:
> Can you define it as:
> ObserverList<Observer, true>
> 
> So we enforce that the observer list is empty when destroyed?

Done.

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

http://codereview.chromium.org/5005002/diff/21001/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_service_unittest.cc:8: #include
"chrome/browser/net/pref_proxy_config_service.h"
On 2010/11/20 03:19:07, eroman wrote:
> nit: please put this at the top of the file.

Done.

http://codereview.chromium.org/5005002/diff/21001/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_service_unittest.cc:17: using testing::_;
On 2010/11/20 03:19:07, eroman wrote:
> What is this?

This is gmock's "any value" argument matcher for setting up call expectations.

http://codereview.chromium.org/5005002/diff/21001/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_service_unittest.cc:25: class
TestProxyConfigService : public net::ProxyConfigServiceFixed {
On 2010/11/20 03:19:07, eroman wrote:
> ProxyConfigServiceFixed is so small, that I don't think there is any advantage
> to extending it.
> 
> I think you would be better off just creating a standalone class.

Done.

http://codereview.chromium.org/5005002/diff/21001/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_service_unittest.cc:31: void
FireObservers() {
On 2010/11/20 03:19:07, eroman wrote:
> What is a weird about this, is the config didn't actually change (we are
firing
> with the same value as before).
> 
> See comment above about not inheriting from ProxyConfigServiceFixed.
> 
> I propose instead naming this method:
> 
> void SetProxyConfig(const ProxyConfig& new_config);
> 
> And updating our config_ member variable, and then firing the observers.

Done.

http://codereview.chromium.org/5005002/diff/21001/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_service_unittest.cc:47:
ObserverList<net::ProxyConfigService::Observer> observers_;
On 2010/11/20 03:19:07, eroman wrote:
> I recommend adding the ", true" to template trait, to catch cases where we
don't
> remove observers.

Done.

http://codereview.chromium.org/5005002/diff/21001/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_service_unittest.cc:85:
scoped_ptr<TestingPrefService> pref_service_;
On 2010/11/20 03:19:07, eroman wrote:
> nit: I suggest ordering this above proxy_config_service_ (just to emphasize
that
> it outlives it).

Done.

http://codereview.chromium.org/5005002/diff/21001/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_service_unittest.cc:94: class
PrefProxyConfigServiceTest :
On 2010/11/20 03:19:07, eroman wrote:
> style nit: put the colon on the next line (and indent by 4 spaces).

Done.

http://codereview.chromium.org/5005002/diff/21001/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_service_unittest.cc:143:
EXPECT_CALL(observer,
On 2010/11/20 03:19:07, eroman wrote:
> I'll give this a more careful re-read later, I must admit that I don't
actually
> understand gmock.

It declares that the subsequent code will generate a call to
OnProxyConfigChanged with a config object that'll match fixed_config_ (as
determined by the matcher). When we call Mock::VerifyAndClearExpactations below,
gmock will check that this has happened and fail the test if it didn't.

http://codereview.chromium.org/5005002/diff/21001/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_service_unittest.cc:224: for (unsigned int
i = 0; i < arraysize(GetParam().switches); i++) {
On 2010/11/20 03:19:07, eroman wrote:
> nit: I have typically seen size_t used for such loops.

Done.

http://codereview.chromium.org/5005002/diff/21001/chrome/browser/profile.h
File chrome/browser/profile.h (right):

http://codereview.chromium.org/5005002/diff/21001/chrome/browser/profile.h#ne...
chrome/browser/profile.h:482: virtual PrefProxyConfigTracker*
GetProxyConfigTracker() = 0;
On 2010/11/20 03:19:07, eroman wrote:
> comment?

Done.

http://codereview.chromium.org/5005002/diff/21001/chrome/browser/profile_impl.cc
File chrome/browser/profile_impl.cc (right):

http://codereview.chromium.org/5005002/diff/21001/chrome/browser/profile_impl...
chrome/browser/profile_impl.cc:561: pref_proxy_config_tracker_->Shutdown();
On 2010/11/20 03:19:07, eroman wrote:
> For consistency with the callers I notice above, I suggest calling the method
> DestroyingProfile(), or even DestroyingPreferenceService().

There are others above named Shutdown, but I don't feel strong about this, so
changed to DestroyingPrefService().

http://codereview.chromium.org/5005002/diff/21001/chrome/test/testing_profile.h
File chrome/test/testing_profile.h (right):

http://codereview.chromium.org/5005002/diff/21001/chrome/test/testing_profile...
chrome/test/testing_profile.h:298: return NULL;
On 2010/11/20 03:19:07, eroman wrote:
> I wander if this is going to cause problems on any codepath (we aren't
prepared
> to handle a NULL PrefProxyConfigTracker inside of the proxyconfigservice).

Currently the tests don't need this. Let's change this once the need arises.

[Mattias Nissler (ping if slow), 2010-11-22 16:01:54]

I thought about the tracker getter in the profile a bit more and figured it'd be
nicer to just drop that reference from the profile and listen to
PROFILE_DESTROYED for shutting down. Code changed accordingly. WDYT?

[Mattias Nissler (ping if slow), 2010-11-23 18:38:47]

eroman: ping?

[Mattias Nissler (ping if slow), 2010-11-24 09:44:47]

Latest version reverts to the version that doesn't need DeleteOnUIThread. I also
put a proper implementation into TestingProfile, since we assume that the value
returned is always non-NULL.

eroman: Sorry I bothered you, I wasn't aware that you're on vacation this week
as well.
jochen: Can you take over the review from eroman since we need to get this into
M9?

[jochen (gone - plz use gerrit), 2010-11-24 10:04:52]

LGTM assuming you address my comments and try & valgrind bots are happy with it

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

http://codereview.chromium.org/5005002/diff/74001/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_service.cc:72: if (valid_ != valid ||
!pref_config_.Equals(config)) {
hum, so if valid = false, you might still end up overriding the pref_config_?

http://codereview.chromium.org/5005002/diff/74001/chrome/browser/net/pref_pro...
File chrome/browser/net/pref_proxy_config_service.h (right):

http://codereview.chromium.org/5005002/diff/74001/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_service.h:7: 
#pragma once

http://codereview.chromium.org/5005002/diff/74001/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_service.h:18: class PrefProxyConfigTracker;
alphabetical ordering

http://codereview.chromium.org/5005002/diff/74001/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_service.h:29: class Observer {
nit ObserverInterface :)

http://codereview.chromium.org/5005002/diff/74001/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_service.h:38: void AddObserver(Observer*
observer);
annotate with the thread to invoke the method on (also for other public members)

http://codereview.chromium.org/5005002/diff/74001/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_service.h:48: void DestroyingPrefService();
nit. maybe name DetachFromPrefService?

http://codereview.chromium.org/5005002/diff/74001/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_service.h:56: // Install a new
configuration (to be called on the IO thread).
what's |valid| doing? maybe you should have a separate method
InvalidateProxyConfig?

since this is a private method, you probably don't need to annotate it with the
thread to invoke it on

http://codereview.chromium.org/5005002/diff/74001/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_service.h:87: public
net::ProxyConfigService::Observer {
nit alphabetical ordering

[Mattias Nissler (ping if slow), 2010-11-24 11:19:29]

New version with Jochen's comments addressed is up. Now running through trybots
(including valgrind), will commit if I don't hear objections.

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

http://codereview.chromium.org/5005002/diff/74001/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_service.cc:72: if (valid_ != valid ||
!pref_config_.Equals(config)) {
On 2010/11/24 10:04:52, jochen wrote:
> hum, so if valid = false, you might still end up overriding the pref_config_?

That change doesn't make any semantic difference (we don't return pref_config_
through the ProxyConfigService interface if valid_ is false), but I guess it's
reasonable to change it nevertheless because it matches what people would expect
when reading this. Done.

http://codereview.chromium.org/5005002/diff/74001/chrome/browser/net/pref_pro...
File chrome/browser/net/pref_proxy_config_service.h (right):

http://codereview.chromium.org/5005002/diff/74001/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_service.h:7: 
On 2010/11/24 10:04:52, jochen wrote:
> #pragma once

Done.

http://codereview.chromium.org/5005002/diff/74001/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_service.h:18: class PrefProxyConfigTracker;
On 2010/11/24 10:04:52, jochen wrote:
> alphabetical ordering

Done.

http://codereview.chromium.org/5005002/diff/74001/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_service.h:29: class Observer {
On 2010/11/24 10:04:52, jochen wrote:
> nit ObserverInterface :)

Done.

http://codereview.chromium.org/5005002/diff/74001/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_service.h:38: void AddObserver(Observer*
observer);
On 2010/11/24 10:04:52, jochen wrote:
> annotate with the thread to invoke the method on (also for other public
members)

Done.

http://codereview.chromium.org/5005002/diff/74001/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_service.h:48: void DestroyingPrefService();
On 2010/11/24 10:04:52, jochen wrote:
> nit. maybe name DetachFromPrefService?

Done.

http://codereview.chromium.org/5005002/diff/74001/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_service.h:56: // Install a new
configuration (to be called on the IO thread).
On 2010/11/24 10:04:52, jochen wrote:
> what's |valid| doing? maybe you should have a separate method
> InvalidateProxyConfig?
> 
> since this is a private method, you probably don't need to annotate it with
the
> thread to invoke it on

Wrote better documentation.

http://codereview.chromium.org/5005002/diff/74001/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_service.h:87: public
net::ProxyConfigService::Observer {
On 2010/11/24 10:04:52, jochen wrote:
> nit alphabetical ordering

Done.

[eroman, 2010-11-30 02:58:54]

LGTM.

Sorry I didn't get to the final pass before leaving on vacation! I realize you
have already committed, but please consider addressing my following comments.
Thanks.

http://codereview.chromium.org/5005002/diff/80001/chrome/browser/net/chrome_u...
File chrome/browser/net/chrome_url_request_context.h (right):

http://codereview.chromium.org/5005002/diff/80001/chrome/browser/net/chrome_u...
chrome/browser/net/chrome_url_request_context.h:38: class ProxyConfig;
Please delete this as it is no longer referenced.

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

http://codereview.chromium.org/5005002/diff/80001/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_service.cc:73: if (valid_ != valid ||
!pref_config_.Equals(config)) {
This is a little weird in the case where (!valid_ && !valid). 

It probably doesn't make sense to compare pref_config_ to config in this case
(since by definition it "isn't valid").

It looks like it is still correct because config is reset to net::ProxyConfig()
by ReadPrefConfig in this case, but that isn't immediately obvious.

http://codereview.chromium.org/5005002/diff/80001/chrome/browser/net/pref_pro...
File chrome/browser/net/pref_proxy_config_service.h (right):

http://codereview.chromium.org/5005002/diff/80001/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_service.h:29: class ObserverInterface {
nit: why not call this "Observer", which is more typical naming in Chrome.

http://codereview.chromium.org/5005002/diff/80001/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_service.h:71: // (expect for construction).
typo: "expect" --> "except".

http://codereview.chromium.org/5005002/diff/80001/chrome/test/testing_profile.cc
File chrome/test/testing_profile.cc (right):

http://codereview.chromium.org/5005002/diff/80001/chrome/test/testing_profile...
chrome/test/testing_profile.cc:120: class TestURLRequestContextGetter : public
URLRequestContextGetter {
What is this used for? (I don't see it referenced in here).

[battre (please use the other), 2010-12-02 18:06:12]

I addressed the issues Eric raised in http://codereview.chromium.org/5537002/

http://codereview.chromium.org/5005002/diff/80001/chrome/browser/net/chrome_u...
File chrome/browser/net/chrome_url_request_context.h (right):

http://codereview.chromium.org/5005002/diff/80001/chrome/browser/net/chrome_u...
chrome/browser/net/chrome_url_request_context.h:38: class ProxyConfig;
On 2010/11/30 02:58:54, eroman wrote:
> Please delete this as it is no longer referenced.

Done.

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

http://codereview.chromium.org/5005002/diff/80001/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_service.cc:73: if (valid_ != valid ||
!pref_config_.Equals(config)) {
On 2010/11/30 02:58:54, eroman wrote:
> This is a little weird in the case where (!valid_ && !valid). 
> 
> It probably doesn't make sense to compare pref_config_ to config in this case
> (since by definition it "isn't valid").
> 
> It looks like it is still correct because config is reset to
net::ProxyConfig()
> by ReadPrefConfig in this case, but that isn't immediately obvious.

Done.

http://codereview.chromium.org/5005002/diff/80001/chrome/browser/net/pref_pro...
File chrome/browser/net/pref_proxy_config_service.h (right):

http://codereview.chromium.org/5005002/diff/80001/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_service.h:29: class ObserverInterface {
On 2010/11/30 02:58:54, eroman wrote:
> nit: why not call this "Observer", which is more typical naming in Chrome.

Done.

http://codereview.chromium.org/5005002/diff/80001/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_service.h:71: // (expect for construction).
On 2010/11/30 02:58:54, eroman wrote:
> typo: "expect" --> "except".

Done.

http://codereview.chromium.org/5005002/diff/80001/chrome/test/testing_profile.cc
File chrome/test/testing_profile.cc (right):

http://codereview.chromium.org/5005002/diff/80001/chrome/test/testing_profile...
chrome/test/testing_profile.cc:120: class TestURLRequestContextGetter : public
URLRequestContextGetter {
On 2010/11/30 02:58:54, eroman wrote:
> What is this used for? (I don't see it referenced in here).

Line 427, see below.