Re: Change ProxyRules to handle ProxyLists rather than just single ProxyServer instances. (issue 12315019)

[ero...@chromium.org]

The overall approach is good, but I would like some details cleaned up
before
approving.


https://codereview.chromium.org/12315019/diff/1/net/proxy/proxy_config.cc
File net/proxy/proxy_config.cc (right):

https://codereview.chromium.org/12315019/diff/1/net/proxy/proxy_config.cc#newcode17
net/proxy/proxy_config.cc:17: // If |proxy| is valid, sets it in |dict|
under the key |name|.
"is valid" needs to be updated.

https://codereview.chromium.org/12315019/diff/1/net/proxy/proxy_config.cc#newcode18
net/proxy/proxy_config.cc:18: void AddProxyToValue(const char* name,
This should be renamed to AddProxyListToValue()

https://codereview.chromium.org/12315019/diff/1/net/proxy/proxy_config.cc#newcode29
net/proxy/proxy_config.cc:29: type(TYPE_NO_RULES){
Why remove the space? That is counter to the style AFAIK.

https://codereview.chromium.org/12315019/diff/1/net/proxy/proxy_config.cc#newcode51
net/proxy/proxy_config.cc:51:
result->UsePacString(single_proxy.ToPacString());
Rather than round-tripping through the PAC string format, this should
just set the values directly.

Perhaps add a method like:
result->UseProxyList(single_proxy);

https://codereview.chromium.org/12315019/diff/1/net/proxy/proxy_config.cc#newcode57
net/proxy/proxy_config.cc:57:
result->UsePacString(entry->ToPacString());
See comment above.

https://codereview.chromium.org/12315019/diff/1/net/proxy/proxy_config.cc#newcode117
net/proxy/proxy_config.cc:117: // 'socks' maps to SOCKS5 in
ProxyServer::GetSchemeFromURIInternal.
Yeah, icky carry-over from the windows interpretation of this string
(which is where the format comes from). It is convenient having a
cross-platform implementation for the purpose of command line flags, but
maybe it doesn't make sense and we should invent our own for that.

https://codereview.chromium.org/12315019/diff/1/net/proxy/proxy_config.cc#newcode124
net/proxy/proxy_config.cc:124: // Don't have DIRECT as the first proxy
in a list.
Why disallow this?

https://codereview.chromium.org/12315019/diff/1/net/proxy/proxy_config.h
File net/proxy/proxy_config.h (right):

https://codereview.chromium.org/12315019/diff/1/net/proxy/proxy_config.h#newcode56
net/proxy/proxy_config.h:56: // Sets |result| with the proxy to use for
|url| based on the current rules.
update: proxy --> proxies

https://codereview.chromium.org/12315019/diff/1/net/proxy/proxy_config.h#newcode72
net/proxy/proxy_config.h:72: // that 'socks' server added.
What about instead changing the grammar from:
url-scheme"="proxy-uri

to:
url-scheme"="proxy-uri-list

where proxy-uri-list is a COMMA separated list of proxy-uri.

Do you really care about this string form (i.e. do you plan on setting
things this way), or can you just access it programmatically for your
purposes.

(the particular grammar here is rather awkward/fragile, and forms the
basis of shared code with a platform-specific settings parser. it was
extracted more for convenience to re-use from command-line flags).

https://codereview.chromium.org/12315019/diff/1/net/proxy/proxy_config.h#newcode109
net/proxy/proxy_config.h:109: ProxyList single_proxy;
These names are no longer good, since they are actually proxy lists. At
a minimum please add a TODO to rename these.

https://codereview.chromium.org/12315019/diff/1/net/proxy/proxy_config.h#newcode116
net/proxy/proxy_config.h:116: // Used when a fallback (a socks= scheme)
has been defined and the
I don't think this comment should mention socks= here. That is a concept
of the serialization format (ParseFromString), which honestly doesn't
make much sense, but is the format inherited from the Windows settings.

https://codereview.chromium.org/12315019/diff/1/net/proxy/proxy_config_service_common_unittest.cc
File net/proxy/proxy_config_service_common_unittest.cc (right):

https://codereview.chromium.org/12315019/diff/1/net/proxy/proxy_config_service_common_unittest.cc#newcode84
net/proxy/proxy_config_service_common_unittest.cc:84:
rules.single_proxy.Get(), &failure_details, &failed);
It seems it would be appropriate to change the expectations here to a
PAC style string similar to what you did in the proxy_config_unitest.cc.
That may cause lots of other updates.

At a minimum please assert that the list size is 1 here, since that is
important for correctness of the test verification (whereas changing to
PAC strings is about flexibility).

https://codereview.chromium.org/12315019/diff/1/net/proxy/proxy_config_unittest.cc
File net/proxy/proxy_config_unittest.cc (right):

https://codereview.chromium.org/12315019/diff/1/net/proxy/proxy_config_unittest.cc#newcode260
net/proxy/proxy_config_unittest.cc:260: // direct:// discarded as a
first or only option for a scheme.
Why do we do this?

https://codereview.chromium.org/12315019/diff/1/net/proxy/proxy_list.cc
File net/proxy/proxy_list.cc (right):

https://codereview.chromium.org/12315019/diff/1/net/proxy/proxy_list.cc#newcode121
net/proxy/proxy_list.cc:121: return ToPacString() ==
other.ToPacString();
Not ideal. I will allow it though.

https://codereview.chromium.org/12315019/

[ma...@google.com]

Thanks, PTAL.

https://codereview.chromium.org/12315019/diff/1/net/proxy/proxy_config.cc
File net/proxy/proxy_config.cc (right):

https://codereview.chromium.org/12315019/diff/1/net/proxy/proxy_config.cc#newcode17
net/proxy/proxy_config.cc:17: // If |proxy| is valid, sets it in |dict|
under the key |name|.

On 2013/02/26 01:13:50, eroman wrote:
> "is valid" needs to be updated.

Done.

https://codereview.chromium.org/12315019/diff/1/net/proxy/proxy_config.cc#newcode18
net/proxy/proxy_config.cc:18: void AddProxyToValue(const char* name,

On 2013/02/26 01:13:50, eroman wrote:
> This should be renamed to AddProxyListToValue()

Done.

https://codereview.chromium.org/12315019/diff/1/net/proxy/proxy_config.cc#newcode29
net/proxy/proxy_config.cc:29: type(TYPE_NO_RULES){

On 2013/02/26 01:13:50, eroman wrote:
> Why remove the space? That is counter to the style AFAIK.

Done.

https://codereview.chromium.org/12315019/diff/1/net/proxy/proxy_config.cc#newcode51
net/proxy/proxy_config.cc:51:
result->UsePacString(single_proxy.ToPacString());

On 2013/02/26 01:13:50, eroman wrote:
> Rather than round-tripping through the PAC string format, this should
just set
> the values directly.

> Perhaps add a method like:
> result->UseProxyList(single_proxy);

Done.

https://codereview.chromium.org/12315019/diff/1/net/proxy/proxy_config.cc#newcode57
net/proxy/proxy_config.cc:57:
result->UsePacString(entry->ToPacString());

On 2013/02/26 01:13:50, eroman wrote:
> See comment above.

Done.

https://codereview.chromium.org/12315019/diff/1/net/proxy/proxy_config.cc#newcode117
net/proxy/proxy_config.cc:117: // 'socks' maps to SOCKS5 in
ProxyServer::GetSchemeFromURIInternal.

On 2013/02/26 01:13:50, eroman wrote:
> Yeah, icky carry-over from the windows interpretation of this string
(which is
> where the format comes from). It is convenient having a cross-platform
> implementation for the purpose of command line flags, but maybe it
doesn't make
> sense and we should invent our own for that.

Not my call; I just added the comment because it's non-obvious.

https://codereview.chromium.org/12315019/diff/1/net/proxy/proxy_config.cc#newcode124
net/proxy/proxy_config.cc:124: // Don't have DIRECT as the first proxy
in a list.

On 2013/02/26 01:13:50, eroman wrote:
> Why disallow this?

I had assumed that this would create a wasteful situation where a
ProxyInfo would not return true from IsDirect, but would nonetheless
function that way. On closer reading of the code, that actually isn't
the case, so I've removed this condition.

https://codereview.chromium.org/12315019/diff/1/net/proxy/proxy_config.h
File net/proxy/proxy_config.h (right):

https://codereview.chromium.org/12315019/diff/1/net/proxy/proxy_config.h#newcode56
net/proxy/proxy_config.h:56: // Sets |result| with the proxy to use for
|url| based on the current rules.

On 2013/02/26 01:13:50, eroman wrote:
> update: proxy --> proxies

Done.

https://codereview.chromium.org/12315019/diff/1/net/proxy/proxy_config.h#newcode72
net/proxy/proxy_config.h:72: // that 'socks' server added.

On 2013/02/26 01:13:50, eroman wrote:
> What about instead changing the grammar from:
> url-scheme"="proxy-uri

> to:
> url-scheme"="proxy-uri-list

> where proxy-uri-list is a COMMA separated list of proxy-uri.

> Do you really care about this string form (i.e. do you plan on setting
things
> this way), or can you just access it programmatically for your
purposes.

> (the particular grammar here is rather awkward/fragile, and forms the
basis of
> shared code with a platform-specific settings parser. it was extracted
more for
> convenience to re-use from command-line flags).

Using this format to set things is the current plan; the alternative
seems to be to define a new proxy mode and many new proxy prefs that
could be used to construct a proxy list.

I'm fine with commas instead of the repeated-entry list; the latter was
a bit easier to parse.

https://codereview.chromium.org/12315019/diff/1/net/proxy/proxy_config.h#newcode109
net/proxy/proxy_config.h:109: ProxyList single_proxy;

On 2013/02/26 01:13:50, eroman wrote:
> These names are no longer good, since they are actually proxy lists.
At a
> minimum please add a TODO to rename these.

Done.

https://codereview.chromium.org/12315019/diff/1/net/proxy/proxy_config.h#newcode116
net/proxy/proxy_config.h:116: // Used when a fallback (a socks= scheme)
has been defined and the

On 2013/02/26 01:13:50, eroman wrote:
> I don't think this comment should mention socks= here. That is a
concept of the
> serialization format (ParseFromString), which honestly doesn't make
much sense,
> but is the format inherited from the Windows settings.

Done.

https://codereview.chromium.org/12315019/diff/1/net/proxy/proxy_config_service_common_unittest.cc
File net/proxy/proxy_config_service_common_unittest.cc (right):

https://codereview.chromium.org/12315019/diff/1/net/proxy/proxy_config_service_common_unittest.cc#newcode84
net/proxy/proxy_config_service_common_unittest.cc:84:
rules.single_proxy.Get(), &failure_details, &failed);

On 2013/02/26 01:13:50, eroman wrote:
> It seems it would be appropriate to change the expectations here to a
PAC style
> string similar to what you did in the proxy_config_unitest.cc. That
may cause
> lots of other updates.

> At a minimum please assert that the list size is 1 here, since that is
important
> for correctness of the test verification (whereas changing to PAC
strings is
> about flexibility).

Done.

https://codereview.chromium.org/12315019/diff/1/net/proxy/proxy_config_unittest.cc
File net/proxy/proxy_config_unittest.cc (right):

https://codereview.chromium.org/12315019/diff/1/net/proxy/proxy_config_unittest.cc#newcode260
net/proxy/proxy_config_unittest.cc:260: // direct:// discarded as a
first or only option for a scheme.

On 2013/02/26 01:13:50, eroman wrote:
> Why do we do this?

We don't any more.

https://codereview.chromium.org/12315019/diff/1/net/proxy/proxy_list.cc
File net/proxy/proxy_list.cc (right):

https://codereview.chromium.org/12315019/diff/1/net/proxy/proxy_list.cc#newcode121
net/proxy/proxy_list.cc:121: return ToPacString() ==
other.ToPacString();

On 2013/02/26 01:13:50, eroman wrote:
> Not ideal. I will allow it though.

ProxyServer has an == operator, so I can just compare the vectors
directly.

https://codereview.chromium.org/12315019/

[ero...@chromium.org]

Thanks for making the larger changes; we are almost there!


https://codereview.chromium.org/12315019/diff/7001/net/proxy/proxy_config.cc
File net/proxy/proxy_config.cc (right):

https://codereview.chromium.org/12315019/diff/7001/net/proxy/proxy_config.cc#newcode19
net/proxy/proxy_config.cc:19: const ProxyList& proxies,
fix indentation.

https://codereview.chromium.org/12315019/diff/7001/net/proxy/proxy_config.cc#newcode90
net/proxy/proxy_config.cc:90:
Remove this extra line

https://codereview.chromium.org/12315019/diff/7001/net/proxy/proxy_config.cc#newcode97
net/proxy/proxy_config.cc:97: single_proxies.SetSingleProxyServer(
This should support comma separated list like the others.

https://codereview.chromium.org/12315019/diff/7001/net/proxy/proxy_config.cc#newcode123
net/proxy/proxy_config.cc:123: // Add each entry from the uri list in
turn
See comment above -- I believe this should be extracted so it can be
re-used.

https://codereview.chromium.org/12315019/diff/7001/net/proxy/proxy_config.h
File net/proxy/proxy_config.h (right):

https://codereview.chromium.org/12315019/diff/7001/net/proxy/proxy_config.h#newcode78
net/proxy/proxy_config.h:78: // "foopy:80" -- use
HTTP proxy "foopy:80" for all URLs.
It would make sense for proxy-uri-list to also be supported without any
schemes. For instance the input:

"foopy:80,fallback1,direct://"

https://codereview.chromium.org/12315019/diff/7001/net/proxy/proxy_config.h#newcode92
net/proxy/proxy_config.h:92: // Returns one of {&proxy_for_http,
&proxy_for_https, &proxy_for_ftp,
Please update this comment to reflect new names.

https://codereview.chromium.org/12315019/diff/7001/net/proxy/proxy_config.h#newcode95
net/proxy/proxy_config.h:95: const ProxyList* MapUrlSchemeToProxy(const
std::string& url_scheme) const;
Can you rename "ToProxy" --> "ToProxies" or "ToProxyList"

https://codereview.chromium.org/12315019/diff/7001/net/proxy/proxy_config.h#newcode109
net/proxy/proxy_config.h:109: ProxyList single_proxies;
Side-comment [no action necessary]: Can probably just get rid of this,
and use |fallback_proxies| instead.

https://codereview.chromium.org/12315019/diff/7001/net/proxy/proxy_config.h#newcode121
net/proxy/proxy_config.h:121: // Returns one of {&proxy_for_http,
&proxy_for_https, &proxy_for_ftp}
Please update this comment.

https://codereview.chromium.org/12315019/diff/7001/net/proxy/proxy_config.h#newcode124
net/proxy/proxy_config.h:124: ProxyList*
MapUrlSchemeToProxyNoFallback(const std::string& scheme);
See earlier rename suggestion.

https://codereview.chromium.org/12315019/diff/7001/net/proxy/proxy_config_service_common_unittest.cc
File net/proxy/proxy_config_service_common_unittest.cc (right):

https://codereview.chromium.org/12315019/diff/7001/net/proxy/proxy_config_service_common_unittest.cc#newcode18
net/proxy/proxy_config_service_common_unittest.cc:18: // Helper to
verify that |expected_proxy| matches |actual_proxy|. If it does
Comment no longer matches.

https://codereview.chromium.org/12315019/diff/7001/net/proxy/proxy_config_unittest.cc
File net/proxy/proxy_config_unittest.cc (left):

https://codereview.chromium.org/12315019/diff/7001/net/proxy/proxy_config_unittest.cc#oldcode173
net/proxy/proxy_config_unittest.cc:173: // Include duplicate entries --
last one wins.
Please keep this test as well (need a test to see what happens when
there are multiple scheme=<list> entries; i don't really care what the
particular policy is, as long as it is tested).

https://codereview.chromium.org/12315019/

[ma...@google.com]

Thanks, PTAL.

(There are a couple of extra blank lines I'm removing).

https://codereview.chromium.org/12315019/diff/7001/net/proxy/proxy_config.cc
File net/proxy/proxy_config.cc (right):

https://codereview.chromium.org/12315019/diff/7001/net/proxy/proxy_config.cc#newcode19
net/proxy/proxy_config.cc:19: const ProxyList& proxies,

On 2013/03/01 00:05:13, eroman wrote:
> fix indentation.

Done.

https://codereview.chromium.org/12315019/diff/7001/net/proxy/proxy_config.cc#newcode90
net/proxy/proxy_config.cc:90:
On 2013/03/01 00:05:13, eroman wrote:
> Remove this extra line

Done.

https://codereview.chromium.org/12315019/diff/7001/net/proxy/proxy_config.cc#newcode97
net/proxy/proxy_config.cc:97: single_proxies.SetSingleProxyServer(

On 2013/03/01 00:05:13, eroman wrote:
> This should support comma separated list like the others.

Done.

https://codereview.chromium.org/12315019/diff/7001/net/proxy/proxy_config.cc#newcode123
net/proxy/proxy_config.cc:123: // Add each entry from the uri list in
turn

On 2013/03/01 00:05:13, eroman wrote:
> See comment above -- I believe this should be extracted so it can be
re-used.

Done.

https://codereview.chromium.org/12315019/diff/7001/net/proxy/proxy_config.h
File net/proxy/proxy_config.h (right):

https://codereview.chromium.org/12315019/diff/7001/net/proxy/proxy_config.h#newcode78
net/proxy/proxy_config.h:78: // "foopy:80" -- use
HTTP proxy "foopy:80" for all URLs.

On 2013/03/01 00:05:13, eroman wrote:
> It would make sense for proxy-uri-list to also be supported without
any schemes.
> For instance the input:

> "foopy:80,fallback1,direct://"

Done.

https://codereview.chromium.org/12315019/diff/7001/net/proxy/proxy_config.h#newcode92
net/proxy/proxy_config.h:92: // Returns one of {&proxy_for_http,
&proxy_for_https, &proxy_for_ftp,

On 2013/03/01 00:05:13, eroman wrote:
> Please update this comment to reflect new names.

Done.

https://codereview.chromium.org/12315019/diff/7001/net/proxy/proxy_config.h#newcode95
net/proxy/proxy_config.h:95: const ProxyList* MapUrlSchemeToProxy(const
std::string& url_scheme) const;

On 2013/03/01 00:05:13, eroman wrote:
> Can you rename "ToProxy" --> "ToProxies" or "ToProxyList"

Done.

https://codereview.chromium.org/12315019/diff/7001/net/proxy/proxy_config.h#newcode109
net/proxy/proxy_config.h:109: ProxyList single_proxies;

On 2013/03/01 00:05:13, eroman wrote:
> Side-comment [no action necessary]: Can probably just get rid of this,
and use
> |fallback_proxies| instead.

Ack.

https://codereview.chromium.org/12315019/diff/7001/net/proxy/proxy_config.h#newcode121
net/proxy/proxy_config.h:121: // Returns one of {&proxy_for_http,
&proxy_for_https, &proxy_for_ftp}

On 2013/03/01 00:05:13, eroman wrote:
> Please update this comment.

Done.

https://codereview.chromium.org/12315019/diff/7001/net/proxy/proxy_config.h#newcode124
net/proxy/proxy_config.h:124: ProxyList*
MapUrlSchemeToProxyNoFallback(const std::string& scheme);

On 2013/03/01 00:05:13, eroman wrote:
> See earlier rename suggestion.

Done.

https://codereview.chromium.org/12315019/diff/7001/net/proxy/proxy_config_service_common_unittest.cc
File net/proxy/proxy_config_service_common_unittest.cc (right):

https://codereview.chromium.org/12315019/diff/7001/net/proxy/proxy_config_service_common_unittest.cc#newcode18
net/proxy/proxy_config_service_common_unittest.cc:18: // Helper to
verify that |expected_proxy| matches |actual_proxy|. If it does

On 2013/03/01 00:05:13, eroman wrote:
> Comment no longer matches.

Done.

https://codereview.chromium.org/12315019/diff/7001/net/proxy/proxy_config_unittest.cc
File net/proxy/proxy_config_unittest.cc (left):

https://codereview.chromium.org/12315019/diff/7001/net/proxy/proxy_config_unittest.cc#oldcode173
net/proxy/proxy_config_unittest.cc:173: // Include duplicate entries --
last one wins.

On 2013/03/01 00:05:13, eroman wrote:
> Please keep this test as well (need a test to see what happens when
there are
> multiple scheme=<list> entries; i don't really care what the
particular policy
> is, as long as it is tested).

Done.

https://codereview.chromium.org/12315019/

[ero...@chromium.org]

LGTM


https://codereview.chromium.org/12315019/diff/19001/net/proxy/proxy_config.cc
File net/proxy/proxy_config.cc (right):

https://codereview.chromium.org/12315019/diff/19001/net/proxy/proxy_config.cc#newcode26
net/proxy/proxy_config.cc:26: void
AddProxyURIListToProxyList(std::string uri_list,
const std::string&

(Technically StringPiece would be good too, but that would be
inconsistent with the rest of this file, so a refactor for another day
:)

https://codereview.chromium.org/12315019/diff/19001/net/proxy/proxy_config.h
File net/proxy/proxy_config.h (right):

https://codereview.chromium.org/12315019/diff/19001/net/proxy/proxy_config.h#newcode68
net/proxy/proxy_config.h:68: // If all URLs should use the same proxies,
|proxy_rules| should be a
[optional] Your current wording is fine. Another way to think about this
is that the grammar takes a semi-colon separated list of ordered proxies
that apply to a particular url-scheme. If the <url-scheme>"=" prefix is
left off, then it is assumed to apply to all URL schemes (i.e. the
"fallback" proxies). If the url-scheme is set to "socks" than this also
means a fallback list that applies to all url-schemes, however the
default proxy-scheme when parsing the elements is understood to be
socks4:// if left unspecified.

X [
X = [<url-scheme>]

https://codereview.chromium.org/12315019/diff/19001/net/proxy/proxy_config.h#newcode77
net/proxy/proxy_config.h:77: // specified will have that 'socks' server
added.
This is best understood in terms of fallback-proxy list, since non-socks
proxy servers can also be enumerated in here.

https://codereview.chromium.org/12315019/diff/19001/net/proxy/proxy_config.h#newcode90
net/proxy/proxy_config.h:90: // "http=foopy,bar.com -- use
HTTP proxy "foopy" for http URLs,
To make this example more interesting, I suggest changing it to:

http=foop,socks5://bar.com

https://codereview.chromium.org/12315019/diff/19001/net/proxy/proxy_config_unittest.cc
File net/proxy/proxy_config_unittest.cc (right):

https://codereview.chromium.org/12315019/diff/19001/net/proxy/proxy_config_unittest.cc#newcode271
net/proxy/proxy_config_unittest.cc:271: // direct:// discarded as a

first or only option for a scheme.

This comment is no longer applicable.

https://codereview.chromium.org/12315019/

[ma...@google.com]

Thanks. What's the procedure for getting this submitted now?

https://codereview.chromium.org/12315019/diff/19001/net/proxy/proxy_config.cc
File net/proxy/proxy_config.cc (right):

https://codereview.chromium.org/12315019/diff/19001/net/proxy/proxy_config.cc#newcode26
net/proxy/proxy_config.cc:26: void
AddProxyURIListToProxyList(std::string uri_list,

On 2013/03/05 01:55:55, eroman wrote:
> const std::string&

> (Technically StringPiece would be good too, but that would be
inconsistent with
> the rest of this file, so a refactor for another day :)

Ack.

https://codereview.chromium.org/12315019/diff/19001/net/proxy/proxy_config.h
File net/proxy/proxy_config.h (right):

https://codereview.chromium.org/12315019/diff/19001/net/proxy/proxy_config.h#newcode68
net/proxy/proxy_config.h:68: // If all URLs should use the same proxies,
|proxy_rules| should be a

On 2013/03/05 01:55:55, eroman wrote:
> [optional] Your current wording is fine. Another way to think about
this is that
> the grammar takes a semi-colon separated list of ordered proxies that
apply to a
> particular url-scheme. If the <url-scheme>"=" prefix is left off, then
it is
> assumed to apply to all URL schemes (i.e. the "fallback" proxies). If
the
> url-scheme is set to "socks" than this also means a fallback list that
applies
> to all url-schemes, however the default proxy-scheme when parsing the
elements
> is understood to be socks4:// if left unspecified.

> X [
> X = [<url-scheme>]

Done.

https://codereview.chromium.org/12315019/diff/19001/net/proxy/proxy_config.h#newcode77
net/proxy/proxy_config.h:77: // specified will have that 'socks' server
added.

On 2013/03/05 01:55:55, eroman wrote:
> This is best understood in terms of fallback-proxy list, since
non-socks proxy
> servers can also be enumerated in here.

Done.

https://codereview.chromium.org/12315019/diff/19001/net/proxy/proxy_config.h#newcode90
net/proxy/proxy_config.h:90: // "http=foopy,bar.com -- use
HTTP proxy "foopy" for http URLs,

On 2013/03/05 01:55:55, eroman wrote:
> To make this example more interesting, I suggest changing it to:

> http://http=foop%2Csocks5://bar.com

Done.

https://codereview.chromium.org/12315019/diff/19001/net/proxy/proxy_config_unittest.cc
File net/proxy/proxy_config_unittest.cc (right):

https://codereview.chromium.org/12315019/diff/19001/net/proxy/proxy_config_unittest.cc#newcode271
net/proxy/proxy_config_unittest.cc:271: // direct:// discarded as a
first or only option for a scheme.

On 2013/03/05 01:55:55, eroman wrote:
> This comment is no longer applicable.

Done.

https://codereview.chromium.org/12315019/

[commi...@chromium.org]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ma...@google.com/12315019/21001


https://chromiumcodereview.appspot.com/12315019/

[commi...@chromium.org]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_clang_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_clang_dbg&number=24612
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

https://chromiumcodereview.appspot.com/12315019/

[ma...@google.com]

Updated proxy config services for the other platforms (doh!).

Updated a couple of pieces of code that were accessing the proxy_rules
fields
under their old names and types.

Fixed MatchesProxyServerHelper() when an empty proxy is expected.

On 2013/03/05 23:28:55, I haz the power (commit-bot) wrote:
> Sorry for I got bad news for ya.
> Compile failed with a clobber build on android_clang_dbg.

http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_clang_dbg&number=24612
> Your code is likely broken or HEAD is junk. Please ensure your
> code is not broken then alert the build sheriffs.
> Look at the try server FAQ for more details.

https://codereview.chromium.org/12315019/

[commi...@chromium.org]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/ma...@google.com/12315019/42001


https://chromiumcodereview.appspot.com/12315019/

[commi...@chromium.org]

Presubmit check for 12315019-42001 failed and returned exit status 1.

INFO:root:Found 16 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py
Running
/b/commit-queue/workdir/chromium/chrome/browser/extensions/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/net/PRESUBMIT.py

** Presubmit Messages **
If this change has an associated bug, add BUG=[bug number].

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
chrome/browser/importer/firefox_proxy_settings.cc
chrome/browser/extensions/api/proxy/proxy_api_helpers.h
chrome/browser/extensions/api/proxy/proxy_api_helpers.cc

Presubmit checks took 1.7s to calculate.



https://chromiumcodereview.appspot.com/12315019/

[ero...@chromium.org]

Still LGTM (however you will need some additional owner approvals)


https://codereview.chromium.org/12315019/diff/42001/net/proxy/proxy_config.h
File net/proxy/proxy_config.h (right):

https://codereview.chromium.org/12315019/diff/42001/net/proxy/proxy_config.h#newcode83
net/proxy/proxy_config.h:83: // * If the scheme is set to 'socks', that
sets a fallback list that
nit: "If the scheme" --> "If the url-scheme"

https://codereview.chromium.org/12315019/diff/42001/net/proxy/proxy_config_service_common_unittest.cc
File net/proxy/proxy_config_service_common_unittest.cc (left):

https://codereview.chromium.org/12315019/diff/42001/net/proxy/proxy_config_service_common_unittest.cc#oldcode89
net/proxy/proxy_config_service_common_unittest.cc:89:
MatchesProxyServerHelper("Bad fallback_proxy", fallback_proxy,
Is it intentional that you removed the check on fallback_proxy?
Shouldn't this be translated to fallback_proxies?

https://codereview.chromium.org/12315019/diff/42001/net/proxy/proxy_config_unittest.cc
File net/proxy/proxy_config_unittest.cc (right):

https://codereview.chromium.org/12315019/diff/42001/net/proxy/proxy_config_unittest.cc#newcode133
net/proxy/proxy_config_unittest.cc:133:
nit: remove empty line

https://codereview.chromium.org/12315019/

[ma...@google.com]

+mirandac for importer/
+battre for extensions/api/proxy/

https://codereview.chromium.org/12315019/diff/42001/net/proxy/proxy_config.h
File net/proxy/proxy_config.h (right):

https://codereview.chromium.org/12315019/diff/42001/net/proxy/proxy_config.h#newcode83
net/proxy/proxy_config.h:83: // * If the scheme is set to 'socks', that
sets a fallback list that

On 2013/03/06 23:26:32, eroman wrote:
> nit: "If the scheme" --> "If the url-scheme"

Done.

https://codereview.chromium.org/12315019/diff/42001/net/proxy/proxy_config_service_common_unittest.cc
File net/proxy/proxy_config_service_common_unittest.cc (left):

https://codereview.chromium.org/12315019/diff/42001/net/proxy/proxy_config_service_common_unittest.cc#oldcode89
net/proxy/proxy_config_service_common_unittest.cc:89:
MatchesProxyServerHelper("Bad fallback_proxy", fallback_proxy,

On 2013/03/06 23:26:32, eroman wrote:
> Is it intentional that you removed the check on fallback_proxy?
Shouldn't this
> be translated to fallback_proxies?

That was a mistake; fixed.

https://codereview.chromium.org/12315019/diff/42001/net/proxy/proxy_config_unittest.cc
File net/proxy/proxy_config_unittest.cc (right):

https://codereview.chromium.org/12315019/diff/42001/net/proxy/proxy_config_unittest.cc#newcode133
net/proxy/proxy_config_unittest.cc:133:

On 2013/03/06 23:26:32, eroman wrote:
> nit: remove empty line

Done.

https://codereview.chromium.org/12315019/

[bat...@chromium.org]

https://codereview.chromium.org/12315019/diff/44009/chrome/browser/extensions/api/proxy/proxy_api_helpers.cc
File chrome/browser/extensions/api/proxy/proxy_api_helpers.cc (right):

https://codereview.chromium.org/12315019/diff/44009/chrome/browser/extensions/api/proxy/proxy_api_helpers.cc#newcode427
chrome/browser/extensions/api/proxy/proxy_api_helpers.cc:427:
CHECK(proxies.size() == 1);
I think this can cause trouble. See
CommandLinePrefStore::ApplyProxyMode().

I think if you pass the command line switch
--proxy-server="http=foopy,direct://" and then use the proxy settings
API to query the current proxy configuration, you trigger this CHECK.

https://codereview.chromium.org/12315019/

[mira...@chromium.org]

marq, I'm tossing this over to Ilya for the importer changes -- I need to
remove
myself from the OWNERS list here, as I've been away from this area for so
long
that I'm no longer qualified to review.

[ma...@google.com]

Changed the proxy api helper to not have the dangerous CHECK. Instead, if
multiple proxy servers are configured for a given scheme,
CreateProxyRulesDict
will return the first (valid) configured server for each scheme.

https://codereview.chromium.org/12315019/diff/44009/chrome/browser/extensions/api/proxy/proxy_api_helpers.cc
File chrome/browser/extensions/api/proxy/proxy_api_helpers.cc (right):

https://codereview.chromium.org/12315019/diff/44009/chrome/browser/extensions/api/proxy/proxy_api_helpers.cc#newcode427
chrome/browser/extensions/api/proxy/proxy_api_helpers.cc:427:
CHECK(proxies.size() == 1);
On 2013/03/07 10:18:54, battre wrote:
> I think this can cause trouble. See
CommandLinePrefStore::ApplyProxyMode().

> I think if you pass the command line switch
> --proxy-server="http=foopy,direct://" and then use the proxy settings
API to
> query the current proxy configuration, you trigger this CHECK.

Yeah, this is bad. Removed.

https://codereview.chromium.org/12315019/