Use data reduction proxy when managed proxy config returns direct

Use data reduction proxy when managed proxy config returns direct

Adds the configured data reduction proxies if any to the proxy info
for a request, if the data reduction proxy is enabled, the data
reduction proxies are not bypassed, and the proxy server returned
by the proxy info if the request is direct.

This change makes it possible for the data reduction proxy to
coexist with managed proxy configurations, instead of being
overwritten by them. The data reduction proxy may be disabled
via managed prefs if this behavior is not desired.

BUG=339237, 339258
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=289528

[bengr, 2014-08-08 22:37:46]

mef:
chrome/browser/net/chrome_network_delegate.*
net/*

marq:
chrome/browser/net/spdyproxy/* 
components/data_reduction_proxy/*

mmenke:
chrome/browser/profiles/*

[marq (ping after 24h), 2014-08-11 16:11:33]

lgtm, just nits.

https://codereview.chromium.org/449973002/diff/60001/chrome/browser/net/chrom...
File chrome/browser/net/chrome_network_delegate.h (right):

https://codereview.chromium.org/449973002/diff/60001/chrome/browser/net/chrom...
chrome/browser/net/chrome_network_delegate.h:178: 
Extra line.

https://codereview.chromium.org/449973002/diff/60001/chrome/browser/net/spdyp...
File chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_configurator.cc
(right):

https://codereview.chromium.org/449973002/diff/60001/chrome/browser/net/spdyp...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_configurator.cc:72:
JoinString(bypass_rules_, ", "));
Indent.

https://codereview.chromium.org/449973002/diff/60001/chrome/browser/net/spdyp...
File chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_configurator.h
(right):

https://codereview.chromium.org/449973002/diff/60001/chrome/browser/net/spdyp...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_configurator.h:61: void
GetProxyConfigOnIO(net::ProxyConfig* config);
OnIOThread, for naming consistency.

[mef, 2014-08-11 19:44:40]

https://codereview.chromium.org/449973002/diff/60001/chrome/browser/net/chrom...
File chrome/browser/net/chrome_network_delegate.cc (right):

https://codereview.chromium.org/449973002/diff/60001/chrome/browser/net/chrom...
chrome/browser/net/chrome_network_delegate.cc:442:
proxy_service->proxy_retry_info(),
If proxy_service is never NULL, should it be a const reference instead of
pointer?

https://codereview.chromium.org/449973002/diff/60001/chrome/browser/net/chrom...
File chrome/browser/net/chrome_network_delegate.h (right):

https://codereview.chromium.org/449973002/diff/60001/chrome/browser/net/chrom...
chrome/browser/net/chrome_network_delegate.h:174: void
set_proxy_config_getter(ProxyConfigGetter getter) {
const &?

https://codereview.chromium.org/449973002/diff/60001/chrome/browser/net/spdyp...
File chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_configurator.h
(right):

https://codereview.chromium.org/449973002/diff/60001/chrome/browser/net/spdyp...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_configurator.h:32:
scoped_refptr<base::SingleThreadTaskRunner> network_task_runner);
Does it have to be SingleThreaded or SequencedTaskRunner will do?

[bengr, 2014-08-11 23:03:19]

https://codereview.chromium.org/449973002/diff/60001/chrome/browser/net/chrom...
File chrome/browser/net/chrome_network_delegate.cc (right):

https://codereview.chromium.org/449973002/diff/60001/chrome/browser/net/chrom...
chrome/browser/net/chrome_network_delegate.cc:442:
proxy_service->proxy_retry_info(),
On 2014/08/11 19:44:40, mef wrote:
> If proxy_service is never NULL, should it be a const reference instead of
> pointer?

Done.

https://codereview.chromium.org/449973002/diff/60001/chrome/browser/net/chrom...
File chrome/browser/net/chrome_network_delegate.h (right):

https://codereview.chromium.org/449973002/diff/60001/chrome/browser/net/chrom...
chrome/browser/net/chrome_network_delegate.h:174: void
set_proxy_config_getter(ProxyConfigGetter getter) {
On 2014/08/11 19:44:40, mef wrote:
> const &?

Done.

https://codereview.chromium.org/449973002/diff/60001/chrome/browser/net/chrom...
chrome/browser/net/chrome_network_delegate.h:178: 
On 2014/08/11 16:11:33, marq_ wrote:
> Extra line.

Done.

https://codereview.chromium.org/449973002/diff/60001/chrome/browser/net/spdyp...
File chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_configurator.cc
(right):

https://codereview.chromium.org/449973002/diff/60001/chrome/browser/net/spdyp...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_configurator.cc:72:
JoinString(bypass_rules_, ", "));
On 2014/08/11 16:11:33, marq_ wrote:
> Indent.

Done.

https://codereview.chromium.org/449973002/diff/60001/chrome/browser/net/spdyp...
File chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_configurator.h
(right):

https://codereview.chromium.org/449973002/diff/60001/chrome/browser/net/spdyp...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_configurator.h:32:
scoped_refptr<base::SingleThreadTaskRunner> network_task_runner);
On 2014/08/11 19:44:40, mef wrote:
> Does it have to be SingleThreaded or SequencedTaskRunner will do?

Done.

https://codereview.chromium.org/449973002/diff/60001/chrome/browser/net/spdyp...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_configurator.h:61: void
GetProxyConfigOnIO(net::ProxyConfig* config);
On 2014/08/11 16:11:33, marq_ wrote:
> OnIOThread, for naming consistency.

Done.

[mef, 2014-08-12 17:56:35]

Couple more notes.

https://codereview.chromium.org/449973002/diff/100001/chrome/browser/net/spdy...
File chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_configurator.h
(right):

https://codereview.chromium.org/449973002/diff/100001/chrome/browser/net/spdy...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_configurator.h:58: //
effective configuration used by the proxy service. Note the ID of the
I think this comment is wrong (starting with "Note the ID...").

https://codereview.chromium.org/449973002/diff/100001/net/proxy/proxy_service...
File net/proxy/proxy_service_unittest.cc (right):

https://codereview.chromium.org/449973002/diff/100001/net/proxy/proxy_service...
net/proxy/proxy_service_unittest.cc:194: return proxy_service_;
I don't see |proxy_service_| being set to anything but NULL.

https://codereview.chromium.org/449973002/diff/100001/net/proxy/proxy_service...
net/proxy/proxy_service_unittest.cc:310: EXPECT_EQ(&service,
delegate.proxy_service());
How does it work?

[mef, 2014-08-12 20:07:37]

lgtm mod fixing the comment.

[bengr, 2014-08-12 20:09:39]

https://codereview.chromium.org/449973002/diff/100001/chrome/browser/net/spdy...
File chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_configurator.h
(right):

https://codereview.chromium.org/449973002/diff/100001/chrome/browser/net/spdy...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_configurator.h:58: //
effective configuration used by the proxy service. Note the ID of the
On 2014/08/12 17:56:35, mef wrote:
> I think this comment is wrong (starting with "Note the ID...").

I think it is correct.

ProxyConfig::ProxyConfig()
    : auto_detect_(false), pac_mandatory_(false),
      source_(PROXY_CONFIG_SOURCE_UNKNOWN), id_(kInvalidConfigID)  {
}

bool is_valid() const { return id_ != kInvalidConfigID; }

https://codereview.chromium.org/449973002/diff/100001/net/proxy/proxy_service...
File net/proxy/proxy_service_unittest.cc (right):

https://codereview.chromium.org/449973002/diff/100001/net/proxy/proxy_service...
net/proxy/proxy_service_unittest.cc:194: return proxy_service_;
On 2014/08/12 17:56:35, mef wrote:
> I don't see |proxy_service_| being set to anything but NULL.

Done.

https://codereview.chromium.org/449973002/diff/100001/net/proxy/proxy_service...
net/proxy/proxy_service_unittest.cc:310: EXPECT_EQ(&service,
delegate.proxy_service());
On 2014/08/12 17:56:35, mef wrote:
> How does it work?

Done.

[mmenke, 2014-08-12 20:15:54]

lgtm

https://codereview.chromium.org/449973002/diff/120001/chrome/browser/profiles...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/449973002/diff/120001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl.cc:666:
data_reduction_proxy_params.Pass());
How is this safe?  Looks like the chrome_configurator may be destroyed on the UI
thread before the getter is called on the IO thread.

[mmenke, 2014-08-12 20:15:55]

lgtm

[mmenke, 2014-08-12 20:15:56]

lgtm

[mmenke, 2014-08-12 20:16:29]

On 2014/08/12 20:15:56, mmenke wrote:
> lgtm

GRRR....  NOT LGTM.  Dang buttons.  Clicked to focus the window, accidentally
clicked a button.

[commit-bot: I haz the power, 2014-08-12 20:18:53]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/bengr@chromium.org/449973002/120001

[commit-bot: I haz the power, 2014-08-12 20:18:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-12 20:19:01]

A disapproval has been posted by following reviewers: mmenke@chromium.org.
Please make sure to get positive review before another CQ attempt.

[mmenke, 2014-08-12 20:19:18]

On 2014/08/12 20:16:29, mmenke wrote:
> On 2014/08/12 20:15:56, mmenke wrote:
> > lgtm
> 
> GRRR....  NOT LGTM.  Dang buttons.  Clicked to focus the window, accidentally
> clicked a button.

Oh, and just in case anyone else didn't know, accidentally clicking again on a
button you accidentally clicked on the first place does not undo the click.  Nor
does a panicked accidental 3rd click.  :)

[bengr, 2014-08-13 01:31:28]

https://codereview.chromium.org/449973002/diff/100001/chrome/browser/net/spdy...
File chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_configurator.h
(right):

https://codereview.chromium.org/449973002/diff/100001/chrome/browser/net/spdy...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_configurator.h:58: //
effective configuration used by the proxy service. Note the ID of the
On 2014/08/12 20:09:39, bengr1 wrote:
> On 2014/08/12 17:56:35, mef wrote:
> > I think this comment is wrong (starting with "Note the ID...").
> 
> I think it is correct.
> 
> ProxyConfig::ProxyConfig()
>     : auto_detect_(false), pac_mandatory_(false),
>       source_(PROXY_CONFIG_SOURCE_UNKNOWN), id_(kInvalidConfigID)  {
> }
> 
> bool is_valid() const { return id_ != kInvalidConfigID; }

Done.

https://codereview.chromium.org/449973002/diff/120001/chrome/browser/profiles...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/449973002/diff/120001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl.cc:666:
data_reduction_proxy_params.Pass());
On 2014/08/12 20:15:53, mmenke wrote:
> How is this safe?  Looks like the chrome_configurator may be destroyed on the
UI
> thread before the getter is called on the IO thread.

Done.

[bengr, 2014-08-13 01:32:08]

sgurun: android_webview

[sgurun-gerrit only, 2014-08-13 02:31:30]

On 2014/08/13 01:32:08, bengr1 wrote:
> sgurun: android_webview

aw lgtm

[mmenke, 2014-08-13 15:19:55]

On 2014/08/13 02:31:30, sgurun wrote:
> On 2014/08/13 01:32:08, bengr1 wrote:
> > sgurun: android_webview
> 
> aw lgtm

LGTM, but could you document the ownership model somewhere?  (This works because
the ProfileIOData::Handle, and thus ProfileIOData, is only destroyed after the
BrowserContextKeyedServices are, and one of those owns (is?) the settings
object).

[bengr, 2014-08-14 00:28:57]

The CQ bit was checked by bengr@chromium.org

[commit-bot: I haz the power, 2014-08-14 00:32:52]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/bengr@chromium.org/449973002/240001

[commit-bot: I haz the power, 2014-08-14 11:46:02]

Committed patchset #7 (240001) as 289528

[Ryan Sleevi, 2014-08-14 19:19:51]

Like Matt, I've found the ownership model very tricky here, and would like to
request some more documentation, as well as suggest some cleanups that should
make it easier to reason about.

I've taken a quick scan of this code, primarily for style/design issues, and
have not dug into the deeper, holistic review of this.

I found the description a bit confusing, at least in relationship to what you're
exposing. That is, I understand the description from the
//components/data_reduction_proxy perspective, but it's perhaps better to
separate out the //net changes, so that we can document how they can be used,
rather than how they are used. This helps us validate that the layering and
testing are at the right places.

I'm a little nervous that there may not be enough testing for the
NetworkDelegate - it feels like that's being outsourced to the user (data
reduction proxy) - rather than being part of the core interface support here. Do
we really have no unittests for the NetworkDelegate? That's quite unfortunate.

https://codereview.chromium.org/449973002/diff/240001/chrome/browser/net/chro...
File chrome/browser/net/chrome_network_delegate.cc (right):

https://codereview.chromium.org/449973002/diff/240001/chrome/browser/net/chro...
chrome/browser/net/chrome_network_delegate.cc:437:
!proxy_config_getter_.is_null()) {
Is this correct? It now forces proxy_config_getter_ to be non-NULL, which isn't
documented in the header as being required (it suggests it's optional, by virtue
of having an extra setter)

https://codereview.chromium.org/449973002/diff/240001/chrome/browser/net/chro...
File chrome/browser/net/chrome_network_delegate.h (right):

https://codereview.chromium.org/449973002/diff/240001/chrome/browser/net/chro...
chrome/browser/net/chrome_network_delegate.h:85: // proxy resolution.
Having read through this header, and the related code, I'm still a little
confused as to what this hook is meant to provide/how to use this correctly.

https://codereview.chromium.org/449973002/diff/240001/chrome/browser/net/spdy...
File chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_configurator.cc
(right):

https://codereview.chromium.org/449973002/diff/240001/chrome/browser/net/spdy...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_configurator.cc:78:
config.set_id(unused_id);
why not just set_id(1)? Why do you need to declare the local ID?

https://codereview.chromium.org/449973002/diff/240001/chrome/browser/net/spdy...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_configurator.cc:83:
base::Unretained(this),
This is a very dangerous usage.

No where in the header are the lifetime requirements spelled out, so you have no
guarantee that this object ("this") will still be valid when this code executes
on the IO thread.

While you could spell it out with a comment, a better design would be to avoid
this requirement altogether.

That is, favour complexity *in* this class if it means you can *avoid*
complexity in users of this class (which lifetime management is often the
high-order bit of complexity)

https://codereview.chromium.org/449973002/diff/240001/chrome/browser/net/spdy...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_configurator.cc:93:
dict->SetString("bypass_list", "");
cleanup: These should all be std::string() instead of ""

https://codereview.chromium.org/449973002/diff/240001/chrome/browser/net/spdy...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_configurator.cc:100:
config));
Did git-cl-format do this? It seems odd for it.

https://codereview.chromium.org/449973002/diff/240001/chrome/browser/net/spdy...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_configurator.cc:123:
void DataReductionProxyChromeConfigurator::UpdateProxyConfigOnIO(
If you decide to keep these methods (ideally, you wouldn't), you should add
DCHECKs to these methods to annotate throughout this class that asserts it's
used correctly

However, from looking at this (rather than just the header), it doesn't seem
necessary to
1) Expose these as public methods (as opposed to private)
2) Require they be methods on the class (rather, you could have a helper ::Core
class that lives entirely on the IO thread, and which the parent marshals
messages to/from)

https://codereview.chromium.org/449973002/diff/240001/chrome/browser/net/spdy...
File chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_configurator.h
(right):

https://codereview.chromium.org/449973002/diff/240001/chrome/browser/net/spdy...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_configurator.h:15: 
IWYU: You need to include "base/memory/ref_counted.h" to use scoped_refptr

https://codereview.chromium.org/449973002/diff/240001/chrome/browser/net/spdy...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_configurator.h:32:
scoped_refptr<base::SequencedTaskRunner> network_task_runner);
This should be a const-ref scoped_refptr

scoped_ptr is the only class in Chrome that should be passed by value. All of
the rest should be const-ref or pointers.

(For scoped_refptr<>s, it means you're adding an additional AddRef/Release call
that isn't needed)

https://codereview.chromium.org/449973002/diff/240001/chrome/browser/net/spdy...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_configurator.h:54: //
Updates the config for use on the IO thread.
So, this generally represents a dangerous design patern, in that now it means
the DataReductionProxyChromeConfigurator lives on multiple threads.

The d_r_p:DRPC doesn't have this requirement, and you're exposing these methods
publicly, which makes it very easy to get ownership wrong.

Put differently, it bakes how this class is used into the design, which is
usually a sign of a layering violation occuring.

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

https://codereview.chromium.org/449973002/diff/240001/chrome/browser/net/spdy...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.cc:7: #include
"base/memory/scoped_ptr.h"
1) IWYU: This is no longer needed
2) IWYU: This was already included in the header so wasn't needed to begin with

https://codereview.chromium.org/449973002/diff/240001/chrome/browser/net/spdy...
File chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.h
(right):

https://codereview.chromium.org/449973002/diff/240001/chrome/browser/net/spdy...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.h:8: #include
"base/memory/scoped_ptr.h"
IWYU: This is no longer needed

https://codereview.chromium.org/449973002/diff/240001/net/base/network_delega...
File net/base/network_delegate.h (right):

https://codereview.chromium.org/449973002/diff/240001/net/base/network_delega...
net/base/network_delegate.h:135: // may override the decision by modifying the
ProxyInfo |result|.
Please update this documentation to reflect how proxy_service can and should be
used. I could not find documentation that spells it out, beyond the comments on
this review.