Check for *.googlezip.net proxies when migrating proxy pref for DRP.

Check for *.googlezip.net proxies when migrating proxy pref for DRP.

Previously, the proxy pref would not be cleared during migration if the
proxy pref was set to a Data Reduction Proxy that didn't match a
currently configured DRP, e.g. if the pref was for
"proxy.googlezip.net", but the configured DRP is
"proxy-dev.googlezip.net".

This change causes Chrome to clear the proxy pref if it contains any
proxy with a host ending with ".googlezip.net".

BUG=476610
Committed: https://crrev.com/8f9fd102faa07d91dbe680f160f9ea0057e4eb5f
Cr-Commit-Position: refs/heads/master@{#327357}

[sclittle, 2015-04-28 00:45:50]

sclittle@chromium.org changed reviewers:
+ bengr@chromium.org

[sclittle, 2015-04-28 00:45:51]

PTAL

[bengr, 2015-04-28 16:52:27]

https://codereview.chromium.org/1104413003/diff/20001/chrome/browser/net/spdy...
File chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.cc
(right):

https://codereview.chromium.org/1104413003/diff/20001/chrome/browser/net/spdy...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.cc:99:
!ContainsDataReductionProxy(proxy_rules)) {
You might want to rename this method to be clearer, e.g.,
ContainsDataReductionProxyDefaultHostSuffix. Might also be better to move it to
Config, and I'd even consider adding these rules to Config's
ContainsDataReductionProxy, since no one else calls that method.

https://codereview.chromium.org/1104413003/diff/20001/chrome/browser/net/spdy...
File
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings_unittest.cc
(right):

https://codereview.chromium.org/1104413003/diff/20001/chrome/browser/net/spdy...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings_unittest.cc:108:
dict_->SetString("server", "http=http://proxy-dev.googlezip.net");
Add a few more strings to test. E.g "http://googlezip.net"

[sclittle, 2015-04-28 18:27:57]

https://codereview.chromium.org/1104413003/diff/20001/chrome/browser/net/spdy...
File chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.cc
(right):

https://codereview.chromium.org/1104413003/diff/20001/chrome/browser/net/spdy...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.cc:99:
!ContainsDataReductionProxy(proxy_rules)) {
On 2015/04/28 16:52:27, bengr wrote:
> You might want to rename this method to be clearer, e.g.,
> ContainsDataReductionProxyDefaultHostSuffix. Might also be better to move it
to
> Config, and I'd even consider adding these rules to Config's
> ContainsDataReductionProxy, since no one else calls that method.

Renamed.

I'd lean toward keeping this here. Config only cares about the currently
configured Data Reduction Proxies - this method specifically doesn't care about
the currently configured DRPs and just looks for .googlezip.net.

Also, this method is somewhat slow, which doesn't really matter here because we
just loaded a pref and parsed the proxy rules from a string anyways, but that
slowness could be significant if it were run on every request like most of the
stuff in Config is.

It's also easier to merge if it's just in this file.

https://codereview.chromium.org/1104413003/diff/20001/chrome/browser/net/spdy...
File
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings_unittest.cc
(right):

https://codereview.chromium.org/1104413003/diff/20001/chrome/browser/net/spdy...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings_unittest.cc:108:
dict_->SetString("server", "http=http://proxy-dev.googlezip.net");
On 2015/04/28 16:52:27, bengr wrote:
> Add a few more strings to test. E.g "http://googlezip.net"

Sure, added some more.

"http://googlezip.net" doesn't actually get matched here, since this only
matches "*.googlezip.net" (note the leading ".").

The idea was that someone might have a proxy at a domain name ending in
"googlezip.net", such as "thisismyproxynotgooglezip.net" or
"arbitraryprefixgooglezip.net", and we wouldn't want to delete those proxy prefs
inadvertently.

[bengr, 2015-04-28 18:31:53]

https://codereview.chromium.org/1104413003/diff/20001/chrome/browser/net/spdy...
File
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings_unittest.cc
(right):

https://codereview.chromium.org/1104413003/diff/20001/chrome/browser/net/spdy...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings_unittest.cc:108:
dict_->SetString("server", "http=http://proxy-dev.googlezip.net");
On 2015/04/28 18:27:57, sclittle wrote:
> On 2015/04/28 16:52:27, bengr wrote:
> > Add a few more strings to test. E.g "http://googlezip.net"
> 
> Sure, added some more.
> 
> "http://googlezip.net" doesn't actually get matched here, since this only
> matches "*.googlezip.net" (note the leading ".").
> 
> The idea was that someone might have a proxy at a domain name ending in
> "googlezip.net", such as "thisismyproxynotgooglezip.net" or
> "arbitraryprefixgooglezip.net", and we wouldn't want to delete those proxy
prefs
> inadvertently.

I understand the intention, but please also test http://googlezip.net to make it
clear that it would not match.

[sclittle, 2015-04-28 19:25:20]

https://codereview.chromium.org/1104413003/diff/20001/chrome/browser/net/spdy...
File
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings_unittest.cc
(right):

https://codereview.chromium.org/1104413003/diff/20001/chrome/browser/net/spdy...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings_unittest.cc:108:
dict_->SetString("server", "http=http://proxy-dev.googlezip.net");
On 2015/04/28 18:31:53, bengr wrote:
> On 2015/04/28 18:27:57, sclittle wrote:
> > On 2015/04/28 16:52:27, bengr wrote:
> > > Add a few more strings to test. E.g "http://googlezip.net"
> > 
> > Sure, added some more.
> > 
> > "http://googlezip.net" doesn't actually get matched here, since this only
> > matches "*.googlezip.net" (note the leading ".").
> > 
> > The idea was that someone might have a proxy at a domain name ending in
> > "googlezip.net", such as "thisismyproxynotgooglezip.net" or
> > "arbitraryprefixgooglezip.net", and we wouldn't want to delete those proxy
> prefs
> > inadvertently.
> 
> I understand the intention, but please also test http://googlezip.net to make
it
> clear that it would not match.

Done.

[bengr, 2015-04-28 19:27:56]

lgtm

[sclittle, 2015-04-28 19:28:18]

The CQ bit was checked by sclittle@chromium.org

[commit-bot: I haz the power, 2015-04-28 19:28:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1104413003/60001

[commit-bot: I haz the power, 2015-04-28 20:29:57]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-04-28 20:30:40]

Patchset 4 (id:??) landed as
https://crrev.com/8f9fd102faa07d91dbe680f160f9ea0057e4eb5f
Cr-Commit-Position: refs/heads/master@{#327357}