Making command-line specified proxy settings more flexible - allowing for set...

Making command-line specified proxy settings more flexible - allowing for setting of auto-detect, pac url, per-schema proxy settings, proxy bypass urls.

BUG=http://crbug.com/266


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

[darin (slow to review), 2009-05-06 21:36:21]

http://codereview.chromium.org/115029/diff/1038/1050
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/115029/diff/1038/1050#newcode243
Line 243: // Specify a list of URLs for whom we bypass proxy settings and use
direct
while you are at it, it might be nice to add a --proxy-server-none option.  or
something like that to disable the use of proxies.

[eroman, 2009-05-07 21:11:38]

looks good! just a few comments I would like addressed.

you should also list "BUG=266" in the description, as this will be useful to
that issue (gives users a solution for bypassing the windows proxy settings)

http://codereview.chromium.org/115029/diff/1057/1069
File chrome/browser/net/chrome_url_request_context.cc (right):

http://codereview.chromium.org/115029/diff/1057/1069#newcode41
Line 41: // Ignore all the other proxy config information if we get the
Might be useful to log a warning if any of the other flags were specified.

http://codereview.chromium.org/115029/diff/1057/1072
File chrome/browser/net/chrome_url_request_context_unittest.cc (right):

http://codereview.chromium.org/115029/diff/1057/1072#newcode16
Line 16: extern net::ProxyConfig* CreateProxyConfig(const CommandLine&
command_line);
how about exposing this in chrome_url_request_context.h ?

http://codereview.chromium.org/115029/diff/1057/1063
File net/proxy/proxy_config.cc (right):

http://codereview.chromium.org/115029/diff/1057/1063#newcode7
Line 7: #include "base/command_line.h"
Please remove.

http://codereview.chromium.org/115029/diff/1057/1063#newcode10
Line 10: #include "chrome/common/chrome_switches.h"
Ditto to above.
The net module should not include anything from "chrome/".

