AU: Proxy Resolver classes

AU: Proxy Resolver classes

A collection of classes (abstract ProxyResolver interface and two
concrete implementations). The abstraction is one that should suit us
moving forward: a string URL is provided and a collection of proxy
servers is returned. One implementation always returns [no
proxy]. Another returns [manual settings from chrome, if applicable,
..., no proxy].

A future concrete implementation will consult Chrome via DBus with the
URL in question, however this is delayed until Chrome exposes such an
API.

As a result of this API missing from Chrome, this CL only resolves
proxies for a URL with manually input proxy settings.

Future CLs will integrate this into the rest of the update system.

BUG=3167
TEST=unit tests, (with future CLs) working proxy support on device

Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=9cd120d

[petkov, 2010-11-18 23:26:46]

Mostly nits.

http://codereview.chromium.org/5151005/diff/2001/chrome_proxy_resolver.cc
File chrome_proxy_resolver.cc (right):

http://codereview.chromium.org/5151005/diff/2001/chrome_proxy_resolver.cc#new...
chrome_proxy_resolver.cc:1: // Copyright (c) 2010 The Chromium Authors. All
rights reserved.
Chromium OS

http://codereview.chromium.org/5151005/diff/2001/chrome_proxy_resolver.cc#new...
chrome_proxy_resolver.cc:7: #include <string>
no need to include it again

http://codereview.chromium.org/5151005/diff/2001/chrome_proxy_resolver.cc#new...
chrome_proxy_resolver.cc:20: extern const char kNoProxy[];
declared in proxy_resolver.h already? remove

http://codereview.chromium.org/5151005/diff/2001/chrome_proxy_resolver.cc#new...
chrome_proxy_resolver.cc:25: const char kSessionManagerRetreivePropertyMethod[]
=
Retrieve

