Split ProxyResolver into two interfaces:...

Split ProxyResolver into two interfaces:  A. interface for retrieving the system proxy settings (ProxyConfigService)  B. interface for resolving the proxy (ProxyResolver)The motivation behind this change is:  1. Simplify sharing the WinHTTP code that fetches IE settings, with the V8 proxy resolver (avoids having platform-specific code in ProxyResolverV8).  2. Restrict objects to one thread. (ProxyService calls the config getter on IO thread, and the proxy resolving on the PAC thread).(ProxyResolver is now only 1 method, but this will grow shortly).

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

[eroman, 2008-12-19 00:50:57]

http://codereview.chromium.org/15070/diff/1/3
File net/http/http_network_layer_unittest.cc (right):

http://codereview.chromium.org/15070/diff/1/3#newcode24
Line 24: scoped_ptr<net::ProxyService>
proxy_service(net::ProxyService::CreateNull());
Re-introducing Proxy::CreateNull() after the earlier CL got scrapped.

[wtc, 2008-12-19 19:36:05]

LGTM.

Thanks for this wonderful Christmas gift.  It isn't as long
as I feared.

Please fix the nits below before you check in.

You can rename the classes/files later.  (You can also not
rename ProxyConfigFetcher.)

http://codereview.chromium.org/15070/diff/232/247
File net/proxy/proxy_config_fetcher_ie.h (right):

http://codereview.chromium.org/15070/diff/232/247#newcode13
Line 13: // used by Internet Explorer.
You may want to call these "WinInet proxy settings" or
"system proxy settings" instead.

