Whenever proxy configurations contain socks and http/https/ftp proxies, socks...

Whenever proxy configurations contain socks and http/https/ftp proxies, socks configuration over rid the other proxies. Fixes the issue. Attached test cases.

BUG=15738
TEST=unittest (ProxyConfigTest.ParseProxyRules, ProxyServiceTest.DefaultProxyFallbackToSOCKS)



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

[Arindam, 2009-07-06 03:40:33]

http://codereview.chromium.org/149191/diff/1/3
File net/proxy/proxy_config.cc (right):

http://codereview.chromium.org/149191/diff/1/3#newcode67
Line 67: proxy_server_token.insert(0, "socks://");
I append socks:// to make it look like socks://socks-proxy
since otherwise the default port will not be taken up.

http://codereview.chromium.org/149191/diff/1/3#newcode97
Line 97: if (scheme == "socks")
socks is mapped to single_proxy

(I am assuming the proxy configuration uses single_proxy as a fallback, i.e. if
no specific proxy is attached to https, we use socks.

[eroman, 2009-07-06 23:32:48]

Note that there is still a problem in ProxyService::ApplyProxyRules().
In particular check out this clause:

case ProxyConfig::ProxyRules::TYPE_PROXY_PER_SCHEME: {
  const ProxyServer* entry = proxy_rules.MapSchemeToProxy(url.scheme());
  if (entry) {
    result->UseProxyServer(*entry);
  } else {
    // We failed to find a matching proxy server for the current URL
    // scheme. Default to direct.
    result->UseDirect();
  }
  break;
}

As written right now, it will not fallback to SOCKS when there were other
per-scheme overrides.

(1) Please add a unit-test that covers this case.
(2) See my comment written in MapSchemeToProxy() on how I think we should solve
this -- by introducing a MapUrlSchemeToProxy() method.

http://codereview.chromium.org/149191/diff/1005/1007
File net/proxy/proxy_config.cc (right):

http://codereview.chromium.org/149191/diff/1005/1007#newcode67
Line 67: proxy_server_token.find("://") == std::string::npos)
I feel the right thing to do here, is instead modify ProxyServer::FromURI() to
take a |Scheme default_scheme| argument.

Then this block can become:

ProxyServer::Scheme default_scheme = (entry == &proxy_for_socks) ?
    ProxyServer::SCHEME_SOCKS4 : ProxyServer::SCHEME_HTTP;

*entry = ProxyServer::FromURI(
    proxy_server_for_scheme.token(), default_scheme);

Of course, the annoying part is that since google-style does not allow default
arguments, you now need to update all the existing callers of
ProxyServer::FromURI from:
  ProxyServer::FromURI(uri);
to:
  ProxyServer::FromURI(uri, ProxyServer::SCHEME_HTTP);

http://codereview.chromium.org/149191/diff/1005/1007#newcode98
Line 98: return &single_proxy;
Instead of re-using single_proxy, please add a new |proxy_for_socks| variable.

Also, see my comment earlier about problem in proxy_service.cc because it is
also using this method.

I think the best thing to do is to split the current MapSchemeToProxy() into two
methods, to reflect the two desired behaviors.

// (1)
public:
  const ProxyServer* MapUrlSchemeToProxy()
// Returns &proxy_for_socks instead of NULL in cases where 
// proxy_for_socks.is_valid() -- i.e. fallback to SOCKS if there
// is nothing more specific.

// (2)
private:
 const ProxyServer* MapUrlSchemeToProxy()
// Internal method used by ParseFromString(). basically the same
// as the current version; |if (scheme == "socks")| returns
// &proxy_for_socks.

http://codereview.chromium.org/149191/diff/1005/1006
File net/proxy/proxy_config_unittest.cc (right):

http://codereview.chromium.org/149191/diff/1005/1006#newcode186
Line 186: "http=httpproxy ; https=httpsproxy ; ftp=ftpproxy ; socks=foopy ",
Can you copy this test case into "proxy_config_service_win_unittest.cc", with
the exception of there being no spaces, and the port numbers being specified ?

(So we hard-code a platform-specific example, in a platform-specific unit-test).