http://codereview.chromium.org/5151005/diff/2001/chrome_proxy_resolver.cc#new...
chrome_proxy_resolver.cc:41: bool
ChromeProxyResolver::JsonProxySettings(DBusGProxy* proxy,
add empty line before

http://codereview.chromium.org/5151005/diff/2001/chrome_proxy_resolver.cc#new...
chrome_proxy_resolver.cc:64: if (!bus) {
You may as well TEST_AND_RETURN_FALSE(bus) here unless you also want to print
the |error|.

http://codereview.chromium.org/5151005/diff/2001/chrome_proxy_resolver.cc#new...
chrome_proxy_resolver.cc:167: 
remove extra blank line

http://codereview.chromium.org/5151005/diff/2001/chrome_proxy_resolver.h
File chrome_proxy_resolver.h (right):

http://codereview.chromium.org/5151005/diff/2001/chrome_proxy_resolver.h#newc...
chrome_proxy_resolver.h:1: // Copyright (c) 2010 The Chromium Authors. All
rights reserved.
Chromium OS Authors

http://codereview.chromium.org/5151005/diff/2001/chrome_proxy_resolver.h#newc...
chrome_proxy_resolver.h:12: 
remove blank line

http://codereview.chromium.org/5151005/diff/2001/chrome_proxy_resolver.h#newc...
chrome_proxy_resolver.h:13: #include "gtest/gtest_prod.h"  // for FRIEND_TEST
<gtest/gtest_prod.h>

http://codereview.chromium.org/5151005/diff/2001/chrome_proxy_resolver.h#newc...
chrome_proxy_resolver.h:23: extern const char
kSessionManagerRetreivePropertyMethod[];
typo: Retrieve

http://codereview.chromium.org/5151005/diff/2001/chrome_proxy_resolver.h#newc...
chrome_proxy_resolver.h:41: static bool ProxyTypeForProxy(const std::string&
proxy,
GetProxyTypeForProxy

http://codereview.chromium.org/5151005/diff/2001/chrome_proxy_resolver.h#newc...
chrome_proxy_resolver.h:45: FRIEND_TEST(ChromeProxyResolverTest,
ProxyForUrlWithSettingsTest);
add an empty line after

http://codereview.chromium.org/5151005/diff/2001/chrome_proxy_resolver.h#newc...
chrome_proxy_resolver.h:47: bool DbusProxy(DBusGProxy** out_proxy);
Maybe simplify the prototype to:

DBusGProxy* GetDbusProxy();

returns NULL on failure.

http://codereview.chromium.org/5151005/diff/2001/chrome_proxy_resolver.h#newc...
chrome_proxy_resolver.h:50: bool JsonProxySettings(DBusGProxy* proxy,
std::string* out_json);
GetJsonProxySettings

http://codereview.chromium.org/5151005/diff/2001/chrome_proxy_resolver.h#newc...
chrome_proxy_resolver.h:53: // returns the proper proxy server.
It returns a list of servers, right?

http://codereview.chromium.org/5151005/diff/2001/chrome_proxy_resolver.h#newc...
chrome_proxy_resolver.h:54: bool ProxyForUrlWithSettings(const std::string& url,
GetProxyForURLWithSettings?

http://codereview.chromium.org/5151005/diff/2001/chrome_proxy_resolver.h#newc...
chrome_proxy_resolver.h:59: };
DISALLOW_COPY_AND_ASSIGN?

http://codereview.chromium.org/5151005/diff/2001/chrome_proxy_resolver_unitte...
File chrome_proxy_resolver_unittest.cc (right):

http://codereview.chromium.org/5151005/diff/2001/chrome_proxy_resolver_unitte...
chrome_proxy_resolver_unittest.cc:128: // static bool ProxyTypeForProxy(const
std::string& proxy,
remove

http://codereview.chromium.org/5151005/diff/2001/dbus_interface.h
File dbus_interface.h (right):

http://codereview.chromium.org/5151005/diff/2001/dbus_interface.h#newcode38
dbus_interface.h:38: // Mock calls only go up to 10 args, so this is not
required to be
add an empty line before.

http://codereview.chromium.org/5151005/diff/2001/dbus_interface.h#newcode43
dbus_interface.h:43: GType, const char*,
per style, you have to name the parameters, right?

http://codereview.chromium.org/5151005/diff/2001/dbus_interface.h#newcode47
dbus_interface.h:47: GType) {
The last argument is always INVALID, right? So you could infer it and remove the
last argument and avoid the 10-argument-limit issues. dbus_g_proxy_call can't
infer it because it uses varargs, I think.

http://codereview.chromium.org/5151005/diff/2001/dbus_interface.h#newcode84
dbus_interface.h:84: virtual gboolean ProxyCall(DBusGProxy* proxy,
add an empty line before.

http://codereview.chromium.org/5151005/diff/2001/dbus_interface.h#newcode92
dbus_interface.h:92: return dbus_g_proxy_call(
Do you want to use a dbus_g_proxy_call with a timeout, just in case? Or is there
some default timeout that's used here?

http://codereview.chromium.org/5151005/diff/2001/proxy_resolver.h
File proxy_resolver.h (right):

http://codereview.chromium.org/5151005/diff/2001/proxy_resolver.h#newcode1
proxy_resolver.h:1: // Copyright (c) 2010 The Chromium Authors. All rights
reserved.
Chromium OS

http://codereview.chromium.org/5151005/diff/2001/proxy_resolver.h#newcode24
proxy_resolver.h:24: // empy string - no proxy
typo: empty

http://codereview.chromium.org/5151005/diff/2001/proxy_resolver.h#newcode25
proxy_resolver.h:25: virtual bool ProxyForUrl(const std::string& url,
You should rename most of the new methods to include a verb. E.g.,
GetProxyForUrl.

PS. I also prefer keeping all caps for abbreviations in CamelCase (i.e,
GetProxyForURL) but feel free to ignore...

http://codereview.chromium.org/5151005/diff/2001/proxy_resolver.h#newcode27
proxy_resolver.h:27: };
DISALLOW_COPY_AND_ASSIGN?

http://codereview.chromium.org/5151005/diff/2001/proxy_resolver.h#newcode33
proxy_resolver.h:33: out_proxies->clear();
I'd suggest moving this code to the .cc file.

http://codereview.chromium.org/5151005/diff/2001/proxy_resolver.h#newcode37
proxy_resolver.h:37: };
DISALLOW_COPY_AND_ASSIGN?

[petkov, 2010-11-19 00:28:04]

Btw, if at some point we switch proxy resolution from Chrome to flimflam, would
you copy the ChromeProxyResolver class into FlimflamProxyResolver, make the
changes and delete the obsolete ChromeProxyResolver? Or would you rather just
change ChromeProxyResolver in place? If the latter, we should probably pick a
more generic name for the class (e.g., SystemProxyResolver).

[adlr, 2010-11-19 00:41:06]

PTAL. thanks

http://codereview.chromium.org/5151005/diff/2001/chrome_proxy_resolver.cc
File chrome_proxy_resolver.cc (right):

http://codereview.chromium.org/5151005/diff/2001/chrome_proxy_resolver.cc#new...
chrome_proxy_resolver.cc:1: // Copyright (c) 2010 The Chromium Authors. All
rights reserved.
On 2010/11/18 23:26:46, petkov wrote:
> Chromium OS

Done.

http://codereview.chromium.org/5151005/diff/2001/chrome_proxy_resolver.cc#new...
chrome_proxy_resolver.cc:7: #include <string>
On 2010/11/18 23:26:46, petkov wrote:
> no need to include it again

Done.

http://codereview.chromium.org/5151005/diff/2001/chrome_proxy_resolver.cc#new...
chrome_proxy_resolver.cc:20: extern const char kNoProxy[];
On 2010/11/18 23:26:46, petkov wrote:
> declared in proxy_resolver.h already? remove

Done.

http://codereview.chromium.org/5151005/diff/2001/chrome_proxy_resolver.cc#new...
chrome_proxy_resolver.cc:25: const char kSessionManagerRetreivePropertyMethod[]
=
On 2010/11/18 23:26:46, petkov wrote:
> Retrieve

Done.

http://codereview.chromium.org/5151005/diff/2001/chrome_proxy_resolver.cc#new...
chrome_proxy_resolver.cc:41: bool
ChromeProxyResolver::JsonProxySettings(DBusGProxy* proxy,
On 2010/11/18 23:26:46, petkov wrote:
> add empty line before

Done.

http://codereview.chromium.org/5151005/diff/2001/chrome_proxy_resolver.cc#new...
chrome_proxy_resolver.cc:64: if (!bus) {
On 2010/11/18 23:26:46, petkov wrote:
> You may as well TEST_AND_RETURN_FALSE(bus) here unless you also want to print
> the |error|.
> 

Done.

http://codereview.chromium.org/5151005/diff/2001/chrome_proxy_resolver.cc#new...
chrome_proxy_resolver.cc:167: 
On 2010/11/18 23:26:46, petkov wrote:
> remove extra blank line

Done.

http://codereview.chromium.org/5151005/diff/2001/chrome_proxy_resolver.h
File chrome_proxy_resolver.h (right):

http://codereview.chromium.org/5151005/diff/2001/chrome_proxy_resolver.h#newc...
chrome_proxy_resolver.h:1: // Copyright (c) 2010 The Chromium Authors. All
rights reserved.
On 2010/11/18 23:26:46, petkov wrote:
> Chromium OS Authors

Done.

http://codereview.chromium.org/5151005/diff/2001/chrome_proxy_resolver.h#newc...
chrome_proxy_resolver.h:12: 
On 2010/11/18 23:26:46, petkov wrote:
> remove blank line

Done.

http://codereview.chromium.org/5151005/diff/2001/chrome_proxy_resolver.h#newc...
chrome_proxy_resolver.h:13: #include "gtest/gtest_prod.h"  // for FRIEND_TEST
On 2010/11/18 23:26:46, petkov wrote:
> <gtest/gtest_prod.h>

Done.

http://codereview.chromium.org/5151005/diff/2001/chrome_proxy_resolver.h#newc...
chrome_proxy_resolver.h:23: extern const char
kSessionManagerRetreivePropertyMethod[];
On 2010/11/18 23:26:46, petkov wrote:
> typo: Retrieve

Done.

http://codereview.chromium.org/5151005/diff/2001/chrome_proxy_resolver.h#newc...
chrome_proxy_resolver.h:41: static bool ProxyTypeForProxy(const std::string&
proxy,
On 2010/11/18 23:26:46, petkov wrote:
> GetProxyTypeForProxy

Done.

http://codereview.chromium.org/5151005/diff/2001/chrome_proxy_resolver.h#newc...
chrome_proxy_resolver.h:45: FRIEND_TEST(ChromeProxyResolverTest,
ProxyForUrlWithSettingsTest);
On 2010/11/18 23:26:46, petkov wrote:
> add an empty line after

Done.

http://codereview.chromium.org/5151005/diff/2001/chrome_proxy_resolver.h#newc...
chrome_proxy_resolver.h:47: bool DbusProxy(DBusGProxy** out_proxy);
On 2010/11/18 23:26:46, petkov wrote:
> Maybe simplify the prototype to:
> 
> DBusGProxy* GetDbusProxy();
> 
> returns NULL on failure.

Done.

http://codereview.chromium.org/5151005/diff/2001/chrome_proxy_resolver.h#newc...
chrome_proxy_resolver.h:50: bool JsonProxySettings(DBusGProxy* proxy,
std::string* out_json);
On 2010/11/18 23:26:46, petkov wrote:
> GetJsonProxySettings

Done.

http://codereview.chromium.org/5151005/diff/2001/chrome_proxy_resolver.h#newc...
chrome_proxy_resolver.h:53: // returns the proper proxy server.
On 2010/11/18 23:26:46, petkov wrote:
> It returns a list of servers, right?

yes, fixed.

http://codereview.chromium.org/5151005/diff/2001/chrome_proxy_resolver.h#newc...
chrome_proxy_resolver.h:54: bool ProxyForUrlWithSettings(const std::string& url,
On 2010/11/18 23:26:46, petkov wrote:
> GetProxyForURLWithSettings?

GetProxiesForUrlWithSettings

http://codereview.chromium.org/5151005/diff/2001/chrome_proxy_resolver.h#newc...
chrome_proxy_resolver.h:59: };
On 2010/11/18 23:26:46, petkov wrote:
> DISALLOW_COPY_AND_ASSIGN?

Done.

http://codereview.chromium.org/5151005/diff/2001/chrome_proxy_resolver_unitte...
File chrome_proxy_resolver_unittest.cc (right):

http://codereview.chromium.org/5151005/diff/2001/chrome_proxy_resolver_unitte...
chrome_proxy_resolver_unittest.cc:128: // static bool ProxyTypeForProxy(const
std::string& proxy,
On 2010/11/18 23:26:46, petkov wrote:
> remove

Done.

http://codereview.chromium.org/5151005/diff/2001/dbus_interface.h
File dbus_interface.h (right):

http://codereview.chromium.org/5151005/diff/2001/dbus_interface.h#newcode38
dbus_interface.h:38: // Mock calls only go up to 10 args, so this is not
required to be
On 2010/11/18 23:26:46, petkov wrote:
> add an empty line before.

this comment was outdated and has been removed. also, made this method pure
virtual like the others.

http://codereview.chromium.org/5151005/diff/2001/dbus_interface.h#newcode43
dbus_interface.h:43: GType, const char*,
On 2010/11/18 23:26:46, petkov wrote:
> per style, you have to name the parameters, right?

Done.

http://codereview.chromium.org/5151005/diff/2001/dbus_interface.h#newcode47
dbus_interface.h:47: GType) {
On 2010/11/18 23:26:46, petkov wrote:
> The last argument is always INVALID, right? So you could infer it and remove
the
> last argument and avoid the 10-argument-limit issues. dbus_g_proxy_call can't
> infer it because it uses varargs, I think.

i got the mock to implement this, so the impl here has been removed.

http://codereview.chromium.org/5151005/diff/2001/dbus_interface.h#newcode84
dbus_interface.h:84: virtual gboolean ProxyCall(DBusGProxy* proxy,
On 2010/11/18 23:26:46, petkov wrote:
> add an empty line before.

Done.

http://codereview.chromium.org/5151005/diff/2001/dbus_interface.h#newcode92
dbus_interface.h:92: return dbus_g_proxy_call(
On 2010/11/18 23:26:46, petkov wrote:
> Do you want to use a dbus_g_proxy_call with a timeout, just in case? Or is
there
> some default timeout that's used here?
> 

I think there's a default timeout of 30s, which the other proxy call fn uses.
Should be fine, I think...

http://codereview.chromium.org/5151005/diff/2001/proxy_resolver.h
File proxy_resolver.h (right):

http://codereview.chromium.org/5151005/diff/2001/proxy_resolver.h#newcode1
proxy_resolver.h:1: // Copyright (c) 2010 The Chromium Authors. All rights
reserved.
On 2010/11/18 23:26:46, petkov wrote:
> Chromium OS

Done.

http://codereview.chromium.org/5151005/diff/2001/proxy_resolver.h#newcode24
proxy_resolver.h:24: // empy string - no proxy
On 2010/11/18 23:26:46, petkov wrote:
> typo: empty

comment was outdated. fixed

http://codereview.chromium.org/5151005/diff/2001/proxy_resolver.h#newcode25
proxy_resolver.h:25: virtual bool ProxyForUrl(const std::string& url,
On 2010/11/18 23:26:46, petkov wrote:
> You should rename most of the new methods to include a verb. E.g.,
> GetProxyForUrl.

done, i think.

> 
> PS. I also prefer keeping all caps for abbreviations in CamelCase (i.e,
> GetProxyForURL) but feel free to ignore...

The google style guide uses lowercase for the non-first letters of an acronym,
it seems. look for "url" in
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml

http://codereview.chromium.org/5151005/diff/2001/proxy_resolver.h#newcode27
proxy_resolver.h:27: };
On 2010/11/18 23:26:46, petkov wrote:
> DISALLOW_COPY_AND_ASSIGN?

Done.

http://codereview.chromium.org/5151005/diff/2001/proxy_resolver.h#newcode33
proxy_resolver.h:33: out_proxies->clear();
On 2010/11/18 23:26:46, petkov wrote:
> I'd suggest moving this code to the .cc file.

Done.

http://codereview.chromium.org/5151005/diff/2001/proxy_resolver.h#newcode37
proxy_resolver.h:37: };
On 2010/11/18 23:26:46, petkov wrote:
> DISALLOW_COPY_AND_ASSIGN?

Done.

[adlr, 2010-11-19 00:51:17]

I think at that point we would have yet another type of ProxyResolver, and
would make a FlimFlamProxyResolver.

We would maybe then want to combine it w/ FlimFlamProxy, or maybe rename
FlimFlamProxy? But, I think the code for that would be different from the
ChromeProxyResolver, and the two could potentially both be functional.

On Thu, Nov 18, 2010 at 4:28 PM, <petkov@chromium.org> wrote:

> Btw, if at some point we switch proxy resolution from Chrome to flimflam,
> would
> you copy the ChromeProxyResolver class into FlimflamProxyResolver, make the
> changes and delete the obsolete ChromeProxyResolver? Or would you rather
> just
> change ChromeProxyResolver in place? If the latter, we should probably pick
> a
> more generic name for the class (e.g., SystemProxyResolver).
>
>
>
> http://codereview.chromium.org/5151005/
>

[petkov, 2010-11-19 01:09:19]

LGTM w/ some nits and responses below.

http://codereview.chromium.org/5151005/diff/2001/dbus_interface.h
File dbus_interface.h (right):

http://codereview.chromium.org/5151005/diff/2001/dbus_interface.h#newcode47
dbus_interface.h:47: GType) {
On 2010/11/19 00:41:06, adlr wrote:
> On 2010/11/18 23:26:46, petkov wrote:
> > The last argument is always INVALID, right? So you could infer it and remove
> the
> > last argument and avoid the 10-argument-limit issues. dbus_g_proxy_call
can't
> > infer it because it uses varargs, I think.
> 
> i got the mock to implement this, so the impl here has been removed.

OK, although the last argument is still unnecessary and can be removed.

http://codereview.chromium.org/5151005/diff/2001/proxy_resolver.h
File proxy_resolver.h (right):

http://codereview.chromium.org/5151005/diff/2001/proxy_resolver.h#newcode25
proxy_resolver.h:25: virtual bool ProxyForUrl(const std::string& url,
On 2010/11/19 00:41:06, adlr wrote:
> On 2010/11/18 23:26:46, petkov wrote:
> > You should rename most of the new methods to include a verb. E.g.,
> > GetProxyForUrl.
> 
> done, i think.
> 
> > 
> > PS. I also prefer keeping all caps for abbreviations in CamelCase (i.e,
> > GetProxyForURL) but feel free to ignore...
> 
> The google style guide uses lowercase for the non-first letters of an acronym,
> it seems. look for "url" in
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml

Ugly. They also use kOK. And we have JSONParser class with a JsonToValue method.
If you search for CamelCase on the web, standard is all caps for acronyms. But
anyway, it's all messed up already anyway.

http://codereview.chromium.org/5151005/diff/21001/chrome_proxy_resolver.cc
File chrome_proxy_resolver.cc (right):

http://codereview.chromium.org/5151005/diff/21001/chrome_proxy_resolver.cc#ne...
chrome_proxy_resolver.cc:26: std::vector<std::string>* out_proxies) {
indent is off

http://codereview.chromium.org/5151005/diff/21001/chrome_proxy_resolver.h
File chrome_proxy_resolver.h (right):

http://codereview.chromium.org/5151005/diff/21001/chrome_proxy_resolver.h#new...
chrome_proxy_resolver.h:40: static bool GetTypeForProxy(const std::string&
proxy,
suggestion: GetProxyType

http://codereview.chromium.org/5151005/diff/21001/chrome_proxy_resolver_unitt...
File chrome_proxy_resolver_unittest.cc (right):

http://codereview.chromium.org/5151005/diff/21001/chrome_proxy_resolver_unitt...
chrome_proxy_resolver_unittest.cc:72:
EXPECT_FALSE(resolver.GetProxiesForUrlWithSettings("http://foo.com", "}",
&out));
80 chars

http://codereview.chromium.org/5151005/diff/21001/chrome_proxy_resolver_unitt...
chrome_proxy_resolver_unittest.cc:121:
EXPECT_TRUE(resolver.GetProxiesForUrl("http://user:pass@foo.com:22", &proxies));
80 chars

http://codereview.chromium.org/5151005/diff/21001/proxy_resolver.h
File proxy_resolver.h (right):

http://codereview.chromium.org/5151005/diff/21001/proxy_resolver.h#newcode22
proxy_resolver.h:22: // Stores a proxy for a given |url| in |out_proxy|. Returns
true on success.
Stores a list of proxies for a given ...

[adlr, 2010-11-19 02:00:52]

http://codereview.chromium.org/5151005/diff/2001/dbus_interface.h
File dbus_interface.h (right):

http://codereview.chromium.org/5151005/diff/2001/dbus_interface.h#newcode47
dbus_interface.h:47: GType) {
On 2010/11/19 01:09:19, petkov wrote:
> On 2010/11/19 00:41:06, adlr wrote:
> > On 2010/11/18 23:26:46, petkov wrote:
> > > The last argument is always INVALID, right? So you could infer it and
remove
> > the
> > > last argument and avoid the 10-argument-limit issues. dbus_g_proxy_call
> can't
> > > infer it because it uses varargs, I think.
> > 
> > i got the mock to implement this, so the impl here has been removed.
> 
> OK, although the last argument is still unnecessary and can be removed.

It's true that it's not necessary since in practice it's always 'invalid', but I
would argue it's nice to keep the interface faithful to the real dbus calls, and
deal with gmocks' trouble with 11-arg functions only in the tests. Should gmock
ever change and support > 10 arg functions, we would probably want the dbus
interface to have 11 args in this function.

http://codereview.chromium.org/5151005/diff/2001/proxy_resolver.h
File proxy_resolver.h (right):

http://codereview.chromium.org/5151005/diff/2001/proxy_resolver.h#newcode25
proxy_resolver.h:25: virtual bool ProxyForUrl(const std::string& url,
On 2010/11/19 01:09:19, petkov wrote:
> On 2010/11/19 00:41:06, adlr wrote:
> > On 2010/11/18 23:26:46, petkov wrote:
> > > You should rename most of the new methods to include a verb. E.g.,
> > > GetProxyForUrl.
> > 
> > done, i think.
> > 
> > > 
> > > PS. I also prefer keeping all caps for abbreviations in CamelCase (i.e,
> > > GetProxyForURL) but feel free to ignore...
> > 
> > The google style guide uses lowercase for the non-first letters of an
acronym,
> > it seems. look for "url" in
> > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml
> 
> Ugly. They also use kOK. And we have JSONParser class with a JsonToValue
method.
> If you search for CamelCase on the web, standard is all caps for acronyms. But
> anyway, it's all messed up already anyway.

If you look at the update_engine code today, we use Url a lot more than URL.
should we change them all?

for now i'll push as-is

http://codereview.chromium.org/5151005/diff/21001/chrome_proxy_resolver.cc
File chrome_proxy_resolver.cc (right):

http://codereview.chromium.org/5151005/diff/21001/chrome_proxy_resolver.cc#ne...
chrome_proxy_resolver.cc:26: std::vector<std::string>* out_proxies) {
On 2010/11/19 01:09:19, petkov wrote:
> indent is off

Done.

http://codereview.chromium.org/5151005/diff/21001/chrome_proxy_resolver.h
File chrome_proxy_resolver.h (right):

http://codereview.chromium.org/5151005/diff/21001/chrome_proxy_resolver.h#new...
chrome_proxy_resolver.h:40: static bool GetTypeForProxy(const std::string&
proxy,
On 2010/11/19 01:09:19, petkov wrote:
> suggestion: GetProxyType

Done.

http://codereview.chromium.org/5151005/diff/21001/chrome_proxy_resolver_unitt...
File chrome_proxy_resolver_unittest.cc (right):

http://codereview.chromium.org/5151005/diff/21001/chrome_proxy_resolver_unitt...
chrome_proxy_resolver_unittest.cc:72:
EXPECT_FALSE(resolver.GetProxiesForUrlWithSettings("http://foo.com", "}",
&out));
On 2010/11/19 01:09:19, petkov wrote:
> 80 chars

Done.

http://codereview.chromium.org/5151005/diff/21001/chrome_proxy_resolver_unitt...
chrome_proxy_resolver_unittest.cc:121:
EXPECT_TRUE(resolver.GetProxiesForUrl("http://user:pass@foo.com:22", &proxies));
On 2010/11/19 01:09:19, petkov wrote:
> 80 chars

Done.

http://codereview.chromium.org/5151005/diff/21001/proxy_resolver.h
File proxy_resolver.h (right):

http://codereview.chromium.org/5151005/diff/21001/proxy_resolver.h#newcode22
proxy_resolver.h:22: // Stores a proxy for a given |url| in |out_proxy|. Returns
true on success.
On 2010/11/19 01:09:19, petkov wrote:
> Stores a list of proxies for a given ...

Done.