http://codereview.chromium.org/15070/diff/232/247#newcode14
Line 14: class ProxyConfigFetcherIE : public ProxyConfigFetcher {
Unless we will have a second ProxyConfigFetcher
implementation for Windows, it is better to name this class
ProxyConfigFetcherWin.

http://codereview.chromium.org/15070/diff/232/247#newcode16
Line 16: virtual int GetProxyConfig(ProxyConfig* config);
Should we add a "// ProxyConfigFetcher methods:" comment?

http://codereview.chromium.org/15070/diff/232/246
File net/proxy/proxy_resolver_mac.h (right):

http://codereview.chromium.org/15070/diff/232/246#newcode22
Line 22: class ProxyConfigFetcherMac : public ProxyConfigFetcher {
Are you trying to avoid creating the proxy_config_fetcher_mac.{h,cc} files? :-)

http://codereview.chromium.org/15070/diff/232/238
File net/proxy/proxy_service.cc (right):

http://codereview.chromium.org/15070/diff/232/238#newcode323
Line 323: // The ProxyResolver is set to NULL, since it should never be called.
Nit: remove the period on this line.

This comment would also be helpful in
ProxyService::Create(const ProxyInfo* pi) above.

http://codereview.chromium.org/15070/diff/232/239
File net/proxy/proxy_service.h (right):

http://codereview.chromium.org/15070/diff/232/239#newcode29
Line 29: class ProxyConfigFetcher;
Nit: should we list the forward declarations in alphabetical
order?

http://codereview.chromium.org/15070/diff/232/239#newcode298
Line 298: // Synchronously fetch the system's proxy configuration settings.
Called on
I hope this only requires reading some registry keys.  If it
requires doing DNS or DHCP to discover the location of the
PAC script, we'll need to move this off the IO thread.

http://codereview.chromium.org/15070/diff/232/239#newcode300
Line 300: class ProxyConfigFetcher {
You may want to rename this class ProxyConfigService to be
consistent with SSLConfigService.  (See ssl_config_service.h.}

http://codereview.chromium.org/15070/diff/232/239#newcode305
Line 305: // otherwise.  |config| should be in its initial state when this
method is
Nit: "if otherwise" => "otherwise".

Why does |config| need to be in its initial state when this
method is called?  Isn't this method going to fill in
|config|, overwriting whatever was in it before?

http://codereview.chromium.org/15070/diff/232/237
File net/proxy/proxy_service_unittest.cc (right):

http://codereview.chromium.org/15070/diff/232/237#newcode12
Line 12: // TODO(eroman): get rid of synchronous usages of
ProxyService::ProxyResolve().
"ProxyResolve" => "ResolveProxy"?

http://codereview.chromium.org/15070/diff/232/237#newcode16
Line 16: 
Delete this blank line?

[eroman, 2008-12-19 20:57:41]

thx for the review.
all comments addressed; will commit shortly.

http://codereview.chromium.org/15070/diff/232/247
File net/proxy/proxy_config_fetcher_ie.h (right):

http://codereview.chromium.org/15070/diff/232/247#newcode13
Line 13: // used by Internet Explorer.
On 2008/12/19 19:36:05, wtc wrote:
> You may want to call these "WinInet proxy settings" or
> "system proxy settings" instead.

Done.

http://codereview.chromium.org/15070/diff/232/247#newcode14
Line 14: class ProxyConfigFetcherIE : public ProxyConfigFetcher {
On 2008/12/19 19:36:05, wtc wrote:
> Unless we will have a second ProxyConfigFetcher
> implementation for Windows, it is better to name this class
> ProxyConfigFetcherWin.

Done. Renamed to ProxyConfigServiceWin.

http://codereview.chromium.org/15070/diff/232/247#newcode16
Line 16: virtual int GetProxyConfig(ProxyConfig* config);
On 2008/12/19 19:36:05, wtc wrote:
> Should we add a "// ProxyConfigFetcher methods:" comment?

Done.

http://codereview.chromium.org/15070/diff/232/246
File net/proxy/proxy_resolver_mac.h (right):

http://codereview.chromium.org/15070/diff/232/246#newcode22
Line 22: class ProxyConfigFetcherMac : public ProxyConfigFetcher {
On 2008/12/19 19:36:05, wtc wrote:
> Are you trying to avoid creating the proxy_config_fetcher_mac.{h,cc} files?
:-)

I fear mac build files more than the Chewbacabra.
Will split those files in a second nail-biting changelist.

http://codereview.chromium.org/15070/diff/232/238
File net/proxy/proxy_service.cc (right):

http://codereview.chromium.org/15070/diff/232/238#newcode323
Line 323: // The ProxyResolver is set to NULL, since it should never be called.
On 2008/12/19 19:36:05, wtc wrote:
> Nit: remove the period on this line.

Done.

> 
> This comment would also be helpful in
> ProxyService::Create(const ProxyInfo* pi) above.

Done.

http://codereview.chromium.org/15070/diff/232/239
File net/proxy/proxy_service.h (right):

http://codereview.chromium.org/15070/diff/232/239#newcode29
Line 29: class ProxyConfigFetcher;
On 2008/12/19 19:36:05, wtc wrote:
> Nit: should we list the forward declarations in alphabetical
> order?

Done.

http://codereview.chromium.org/15070/diff/232/239#newcode298
Line 298: // Synchronously fetch the system's proxy configuration settings.
Called on
On 2008/12/19 19:36:05, wtc wrote:
> I hope this only requires reading some registry keys.  If it
> requires doing DNS or DHCP to discover the location of the
> PAC script, we'll need to move this off the IO thread.

According to a comment in ProxyService::ResolveProxy():

// The overhead of calling WinHttpGetIEProxyConfigForCurrentUser is very low.
const TimeDelta kProxyConfigMaxAge = TimeDelta::FromSeconds(5);


I have moved this comment into proxy_config_service_win.cc.
And added a similar platform-independent comment to proxy_service.h, claiming
that implementations must execute quickly.

http://codereview.chromium.org/15070/diff/232/239#newcode300
Line 300: class ProxyConfigFetcher {
On 2008/12/19 19:36:05, wtc wrote:
> You may want to rename this class ProxyConfigService to be
> consistent with SSLConfigService.  (See ssl_config_service.h.}

Done.

I like that name much better, thank you.

http://codereview.chromium.org/15070/diff/232/239#newcode305
Line 305: // otherwise.  |config| should be in its initial state when this
method is
On 2008/12/19 19:36:05, wtc wrote:
> Nit: "if otherwise" => "otherwise".
> 
> Why does |config| need to be in its initial state when this
> method is called?  Isn't this method going to fill in
> |config|, overwriting whatever was in it before?

That is not a new comment, I am simply moving lines around.

That subtle comment still applies, so I have left it in. I believe what it means
is, implementations rely on |config| having default values, since they do not
set all the members. For example ProxyResolverWinHttp does:

  if (ie_config.fAutoDetect)
    config->auto_detect = true;

Rather than:

    config->auto_detect = ie_config.fAutoDetect;

SO if config->auto_detect was set to true when calling, it may remain
(incorrectly) as true on return.

In a subsequent CL I can go through the code and remove the need for this
guarantee. (But I don't want to do that here since it would make the change
harder to understand).

http://codereview.chromium.org/15070/diff/232/237
File net/proxy/proxy_service_unittest.cc (right):

http://codereview.chromium.org/15070/diff/232/237#newcode12
Line 12: // TODO(eroman): get rid of synchronous usages of
ProxyService::ProxyResolve().
On 2008/12/19 19:36:05, wtc wrote:
> "ProxyResolve" => "ResolveProxy"?

Done.

Somehow my brain refuses to order {"proxy", "resolve", "url"} properly.

http://codereview.chromium.org/15070/diff/232/237#newcode16
Line 16: 
On 2008/12/19 19:36:05, wtc wrote:
> Delete this blank line?

Done.