Extract the parsing of proxy rules to ProxyConfig::ProxyRules, and unit-test....

Extract the parsing of proxy rules to ProxyConfig::ProxyRules, and unit-test.

This avoids re-parsing the rules every time a proxy resolve is done, and also adds extra tolerance for white space.

The other motivation is to not have to fiddle around with strings as much in the various ProxyConfigServceXXXX implementations.


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

[wtc, 2009-03-30 19:06:32]

LGTM.

http://codereview.chromium.org/57011/diff/1001/1017
File net/net.gyp (right):

http://codereview.chromium.org/57011/diff/1001/1017#newcode429
Line 429: 'proxy/proxy_config_unittest.cc',
Nit: should we list this after
'proxy/proxy_config_service_win_unittest.cc'?

http://codereview.chromium.org/57011/diff/1001/1015
File net/proxy/proxy_config.h (right):

http://codereview.chromium.org/57011/diff/1001/1015#newcode36
Line 36: struct ProxyRules {
Nit: in this struct, the type definitions, constructor,
members, and methods are not listed in the usual order.

http://codereview.chromium.org/57011/diff/1001/1015#newcode47
Line 47: // Set if |type| is SINGLE_PROXY.
In C code, single_proxy and
proxy_for_http/proxy_for_https/proxy_for_ftp
would be put in a union and 'type' would be used to select
the appropriate union members.  Should we do that here?

http://codereview.chromium.org/57011/diff/1001/1015#newcode76
Line 76: // Returns one of {&proxy_for_http, &proxy_for_https, &proxy_for_ftp},
If 'type' is TYPE_SINGLE_PROXY, then we return &single_proxy,
right?

Or do you only call MapSchemeToProxy if 'type' is
TYPE_PROXY_PER_SCHEME?

http://codereview.chromium.org/57011/diff/1001/1015#newcode80
Line 80: bool operator==(const ProxyRules& other) const {
This operator will need to be updated if you decide to use
a union in this struct.

http://codereview.chromium.org/57011/diff/1001/1002
File net/proxy/proxy_info.h (right):

http://codereview.chromium.org/57011/diff/1001/1002#newcode37
Line 37: void UseProxyServer(const ProxyServer& proxy_server);
Please add a comment.  This method seems similar to
UseNamedProxy.

http://codereview.chromium.org/57011/diff/1001/1008
File net/proxy/proxy_server.h (right):

http://codereview.chromium.org/57011/diff/1001/1008#newcode106
Line 106: bool operator==(const ProxyServer& other) const {
Isn't this the same as the default operator==?
Why do you need to define it yourself?

http://codereview.chromium.org/57011/diff/1001/1003
File net/proxy/proxy_service.cc (right):

http://codereview.chromium.org/57011/diff/1001/1003#newcode334
Line 334: proxy_rules.MapSchemeToProxy(url.scheme())) {
Nit: should this line be aligned with the opening parenthesis
'(' on the previous line?

http://codereview.chromium.org/57011/diff/1001/1003#newcode343
Line 343: NOTREACHED();
Do you want to provide a meaningful fallback in release builds?

[eroman, 2009-03-30 20:27:43]

http://codereview.chromium.org/57011/diff/1001/1017
File net/net.gyp (right):

http://codereview.chromium.org/57011/diff/1001/1017#newcode429
Line 429: 'proxy/proxy_config_unittest.cc',
On 2009/03/30 19:06:32, wtc wrote:
> Nit: should we list this after
> 'proxy/proxy_config_service_win_unittest.cc'?

Done.
(I agree that is better, as it is what 'sort' command does).

http://codereview.chromium.org/57011/diff/1001/1015
File net/proxy/proxy_config.h (right):

http://codereview.chromium.org/57011/diff/1001/1015#newcode36
Line 36: struct ProxyRules {
On 2009/03/30 19:06:32, wtc wrote:
> Nit: in this struct, the type definitions, constructor,
> members, and methods are not listed in the usual order.

Done.
Sorry about that.

http://codereview.chromium.org/57011/diff/1001/1015#newcode47
Line 47: // Set if |type| is SINGLE_PROXY.
On 2009/03/30 19:06:32, wtc wrote:
> In C code, single_proxy and
> proxy_for_http/proxy_for_https/proxy_for_ftp
> would be put in a union and 'type' would be used to select
> the appropriate union members.  Should we do that here?

I would really prefer not to use a union.

If you are concerned about the extra sizeof(ProxyServer) storage used by
non-union implementation, I can re-use |proxy_for_http| for the single-proxy
case (but I would prefer not to for readability).

http://codereview.chromium.org/57011/diff/1001/1015#newcode76
Line 76: // Returns one of {&proxy_for_http, &proxy_for_https, &proxy_for_ftp},
On 2009/03/30 19:06:32, wtc wrote:
> If 'type' is TYPE_SINGLE_PROXY, then we return &single_proxy,
> right?

Correct (the latter).

I have updated the documentation to call this out, and added a DCHECK to catch
bad callers. In non-debug mode it will return NULL.

http://codereview.chromium.org/57011/diff/1001/1002
File net/proxy/proxy_info.h (right):

http://codereview.chromium.org/57011/diff/1001/1002#newcode37
Line 37: void UseProxyServer(const ProxyServer& proxy_server);
On 2009/03/30 19:06:32, wtc wrote:
> Please add a comment.  This method seems similar to
> UseNamedProxy.

Done:
// Set the proxy list to a single entry, |proxy_server|.

This serves the same function as UseNamedProxy(), the difference is in the input
type.

Whereas UseNamedProxyServer() takes a string and parses it into a list of
ProxyServer's, this directly takes a single ProxyServer (therefore avoiding the
parsing).

http://codereview.chromium.org/57011/diff/1001/1008
File net/proxy/proxy_server.h (right):

http://codereview.chromium.org/57011/diff/1001/1008#newcode106
Line 106: bool operator==(const ProxyServer& other) const {
On 2009/03/30 19:06:32, wtc wrote:
> Isn't this the same as the default operator==?
> Why do you need to define it yourself?

Since ProxyServer defines a constructor, the default operator== is not
generated.  C++ is fickle that way :)

http://codereview.chromium.org/57011/diff/1001/1003
File net/proxy/proxy_service.cc (right):

http://codereview.chromium.org/57011/diff/1001/1003#newcode334
Line 334: proxy_rules.MapSchemeToProxy(url.scheme())) {
On 2009/03/30 19:06:32, wtc wrote:
> Nit: should this line be aligned with the opening parenthesis
> '(' on the previous line?

Done.

Maybe I should just define entry prior to the "if"? (The main reason I avoided
that, is then I need to put braces around the "case".

http://codereview.chromium.org/57011/diff/1001/1003#newcode343
Line 343: NOTREACHED();
On 2009/03/30 19:06:32, wtc wrote:
> Do you want to provide a meaningful fallback in release builds?

Done:
result->UseDirect().

The main reason I have default is to catch case where new ProxyRules::Type get
added, but this callsite forgets to be updated.

[wtc, 2009-03-30 22:53:40]

LGTM.

http://codereview.chromium.org/57011/diff/1001/1015
File net/proxy/proxy_config.h (right):

http://codereview.chromium.org/57011/diff/1001/1015#newcode47
Line 47: // Set if |type| is SINGLE_PROXY.
On 2009/03/30 20:27:43, eroman wrote:
>
> I would really prefer not to use a union.
> 
> If you are concerned about the extra sizeof(ProxyServer) storage used by
> non-union implementation, I can re-use |proxy_for_http| for the single-proxy
> case (but I would prefer not to for readability).

My rationale for using a union is that it makes your
intention clear to people familiar with unions.  You
are relying on comments to convey your intention.  If
you use a union, the code will become self-documenting.

I am not that concerned about wasting an empty std::string
here.  In any case, I'll let you decide.

http://codereview.chromium.org/57011/diff/1001/1003
File net/proxy/proxy_service.cc (right):

http://codereview.chromium.org/57011/diff/1001/1003#newcode334
Line 334: proxy_rules.MapSchemeToProxy(url.scheme())) {
On 2009/03/30 20:27:43, eroman wrote:
>
> Maybe I should just define entry prior to the "if"? (The main reason I avoided
> that, is then I need to put braces around the "case".

Oh, I didn't realize you're declaring a local variable
'entry' there.  So you may ignore my previous suggestion
of aligning this line with the opening '(' on the previous
line.

Yes, I would declare 'entry' before the "if", at the cost
of adding braces around this case.

http://codereview.chromium.org/57011/diff/40/54
File net/proxy/proxy_config.h (right):

http://codereview.chromium.org/57011/diff/40/54#newcode67
Line 67: // or NULL if it is a scheme that we don't have a mapping for. Shoud
only
Nit: typo "Shoud"

http://codereview.chromium.org/57011/diff/40/42
File net/proxy/proxy_service.cc (right):

http://codereview.chromium.org/57011/diff/40/42#newcode344
Line 344: NOTREACHED();
Nit: add a 'break' at the end of the 'default' case.

[eroman, 2009-03-30 23:55:28]

thanks for the review!

http://codereview.chromium.org/57011/diff/1001/1015
File net/proxy/proxy_config.h (right):

http://codereview.chromium.org/57011/diff/1001/1015#newcode47
Line 47: // Set if |type| is SINGLE_PROXY.
On 2009/03/30 22:53:40, wtc wrote:
> On 2009/03/30 20:27:43, eroman wrote:
> >
> > I would really prefer not to use a union.
> > 
> > If you are concerned about the extra sizeof(ProxyServer) storage used by
> > non-union implementation, I can re-use |proxy_for_http| for the single-proxy
> > case (but I would prefer not to for readability).
> 
> My rationale for using a union is that it makes your
> intention clear to people familiar with unions.  You
> are relying on comments to convey your intention.  If
> you use a union, the code will become self-documenting.
> 
> I am not that concerned about wasting an empty std::string
> here.  In any case, I'll let you decide.

My reason for opting against unions, is that unions are totally borked in C++.

In particular a union is going to blow up if you give it members that require
initialization. Here for example, the desire is to have the members be of type
ProxyServer. This however is a no-go, since ProxyServer defines a default
constructor. (In fact this won't even compile). To work around this gets ugly,
and you lose the safety net against using uninitialized data.

http://codereview.chromium.org/57011/diff/1001/1003
File net/proxy/proxy_service.cc (right):

http://codereview.chromium.org/57011/diff/1001/1003#newcode334
Line 334: proxy_rules.MapSchemeToProxy(url.scheme())) {
On 2009/03/30 22:53:40, wtc wrote:
> On 2009/03/30 20:27:43, eroman wrote:
> >
> > Maybe I should just define entry prior to the "if"? (The main reason I
avoided
> > that, is then I need to put braces around the "case".
> 
> Oh, I didn't realize you're declaring a local variable
> 'entry' there.  So you may ignore my previous suggestion
> of aligning this line with the opening '(' on the previous
> line.
> 
> Yes, I would declare 'entry' before the "if", at the cost
> of adding braces around this case.

Done.

http://codereview.chromium.org/57011/diff/40/42#newcode344
Line 344: }
On 2009/03/30 22:53:41, wtc wrote:
> Nit: add a 'break' at the end of the 'default' case.

Done.