Issue 6450006: Migrate Proxy Settings API to net::ProxyServer

Issue 6450006: Migrate Proxy Settings API to net::ProxyServer (Closed)

Created:
9 years, 10 months ago by battre

Modified:
9 years, 7 months ago

Reviewers:
eroman

CC:
chromium-reviews, cbentzel+watch_chromium.org, Erik does not do reviews, Aaron Boodman, pam+watch_chromium.org, Paweł Hajdan Jr., darin-cc_chromium.org

Base URL:
svn://svn.chromium.org/chrome/trunk/src/out/Debug

Visibility:
Public.

Description

Migrate Proxy Settings API to net::ProxyServer BUG=none TEST=none Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=74293

Patch Set 3 : Addressed Eric's comments #

Patch Set 4 : Fixed one more whitespace #

Created: 9 years, 10 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+131 lines, -134 lines)			Patch
M	chrome/browser/extensions/extension_proxy_api.h	View		1 chunk	+0 lines, -24 lines	0 comments	Download
M	chrome/browser/extensions/extension_proxy_api.cc	View	1 2	4 chunks	+95 lines, -85 lines	0 comments	Download
M	chrome/browser/extensions/extension_proxy_apitest.cc	View	1 2 3	5 chunks	+15 lines, -15 lines	0 comments	Download
M	chrome/test/data/extensions/api_test/proxy/individual/test.js	View	1 2	1 chunk	+0 lines, -1 line	0 comments	Download
M	net/proxy/proxy_server.h	View	1 2	1 chunk	+6 lines, -0 lines	0 comments	Download
M	net/proxy/proxy_server.cc	View	1 2	5 chunks	+15 lines, -9 lines	0 comments	Download

Messages

Total messages: 3 (0 generated)

Expand Messages | Collapse Messages

battre

Hi Eric, based on you comments here http://codereview.chromium.org/6312088/ I have migrated the Proxy Settings API ...

9 years, 10 months ago (2011-02-08 18:03:33 UTC) #1

eroman

LGTM after addressing my comments (the first one is the most important).Thanks! http://codereview.chromium.org/6450006/diff/2001/chrome/browser/extensions/extension_proxy_api.cc File chrome/browser/extensions/extension_proxy_api.cc ...

9 years, 10 months ago (2011-02-09 05:12:46 UTC) #2

battre

9 years, 10 months ago (2011-02-09 10:04:38 UTC) #3

Thanks for the review.

http://codereview.chromium.org/6450006/diff/2001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_proxy_api.cc (right):

http://codereview.chromium.org/6450006/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_proxy_api.cc:53:
net::ProxyServer::SCHEME_HTTPS,
On 2011/02/09 05:12:46, eroman wrote:
> This should be SCHEME_HTTP.
> 
> SCHEME_HTTPS is some special experimental proxy type that rch is adding for
> spdy. User's expectation for setting proxy on "https://" though, is still to
be
> using an HTTP proxy :)
> 
> Heh, for what it's worth ChromeOS made this same mistake:
> (http://code.google.com/p/chromium-os/issues/detail?id=8423)

Done.

http://codereview.chromium.org/6450006/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_proxy_api.cc:65:
COMPILE_ASSERT(arraysize(default_schemes) == SCHEME_MAX + 1,
On 2011/02/09 05:12:46, eroman wrote:
> To be honest, I think it might be simpler to simply have a special case for
> "socks". Since every other scheme is going to be HTTP.

Done.

http://codereview.chromium.org/6450006/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_proxy_api.cc:73: const DictionaryValue*
dict,
On 2011/02/09 05:12:46, eroman wrote:
> nit: rather than making these two functions "static", how about moving them
into
> the anonymous namespace section above?

Done.

http://codereview.chromium.org/6450006/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_proxy_api.cc:101: DictionaryValue*
proxy_rules,
On 2011/02/09 05:12:46, eroman wrote:
> nit: can this fit all on one line?

Done.

http://codereview.chromium.org/6450006/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_proxy_api.cc:138: // Handle case the
anything but singleProxy is specified.
On 2011/02/09 05:12:46, eroman wrote:
> nit: typo in wording.

Done.

http://codereview.chromium.org/6450006/diff/2001/net/proxy/proxy_server.h
File net/proxy/proxy_server.h (right):

http://codereview.chromium.org/6450006/diff/2001/net/proxy/proxy_server.h#new...
net/proxy/proxy_server.h:131: // Parse the proxy scheme from a URL-like
representation, to a
On 2011/02/09 05:12:46, eroman wrote:
> nit: please use passive tense, as in "Parses"

Done.

http://codereview.chromium.org/6450006/diff/2001/net/proxy/proxy_server.h#new...
net/proxy/proxy_server.h:134: // |scheme| can be one from http, https, socks,
socks4, socks5, direct.
On 2011/02/09 05:12:46, eroman wrote:
> nit: "from" ---> "of"

Done.

Expand Messages | Collapse Messages