http://codereview.chromium.org/115029/diff/1057/1063#newcode69
Line 69: void ProxyConfig::ProxyRules::SetDirect() {
Do you feel strongly about having this method? The default constructor will
create a config with type = TYPE_NO_RULES. Which is treated by ProxyService as
meaning direct connection. I would prefer using such default-constructed
configs.

http://codereview.chromium.org/115029/diff/1057/1063#newcode124
Line 124: while (no_proxy_list.GetNext()) {
I didn't look too hard through this, it is just moving existing code right?

http://codereview.chromium.org/115029/diff/1057/1064
File net/proxy/proxy_config.h (right):

http://codereview.chromium.org/115029/diff/1057/1064#newcode15
Line 15: class CommandLine;
Please remove this.

http://codereview.chromium.org/115029/diff/1057/1064#newcode71
Line 71: void SetDirect();
The default constructor which initializes to TYPE_NO_RULES is equivalent to
direct. This method probably isn't necessary.

http://codereview.chromium.org/115029/diff/1057/1064#newcode100
Line 100: // configurations should be bypassed and appends them to proxy_bypass.
Instead of defining this as an appending operation how about making it a setting
method (i.e. it starts by clearing proxy_bypass).

Also, now that this has been moved from proxy_config_service_linux, can you move
add some unit tests under proxy_config_unittests.cc ?

http://codereview.chromium.org/115029/diff/1057/1065
File net/proxy/proxy_config_service_fixed.h (right):

http://codereview.chromium.org/115029/diff/1057/1065#newcode1
Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
should this range be updated?

http://codereview.chromium.org/115029/diff/1057/1062
File net/proxy/proxy_script_fetcher_unittest.cc (right):

http://codereview.chromium.org/115029/diff/1057/1062#newcode31
Line 31: no_proxy.proxy_rules.SetDirect();
See earlier comment. I would recommend deleting this line, and letting the
default construct init it to "direct".

http://codereview.chromium.org/115029/diff/1057/1060
File net/proxy/proxy_service.cc (right):

http://codereview.chromium.org/115029/diff/1057/1060#newcode200
Line 200: proxy_config_service = new ProxyConfigServiceFixed(*pc);
I believe this change will cause a regression under linux.

In particular, on the linux codepath, this will now return CreateNull()  --
(which means no proxy).
Whereas before it would return a Proxy service with fixed config service.

When addressing this, also be aware of the subtlety between ProxyService::Create
vs ProxyService::CreateUsingV8Resolver (see the header).
Basically, on linux we don't have a fallback to use for evaluating PAC (we have
only V8 implementation). Hence in this function (which is reached when for one
reason or another we aren't to use V8), we don't have the ability to support a
|pc| which could need a ProxyResolver.

So if you end up with returning
   new ProxyService(new ProxyConfigServiceFixed(*pc), NULL)
on linux, be aware that if the proxy service indicates to use PAC then
ProxyService will try to use the ProxyResolver argument (which in this case will
be NULL and hence crash).

http://codereview.chromium.org/115029/diff/1057/1061
File net/proxy/proxy_service.h (right):

http://codereview.chromium.org/115029/diff/1057/1061#newcode89
Line 89: // Creates a proxy service using the specified settings. If |pi| is
NULL then
|pi| --> |pc|

http://codereview.chromium.org/115029/diff/1057/1061#newcode94
Line 94: // Creates a proxy service using the specified settings. If |pi| is
NULL then
|pi| --> |pc|.

http://codereview.chromium.org/115029/diff/1057/1061#newcode96
Line 96: // same as Create(const ProxyInfo*), however under the hood it uses a
ProxyInfo --> ProxyConfig

http://codereview.chromium.org/115029/diff/1057/1058
File webkit/tools/test_shell/test_shell_request_context.cc (right):

http://codereview.chromium.org/115029/diff/1057/1058#newcode33
Line 33: proxy_config.proxy_rules.SetDirect();
same comment as earlier, I would just delete this line and let default
constructor do its thang.

[robertshield, 2009-05-08 16:30:40]

Thanks for your feedback, I've addressed individual comments below. I've also
added a few more unit tests.

I've got a little bit of mac/linux linking grief to resolve (my gyp-fu isn't yet
very strong) but I figured I'd post these responses now. 

Thanks,

Robert

http://codereview.chromium.org/115029/diff/1038/1050
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/115029/diff/1038/1050#newcode243
Line 243: // Specify a list of URLs for whom we bypass proxy settings and use
direct
On 2009/05/06 21:36:21, darin wrote:
> while you are at it, it might be nice to add a --proxy-server-none option.  or
> something like that to disable the use of proxies.

Added, thanks.

http://codereview.chromium.org/115029/diff/1057/1069
File chrome/browser/net/chrome_url_request_context.cc (right):

http://codereview.chromium.org/115029/diff/1057/1069#newcode41
Line 41: // Ignore all the other proxy config information if we get the
On 2009/05/07 21:11:38, eroman wrote:
> Might be useful to log a warning if any of the other flags were specified.

Good idea, done.

http://codereview.chromium.org/115029/diff/1057/1072
File chrome/browser/net/chrome_url_request_context_unittest.cc (right):

http://codereview.chromium.org/115029/diff/1057/1072#newcode16
Line 16: extern net::ProxyConfig* CreateProxyConfig(const CommandLine&
command_line);
On 2009/05/07 21:11:38, eroman wrote:
> how about exposing this in chrome_url_request_context.h ?

Done.

http://codereview.chromium.org/115029/diff/1057/1063
File net/proxy/proxy_config.cc (right):

http://codereview.chromium.org/115029/diff/1057/1063#newcode7
Line 7: #include "base/command_line.h"
On 2009/05/07 21:11:38, eroman wrote:
> Please remove.

Done.

http://codereview.chromium.org/115029/diff/1057/1063#newcode10
Line 10: #include "chrome/common/chrome_switches.h"
On 2009/05/07 21:11:38, eroman wrote:
> Ditto to above.
> The net module should not include anything from "chrome/".

Yep, sorry, this was some left-over cruft. I had tried to move the ProxyConfig
command line parsing into this class before realizing that that would introduce
a net->chrome/common dependency. I backed out most of that but these stragglers
got missed. Now fixed.

http://codereview.chromium.org/115029/diff/1057/1063#newcode69
Line 69: void ProxyConfig::ProxyRules::SetDirect() {
On 2009/05/07 21:11:38, eroman wrote:
> Do you feel strongly about having this method? The default constructor will
> create a config with type = TYPE_NO_RULES. Which is treated by ProxyService as
> meaning direct connection. I would prefer using such default-constructed
> configs.

I built this to avoid the TYPE_NO_RULES state since that felt kind of like it
meant "this object hasn't been initialized yet". I hadn't grokked the
TYPE_NO_RULES == "direct://" link at the time. I've removed the SetDirect()
method and documented how the default settings result in direct connections
being made.

http://codereview.chromium.org/115029/diff/1057/1063#newcode124
Line 124: while (no_proxy_list.GetNext()) {
On 2009/05/07 21:11:38, eroman wrote:
> I didn't look too hard through this, it is just moving existing code right?

Yep, it's a straight move of Stephane's parsing code.

http://codereview.chromium.org/115029/diff/1057/1064
File net/proxy/proxy_config.h (right):

http://codereview.chromium.org/115029/diff/1057/1064#newcode71
Line 71: void SetDirect();
On 2009/05/07 21:11:38, eroman wrote:
> The default constructor which initializes to TYPE_NO_RULES is equivalent to
> direct. This method probably isn't necessary.

Removed and added documentation to this effect (see earlier comment response).

http://codereview.chromium.org/115029/diff/1057/1064#newcode100
Line 100: // configurations should be bypassed and appends them to proxy_bypass.
On 2009/05/07 21:11:38, eroman wrote:
> Instead of defining this as an appending operation how about making it a
setting
> method (i.e. it starts by clearing proxy_bypass).
> 

Ok, done.

> Also, now that this has been moved from proxy_config_service_linux, can you
move
> add some unit tests under proxy_config_unittests.cc ?

I've added some of tests to proxy_config_unittests.cc.

http://codereview.chromium.org/115029/diff/1057/1065
File net/proxy/proxy_config_service_fixed.h (right):

http://codereview.chromium.org/115029/diff/1057/1065#newcode1
Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
On 2009/05/07 21:11:38, eroman wrote:
> should this range be updated?

I've read conflicting accounts. Updated in any case.

http://codereview.chromium.org/115029/diff/1057/1062
File net/proxy/proxy_script_fetcher_unittest.cc (right):

http://codereview.chromium.org/115029/diff/1057/1062#newcode31
Line 31: no_proxy.proxy_rules.SetDirect();
On 2009/05/07 21:11:38, eroman wrote:
> See earlier comment. I would recommend deleting this line, and letting the
> default construct init it to "direct".

Done.

http://codereview.chromium.org/115029/diff/1057/1060
File net/proxy/proxy_service.cc (right):

http://codereview.chromium.org/115029/diff/1057/1060#newcode200
Line 200: proxy_config_service = new ProxyConfigServiceFixed(*pc);
On 2009/05/07 21:11:38, eroman wrote:
> I believe this change will cause a regression under linux.
> 
> In particular, on the linux codepath, this will now return CreateNull()  --
> (which means no proxy).
> Whereas before it would return a Proxy service with fixed config service.
> 
> When addressing this, also be aware of the subtlety between
ProxyService::Create
> vs ProxyService::CreateUsingV8Resolver (see the header).
> Basically, on linux we don't have a fallback to use for evaluating PAC (we
have
> only V8 implementation). Hence in this function (which is reached when for one
> reason or another we aren't to use V8), we don't have the ability to support a
> |pc| which could need a ProxyResolver.
> 
> So if you end up with returning
>    new ProxyService(new ProxyConfigServiceFixed(*pc), NULL)
> on linux, be aware that if the proxy service indicates to use PAC then
> ProxyService will try to use the ProxyResolver argument (which in this case
will
> be NULL and hence crash).

Yeah, this wasn't right at all :-(

I've taken a stab at addressing this by adding a NeedsResolver() method to
ProxyConfig. If in ProxyService::Create, pc->NeedsResolver() is true, then I
return 
  CreateNull()
otherwise I return 
  new ProxyService(new ProxyConfigServiceFixed(*pc), NULL).

This working is predicated on my implementation of NeedsResolver() being
correct, please could you double check it? (it's basically pc->auto_detect ||
!pc->pac_url.is_empty()).

http://codereview.chromium.org/115029/diff/1057/1061
File net/proxy/proxy_service.h (right):

http://codereview.chromium.org/115029/diff/1057/1061#newcode89
Line 89: // Creates a proxy service using the specified settings. If |pi| is
NULL then
On 2009/05/07 21:11:38, eroman wrote:
> |pi| --> |pc|

Done.

http://codereview.chromium.org/115029/diff/1057/1061#newcode94
Line 94: // Creates a proxy service using the specified settings. If |pi| is
NULL then
On 2009/05/07 21:11:38, eroman wrote:
> |pi| --> |pc|.

Done.

http://codereview.chromium.org/115029/diff/1057/1061#newcode96
Line 96: // same as Create(const ProxyInfo*), however under the hood it uses a
On 2009/05/07 21:11:38, eroman wrote:
> ProxyInfo --> ProxyConfig

Thanks, done.

http://codereview.chromium.org/115029/diff/1057/1058
File webkit/tools/test_shell/test_shell_request_context.cc (right):

http://codereview.chromium.org/115029/diff/1057/1058#newcode33
Line 33: proxy_config.proxy_rules.SetDirect();
On 2009/05/07 21:11:38, eroman wrote:
> same comment as earlier, I would just delete this line and let default
> constructor do its thang.

Done.

[robertshield, 2009-05-09 02:39:40]

I've believe I have now ironed out all the wrinkles, unit tests are added, all
try bot builders are green. Please could you take one more look?

Thanks for putting up with this rather hefty patch :-)

[eroman, 2009-05-12 00:13:47]

LGTM!

http://codereview.chromium.org/115029/diff/1227/1240
File chrome/browser/net/chrome_url_request_context.cc (right):

http://codereview.chromium.org/115029/diff/1227/1240#newcode28
Line 28: const wchar_t* proxy_switches[] = {
I like this change over earlier patch.

Can you make this |static| too?

http://codereview.chromium.org/115029/diff/1227/1241
File chrome/browser/net/chrome_url_request_context.h (right):

http://codereview.chromium.org/115029/diff/1227/1241#newcode104
Line 104: // Sets up proxy info if overrides were specified on the command line.
minor nit: the original comment we had wasn't too good, maybe it could be
rephrased as:

Creates a proxy configuration using the overrides specified on the command line.
Returns NULL if the system defaults should be used instead.

http://codereview.chromium.org/115029/diff/1227/1244
File chrome/browser/net/chrome_url_request_context_unittest.cc (right):

http://codereview.chromium.org/115029/diff/1227/1244#newcode17
Line 17: // Build the input command lines here
nit: end in period.

http://codereview.chromium.org/115029/diff/1227/1247
File net/net.gyp (right):

http://codereview.chromium.org/115029/diff/1227/1247#newcode572
Line 572: 'proxy/proxy_config_service_common_unittest.cc',
good call.

http://codereview.chromium.org/115029/diff/1227/1234
File net/proxy/proxy_config.h (right):

http://codereview.chromium.org/115029/diff/1227/1234#newcode97
Line 97: // configurations should be bypassed. Clears proxy_bypass sets it to
the
nit: "Clears proxy_bypass ^and^ sets ..."

http://codereview.chromium.org/115029/diff/1227/1234#newcode113
Line 113: // Returns true if this config will require the proxy service to use a
Can you rephrase this as "may require" instead of "will require"?

The reason for the uncertainty is "auto detect" -- if we specify autodetect but
WPAD finds no PAC script, then a resolver may not be necessary.

Along these lines, it might be clearer to name this method "MayRequirePAC()".

http://codereview.chromium.org/115029/diff/1227/1238
File net/proxy/proxy_config_unittest.cc (right):

http://codereview.chromium.org/115029/diff/1227/1238#newcode8
Line 8: 
minor nit: is the extra newline intentional?

http://codereview.chromium.org/115029/diff/1227/1230
File net/proxy/proxy_service.cc (right):

http://codereview.chromium.org/115029/diff/1227/1230#newcode218
Line 218: delete proxy_config_service;
instead of these manual deletes, please use:

scoped_ptr<ProxyResolver> proxy_resolver;

and then proxy_resolver.release() for the places where ownership is passed.

http://codereview.chromium.org/115029/diff/1227/1230#newcode219
Line 219: return CreateNull();
Would be great if you can log a warning here too.

[robertshield, 2009-05-12 15:05:27]

Thanks for the review!

http://codereview.chromium.org/115029/diff/1227/1240
File chrome/browser/net/chrome_url_request_context.cc (right):

http://codereview.chromium.org/115029/diff/1227/1240#newcode28
Line 28: const wchar_t* proxy_switches[] = {
On 2009/05/12 00:13:48, eroman wrote:
> I like this change over earlier patch.
> 
> Can you make this |static| too?

Done.

http://codereview.chromium.org/115029/diff/1227/1241
File chrome/browser/net/chrome_url_request_context.h (right):

http://codereview.chromium.org/115029/diff/1227/1241#newcode104
Line 104: // Sets up proxy info if overrides were specified on the command line.
On 2009/05/12 00:13:48, eroman wrote:
> minor nit: the original comment we had wasn't too good, maybe it could be
> rephrased as:
> 
> Creates a proxy configuration using the overrides specified on the command
line.
> Returns NULL if the system defaults should be used instead.

Rephrased.

http://codereview.chromium.org/115029/diff/1227/1244
File chrome/browser/net/chrome_url_request_context_unittest.cc (right):

http://codereview.chromium.org/115029/diff/1227/1244#newcode17
Line 17: // Build the input command lines here
On 2009/05/12 00:13:48, eroman wrote:
> nit: end in period.

Done.

http://codereview.chromium.org/115029/diff/1227/1247
File net/net.gyp (right):

http://codereview.chromium.org/115029/diff/1227/1247#newcode572
Line 572: 'proxy/proxy_config_service_common_unittest.cc',
On 2009/05/12 00:13:48, eroman wrote:
> good call.

Thanks, although kudos must go to skg for pointing me at the net_test_support
lib.

http://codereview.chromium.org/115029/diff/1227/1234
File net/proxy/proxy_config.h (right):

http://codereview.chromium.org/115029/diff/1227/1234#newcode97
Line 97: // configurations should be bypassed. Clears proxy_bypass sets it to
the
On 2009/05/12 00:13:48, eroman wrote:
> nit: "Clears proxy_bypass ^and^ sets ..."

Done.

http://codereview.chromium.org/115029/diff/1227/1234#newcode113
Line 113: // Returns true if this config will require the proxy service to use a
On 2009/05/12 00:13:48, eroman wrote:
> Can you rephrase this as "may require" instead of "will require"?
> 
> The reason for the uncertainty is "auto detect" -- if we specify autodetect
but
> WPAD finds no PAC script, then a resolver may not be necessary.
> 
> Along these lines, it might be clearer to name this method "MayRequirePAC()".

Done.

http://codereview.chromium.org/115029/diff/1227/1238
File net/proxy/proxy_config_unittest.cc (right):

http://codereview.chromium.org/115029/diff/1227/1238#newcode8
Line 8: 
On 2009/05/12 00:13:48, eroman wrote:
> minor nit: is the extra newline intentional?

Quite unintentional, in fact :-) Removed.

http://codereview.chromium.org/115029/diff/1227/1230
File net/proxy/proxy_service.cc (right):

http://codereview.chromium.org/115029/diff/1227/1230#newcode218
Line 218: delete proxy_config_service;
On 2009/05/12 00:13:48, eroman wrote:
> instead of these manual deletes, please use:
> 
> scoped_ptr<ProxyResolver> proxy_resolver;
> 
> and then proxy_resolver.release() for the places where ownership is passed.

Done.

http://codereview.chromium.org/115029/diff/1227/1230#newcode219
Line 219: return CreateNull();
On 2009/05/12 00:13:48, eroman wrote:
> Would be great if you can log a warning here too.

Done.

[wtc, 2009-05-14 23:14:10]

Robert,

Thanks for writing this changelist.

Since these are command-line options, they should be as
short as possible, so I suggest they be shortened to:
  proxy-bypass-urls
  proxy-auto-detect
  pac-url

If you think it's nice to have all proxy related options
start with "proxy", we can rename the last option
  proxy-pac-url
or
  proxy-autoconfig-url

I can take care of the renaming if you agree it's a good
idea.

I'm also wondering if we can replace
  --no-proxy-server
by something like
  --proxy-server=direct

We should add a page on dev.chromium.org to document
these new options.

[eroman, 2009-05-15 02:52:29]

--proxy-server="direct://" should work with the current parsing as-is.

[robertshield, 2009-05-15 03:26:33]

I'm certainly not all that attached to the current names. They are a bit long,
so feel free to change them :-) I think it's convenient to have all the proxy
related flags start with the word "proxy", but your naming suggestions sound
good.

--no-proxy-server exists mainly as a convenience. As Eric pointed out,
--proxy-server="direct://" works now as well although --no-proxy-server has the
added side effect of ignoring all other proxy-related flag parsing. We can drop
it if you feel that it should be removed.

 I'll write up a short description of the new flags tomorrow morning.

Robert

[wtc, 2009-05-15 17:21:07]

On 2009/05/15 02:52:29, eroman wrote:
> --proxy-server="direct://" should work with the current parsing as-is.

Thanks for the tip.  This (the need for the "://") is not obvious at
all, so let's keep the --no-proxy-server option.