[Arindam, 2009-07-08 05:45:56]

Added test to verify SOCKS fallback

http://codereview.chromium.org/149191/diff/1005/1007
File net/proxy/proxy_config.cc (right):

http://codereview.chromium.org/149191/diff/1005/1007#newcode67
Line 67: proxy_server_token.find("://") == std::string::npos)
On 2009/07/06 23:32:48, eroman wrote:
> I feel the right thing to do here, is instead modify ProxyServer::FromURI() to
> take a |Scheme default_scheme| argument.
> 
> Then this block can become:
> 
> ProxyServer::Scheme default_scheme = (entry == &proxy_for_socks) ?
>     ProxyServer::SCHEME_SOCKS4 : ProxyServer::SCHEME_HTTP;
> 
> *entry = ProxyServer::FromURI(
>     proxy_server_for_scheme.token(), default_scheme);
> 
> Of course, the annoying part is that since google-style does not allow default
> arguments, you now need to update all the existing callers of
> ProxyServer::FromURI from:
>   ProxyServer::FromURI(uri);
> to:
>   ProxyServer::FromURI(uri, ProxyServer::SCHEME_HTTP);
> 
Done.
The last part was truly annoying, though :(

http://codereview.chromium.org/149191/diff/1005/1007#newcode98
Line 98: return &single_proxy;
On 2009/07/06 23:32:48, eroman wrote:
> Instead of re-using single_proxy, please add a new |proxy_for_socks| variable.
> 
> Also, see my comment earlier about problem in proxy_service.cc because it is
> also using this method.
> 
> I think the best thing to do is to split the current MapSchemeToProxy() into
two
> methods, to reflect the two desired behaviors.
> 
> // (1)
> public:
>   const ProxyServer* MapUrlSchemeToProxy()
> // Returns &proxy_for_socks instead of NULL in cases where 
> // proxy_for_socks.is_valid() -- i.e. fallback to SOCKS if there
> // is nothing more specific.
> 
> // (2)
> private:
>  const ProxyServer* MapUrlSchemeToProxy()
> // Internal method used by ParseFromString(). basically the same
> // as the current version; |if (scheme == "socks")| returns
> // &proxy_for_socks.
> 
> 

Done. Using |socks_proxy|, I took your advice and renamed the function to
MapUrlSchemeToProxy(), although I do not understand the context of Url here.

[eroman, 2009-07-08 07:18:42]

LGTM!

thanks for doing the larger change (adding the extra parameter to FromURI).

http://codereview.chromium.org/149191/diff/1005/1007
File net/proxy/proxy_config.cc (right):

http://codereview.chromium.org/149191/diff/1005/1007#newcode98
Line 98: return &single_proxy;
On 2009/07/08 05:45:56, Arindam wrote:
> On 2009/07/06 23:32:48, eroman wrote:
> > Instead of re-using single_proxy, please add a new |proxy_for_socks|
variable.
> > 
> > Also, see my comment earlier about problem in proxy_service.cc because it is
> > also using this method.
> > 
> > I think the best thing to do is to split the current MapSchemeToProxy() into
> two
> > methods, to reflect the two desired behaviors.
> > 
> > // (1)
> > public:
> >   const ProxyServer* MapUrlSchemeToProxy()
> > // Returns &proxy_for_socks instead of NULL in cases where 
> > // proxy_for_socks.is_valid() -- i.e. fallback to SOCKS if there
> > // is nothing more specific.
> > 
> > // (2)
> > private:
> >  const ProxyServer* MapUrlSchemeToProxy()
> > // Internal method used by ParseFromString(). basically the same
> > // as the current version; |if (scheme == "socks")| returns
> > // &proxy_for_socks.
> > 
> > 
> 
> Done. Using |socks_proxy|, I took your advice and renamed the function to
> MapUrlSchemeToProxy(), although I do not understand the context of Url here.

My thinking behind "UrlScheme" is in how ProxyService calls it -- notably, by
passing |url.scheme()|:

    const ProxyServer* entry = proxy_rules.MapUrlSchemeToProxy(url.scheme());

since "socks://" is not a "URL scheme", naming it this way makes it more
apparant that "socks" is not an expected input. Whereas for MapSchemeToProxy()
it can be defined to have meaning (the latter function still has a weak name,
but I am at a loss for suggestions on it).

http://codereview.chromium.org/149191/diff/1005/1006
File net/proxy/proxy_config_unittest.cc (right):

http://codereview.chromium.org/149191/diff/1005/1006#newcode186
Line 186: "http=httpproxy ; https=httpsproxy ; ftp=ftpproxy ; socks=foopy ",
On 2009/07/06 23:32:48, eroman wrote:
> Can you copy this test case into "proxy_config_service_win_unittest.cc", with
> the exception of there being no spaces, and the port numbers being specified ?
> 
> (So we hard-code a platform-specific example, in a platform-specific
unit-test).

Any comment on this ask? You can skip it, but I think it is a good idea for
testing/documenting the different tiers.

http://codereview.chromium.org/149191/diff/1038/13
File net/proxy/proxy_config.cc (right):

http://codereview.chromium.org/149191/diff/1038/13#newcode66
Line 66: if (const ProxyServer* entry = MapSchemeToProxy(url_scheme)) {
See suggestion about making MapSchemeToProxy non-const.

http://codereview.chromium.org/149191/diff/1038/13#newcode68
Line 68: if (url_scheme == "socks") {
can you write this as |entry == &socks_proxy| instead? -- that way we leave the
string checking focused in just MapSchemeToProxy.

http://codereview.chromium.org/149191/diff/1038/13#newcode69
Line 69: *const_cast<ProxyServer*>(entry) =
See suggestion about making MapSchemeToProxy non-const.

http://codereview.chromium.org/149191/diff/1038/13#newcode73
Line 73: *const_cast<ProxyServer*>(entry) =
See suggestion about making MapSchemeToProxy non-const.

http://codereview.chromium.org/149191/diff/1038/13#newcode74
Line 74: ProxyServer::FromURI(proxy_server_token,
minor nit: I suggest breaking the FromURI() call out of the if() block:

ProxyServer::Scheme default_scheme = (entry == &socks_proxy) ?
    ProxyServer::SCHEME_SOCKS4 : ProxyServer::SCHEME_HTTP;
*entry = ProxyServer::FromURI(proxy_server_from_scheme.token(), default_scheme);

Feel free to disregard if you disagree (this is purely an aesthetic thing).

http://codereview.chromium.org/149191/diff/1038/13#newcode83
Line 83: const std::string& scheme) const {
can you rename this parameter to |url_scheme|.

http://codereview.chromium.org/149191/diff/1038/21
File net/proxy/proxy_config.h (right):

http://codereview.chromium.org/149191/diff/1038/21#newcode76
Line 76: const ProxyServer* MapUrlSchemeToProxy(const std::string& scheme)
const;
can you rename this parameter to |url_scheme|

http://codereview.chromium.org/149191/diff/1038/21#newcode96
Line 96: // Set if configuration has SOCKS proxy.
Might want to cluster this with the other TYPE_PROXY_PER_SCHEME entries (i.e.
delete a new line).

http://codereview.chromium.org/149191/diff/1038/21#newcode102
Line 102: const ProxyServer* MapSchemeToProxy(const std::string& scheme) const;
Can you make this return a ProxyServer* (no constness), it will avoid having to
cast |entry|.

(The only reason the original method used const was because that was the public
API usage for it, and it just happened to be re-used internally too).

http://codereview.chromium.org/149191/diff/1038/12
File net/proxy/proxy_config_unittest.cc (right):

http://codereview.chromium.org/149191/diff/1038/12#newcode187
Line 187: net::ProxyConfig::ProxyRules::TYPE_PROXY_PER_SCHEME,
I wander if this shouldn't be TYPE_SINGLE_PROXY.

This is fine as is, and good to go for this change, but something to think
about.

(Since from a consistency point of view, I believe if you start up chrome with
--proxy-server="socks://foopy", you will get TYPE_SINGLE_PROXY, but have the
same behavior as this test case.).

http://codereview.chromium.org/149191/diff/1038/19
File net/proxy/proxy_service_unittest.cc (right):

http://codereview.chromium.org/149191/diff/1038/19#newcode964
Line 964: SyncProxyService service2(new MockProxyConfigService(config),
Is there a need to create multiple SyncProxyServices (as opposed to re-using
service1?)

I see that this is using the same model as the test above, so no big deal either
way. Just curious.

[Arindam, 2009-07-10 19:25:16]

http://codereview.chromium.org/149191/diff/1005/1007
File net/proxy/proxy_config.cc (right):

http://codereview.chromium.org/149191/diff/1005/1007#newcode98
Line 98: return &single_proxy;
On 2009/07/08 07:18:42, eroman wrote:
> On 2009/07/08 05:45:56, Arindam wrote:
> > On 2009/07/06 23:32:48, eroman wrote:
> > > Instead of re-using single_proxy, please add a new |proxy_for_socks|
> variable.
> > > 
> > > Also, see my comment earlier about problem in proxy_service.cc because it
is
> > > also using this method.
> > > 
> > > I think the best thing to do is to split the current MapSchemeToProxy()
into
> > two
> > > methods, to reflect the two desired behaviors.
> > > 
> > > // (1)
> > > public:
> > >   const ProxyServer* MapUrlSchemeToProxy()
> > > // Returns &proxy_for_socks instead of NULL in cases where 
> > > // proxy_for_socks.is_valid() -- i.e. fallback to SOCKS if there
> > > // is nothing more specific.
> > > 
> > > // (2)
> > > private:
> > >  const ProxyServer* MapUrlSchemeToProxy()
> > > // Internal method used by ParseFromString(). basically the same
> > > // as the current version; |if (scheme == "socks")| returns
> > > // &proxy_for_socks.
> > > 
> > > 
> > 
> > Done. Using |socks_proxy|, I took your advice and renamed the function to
> > MapUrlSchemeToProxy(), although I do not understand the context of Url here.
> 
> My thinking behind "UrlScheme" is in how ProxyService calls it -- notably, by
> passing |url.scheme()|:
> 
>     const ProxyServer* entry = proxy_rules.MapUrlSchemeToProxy(url.scheme());
> 
> since "socks://" is not a "URL scheme", naming it this way makes it more
> apparant that "socks" is not an expected input. Whereas for MapSchemeToProxy()
> it can be defined to have meaning (the latter function still has a weak name,
> but I am at a loss for suggestions on it).
Ok, now it begins to make sense for me. Thanks for going through the trouble of
explaining it :)

http://codereview.chromium.org/149191/diff/1005/1006
File net/proxy/proxy_config_unittest.cc (right):

http://codereview.chromium.org/149191/diff/1005/1006#newcode186
Line 186: "http=httpproxy ; https=httpsproxy ; ftp=ftpproxy ; socks=foopy ",
On 2009/07/08 07:18:42, eroman wrote:
> On 2009/07/06 23:32:48, eroman wrote:
> > Can you copy this test case into "proxy_config_service_win_unittest.cc",
with
> > the exception of there being no spaces, and the port numbers being specified
?
> > 
> > (So we hard-code a platform-specific example, in a platform-specific
> unit-test).
> 
> Any comment on this ask? You can skip it, but I think it is a good idea for
> testing/documenting the different tiers.
My idea was that since
a) this tests for correct parsing
b) the other test ProxyServiceTest.DefaultProxyFallbackToSOCKS tests for
fallback, any test for windows will only be a subset of this. Which is why I did
not add it.
Have a look at http://codereview.chromium.org/149191/patch/1067/68 which
contains a disabled test (I still need to move some classes around to get it to
compile)

http://codereview.chromium.org/149191/diff/1038/13
File net/proxy/proxy_config.cc (right):

http://codereview.chromium.org/149191/diff/1038/13#newcode66
Line 66: if (const ProxyServer* entry = MapSchemeToProxy(url_scheme)) {
On 2009/07/08 07:18:42, eroman wrote:
> See suggestion about making MapSchemeToProxy non-const.

Done.

http://codereview.chromium.org/149191/diff/1038/13#newcode68
Line 68: if (url_scheme == "socks") {
On 2009/07/08 07:18:42, eroman wrote:
> can you write this as |entry == &socks_proxy| instead? -- that way we leave
the
> string checking focused in just MapSchemeToProxy.

Done.

http://codereview.chromium.org/149191/diff/1038/13#newcode69
Line 69: *const_cast<ProxyServer*>(entry) =
On 2009/07/08 07:18:42, eroman wrote:
> See suggestion about making MapSchemeToProxy non-const.

Done.

http://codereview.chromium.org/149191/diff/1038/13#newcode73
Line 73: *const_cast<ProxyServer*>(entry) =
On 2009/07/08 07:18:42, eroman wrote:
> See suggestion about making MapSchemeToProxy non-const.

Done.

http://codereview.chromium.org/149191/diff/1038/13#newcode74
Line 74: ProxyServer::FromURI(proxy_server_token,
On 2009/07/08 07:18:42, eroman wrote:
> minor nit: I suggest breaking the FromURI() call out of the if() block:
> 
> ProxyServer::Scheme default_scheme = (entry == &socks_proxy) ?
>     ProxyServer::SCHEME_SOCKS4 : ProxyServer::SCHEME_HTTP;
> *entry = ProxyServer::FromURI(proxy_server_from_scheme.token(),
default_scheme);
> 
> Feel free to disregard if you disagree (this is purely an aesthetic thing).

Done.

http://codereview.chromium.org/149191/diff/1038/13#newcode83
Line 83: const std::string& scheme) const {
On 2009/07/08 07:18:42, eroman wrote:
> can you rename this parameter to |url_scheme|.

Done.

http://codereview.chromium.org/149191/diff/1038/21
File net/proxy/proxy_config.h (right):

http://codereview.chromium.org/149191/diff/1038/21#newcode76
Line 76: const ProxyServer* MapUrlSchemeToProxy(const std::string& scheme)
const;
On 2009/07/08 07:18:42, eroman wrote:
> can you rename this parameter to |url_scheme|

Done.

http://codereview.chromium.org/149191/diff/1038/21#newcode102
Line 102: const ProxyServer* MapSchemeToProxy(const std::string& scheme) const;
On 2009/07/08 07:18:42, eroman wrote:
> Can you make this return a ProxyServer* (no constness), it will avoid having
to
> cast |entry|.
> 
> (The only reason the original method used const was because that was the
public
> API usage for it, and it just happened to be re-used internally too).
Done.
Unfortunately, since MapUrlSchemeToProxy calls this func, I needed to do a
const_cast on |*this| within MapUrlSchemeToProxy().

http://codereview.chromium.org/149191/diff/1038/12
File net/proxy/proxy_config_unittest.cc (right):

http://codereview.chromium.org/149191/diff/1038/12#newcode187
Line 187: net::ProxyConfig::ProxyRules::TYPE_PROXY_PER_SCHEME,
On 2009/07/08 07:18:42, eroman wrote:
> I wander if this shouldn't be TYPE_SINGLE_PROXY.
> 
> This is fine as is, and good to go for this change, but something to think
> about.
> 
> (Since from a consistency point of view, I believe if you start up chrome with
> --proxy-server="socks://foopy", you will get TYPE_SINGLE_PROXY, but have the
> same behavior as this test case.).
I think when we start chrome with --proxy-server="socks://foopy", we would be
parsing the config string "socks://foopy" equivalent to "http-proxy:80", but not
"socks=foopy", which happens only when someone does per scheme configuration (or
via PAC).

http://codereview.chromium.org/149191/diff/1038/19
File net/proxy/proxy_service_unittest.cc (right):

http://codereview.chromium.org/149191/diff/1038/19#newcode964
Line 964: SyncProxyService service2(new MockProxyConfigService(config),
On 2009/07/08 07:18:42, eroman wrote:
> Is there a need to create multiple SyncProxyServices (as opposed to re-using
> service1?)
> 
> I see that this is using the same model as the test above, so no big deal
either
> way. Just curious.
Probably not. I just left it as it is for consistency.
Although I usually find enclosing the code in braces (blocks) helps reduce both
the problems (initialization, re-usage ) ;)

http://codereview.chromium.org/149191/diff/1067/68
File net/proxy/proxy_config_service_win_unittest.cc (right):

http://codereview.chromium.org/149191/diff/1067/68#newcode176
Line 176: #if defined(NOT_DEFINED_VALUE)
I believe this is the test you wanted. However, as mentioned earlier, this
probably is a repeat of what is happening in proxy_service_unittest.cc and
proxy_config_unittest.cc

[eroman, 2009-07-10 21:24:46]

LGTM

http://codereview.chromium.org/149191/diff/1038/19
File net/proxy/proxy_service_unittest.cc (right):

http://codereview.chromium.org/149191/diff/1038/19#newcode964
Line 964: SyncProxyService service2(new MockProxyConfigService(config),
On 2009/07/10 19:25:17, Arindam wrote:
> On 2009/07/08 07:18:42, eroman wrote:
> > Is there a need to create multiple SyncProxyServices (as opposed to re-using
> > service1?)
> > 
> > I see that this is using the same model as the test above, so no big deal
> either
> > way. Just curious.
> Probably not. I just left it as it is for consistency.

Ok cool, thats fine.

> Although I usually find enclosing the code in braces (blocks) helps reduce
both
> the problems (initialization, re-usage ) ;)
> 

Strongly agreed! But that is a refactor for another time :)

http://codereview.chromium.org/149191/diff/1067/56
File net/proxy/proxy_config.cc (right):

http://codereview.chromium.org/149191/diff/1067/56#newcode80
Line 80: if (proxy_server != NULL && proxy_server->is_valid())
nit: leave off the "!= NULL" .

http://codereview.chromium.org/149191/diff/1067/66
File net/proxy/proxy_config_service_linux.cc (right):

http://codereview.chromium.org/149191/diff/1067/66#newcode390
Line 390: // TODO(arindam): verify on linux
what is this todo about? this looks good to me.

you may want to instead add a TODO about cleaning-up "FixupProxyHostScheme()" --
now that we have the default_scheme parameter, there is no need for it to be
manipulating the URI strings.

http://codereview.chromium.org/149191/diff/1067/67
File net/proxy/proxy_config_service_win.h (right):

http://codereview.chromium.org/149191/diff/1067/67#newcode25
Line 25: FRIEND_TEST(ProxyConfigServiceWinTest, FallbackToSOCKSProxy);
probably no need for this --  in my opinion, this test can be added within
SetFromIEConfig().

http://codereview.chromium.org/149191/diff/1067/68
File net/proxy/proxy_config_service_win_unittest.cc (right):

http://codereview.chromium.org/149191/diff/1067/68#newcode176
Line 176: #if defined(NOT_DEFINED_VALUE)
On 2009/07/10 19:25:17, Arindam wrote:
> I believe this is the test you wanted. However, as mentioned earlier, this
> probably is a repeat of what is happening in proxy_service_unittest.cc and
> proxy_config_unittest.cc

The test I was thinking of is to add an extra block to the existing
"ProxyConfigServiceWinTest.SetFromIEConfig" test, similar to the existing
"Per-scheme proxy rules." test in that file (see code block below).

Although the winhttp-based config service happens to share code with the
ProxyRules parser, this is just a coincidence (I just happens that the formats
are close enough to be compatible).

But It could well be that in the future, a change is made to the format for
ProxyRules, and this could break the winhttp parser which has a fixed input
format. If we have tests for ProxyConfigServiceWin however, such a policy change
would not go unnoticed, since we verify that the format we care about works.



    {
      { // Input.
        FALSE,                                            // fAutoDetect
        NULL,                                             // lpszAutoConfigUrl
       
L"http=httpproxy:80;https=httpsproxy:70;ftp=ftpproxy:20;socks=foopy:130",  //
lpszProxy
        NULL,                                             // lpszProxy_bypass
      },

      // Expected result.
      false,                                   // auto_detect
      GURL(),                                  // pac_url
      MakeProxyPerSchemeRules("httpproxy:80", "httpsproxy:70", "ftpproxy:20",
"foopy:130"),
      "",                                      // proxy_bypass_list
      false,                                   // bypass_local_names
    },

http://codereview.chromium.org/149191/diff/1067/59
File net/proxy/proxy_server.h (right):

http://codereview.chromium.org/149191/diff/1067/59#newcode73
Line 73: // Examples:
maybe here we should note that default_scheme=SCHEME_HTTP.

// Examples (for |default_scheme=SCHEME_HTTP|).

[Arindam, 2009-07-11 00:53:34]

Please have a look at 
http://codereview.chromium.org/149191/diff/1067/61#newcode36
http://codereview.chromium.org/149191/diff/1067/56#newcode80

http://codereview.chromium.org/149191/diff/1067/56
File net/proxy/proxy_config.cc (right):

http://codereview.chromium.org/149191/diff/1067/56#newcode80
Line 80: if (proxy_server != NULL && proxy_server->is_valid())
On 2009/07/10 21:24:46, eroman wrote:
> nit: leave off the "!= NULL" .

We need this, in case the url_scheme is invalid, then MapSchemeToProxy returns
NULL. This is required for this case :
http://codereview.chromium.org/149191/diff/1079/76#oldcode184

http://codereview.chromium.org/149191/diff/1067/61
File net/proxy/proxy_config_service_common_unittest.h (right):

http://codereview.chromium.org/149191/diff/1067/61#newcode36
Line 36: const char* socks_proxy);
I am wondering whether this is acceptable google code. I let
MakeProxyPerSchemeRules have an optional parameter by polymorphing it.

http://codereview.chromium.org/149191/diff/1067/66
File net/proxy/proxy_config_service_linux.cc (right):

http://codereview.chromium.org/149191/diff/1067/66#newcode390
Line 390: // TODO(arindam): verify on linux
On 2009/07/10 21:24:46, eroman wrote:
> what is this todo about? this looks good to me.

It was a legacy comment, I guess.

> you may want to instead add a TODO about cleaning-up "FixupProxyHostScheme()"
--
> now that we have the default_scheme parameter, there is no need for it to be
> manipulating the URI strings.

Done.

http://codereview.chromium.org/149191/diff/1067/67
File net/proxy/proxy_config_service_win.h (right):

http://codereview.chromium.org/149191/diff/1067/67#newcode25
Line 25: FRIEND_TEST(ProxyConfigServiceWinTest, FallbackToSOCKSProxy);
On 2009/07/10 21:24:46, eroman wrote:
> probably no need for this --  in my opinion, this test can be added within
> SetFromIEConfig().

Done.

http://codereview.chromium.org/149191/diff/1067/67#newcode25
Line 25: FRIEND_TEST(ProxyConfigServiceWinTest, FallbackToSOCKSProxy);
On 2009/07/10 21:24:46, eroman wrote:
> probably no need for this --  in my opinion, this test can be added within
> SetFromIEConfig().

Done.

http://codereview.chromium.org/149191/diff/1067/68
File net/proxy/proxy_config_service_win_unittest.cc (right):

http://codereview.chromium.org/149191/diff/1067/68#newcode176
Line 176: #if defined(NOT_DEFINED_VALUE)
On 2009/07/10 21:24:46, eroman wrote:
> On 2009/07/10 19:25:17, Arindam wrote:
> > I believe this is the test you wanted. However, as mentioned earlier, this
> > probably is a repeat of what is happening in proxy_service_unittest.cc and
> > proxy_config_unittest.cc
> 
> The test I was thinking of is to add an extra block to the existing
> "ProxyConfigServiceWinTest.SetFromIEConfig" test, similar to the existing
> "Per-scheme proxy rules." test in that file (see code block below).
> 
> Although the winhttp-based config service happens to share code with the
> ProxyRules parser, this is just a coincidence (I just happens that the formats
> are close enough to be compatible).
> 
> But It could well be that in the future, a change is made to the format for
> ProxyRules, and this could break the winhttp parser which has a fixed input
> format. If we have tests for ProxyConfigServiceWin however, such a policy
change
> would not go unnoticed, since we verify that the format we care about works.
> 
> 
> 
>     {
>       { // Input.
>         FALSE,                                            // fAutoDetect
>         NULL,                                             // lpszAutoConfigUrl
>        
> L"http=httpproxy:80;https=httpsproxy:70;ftp=ftpproxy:20;socks=foopy:130",  //
> lpszProxy
>         NULL,                                             // lpszProxy_bypass
>       },
> 
>       // Expected result.
>       false,                                   // auto_detect
>       GURL(),                                  // pac_url
>       MakeProxyPerSchemeRules("httpproxy:80", "httpsproxy:70", "ftpproxy:20",
> "foopy:130"),
>       "",                                      // proxy_bypass_list
>       false,                                   // bypass_local_names
>     },
> 

Now I get it. Done.

http://codereview.chromium.org/149191/diff/1067/59
File net/proxy/proxy_server.h (right):

http://codereview.chromium.org/149191/diff/1067/59#newcode73
Line 73: // Examples:
On 2009/07/10 21:24:46, eroman wrote:
> maybe here we should note that default_scheme=SCHEME_HTTP.
> 
> // Examples (for |default_scheme=SCHEME_HTTP|).

Done.

http://codereview.chromium.org/149191/diff/1079/76
File net/proxy/proxy_config_unittest.cc (left):

http://codereview.chromium.org/149191/diff/1079/76#oldcode184
Line 184: // Include unsupported schemes -- they are discarded.
for this test, the MapSchemeToProxy() returns NULL ProxyServer* pointers.

[eroman, 2009-07-13 23:41:31]

LGTM.

http://codereview.chromium.org/149191/diff/1067/56
File net/proxy/proxy_config.cc (right):

http://codereview.chromium.org/149191/diff/1067/56#newcode80
Line 80: if (proxy_server != NULL && proxy_server->is_valid())
On 2009/07/11 00:53:35, Arindam wrote:
> On 2009/07/10 21:24:46, eroman wrote:
> > nit: leave off the "!= NULL" .
> 
> We need this, in case the url_scheme is invalid, then MapSchemeToProxy returns
> NULL. This is required for this case :
> http://codereview.chromium.org/149191/diff/1079/76#oldcode184

What I mean was purely a stylistic comment:

Rewrite 
  if (proxy_server != NULL && proxy_server->is_valid())
AS:
  if (proxy_server && proxy_server->is_valid())

(We prefer the former style in the network code.)

http://codereview.chromium.org/149191/diff/1067/61
File net/proxy/proxy_config_service_common_unittest.h (right):

http://codereview.chromium.org/149191/diff/1067/61#newcode36
Line 36: const char* socks_proxy);
On 2009/07/11 00:53:35, Arindam wrote:
> I am wondering whether this is acceptable google code. I let
> MakeProxyPerSchemeRules have an optional parameter by polymorphing it.

Yep, this is fine.

For test code, it is perfectly valid to do these sorts of things for
convenience. It for non-test code that one needs to be more skeptical of
function overloading (since it can be a source of bugs/misunderstandings).

http://codereview.chromium.org/149191/diff/1079/87
File net/proxy/proxy_config_service_linux.cc (right):

http://codereview.chromium.org/149191/diff/1079/87#newcode58
Line 58: // TODO(arindam): Remove URI string manipulation by using
MapUrlSchemeToProxy.
OK.

Although specifically the cleanup I was thinking was to simply pass in a default
scheme. I.E the the _caller_ of FixupProxyHostScheme() currently ends up doing:

ProxyServer proxy_server =
    ProxyServer::FromURI(env_value, ProxyServer::SCHEME_HTTP);

Really instead of SCHEME_HTTP, they should be passing SCHEME_SOCKS4 /
SCHEME_SOCKS5 in the case where the env value is for a SOCKS proxy type (so
FixupProxyHostScheme() doesn't need to complete it to a full scheme://host
string).

http://codereview.chromium.org/149191/diff/1079/88
File net/proxy/proxy_config_service_win_unittest.cc (right):

http://codereview.chromium.org/149191/diff/1079/88#newcode117
Line 117: L"ftp=ftpproxy:20;socks=foopy:130",                // lpszProxy
nit: one less space to lineup the comment :)

http://codereview.chromium.org/149191/diff/1079/88#newcode124
Line 124: MakeProxyPerSchemeRules("www.google.com:80", "www.foo.com:110",
Thanks!