Change ProxyRules to handle ProxyLists rather than just single ProxyServer instances.

Change ProxyRules to handle ProxyLists rather than just single ProxyServer instances.

- All of the ProxyRules members that used to just be ProxyServers are now ProxyLists.
- ParseFromString will now accept multiple proxy specifications per scheme.
- ProxyRules::Apply now sets the ProxyInfo result to use a PAC string if any proxying
will be done.
- ProxyList now has an AddProxyServer method.
- In general, beacuse ProxyList checks for the validity of its contents before adding
new entries, IsEmpty() is used as a validity test where is_valid() used to be.
- Writing ProxyRules into a dictionary will write out each constituent ProxyList as a
PAC string.
- Slight functional change to proxy_info: Calling UsePacString() now invokes Reset(),
in line with the other Use* methods.
- Updated proxy_config_unittest to compare PAC strings of proxy rules components.

BUG=


Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=188604

[Ryan Sleevi, 2013-02-22 01:31:42]

I'm assuming you meant to publish comments for our review? :)

[marq_use_my_chromium_address, 2013-02-22 01:42:25]

On 2013/02/22 01:31:42, Ryan Sleevi wrote:
> I'm assuming you meant to publish comments for our review? :)

Someday I will learn how to use all the tools. And then they will change again.

[eroman, 2013-02-22 01:57:26]

Note: I probably won't review this until tomorrow (on my way out right now).

Cheers.

[eroman, 2013-02-26 01:13:50]

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#new...
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#new...
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#new...
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#new...
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#new...
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#new...
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#new...
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#newc...
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#newc...
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#newc...
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#newc...
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_servic...
File net/proxy/proxy_config_service_common_unittest.cc (right):

https://codereview.chromium.org/12315019/diff/1/net/proxy/proxy_config_servic...
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_unitte...
File net/proxy/proxy_config_unittest.cc (right):

https://codereview.chromium.org/12315019/diff/1/net/proxy/proxy_config_unitte...
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#newco...
net/proxy/proxy_list.cc:121: return ToPacString() == other.ToPacString();
Not ideal. I will allow it though.

[marq_use_my_chromium_address, 2013-02-27 23:08:19]

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#new...
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#new...
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#new...
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#new...
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#new...
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#new...
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#new...
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#newc...
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#newc...
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#newc...
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#newc...
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_servic...
File net/proxy/proxy_config_service_common_unittest.cc (right):

https://codereview.chromium.org/12315019/diff/1/net/proxy/proxy_config_servic...
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_unitte...
File net/proxy/proxy_config_unittest.cc (right):

https://codereview.chromium.org/12315019/diff/1/net/proxy/proxy_config_unitte...
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#newco...
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.

[eroman, 2013-03-01 00:05:13]

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#...
net/proxy/proxy_config.cc:19: const ProxyList& proxies,
fix indentation.

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

https://codereview.chromium.org/12315019/diff/7001/net/proxy/proxy_config.cc#...
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#...
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#n...
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#n...
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#n...
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#n...
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#n...
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#n...
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_ser...
File net/proxy/proxy_config_service_common_unittest.cc (right):

https://codereview.chromium.org/12315019/diff/7001/net/proxy/proxy_config_ser...
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_uni...
File net/proxy/proxy_config_unittest.cc (left):

https://codereview.chromium.org/12315019/diff/7001/net/proxy/proxy_config_uni...
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).

[marq_use_my_chromium_address, 2013-03-04 18:04:20]

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#...
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#...
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#...
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#...
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#n...
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#n...
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#n...
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#n...
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#n...
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#n...
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_ser...
File net/proxy/proxy_config_service_common_unittest.cc (right):

https://codereview.chromium.org/12315019/diff/7001/net/proxy/proxy_config_ser...
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_uni...
File net/proxy/proxy_config_unittest.cc (left):

https://codereview.chromium.org/12315019/diff/7001/net/proxy/proxy_config_uni...
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.

[eroman, 2013-03-05 01:55:55]

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...
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#...
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#...
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#...
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_un...
File net/proxy/proxy_config_unittest.cc (right):

https://codereview.chromium.org/12315019/diff/19001/net/proxy/proxy_config_un...
net/proxy/proxy_config_unittest.cc:271: // direct:// discarded as a first or
only option for a scheme.
This comment is no longer applicable.

[marq_use_my_chromium_address, 2013-03-05 22:52:40]

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...
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#...
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#...
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#...
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_un...
File net/proxy/proxy_config_unittest.cc (right):

https://codereview.chromium.org/12315019/diff/19001/net/proxy/proxy_config_un...
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.

[commit-bot: I haz the power, 2013-03-05 23:03:12]

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

[commit-bot: I haz the power, 2013-03-05 23:28:55]

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_cl...
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.

[marq_use_my_chromium_address, 2013-03-06 23:01:12]

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_cl...
> 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.

[commit-bot: I haz the power, 2013-03-06 23:20:57]

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

[commit-bot: I haz the power, 2013-03-06 23:21:12]

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.

[eroman, 2013-03-06 23:26:32]

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#...
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_se...
File net/proxy/proxy_config_service_common_unittest.cc (left):

https://codereview.chromium.org/12315019/diff/42001/net/proxy/proxy_config_se...
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_un...
File net/proxy/proxy_config_unittest.cc (right):

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

[marq_use_my_chromium_address, 2013-03-07 00:34:29]

+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#...
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_se...
File net/proxy/proxy_config_service_common_unittest.cc (left):

https://codereview.chromium.org/12315019/diff/42001/net/proxy/proxy_config_se...
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_un...
File net/proxy/proxy_config_unittest.cc (right):

https://codereview.chromium.org/12315019/diff/42001/net/proxy/proxy_config_un...
net/proxy/proxy_config_unittest.cc:133: 
On 2013/03/06 23:26:32, eroman wrote:
> nit: remove empty line

Done.

[battre, 2013-03-07 10:18:54]

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

https://codereview.chromium.org/12315019/diff/44009/chrome/browser/extensions...
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.

[Miranda Callahan, 2013-03-07 14:31:23]

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.

On 2013/03/07 10:18:54, battre wrote:
>
https://codereview.chromium.org/12315019/diff/44009/chrome/browser/extensions...
> File chrome/browser/extensions/api/proxy/proxy_api_helpers.cc (right):
> 
>
https://codereview.chromium.org/12315019/diff/44009/chrome/browser/extensions...
> 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.

[marq_use_my_chromium_address, 2013-03-07 20:26:15]

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...
File chrome/browser/extensions/api/proxy/proxy_api_helpers.cc (right):

https://codereview.chromium.org/12315019/diff/44009/chrome/browser/extensions...
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.

[battre, 2013-03-08 08:03:06]

LGTM @ extensions/api/proxy/

[Ilya Sherman, 2013-03-08 20:52:15]

importer/ LGTM

[commit-bot: I haz the power, 2013-03-08 20:54:25]

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

[commit-bot: I haz the power, 2013-03-08 21:14:24]

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_cl...
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.

[marq_use_my_chromium_address, 2013-03-08 22:48:19]

+mpcomplete, +jar for chrome/browser/chromeos/

[Matt Perry, 2013-03-08 23:59:36]

I assume you added me for /extensions/* ? LGTM for that

[marq_use_my_chromium_address, 2013-03-09 00:17:24]

On 2013/03/08 23:59:36, Matt Perry wrote:
> I assume you added me for /extensions/* ? LGTM for that

No, I added you for  chrome/browser/chromeos/ (presubmit suggested you and jar@
among others).

[Matt Perry, 2013-03-09 00:21:09]

On 2013/03/09 00:17:24, marq wrote:
> On 2013/03/08 23:59:36, Matt Perry wrote:
> > I assume you added me for /extensions/* ? LGTM for that
> 
> No, I added you for  chrome/browser/chromeos/ (presubmit suggested you and
jar@
> among others).

Oh then I take it back :). I'm not a chromeos OWNER.

[marq_use_my_chromium_address, 2013-03-09 00:24:30]

On 2013/03/09 00:21:09, Matt Perry wrote:
> On 2013/03/09 00:17:24, marq wrote:
> > On 2013/03/08 23:59:36, Matt Perry wrote:
> > > I assume you added me for /extensions/* ? LGTM for that
> > 
> > No, I added you for  chrome/browser/chromeos/ (presubmit suggested you and
> jar@
> > among others).
> 
> Oh then I take it back :). I'm not a chromeos OWNER.

Yeah, and neither is jar@! Silly presumbmit!

-jar@, -mpcomplete@

+zelidrag@

[marq_use_my_chromium_address, 2013-03-09 00:25:23]

+zelidrag@ for /chrome/browser/chromeos/

[marq_use_my_chromium_address, 2013-03-13 16:11:22]

On 2013/03/09 00:25:23, marq wrote:
> +zelidrag@ for /chrome/browser/chromeos/

Ping??

-zelidrag@
+satorux@, still for /chrome/browser/chromeos/

[marq_use_my_chromium_address, 2013-03-13 23:22:53]

On 2013/03/13 16:11:22, marq wrote:
> On 2013/03/09 00:25:23, marq wrote:
> > +zelidrag@ for /chrome/browser/chromeos/
> 
> Ping??
> 
> -zelidrag@
> +satorux@, still for /chrome/browser/chromeos/

Ping?

If there's a better person to review the two chromeos files, please let me know
ASAP.

[Ilya Sherman, 2013-03-14 00:26:05]

Is there any reason not to just TBR the chromeos changes?

[satorux1, 2013-03-14 00:46:33]

chrome/browser/chromeos LGTM, sorry for the belated response.

[marq_use_my_chromium_address, 2013-03-14 16:44:38]

On 2013/03/14 00:46:33, satorux1 wrote:
> chrome/browser/chromeos LGTM, sorry for the belated response.

Thanks!

[commit-bot: I haz the power, 2013-03-14 16:47:03]

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

[commit-bot: I haz the power, 2013-03-14 17:30:03]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
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.

[commit-bot: I haz the power, 2013-03-14 17:39:06]

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

[commit-bot: I haz the power, 2013-03-14 20:27:00]

Retried try job too often on linux_chromeos for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-03-16 11:28:21]

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

[commit-bot: I haz the power, 2013-03-16 14:04:24]

Retried try job too often on win7_aura for step(s) content_browsertests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...

[commit-bot: I haz the power, 2013-03-16 17:05:59]

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

[commit-bot: I haz the power, 2013-03-16 18:02:04]

Change committed as 188604