Add Data Saver support to Cronet

Add Data Saver support to Cronet

Adds support for the Data Saver compression proxy to the Cronet
library.

BUG=461910
Committed: https://crrev.com/59cb6968d3bcb367ec97f6c3c88da08f7921e697
Cr-Commit-Position: refs/heads/master@{#329674}

[bengr, 2015-02-25 23:26:36]

Patchset #2 (id:20001) has been deleted

[bengr, 2015-02-25 23:26:41]

Patchset #1 (id:1) has been deleted

[bengr, 2015-03-10 00:27:59]

Patchset #1 (id:40001) has been deleted

[bengr, 2015-03-10 00:32:15]

Patchset #1 (id:60001) has been deleted

[bengr, 2015-03-10 00:33:19]

bengr@chromium.org changed reviewers:
+ mef@chromium.org, mmenke@chromium.org

[bengr, 2015-03-10 00:33:19]

I'm looking for general comments to start, and suggestions for tests.

[mmenke, 2015-03-10 15:26:55]

Preliminary comments.  Want to think about testing, and simplifying the hooks a
bit.

https://codereview.chromium.org/937513003/diff/80001/components/cronet/androi...
File components/cronet/android/cronet_url_request_context_adapter.cc (right):

https://codereview.chromium.org/937513003/diff/80001/components/cronet/androi...
components/cronet/android/cronet_url_request_context_adapter.cc:164: prefs_ =
pref_service_factory.Create(pref_registry).Pass();
This doesn't persist anything to disk?

https://codereview.chromium.org/937513003/diff/80001/components/cronet/androi...
components/cronet/android/cronet_url_request_context_adapter.cc:166:
ui_task_runner_ = base::MessageLoop::current()->message_loop_proxy();
Why do we need to do stuff on the main thread?  Can we do everything on the
network thread, if we aren't saving stuff to a file?

Think it makes sense to expose data saved per session to the embedder, longer
term, but not sure we should be persisting it ourselves (Though we *are* going
to be using a prefs object to persist SDCH info, and maybe some other stuff, so
we may want to consider hooking that up to the DRP as well).

https://codereview.chromium.org/937513003/diff/80001/components/cronet/androi...
components/cronet/android/cronet_url_request_context_adapter.cc:172:
data_reduction_proxy::Client::CRONET_ANDROID,
Should we really use one client name for anything that embeds Cronet?

https://codereview.chromium.org/937513003/diff/80001/components/cronet/androi...
components/cronet/android/cronet_url_request_context_adapter.cc:180: new
data_reduction_proxy::DataReductionProxySettings());
I'd like to have some way for external embedders to be able to exclude this
dependency.  Haven't thought about how to do it.

https://codereview.chromium.org/937513003/diff/80001/net/url_request/url_requ...
File net/url_request/url_request_context_builder.cc (right):

https://codereview.chromium.org/937513003/diff/80001/net/url_request/url_requ...
net/url_request/url_request_context_builder.cc:415:
make_scoped_ptr(job_factory).Pass(), url_request_interceptor_.Pass()));
I don't think Pass() isn't needed with make_scoped_ptr, is it?

https://codereview.chromium.org/937513003/diff/80001/net/url_request/url_requ...
File net/url_request/url_request_context_builder.h (right):

https://codereview.chromium.org/937513003/diff/80001/net/url_request/url_requ...
net/url_request/url_request_context_builder.h:32: #include
"net/url_request/url_request_interceptor.h"
Suggest forward declaring this.  May have to de-inline and rename
set_interceptor to so.

https://codereview.chromium.org/937513003/diff/80001/net/url_request/url_requ...
net/url_request/url_request_context_builder.h:181: url_request_interceptor_ =
interceptor.Pass();
We should probably allow an ordered list of interceptors to be set, like we do
in ProfileIOData.  A bit unfortunate that we need to expose quite that level of
access, but I think it's an inevitable change.

[mmenke, 2015-03-10 15:33:16]

https://codereview.chromium.org/937513003/diff/80001/components/cronet/androi...
File components/cronet/android/cronet_pref_store.h (right):

https://codereview.chromium.org/937513003/diff/80001/components/cronet/androi...
components/cronet/android/cronet_pref_store.h:20: class CronetPrefStore : public
PersistentPrefStore {
Can't we just use ValueMapPrefStore?

[bengr, 2015-03-10 23:43:01]

Patchset #2 (id:100001) has been deleted

[bengr, 2015-03-10 23:43:31]

https://codereview.chromium.org/937513003/diff/80001/components/cronet/androi...
File components/cronet/android/cronet_pref_store.h (right):

https://codereview.chromium.org/937513003/diff/80001/components/cronet/androi...
components/cronet/android/cronet_pref_store.h:20: class CronetPrefStore : public
PersistentPrefStore {
On 2015/03/10 15:33:16, mmenke wrote:
> Can't we just use ValueMapPrefStore?

No, because of this:
https://code.google.com/p/chromium/codesearch#chromium/src/base/prefs/pref_se...

https://codereview.chromium.org/937513003/diff/80001/components/cronet/androi...
File components/cronet/android/cronet_url_request_context_adapter.cc (right):

https://codereview.chromium.org/937513003/diff/80001/components/cronet/androi...
components/cronet/android/cronet_url_request_context_adapter.cc:164: prefs_ =
pref_service_factory.Create(pref_registry).Pass();
On 2015/03/10 15:26:55, mmenke wrote:
> This doesn't persist anything to disk?

Right.

https://codereview.chromium.org/937513003/diff/80001/components/cronet/androi...
components/cronet/android/cronet_url_request_context_adapter.cc:166:
ui_task_runner_ = base::MessageLoop::current()->message_loop_proxy();
On 2015/03/10 15:26:55, mmenke wrote:
> Why do we need to do stuff on the main thread?  Can we do everything on the
> network thread, if we aren't saving stuff to a file?
> 
> Think it makes sense to expose data saved per session to the embedder, longer
> term, but not sure we should be persisting it ourselves (Though we *are* going
> to be using a prefs object to persist SDCH info, and maybe some other stuff,
so
> we may want to consider hooking that up to the DRP as well).

Done.

https://codereview.chromium.org/937513003/diff/80001/components/cronet/androi...
components/cronet/android/cronet_url_request_context_adapter.cc:172:
data_reduction_proxy::Client::CRONET_ANDROID,
On 2015/03/10 15:26:55, mmenke wrote:
> Should we really use one client name for anything that embeds Cronet?

For now. Separately, we're working on a a client mapping protocol that will take
an API key and map it to a session ID.

https://codereview.chromium.org/937513003/diff/80001/components/cronet/androi...
components/cronet/android/cronet_url_request_context_adapter.cc:180: new
data_reduction_proxy::DataReductionProxySettings());
On 2015/03/10 15:26:55, mmenke wrote:
> I'd like to have some way for external embedders to be able to exclude this
> dependency.  Haven't thought about how to do it.

I added a quick and dirty approach.

https://codereview.chromium.org/937513003/diff/80001/net/url_request/url_requ...
File net/url_request/url_request_context_builder.cc (right):

https://codereview.chromium.org/937513003/diff/80001/net/url_request/url_requ...
net/url_request/url_request_context_builder.cc:415:
make_scoped_ptr(job_factory).Pass(), url_request_interceptor_.Pass()));
On 2015/03/10 15:26:55, mmenke wrote:
> I don't think Pass() isn't needed with make_scoped_ptr, is it?

Done.

https://codereview.chromium.org/937513003/diff/80001/net/url_request/url_requ...
File net/url_request/url_request_context_builder.h (right):

https://codereview.chromium.org/937513003/diff/80001/net/url_request/url_requ...
net/url_request/url_request_context_builder.h:32: #include
"net/url_request/url_request_interceptor.h"
On 2015/03/10 15:26:55, mmenke wrote:
> Suggest forward declaring this.  May have to de-inline and rename
> set_interceptor to so.

Done.

https://codereview.chromium.org/937513003/diff/80001/net/url_request/url_requ...
net/url_request/url_request_context_builder.h:181: url_request_interceptor_ =
interceptor.Pass();
On 2015/03/10 15:26:55, mmenke wrote:
> We should probably allow an ordered list of interceptors to be set, like we do
> in ProfileIOData.  A bit unfortunate that we need to expose quite that level
of
> access, but I think it's an inevitable change.

Done.

[mmenke, 2015-03-12 21:28:09]

Sorry, been forgetting to get back to this.  I'll get back to it tomorrow.

[mmenke, 2015-03-13 18:16:48]

https://codereview.chromium.org/937513003/diff/140001/components/cronet/andro...
File components/cronet/android/cronet_data_reduction_proxy.cc (right):

https://codereview.chromium.org/937513003/diff/140001/components/cronet/andro...
components/cronet/android/cronet_data_reduction_proxy.cc:82: base::TimeDelta
commit_delay = base::TimeDelta();
explicit initialization not needed.  Suggest just inlining base::TimeDelta() and
getting rid of the variable.

https://codereview.chromium.org/937513003/diff/140001/components/cronet/andro...
components/cronet/android/cronet_data_reduction_proxy.cc:84: statistics_prefs =
make_scoped_ptr(
"make_scoped_ptr(" not needed.  Can just use

scoped_ptr<...> statistics_pref(new ...);

https://codereview.chromium.org/937513003/diff/140001/components/cronet/andro...
File components/cronet/android/cronet_data_reduction_proxy.h (right):

https://codereview.chromium.org/937513003/diff/140001/components/cronet/andro...
components/cronet/android/cronet_data_reduction_proxy.h:35: // Wrapper and
configurator of Data Reduction Proxy objects for cronet.
nit:  Cronet

https://codereview.chromium.org/937513003/diff/140001/components/cronet/andro...
components/cronet/android/cronet_data_reduction_proxy.h:36: class
CronetDataReductionProxy {
Do we need the proxy delegate?

https://codereview.chromium.org/937513003/diff/140001/components/cronet/andro...
components/cronet/android/cronet_data_reduction_proxy.h:42:
scoped_refptr<base::SingleThreadTaskRunner> task_runner,
Should make it clear what the requirements for the task_runner are.

https://codereview.chromium.org/937513003/diff/140001/components/cronet/andro...
components/cronet/android/cronet_data_reduction_proxy.h:45: virtual
~CronetDataReductionProxy();
Virtual not needed.

https://codereview.chromium.org/937513003/diff/140001/components/cronet/andro...
components/cronet/android/cronet_data_reduction_proxy.h:49:
net::NetworkDelegate* CreateNetworkDelegate(
Should return a scoped_ptr<NetworkDelegate>

https://codereview.chromium.org/937513003/diff/140001/components/cronet/andro...
components/cronet/android/cronet_data_reduction_proxy.h:62: static
scoped_ptr<PrefService> CreatePrefService();
This isn't needed in the header - can just be in an anonymous namespace in the
CC file, since it doesn't need access to any of this class's private members.

https://codereview.chromium.org/937513003/diff/140001/components/cronet/andro...
components/cronet/android/cronet_data_reduction_proxy.h:66:
scoped_refptr<net::URLRequestContextGetter> url_request_context_getter_;
I don't think we need to hold on to this.

https://codereview.chromium.org/937513003/diff/140001/components/cronet/andro...
File components/cronet/android/cronet_pref_store.h (right):

https://codereview.chromium.org/937513003/diff/140001/components/cronet/andro...
components/cronet/android/cronet_pref_store.h:20: class CronetPrefStore : public
PersistentPrefStore {
Given that this class is identical to the AwPrefStore, we should probably just
move this into base/prefs, or some other shared location.  Fine with that either
as a followup or a preliminary CL.

https://codereview.chromium.org/937513003/diff/140001/components/cronet/andro...
File components/cronet/android/cronet_url_request_context_adapter.cc (left):

https://codereview.chromium.org/937513003/diff/140001/components/cronet/andro...
components/cronet/android/cronet_url_request_context_adapter.cc:136:
GetNetworkTaskRunner(), nullptr));
Think a CL to fix this has already landed, make sure you've merged (Shouldn't
affect your CL at all, just one fewer steps once you get a signoff)

https://codereview.chromium.org/937513003/diff/140001/components/cronet/andro...
File components/cronet/android/cronet_url_request_context_adapter.cc (right):

https://codereview.chromium.org/937513003/diff/140001/components/cronet/andro...
components/cronet/android/cronet_url_request_context_adapter.cc:138:
base::CommandLine::Init(0, nullptr);
Hrm...Wonder if we should move command line checking out of the component (I may
have been the one that suggested moving it out, though, come to think of it). 
Anyhow, I think this is fine for now, but this call should probably be in
cronet_library_loader.cc.

https://codereview.chromium.org/937513003/diff/140001/components/cronet/andro...
components/cronet/android/cronet_url_request_context_adapter.cc:159: }
Can we just merge this with the "if (DRP)" block below?

https://codereview.chromium.org/937513003/diff/140001/components/cronet/andro...
components/cronet/android/cronet_url_request_context_adapter.cc:241:
GetURLRequestContext());
nit:  Use braces.

https://codereview.chromium.org/937513003/diff/140001/net/url_request/url_req...
File net/url_request/url_request_context_builder.cc (right):

https://codereview.chromium.org/937513003/diff/140001/net/url_request/url_req...
net/url_request/url_request_context_builder.cc:422:
make_scoped_ptr(job_factory);
make_scoped_ptr not needed.

https://codereview.chromium.org/937513003/diff/140001/net/url_request/url_req...
net/url_request/url_request_context_builder.cc:433: }
optional:  Think this would be a little simpler with top_job_factory defined
outside the if block, and no else clause.

https://codereview.chromium.org/937513003/diff/140001/net/url_request/url_req...
File net/url_request/url_request_context_builder.h (right):

https://codereview.chromium.org/937513003/diff/140001/net/url_request/url_req...
net/url_request/url_request_context_builder.h:47: typedef
ScopedVector<URLRequestInterceptor> URLRequestInterceptors;
Optional:  Suggest just inlining ScopedVector<URLRequestInterceptor> everywhere
- think it's clearer.  (Chrome uses the typedef because the interceptors make
their way through 47 gazillion layers as they're passed from content to chrome).

[bengr, 2015-03-19 01:03:39]

Patchset #4 (id:160001) has been deleted

[bengr, 2015-03-19 01:03:51]

https://codereview.chromium.org/937513003/diff/140001/components/cronet/andro...
File components/cronet/android/cronet_data_reduction_proxy.cc (right):

https://codereview.chromium.org/937513003/diff/140001/components/cronet/andro...
components/cronet/android/cronet_data_reduction_proxy.cc:82: base::TimeDelta
commit_delay = base::TimeDelta();
On 2015/03/13 18:16:47, mmenke (sick) wrote:
> explicit initialization not needed.  Suggest just inlining base::TimeDelta()
and
> getting rid of the variable.

Done.

https://codereview.chromium.org/937513003/diff/140001/components/cronet/andro...
components/cronet/android/cronet_data_reduction_proxy.cc:84: statistics_prefs =
make_scoped_ptr(
On 2015/03/13 18:16:47, mmenke (sick) wrote:
> "make_scoped_ptr(" not needed.  Can just use
> 
> scoped_ptr<...> statistics_pref(new ...);

I thought make_scoped pointer was just shorthand for your suggestion because of
this comment:
https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/scoped...

Am I missing something?

https://codereview.chromium.org/937513003/diff/140001/components/cronet/andro...
File components/cronet/android/cronet_data_reduction_proxy.h (right):

https://codereview.chromium.org/937513003/diff/140001/components/cronet/andro...
components/cronet/android/cronet_data_reduction_proxy.h:35: // Wrapper and
configurator of Data Reduction Proxy objects for cronet.
On 2015/03/13 18:16:47, mmenke (sick) wrote:
> nit:  Cronet

Done.

https://codereview.chromium.org/937513003/diff/140001/components/cronet/andro...
components/cronet/android/cronet_data_reduction_proxy.h:36: class
CronetDataReductionProxy {
On 2015/03/13 18:16:47, mmenke (sick) wrote:
> Do we need the proxy delegate?

No. That was used only for DRP CONNECT support. There's a goal of eventually
moving more to that delegate but not yet.

https://codereview.chromium.org/937513003/diff/140001/components/cronet/andro...
components/cronet/android/cronet_data_reduction_proxy.h:42:
scoped_refptr<base::SingleThreadTaskRunner> task_runner,
On 2015/03/13 18:16:47, mmenke (sick) wrote:
> Should make it clear what the requirements for the task_runner are.

Done.

https://codereview.chromium.org/937513003/diff/140001/components/cronet/andro...
components/cronet/android/cronet_data_reduction_proxy.h:45: virtual
~CronetDataReductionProxy();
On 2015/03/13 18:16:47, mmenke (sick) wrote:
> Virtual not needed.

Done.

https://codereview.chromium.org/937513003/diff/140001/components/cronet/andro...
components/cronet/android/cronet_data_reduction_proxy.h:49:
net::NetworkDelegate* CreateNetworkDelegate(
On 2015/03/13 18:16:47, mmenke (sick) wrote:
> Should return a scoped_ptr<NetworkDelegate>

Done.

https://codereview.chromium.org/937513003/diff/140001/components/cronet/andro...
components/cronet/android/cronet_data_reduction_proxy.h:62: static
scoped_ptr<PrefService> CreatePrefService();
On 2015/03/13 18:16:47, mmenke (sick) wrote:
> This isn't needed in the header - can just be in an anonymous namespace in the
> CC file, since it doesn't need access to any of this class's private members.

Done.

https://codereview.chromium.org/937513003/diff/140001/components/cronet/andro...
components/cronet/android/cronet_data_reduction_proxy.h:66:
scoped_refptr<net::URLRequestContextGetter> url_request_context_getter_;
On 2015/03/13 18:16:47, mmenke (sick) wrote:
> I don't think we need to hold on to this.

Someone does, and currently DRPService takes a raw pointer.

https://codereview.chromium.org/937513003/diff/140001/components/cronet/andro...
File components/cronet/android/cronet_pref_store.h (right):

https://codereview.chromium.org/937513003/diff/140001/components/cronet/andro...
components/cronet/android/cronet_pref_store.h:20: class CronetPrefStore : public
PersistentPrefStore {
On 2015/03/13 18:16:48, mmenke (sick) wrote:
> Given that this class is identical to the AwPrefStore, we should probably just
> move this into base/prefs, or some other shared location.  Fine with that
either
> as a followup or a preliminary CL.

Good suggestion. Let's do this as a followup.

https://codereview.chromium.org/937513003/diff/140001/components/cronet/andro...
File components/cronet/android/cronet_url_request_context_adapter.cc (left):

https://codereview.chromium.org/937513003/diff/140001/components/cronet/andro...
components/cronet/android/cronet_url_request_context_adapter.cc:136:
GetNetworkTaskRunner(), nullptr));
On 2015/03/13 18:16:48, mmenke (sick) wrote:
> Think a CL to fix this has already landed, make sure you've merged (Shouldn't
> affect your CL at all, just one fewer steps once you get a signoff)

Acknowledged, though I didn't see a fix yet.

https://codereview.chromium.org/937513003/diff/140001/components/cronet/andro...
File components/cronet/android/cronet_url_request_context_adapter.cc (right):

https://codereview.chromium.org/937513003/diff/140001/components/cronet/andro...
components/cronet/android/cronet_url_request_context_adapter.cc:138:
base::CommandLine::Init(0, nullptr);
On 2015/03/13 18:16:48, mmenke (sick) wrote:
> Hrm...Wonder if we should move command line checking out of the component (I
may
> have been the one that suggested moving it out, though, come to think of it). 
> Anyhow, I think this is fine for now, but this call should probably be in
> cronet_library_loader.cc.

Done.

https://codereview.chromium.org/937513003/diff/140001/components/cronet/andro...
components/cronet/android/cronet_url_request_context_adapter.cc:159: }
On 2015/03/13 18:16:48, mmenke (sick) wrote:
> Can we just merge this with the "if (DRP)" block below?

Done.

https://codereview.chromium.org/937513003/diff/140001/components/cronet/andro...
components/cronet/android/cronet_url_request_context_adapter.cc:241:
GetURLRequestContext());
On 2015/03/13 18:16:48, mmenke (sick) wrote:
> nit:  Use braces.

Done.

https://codereview.chromium.org/937513003/diff/140001/net/url_request/url_req...
File net/url_request/url_request_context_builder.cc (right):

https://codereview.chromium.org/937513003/diff/140001/net/url_request/url_req...
net/url_request/url_request_context_builder.cc:422:
make_scoped_ptr(job_factory);
On 2015/03/13 18:16:48, mmenke (sick) wrote:
> make_scoped_ptr not needed.

Done.

https://codereview.chromium.org/937513003/diff/140001/net/url_request/url_req...
net/url_request/url_request_context_builder.cc:433: }
On 2015/03/13 18:16:48, mmenke (sick) wrote:
> optional:  Think this would be a little simpler with top_job_factory defined
> outside the if block, and no else clause.

Done.

https://codereview.chromium.org/937513003/diff/140001/net/url_request/url_req...
File net/url_request/url_request_context_builder.h (right):

https://codereview.chromium.org/937513003/diff/140001/net/url_request/url_req...
net/url_request/url_request_context_builder.h:47: typedef
ScopedVector<URLRequestInterceptor> URLRequestInterceptors;
On 2015/03/13 18:16:48, mmenke (sick) wrote:
> Optional:  Suggest just inlining ScopedVector<URLRequestInterceptor>
everywhere
> - think it's clearer.  (Chrome uses the typedef because the interceptors make
> their way through 47 gazillion layers as they're passed from content to
chrome).

Done.

[mmenke, 2015-03-19 02:50:18]

Quick responses.  I've taken the last 3 days as sick days, so have a review
backlog, and have a couple perf writeups to do.  I expect I'll actually be
working (from home) tomorrow, but expect me to take a while to get back to this.

https://codereview.chromium.org/937513003/diff/140001/components/cronet/andro...
File components/cronet/android/cronet_data_reduction_proxy.cc (right):

https://codereview.chromium.org/937513003/diff/140001/components/cronet/andro...
components/cronet/android/cronet_data_reduction_proxy.cc:84: statistics_prefs =
make_scoped_ptr(
On 2015/03/19 01:03:50, bengr wrote:
> On 2015/03/13 18:16:47, mmenke (sick) wrote:
> > "make_scoped_ptr(" not needed.  Can just use
> > 
> > scoped_ptr<...> statistics_pref(new ...);
> 
> I thought make_scoped pointer was just shorthand for your suggestion because
of
> this comment:
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/scoped...
> 
> Am I missing something?

You're missing that the "shorthand" is longer in this case.

scoped_ptr<Blah> blah(new Blah)
is shorter than:
scoped_ptr<Blah> blah = make_scoped_ptr(new Blah)

it's useful when passing in a scoped_ptr to a method:
Foo(make_scoped_ptr(new Blah))
is nicer than:
Foo(scoped_ptr(new Blah).Pass())

At least I believe that's what it's for (The pass may no longer be needed in the
second line, but I think it used to be?)

https://codereview.chromium.org/937513003/diff/140001/components/cronet/andro...
File components/cronet/android/cronet_data_reduction_proxy.h (right):

https://codereview.chromium.org/937513003/diff/140001/components/cronet/andro...
components/cronet/android/cronet_data_reduction_proxy.h:66:
scoped_refptr<net::URLRequestContextGetter> url_request_context_getter_;
On 2015/03/19 01:03:51, bengr wrote:
> On 2015/03/13 18:16:47, mmenke (sick) wrote:
> > I don't think we need to hold on to this.
> 
> Someone does, and currently DRPService takes a raw pointer.

That should be fixed.  Happy to review a tiny CL that does that.

[bengr, 2015-03-19 17:56:24]

https://codereview.chromium.org/937513003/diff/140001/components/cronet/andro...
File components/cronet/android/cronet_data_reduction_proxy.cc (right):

https://codereview.chromium.org/937513003/diff/140001/components/cronet/andro...
components/cronet/android/cronet_data_reduction_proxy.cc:84: statistics_prefs =
make_scoped_ptr(
On 2015/03/19 02:50:17, mmenke (sick) wrote:
> On 2015/03/19 01:03:50, bengr wrote:
> > On 2015/03/13 18:16:47, mmenke (sick) wrote:
> > > "make_scoped_ptr(" not needed.  Can just use
> > > 
> > > scoped_ptr<...> statistics_pref(new ...);
> > 
> > I thought make_scoped pointer was just shorthand for your suggestion because
> of
> > this comment:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/scoped...
> > 
> > Am I missing something?
> 
> You're missing that the "shorthand" is longer in this case.
> 
> scoped_ptr<Blah> blah(new Blah)
> is shorter than:
> scoped_ptr<Blah> blah = make_scoped_ptr(new Blah)
> 
> it's useful when passing in a scoped_ptr to a method:
> Foo(make_scoped_ptr(new Blah))
> is nicer than:
> Foo(scoped_ptr(new Blah).Pass())
> 
> At least I believe that's what it's for (The pass may no longer be needed in
the
> second line, but I think it used to be?)

Oh. I misread my own code. Yes, of course. I completely agree. Done.

https://codereview.chromium.org/937513003/diff/140001/components/cronet/andro...
File components/cronet/android/cronet_data_reduction_proxy.h (right):

https://codereview.chromium.org/937513003/diff/140001/components/cronet/andro...
components/cronet/android/cronet_data_reduction_proxy.h:66:
scoped_refptr<net::URLRequestContextGetter> url_request_context_getter_;
On 2015/03/19 02:50:18, mmenke (sick) wrote:
> On 2015/03/19 01:03:51, bengr wrote:
> > On 2015/03/13 18:16:47, mmenke (sick) wrote:
> > > I don't think we need to hold on to this.
> > 
> > Someone does, and currently DRPService takes a raw pointer.
> 
> That should be fixed.  Happy to review a tiny CL that does that.

This can't be fixed easily afaik. In profile_impl_io_data.cc,
InitDataReductionProxySettings gets a raw pointer to the request context getter
from the profile. See:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pro...

[mef, 2015-03-20 18:01:22]

mef@chromium.org changed reviewers:
+ xunjieli@chromium.org

[xunjieli, 2015-03-20 18:39:50]

https://codereview.chromium.org/937513003/diff/200001/components/cronet/andro...
File components/cronet/android/cronet_pref_store.h (right):

https://codereview.chromium.org/937513003/diff/200001/components/cronet/andro...
components/cronet/android/cronet_pref_store.h:19: // PrefService, which in turn
is needed by the Autofill component.
I am not sure "Autofill" component is applicable in the context of Cronet. Maybe
removing the part from the comment?

https://codereview.chromium.org/937513003/diff/200001/components/cronet/andro...
components/cronet/android/cronet_pref_store.h:21: class CronetPrefStore : public
PersistentPrefStore {
I am working on persisting HttpServerProperties on disk through the use of
HttpServerPropertiesManager and PrefStore. I heard that SDCH will be using
PrefStore as well. 

Maybe rename this class to something like CronetInMemoryPrefStore? since there
Cronet will be using other kinds of pref store.

Just curious, I don't know much about data reduction proxy, will it ever need
persisting info on disk? or this memory backed pref store is enough?

[bengr, 2015-03-23 23:53:46]

mmenke/mef: PTAL
xunjieli, I'll take a look at your comments soon.

[mmenke, 2015-04-01 15:46:14]

I'll get to tests later today as well.

https://codereview.chromium.org/937513003/diff/220001/components/cronet/andro...
File components/cronet/android/cronet_data_reduction_proxy.cc (right):

https://codereview.chromium.org/937513003/diff/220001/components/cronet/andro...
components/cronet/android/cronet_data_reduction_proxy.cc:113:
url_request_context_getter_.get()));
Before landing this, could you land a small CL that makes
DataReductionProxyService take the getter as a scoped_refptr, and keep it around
as a scoped_refptr as well?  The fact that it doesn't is really a bug.

https://codereview.chromium.org/937513003/diff/220001/components/cronet/andro...
File components/cronet/android/cronet_pref_store.cc (right):

https://codereview.chromium.org/937513003/diff/220001/components/cronet/andro...
components/cronet/android/cronet_pref_store.cc:15: const base::Value** value)
const {
fix indent

https://codereview.chromium.org/937513003/diff/220001/components/cronet/andro...
components/cronet/android/cronet_pref_store.cc:20: base::Value** value) {
fix indent

https://codereview.chromium.org/937513003/diff/220001/components/cronet/andro...
components/cronet/android/cronet_pref_store.cc:43: ReportValueChanged(key);
fix indent

https://codereview.chromium.org/937513003/diff/220001/components/cronet/andro...
File components/cronet/android/cronet_pref_store.h (right):

https://codereview.chromium.org/937513003/diff/220001/components/cronet/andro...
components/cronet/android/cronet_pref_store.h:19: // PrefService, which in turn
is needed by the Autofill component.
As Helen said, the second sentence really makes no sense here.

https://codereview.chromium.org/937513003/diff/220001/components/cronet/andro...
components/cronet/android/cronet_pref_store.h:21: class CronetPrefStore : public
PersistentPrefStore {
We're actually going to be needing to use an on-disk pref store for SDCH in
Cronet, so this name is a bit misleading.  "CronetInMemoryPrefStore"?

https://codereview.chromium.org/937513003/diff/220001/components/cronet/andro...
components/cronet/android/cronet_pref_store.h:21: class CronetPrefStore : public
PersistentPrefStore {
Ok, so now I'm following why you needs a PersistentPrefStore - the main user
prefs use PrefService instead of PrefStore.

In Chrome, the network stack is using a PrefStore that lives on the IO thread to
store some network information, and I expect we'll add stuff to it over time. 
Wonder if it's worth considering using that for the DRP - may make life easier
if you can move more stuff over to the IO thread.  One down side is we load the
network prefs asynchronously, so we don't block startup on reading them.

Anyhow, beyond the scoped of this CL, but may be worth considering.

Alternatively, could just make the PrefService and StatisticsPrefs optional.  I
don't think we need either of them for Cronet, and they aren't really needed for
the DRP.

https://codereview.chromium.org/937513003/diff/220001/components/cronet/andro...
components/cronet/android/cronet_pref_store.h:35: base::Value** result)
override;
Can this all fit on a single line?  Looks like it did for the class you copied
from.

https://codereview.chromium.org/937513003/diff/220001/components/cronet/andro...
components/cronet/android/cronet_pref_store.h:39: base::Value* value) override;
nit:  fix indent, or can this all fir on a single line?

https://codereview.chromium.org/937513003/diff/220001/components/cronet/andro...
components/cronet/android/cronet_pref_store.h:48: ~CronetPrefStore() override;
Suggest making this private - no one should be inheriting from this.

https://codereview.chromium.org/937513003/diff/220001/components/cronet/andro...
File components/cronet/android/cronet_url_request_context_adapter.cc (right):

https://codereview.chromium.org/937513003/diff/220001/components/cronet/andro...
components/cronet/android/cronet_url_request_context_adapter.cc:174:
net::ProxyService::CreateWithoutProxyResolver(
Why aren't you using CreateUsingSystemProxyResolver, like the builder does?

https://codereview.chromium.org/937513003/diff/220001/components/cronet/andro...
components/cronet/android/cronet_url_request_context_adapter.cc:175:
net::ProxyService::CreateSystemProxyConfigService(
It's my understanding that the Java proxy config service needs to be created on
the main thread, to avoid any races.  All calls from the system to this service
are made on the main thread, and we aren't ready to receive them until we're
done creating the service.

https://codereview.chromium.org/937513003/diff/220001/components/cronet/andro...
File
components/cronet/android/java/src/org/chromium/net/UrlRequestContextConfig.java
(right):

https://codereview.chromium.org/937513003/diff/220001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/UrlRequestContextConfig.java:105:
* Boolean, enable Data Reduction Proxy if true.
Need to describe what "key" is, and maybe rename it (api_key?)

https://codereview.chromium.org/937513003/diff/220001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/UrlRequestContextConfig.java:233:
* proxy name, and a secure proxy check url. E.g:
Secure proxy check?  Should also comment if they're all mandatory.  Also suggest
taking three separate strings.

[xunjieli, 2015-04-01 15:51:29]

xunjieli@chromium.org changed reviewers:
- xunjieli@chromium.org

[mmenke, 2015-04-01 16:48:53]

https://codereview.chromium.org/937513003/diff/220001/components/cronet/andro...
File
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java
(right):

https://codereview.chromium.org/937513003/diff/220001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:137:
String serverHostPort = "127.0.0.1:" + NativeTestServer.getPort();
Can we just use getHostPortPair instead of getPort?  Seems weird to grab just
the port of a URL with a host, and then re-attach the port to it later.

https://codereview.chromium.org/937513003/diff/220001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:140:
mActivity.mUrlRequestContext.shutdown();
Why can't we just start the test server first, and enable the DRP the first
time?

https://codereview.chromium.org/937513003/diff/220001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:159:
assertEquals(serverHostPort, listener.mResponseInfo.getProxyServer());
Check the URL as well?  (http://google.com/datareductionproxysuccess.txt).

https://codereview.chromium.org/937513003/diff/220001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:159:
assertEquals(serverHostPort, listener.mResponseInfo.getProxyServer());
Should we check that the other parameters work as well?  Also, do we use the DRP
before we've gotten a response from the check URL?  Just wondering if there's a
chance we'll racily use a direct connection here while waiting for the probe
result.

https://codereview.chromium.org/937513003/diff/220001/components/data_reducti...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc
(right):

https://codereview.chromium.org/937513003/diff/220001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:91:
}
Why do we need to do this stuff synchronously, here and in the other file?  My
main concern is that the more different ways we do things, the more likely one
way of doing them is to have bugs.

[bengr, 2015-04-24 02:30:43]

https://codereview.chromium.org/937513003/diff/200001/components/cronet/andro...
File components/cronet/android/cronet_pref_store.h (right):

https://codereview.chromium.org/937513003/diff/200001/components/cronet/andro...
components/cronet/android/cronet_pref_store.h:19: // PrefService, which in turn
is needed by the Autofill component.
On 2015/03/20 18:39:49, xunjieli wrote:
> I am not sure "Autofill" component is applicable in the context of Cronet.
Maybe
> removing the part from the comment?

Done.

https://codereview.chromium.org/937513003/diff/200001/components/cronet/andro...
components/cronet/android/cronet_pref_store.h:21: class CronetPrefStore : public
PersistentPrefStore {
On 2015/03/20 18:39:49, xunjieli wrote:
> I am working on persisting HttpServerProperties on disk through the use of
> HttpServerPropertiesManager and PrefStore. I heard that SDCH will be using
> PrefStore as well. 
> 
> Maybe rename this class to something like CronetInMemoryPrefStore? since there
> Cronet will be using other kinds of pref store.
> 
> Just curious, I don't know much about data reduction proxy, will it ever need
> persisting info on disk? or this memory backed pref store is enough?

The pref store for the data reduction proxy holds some statistics, which Cronet
doesn't use yet. We're considering two models for using them, and the more
popular one would not require on disk persistence.

https://codereview.chromium.org/937513003/diff/220001/components/cronet/andro...
File components/cronet/android/cronet_data_reduction_proxy.cc (right):

https://codereview.chromium.org/937513003/diff/220001/components/cronet/andro...
components/cronet/android/cronet_data_reduction_proxy.cc:113:
url_request_context_getter_.get()));
On 2015/04/01 15:46:14, mmenke (Out 4-27 to 5-1) wrote:
> Before landing this, could you land a small CL that makes
> DataReductionProxyService take the getter as a scoped_refptr, and keep it
around
> as a scoped_refptr as well?  The fact that it doesn't is really a bug.

We can't because we get it as a raw pointer in Chrome. See:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pro...

https://codereview.chromium.org/937513003/diff/220001/components/cronet/andro...
File components/cronet/android/cronet_pref_store.cc (right):

https://codereview.chromium.org/937513003/diff/220001/components/cronet/andro...
components/cronet/android/cronet_pref_store.cc:15: const base::Value** value)
const {
On 2015/04/01 15:46:14, mmenke (Out 4-27 to 5-1) wrote:
> fix indent

Done.

https://codereview.chromium.org/937513003/diff/220001/components/cronet/andro...
components/cronet/android/cronet_pref_store.cc:20: base::Value** value) {
On 2015/04/01 15:46:14, mmenke (Out 4-27 to 5-1) wrote:
> fix indent

Done.

https://codereview.chromium.org/937513003/diff/220001/components/cronet/andro...
components/cronet/android/cronet_pref_store.cc:43: ReportValueChanged(key);
On 2015/04/01 15:46:14, mmenke (Out 4-27 to 5-1) wrote:
> fix indent

Done.

https://codereview.chromium.org/937513003/diff/220001/components/cronet/andro...
File components/cronet/android/cronet_pref_store.h (right):

https://codereview.chromium.org/937513003/diff/220001/components/cronet/andro...
components/cronet/android/cronet_pref_store.h:19: // PrefService, which in turn
is needed by the Autofill component.
On 2015/04/01 15:46:14, mmenke (Out 4-27 to 5-1) wrote:
> As Helen said, the second sentence really makes no sense here.

Done.

https://codereview.chromium.org/937513003/diff/220001/components/cronet/andro...
components/cronet/android/cronet_pref_store.h:21: class CronetPrefStore : public
PersistentPrefStore {
On 2015/04/01 15:46:14, mmenke (Out 4-27 to 5-1) wrote:
> Ok, so now I'm following why you needs a PersistentPrefStore - the main user
> prefs use PrefService instead of PrefStore.
> 
> In Chrome, the network stack is using a PrefStore that lives on the IO thread
to
> store some network information, and I expect we'll add stuff to it over time. 
> Wonder if it's worth considering using that for the DRP - may make life easier
> if you can move more stuff over to the IO thread.  One down side is we load
the
> network prefs asynchronously, so we don't block startup on reading them.
> 
> Anyhow, beyond the scoped of this CL, but may be worth considering.
> 
> Alternatively, could just make the PrefService and StatisticsPrefs optional. 
I
> don't think we need either of them for Cronet, and they aren't really needed
for
> the DRP.

Ack. Out of scope for this CL.

https://codereview.chromium.org/937513003/diff/220001/components/cronet/andro...
components/cronet/android/cronet_pref_store.h:21: class CronetPrefStore : public
PersistentPrefStore {
On 2015/04/01 15:46:14, mmenke (Out 4-27 to 5-1) wrote:
> We're actually going to be needing to use an on-disk pref store for SDCH in
> Cronet, so this name is a bit misleading.  "CronetInMemoryPrefStore"?

Done.

https://codereview.chromium.org/937513003/diff/220001/components/cronet/andro...
components/cronet/android/cronet_pref_store.h:35: base::Value** result)
override;
On 2015/04/01 15:46:14, mmenke (Out 4-27 to 5-1) wrote:
> Can this all fit on a single line?  Looks like it did for the class you copied
> from.

Done.

https://codereview.chromium.org/937513003/diff/220001/components/cronet/andro...
components/cronet/android/cronet_pref_store.h:39: base::Value* value) override;
On 2015/04/01 15:46:14, mmenke (Out 4-27 to 5-1) wrote:
> nit:  fix indent, or can this all fir on a single line?

Done.

https://codereview.chromium.org/937513003/diff/220001/components/cronet/andro...
components/cronet/android/cronet_pref_store.h:48: ~CronetPrefStore() override;
On 2015/04/01 15:46:14, mmenke (Out 4-27 to 5-1) wrote:
> Suggest making this private - no one should be inheriting from this.

Done.

https://codereview.chromium.org/937513003/diff/220001/components/cronet/andro...
File components/cronet/android/cronet_url_request_context_adapter.cc (right):

https://codereview.chromium.org/937513003/diff/220001/components/cronet/andro...
components/cronet/android/cronet_url_request_context_adapter.cc:174:
net::ProxyService::CreateWithoutProxyResolver(
On 2015/04/01 15:46:14, mmenke (Out 4-27 to 5-1) wrote:
> Why aren't you using CreateUsingSystemProxyResolver, like the builder does?

Done.

https://codereview.chromium.org/937513003/diff/220001/components/cronet/andro...
components/cronet/android/cronet_url_request_context_adapter.cc:175:
net::ProxyService::CreateSystemProxyConfigService(
On 2015/04/01 15:46:14, mmenke (Out 4-27 to 5-1) wrote:
> It's my understanding that the Java proxy config service needs to be created
on
> the main thread, to avoid any races.  All calls from the system to this
service
> are made on the main thread, and we aren't ready to receive them until we're
> done creating the service.

Done.

https://codereview.chromium.org/937513003/diff/220001/components/cronet/andro...
File
components/cronet/android/java/src/org/chromium/net/UrlRequestContextConfig.java
(right):

https://codereview.chromium.org/937513003/diff/220001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/UrlRequestContextConfig.java:105:
* Boolean, enable Data Reduction Proxy if true.
On 2015/04/01 15:46:14, mmenke (Out 4-27 to 5-1) wrote:
> Need to describe what "key" is, and maybe rename it (api_key?)

Done. The format is TBD.

https://codereview.chromium.org/937513003/diff/220001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/UrlRequestContextConfig.java:233:
* proxy name, and a secure proxy check url. E.g:
On 2015/04/01 15:46:14, mmenke (Out 4-27 to 5-1) wrote:
> Secure proxy check?  Should also comment if they're all mandatory.  Also
suggest
> taking three separate strings.

Done.

https://codereview.chromium.org/937513003/diff/220001/components/cronet/andro...
File
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java
(right):

https://codereview.chromium.org/937513003/diff/220001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:137:
String serverHostPort = "127.0.0.1:" + NativeTestServer.getPort();
On 2015/04/01 16:48:53, mmenke (Out 4-27 to 5-1) wrote:
> Can we just use getHostPortPair instead of getPort?  Seems weird to grab just
> the port of a URL with a host, and then re-attach the port to it later.

NativeTestServer doesn't hold onto the host.

https://codereview.chromium.org/937513003/diff/220001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:140:
mActivity.mUrlRequestContext.shutdown();
On 2015/04/01 16:48:53, mmenke (Out 4-27 to 5-1) wrote:
> Why can't we just start the test server first, and enable the DRP the first
> time?

This was a chicken and egg problem. The server needed a context, but we couldn't
configure the DRP until we got the port from the server.

https://codereview.chromium.org/937513003/diff/220001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:159:
assertEquals(serverHostPort, listener.mResponseInfo.getProxyServer());
On 2015/04/01 16:48:53, mmenke (Out 4-27 to 5-1) wrote:
> Check the URL as well?  (http://google.com/datareductionproxysuccess.txt).

Done.

https://codereview.chromium.org/937513003/diff/220001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:159:
assertEquals(serverHostPort, listener.mResponseInfo.getProxyServer());
On 2015/04/01 16:48:53, mmenke (Out 4-27 to 5-1) wrote:
> Should we check that the other parameters work as well?  Also, do we use the
DRP
> before we've gotten a response from the check URL?  Just wondering if there's
a
> chance we'll racily use a direct connection here while waiting for the probe
> result.

What other parameters? The race doesn't matter. The secure proxy check tells
cronet to use the primary proxy, and the primary proxy is used until cronet gets
the response from the check.

https://codereview.chromium.org/937513003/diff/220001/components/data_reducti...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc
(right):

https://codereview.chromium.org/937513003/diff/220001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:91:
}
On 2015/04/01 16:48:53, mmenke (Out 4-27 to 5-1) wrote:
> Why do we need to do this stuff synchronously, here and in the other file?  My
> main concern is that the more different ways we do things, the more likely one
> way of doing them is to have bugs.

Done.

[mmenke, 2015-04-24 15:54:59]

Quick responses.  Want to get a fix together for a cronet test crash, then I'll
get back to this in the afternoon.

https://codereview.chromium.org/937513003/diff/220001/components/cronet/andro...
File
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java
(right):

https://codereview.chromium.org/937513003/diff/220001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:137:
String serverHostPort = "127.0.0.1:" + NativeTestServer.getPort();
On 2015/04/24 02:30:43, bengr wrote:
> On 2015/04/01 16:48:53, mmenke (Out 4-27 to 5-1) wrote:
> > Can we just use getHostPortPair instead of getPort?  Seems weird to grab
just
> > the port of a URL with a host, and then re-attach the port to it later.
> 
> NativeTestServer doesn't hold onto the host.

It does.  That's how the other NativeTestServer methods pull full URLs from it. 

In the cc file, use HostPortPair::FromURL(g_test_server->base_url()).ToString();

https://codereview.chromium.org/937513003/diff/220001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:140:
mActivity.mUrlRequestContext.shutdown();
On 2015/04/24 02:30:43, bengr wrote:
> On 2015/04/01 16:48:53, mmenke (Out 4-27 to 5-1) wrote:
> > Why can't we just start the test server first, and enable the DRP the first
> > time?
> 
> This was a chicken and egg problem. The server needed a context, but we
couldn't
> configure the DRP until we got the port from the server.

See skipFactoryInitInOnCreate.

https://codereview.chromium.org/937513003/diff/220001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:159:
assertEquals(serverHostPort, listener.mResponseInfo.getProxyServer());
On 2015/04/24 02:30:43, bengr wrote:
> On 2015/04/01 16:48:53, mmenke (Out 4-27 to 5-1) wrote:
> > Should we check that the other parameters work as well?  Also, do we use the
> DRP
> > before we've gotten a response from the check URL?  Just wondering if
there's
> a
> > chance we'll racily use a direct connection here while waiting for the probe
> > result.
> 
> What other parameters? The race doesn't matter. The secure proxy check tells
> cronet to use the primary proxy, and the primary proxy is used until cronet
gets
> the response from the check.

Other parameters -> The second proxy and the proxy check URL.

Ahh, I didn't realize that default behavior was to just use the DRP.  Had
thought we didn't use it until after the probe.

[mmenke, 2015-04-24 19:10:28]

More comments (Still reviewing)

https://codereview.chromium.org/937513003/diff/240001/components/cronet/andro...
File components/cronet/android/cronet_data_reduction_proxy.cc (right):

https://codereview.chromium.org/937513003/diff/240001/components/cronet/andro...
components/cronet/android/cronet_data_reduction_proxy.cc:26: class NetLog;
This is also forward declared in the header, so not needed here.

https://codereview.chromium.org/937513003/diff/240001/components/cronet/andro...
components/cronet/android/cronet_data_reduction_proxy.cc:33: }  // namespace
This block can be removed, per later comment.

https://codereview.chromium.org/937513003/diff/240001/components/cronet/andro...
components/cronet/android/cronet_data_reduction_proxy.cc:36: namespace {
int break after start of namespace (Can include one or not for nested
namespaces, but should be one after the inner namespace)

https://codereview.chromium.org/937513003/diff/240001/components/cronet/andro...
components/cronet/android/cronet_data_reduction_proxy.cc:43:
pref_service_factory.set_read_error_callback(base::Bind(&HandleReadError));
This isn't needed.  PrefServiceFactory does this exact same thing in its
constructor.

https://codereview.chromium.org/937513003/diff/240001/components/cronet/andro...
components/cronet/android/cronet_data_reduction_proxy.cc:51:
DCHECK((primary_proxy.empty() && fallback_proxy.empty() &&
include base/logging.h for DCHECK

https://codereview.chromium.org/937513003/diff/240001/components/cronet/andro...
components/cronet/android/cronet_data_reduction_proxy.cc:52:
secure_proxy_check_url.empty()) ||
If these are all empty, can we just not create this class at all?  (And so get
rid of the first part of this DCHECK)

https://codereview.chromium.org/937513003/diff/240001/components/cronet/andro...
components/cronet/android/cronet_data_reduction_proxy.cc:66: }  // namespace
line break before end of namespace.

https://codereview.chromium.org/937513003/diff/240001/components/cronet/andro...
components/cronet/android/cronet_data_reduction_proxy.cc:78:
base::CommandLine::ForCurrentProcess());
Hrm...  This isn't threadsafe.  If we were creating two of these at once on
different threads, even with the same parameters, bad things could happen.

Also, if the DRP code relies on reading command line options as it needs access
to the parameters, we could also get into serious trouble.

https://codereview.chromium.org/937513003/diff/240001/components/cronet/andro...
File components/cronet/android/cronet_data_reduction_proxy.h (right):

https://codereview.chromium.org/937513003/diff/240001/components/cronet/andro...
components/cronet/android/cronet_data_reduction_proxy.h:35: // Wrapper and
configurator of Data Reduction Proxy objects for Cronet.
Think it's worth mentioning that it configures things so that both the UI and IO
classes are both run on the Cronet's network thread (And maybe also reference
that when setting up the UI classes).

https://codereview.chromium.org/937513003/diff/240001/components/cronet/andro...
File components/cronet/android/cronet_url_request_context_adapter.cc (right):

https://codereview.chromium.org/937513003/diff/240001/components/cronet/andro...
components/cronet/android/cronet_url_request_context_adapter.cc:160: if
(config->enable_data_reduction_proxy) {
Wasn't there a decision to make it so Cronet could be build without the DRP, out
of concerns about binary size (Not that I expect there to be a huge difference).

https://codereview.chromium.org/937513003/diff/240001/components/cronet/andro...
File
components/cronet/android/java/src/org/chromium/net/UrlRequestContextConfig.java
(right):

https://codereview.chromium.org/937513003/diff/240001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/UrlRequestContextConfig.java:108:
public UrlRequestContextConfig enableDataReductionProxy(boolean value,
Wonder if we really need a value - if you don't want it, don't enable it (The
ones above are a bit different, as default may change over time).

https://codereview.chromium.org/937513003/diff/240001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/UrlRequestContextConfig.java:232:
* Overrides Data Reduction Proxy configuration parameters with a primary
Overrides parameters implies we have defaults, but we don't, do we?

https://codereview.chromium.org/937513003/diff/240001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/UrlRequestContextConfig.java:238:
*/
Suggest moving this below the other DRP method (Know the quic methods aren't
next to each other, but seems like the file is easier to read if related methods
are adjacent.

https://codereview.chromium.org/937513003/diff/240001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/UrlRequestContextConfig.java:242:
String secureProxyCheckUrl) {
IF any of these are empty throw an IllegalArgumentException?  It's preferred
over just DCHECKing in the C++ code.

https://codereview.chromium.org/937513003/diff/240001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/UrlRequestContextConfig.java:250:
secureProxyCheckUrl);
Think this would be more legibly as just 3 independent putString calls, any a
return.

[mmenke, 2015-04-24 19:27:23]

https://codereview.chromium.org/937513003/diff/240001/components/cronet/andro...
File components/cronet/android/cronet_data_reduction_proxy.cc (right):

https://codereview.chromium.org/937513003/diff/240001/components/cronet/andro...
components/cronet/android/cronet_data_reduction_proxy.cc:38: PrefRegistrySimple*
pref_registry = new PrefRegistrySimple();
using a scoped_refptr here makes it clearer this isn't leaked (Admittedly,
pref_service_factory.Create takes it as a raw pointer, which is weird, but still
think it's an improvement)

https://codereview.chromium.org/937513003/diff/240001/components/cronet/andro...
components/cronet/android/cronet_data_reduction_proxy.cc:54:
!secure_proxy_check_url.empty()));
You're using 3 space indent here for both of the above lines, rather than 4.

https://codereview.chromium.org/937513003/diff/240001/components/cronet/andro...
File components/cronet/android/cronet_in_memory_pref_store.cc (right):

https://codereview.chromium.org/937513003/diff/240001/components/cronet/andro...
components/cronet/android/cronet_in_memory_pref_store.cc:8: #include
"base/values.h"
I don't think this is needed - we just base Value*s and pass them along.

https://codereview.chromium.org/937513003/diff/240001/components/cronet/andro...
File components/cronet/android/cronet_url_request_context_adapter.cc (right):

https://codereview.chromium.org/937513003/diff/240001/components/cronet/andro...
components/cronet/android/cronet_url_request_context_adapter.cc:178: }
optional:  Think this may be a little cleaner as:

scoped_ptr<NetworkDelegate> network_delegate(new BasicNetworkDelegate());
if (config->enable_data_reduction_proxy) {
  ...
  network_delegate =
      data_reduction_proxy_->CreateNetworkDelegate(network_delegate.release());
  ...
}

context_builder.set_network_delegate(network_delegate.release());

https://codereview.chromium.org/937513003/diff/240001/components/cronet/andro...
components/cronet/android/cronet_url_request_context_adapter.cc:242: if
(data_reduction_proxy_) {
This should probably go just before the "is_context_initialized_ = true;", since
it's really a part of context init.

https://codereview.chromium.org/937513003/diff/240001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.cc
(right):

https://codereview.chromium.org/937513003/diff/240001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.cc:142:
if (spdy_proxy_auth_enabled_.GetValue() != enabled) {
nit:  Extra blank line here doesn't really belong in this CL.

[bengr, 2015-04-24 21:13:51]

https://codereview.chromium.org/937513003/diff/220001/components/cronet/andro...
File
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java
(right):

https://codereview.chromium.org/937513003/diff/220001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:137:
String serverHostPort = "127.0.0.1:" + NativeTestServer.getPort();
On 2015/04/24 15:54:59, mmenke (Out 4-27 to 5-1) wrote:
> On 2015/04/24 02:30:43, bengr wrote:
> > On 2015/04/01 16:48:53, mmenke (Out 4-27 to 5-1) wrote:
> > > Can we just use getHostPortPair instead of getPort?  Seems weird to grab
> just
> > > the port of a URL with a host, and then re-attach the port to it later.
> > 
> > NativeTestServer doesn't hold onto the host.
> 
> It does.  That's how the other NativeTestServer methods pull full URLs from
it. 
> 
> In the cc file, use
HostPortPair::FromURL(g_test_server->base_url()).ToString();

Done.

https://codereview.chromium.org/937513003/diff/220001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:159:
assertEquals(serverHostPort, listener.mResponseInfo.getProxyServer());
On 2015/04/24 15:54:59, mmenke (Out 4-27 to 5-1) wrote:
> On 2015/04/24 02:30:43, bengr wrote:
> > On 2015/04/01 16:48:53, mmenke (Out 4-27 to 5-1) wrote:
> > > Should we check that the other parameters work as well?  Also, do we use
the
> > DRP
> > > before we've gotten a response from the check URL?  Just wondering if
> there's
> > a
> > > chance we'll racily use a direct connection here while waiting for the
probe
> > > result.
> > 
> > What other parameters? The race doesn't matter. The secure proxy check tells
> > cronet to use the primary proxy, and the primary proxy is used until cronet
> gets
> > the response from the check.
> 
> Other parameters -> The second proxy and the proxy check URL.
> 
> Ahh, I didn't realize that default behavior was to just use the DRP.  Had
> thought we didn't use it until after the probe.

These aren't exposed easily. setDataReductionProxyOptions is for test only to
override default settings. The test would fail if the overrides didn't work.

https://codereview.chromium.org/937513003/diff/240001/components/cronet/andro...
File components/cronet/android/cronet_data_reduction_proxy.cc (right):

https://codereview.chromium.org/937513003/diff/240001/components/cronet/andro...
components/cronet/android/cronet_data_reduction_proxy.cc:26: class NetLog;
On 2015/04/24 19:10:28, mmenke (Out 4-27 to 5-1) wrote:
> This is also forward declared in the header, so not needed here.

Done.

https://codereview.chromium.org/937513003/diff/240001/components/cronet/andro...
components/cronet/android/cronet_data_reduction_proxy.cc:33: }  // namespace
On 2015/04/24 19:10:27, mmenke (Out 4-27 to 5-1) wrote:
> This block can be removed, per later comment.

Done.

https://codereview.chromium.org/937513003/diff/240001/components/cronet/andro...
components/cronet/android/cronet_data_reduction_proxy.cc:36: namespace {
On 2015/04/24 19:10:27, mmenke (Out 4-27 to 5-1) wrote:
> int break after start of namespace (Can include one or not for nested
> namespaces, but should be one after the inner namespace)

Done.

https://codereview.chromium.org/937513003/diff/240001/components/cronet/andro...
components/cronet/android/cronet_data_reduction_proxy.cc:38: PrefRegistrySimple*
pref_registry = new PrefRegistrySimple();
On 2015/04/24 19:27:23, mmenke (Out 4-27 to 5-1) wrote:
> using a scoped_refptr here makes it clearer this isn't leaked (Admittedly,
> pref_service_factory.Create takes it as a raw pointer, which is weird, but
still
> think it's an improvement)

Done.

https://codereview.chromium.org/937513003/diff/240001/components/cronet/andro...
components/cronet/android/cronet_data_reduction_proxy.cc:43:
pref_service_factory.set_read_error_callback(base::Bind(&HandleReadError));
On 2015/04/24 19:10:28, mmenke (Out 4-27 to 5-1) wrote:
> This isn't needed.  PrefServiceFactory does this exact same thing in its
> constructor.

Done.

https://codereview.chromium.org/937513003/diff/240001/components/cronet/andro...
components/cronet/android/cronet_data_reduction_proxy.cc:51:
DCHECK((primary_proxy.empty() && fallback_proxy.empty() &&
On 2015/04/24 19:10:27, mmenke (Out 4-27 to 5-1) wrote:
> include base/logging.h for DCHECK

Done.

https://codereview.chromium.org/937513003/diff/240001/components/cronet/andro...
components/cronet/android/cronet_data_reduction_proxy.cc:52:
secure_proxy_check_url.empty()) ||
On 2015/04/24 19:10:27, mmenke (Out 4-27 to 5-1) wrote:
> If these are all empty, can we just not create this class at all?  (And so get
> rid of the first part of this DCHECK)

These override statically configured defaults, so it's fine (and the default
behavior) to leave these blank. These are only used for tests.

https://codereview.chromium.org/937513003/diff/240001/components/cronet/andro...
components/cronet/android/cronet_data_reduction_proxy.cc:54:
!secure_proxy_check_url.empty()));
On 2015/04/24 19:27:23, mmenke (Out 4-27 to 5-1) wrote:
> You're using 3 space indent here for both of the above lines, rather than 4.

Done.

https://codereview.chromium.org/937513003/diff/240001/components/cronet/andro...
components/cronet/android/cronet_data_reduction_proxy.cc:66: }  // namespace
On 2015/04/24 19:10:27, mmenke (Out 4-27 to 5-1) wrote:
> line break before end of namespace.

Done.

https://codereview.chromium.org/937513003/diff/240001/components/cronet/andro...
components/cronet/android/cronet_data_reduction_proxy.cc:78:
base::CommandLine::ForCurrentProcess());
On 2015/04/24 19:10:27, mmenke (Out 4-27 to 5-1) wrote:
> Hrm...  This isn't threadsafe.  If we were creating two of these at once on
> different threads, even with the same parameters, bad things could happen.
> 
> Also, if the DRP code relies on reading command line options as it needs
access
> to the parameters, we could also get into serious trouble.

This is here only for tests. I've added a TODO to not use the command line. This
is planned anyway.

https://codereview.chromium.org/937513003/diff/240001/components/cronet/andro...
File components/cronet/android/cronet_data_reduction_proxy.h (right):

https://codereview.chromium.org/937513003/diff/240001/components/cronet/andro...
components/cronet/android/cronet_data_reduction_proxy.h:35: // Wrapper and
configurator of Data Reduction Proxy objects for Cronet.
On 2015/04/24 19:10:28, mmenke (Out 4-27 to 5-1) wrote:
> Think it's worth mentioning that it configures things so that both the UI and
IO
> classes are both run on the Cronet's network thread (And maybe also reference
> that when setting up the UI classes).

Done.

https://codereview.chromium.org/937513003/diff/240001/components/cronet/andro...
File components/cronet/android/cronet_in_memory_pref_store.cc (right):

https://codereview.chromium.org/937513003/diff/240001/components/cronet/andro...
components/cronet/android/cronet_in_memory_pref_store.cc:8: #include
"base/values.h"
On 2015/04/24 19:27:23, mmenke (Out 4-27 to 5-1) wrote:
> I don't think this is needed - we just base Value*s and pass them along.

Done.

https://codereview.chromium.org/937513003/diff/240001/components/cronet/andro...
File components/cronet/android/cronet_url_request_context_adapter.cc (right):

https://codereview.chromium.org/937513003/diff/240001/components/cronet/andro...
components/cronet/android/cronet_url_request_context_adapter.cc:160: if
(config->enable_data_reduction_proxy) {
On 2015/04/24 19:10:28, mmenke (Out 4-27 to 5-1) wrote:
> Wasn't there a decision to make it so Cronet could be build without the DRP,
out
> of concerns about binary size (Not that I expect there to be a huge
difference).

Yes. I'm going to write that CL write after this one.

https://codereview.chromium.org/937513003/diff/240001/components/cronet/andro...
components/cronet/android/cronet_url_request_context_adapter.cc:178: }
On 2015/04/24 19:27:23, mmenke (Out 4-27 to 5-1) wrote:
> optional:  Think this may be a little cleaner as:
> 
> scoped_ptr<NetworkDelegate> network_delegate(new BasicNetworkDelegate());
> if (config->enable_data_reduction_proxy) {
>   ...
>   network_delegate =
>      
data_reduction_proxy_->CreateNetworkDelegate(network_delegate.release());
>   ...
> }
> 
> context_builder.set_network_delegate(network_delegate.release());

Done.

https://codereview.chromium.org/937513003/diff/240001/components/cronet/andro...
components/cronet/android/cronet_url_request_context_adapter.cc:242: if
(data_reduction_proxy_) {
On 2015/04/24 19:27:23, mmenke (Out 4-27 to 5-1) wrote:
> This should probably go just before the "is_context_initialized_ = true;",
since
> it's really a part of context init.

Done.

https://codereview.chromium.org/937513003/diff/240001/components/cronet/andro...
File
components/cronet/android/java/src/org/chromium/net/UrlRequestContextConfig.java
(right):

https://codereview.chromium.org/937513003/diff/240001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/UrlRequestContextConfig.java:108:
public UrlRequestContextConfig enableDataReductionProxy(boolean value,
On 2015/04/24 19:10:28, mmenke (Out 4-27 to 5-1) wrote:
> Wonder if we really need a value - if you don't want it, don't enable it (The
> ones above are a bit different, as default may change over time).

Probably not necessary immediately, but I wanted eventually to add support to
turn it off/on over time.

https://codereview.chromium.org/937513003/diff/240001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/UrlRequestContextConfig.java:232:
* Overrides Data Reduction Proxy configuration parameters with a primary
On 2015/04/24 19:10:28, mmenke (Out 4-27 to 5-1) wrote:
> Overrides parameters implies we have defaults, but we don't, do we?

We do. They are hard coded in DRP code for now.

https://codereview.chromium.org/937513003/diff/240001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/UrlRequestContextConfig.java:238:
*/
On 2015/04/24 19:10:28, mmenke (Out 4-27 to 5-1) wrote:
> Suggest moving this below the other DRP method (Know the quic methods aren't
> next to each other, but seems like the file is easier to read if related
methods
> are adjacent.

Done.

https://codereview.chromium.org/937513003/diff/240001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/UrlRequestContextConfig.java:242:
String secureProxyCheckUrl) {
On 2015/04/24 19:10:28, mmenke (Out 4-27 to 5-1) wrote:
> IF any of these are empty throw an IllegalArgumentException?  It's preferred
> over just DCHECKing in the C++ code.

Done.

https://codereview.chromium.org/937513003/diff/240001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/UrlRequestContextConfig.java:250:
secureProxyCheckUrl);
On 2015/04/24 19:10:28, mmenke (Out 4-27 to 5-1) wrote:
> Think this would be more legibly as just 3 independent putString calls, any a
> return.

Done.

https://codereview.chromium.org/937513003/diff/240001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.cc
(right):

https://codereview.chromium.org/937513003/diff/240001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.cc:142:
if (spdy_proxy_auth_enabled_.GetValue() != enabled) {
On 2015/04/24 19:27:23, mmenke (Out 4-27 to 5-1) wrote:
> nit:  Extra blank line here doesn't really belong in this CL.

Done.

[mmenke, 2015-04-24 21:17:59]

just one quick response

https://codereview.chromium.org/937513003/diff/240001/components/cronet/andro...
File
components/cronet/android/java/src/org/chromium/net/UrlRequestContextConfig.java
(right):

https://codereview.chromium.org/937513003/diff/240001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/UrlRequestContextConfig.java:108:
public UrlRequestContextConfig enableDataReductionProxy(boolean value,
On 2015/04/24 21:13:51, bengr wrote:
> On 2015/04/24 19:10:28, mmenke (Out 4-27 to 5-1) wrote:
> > Wonder if we really need a value - if you don't want it, don't enable it
(The
> > ones above are a bit different, as default may change over time).
> 
> Probably not necessary immediately, but I wanted eventually to add support to
> turn it off/on over time.

Note that we don't support modifying the config after creation, and really can't
support it, so we'd need to use another method to enable/disable it on a live
RequestContext than just creating a new config

[bengr, 2015-04-24 22:12:44]

https://codereview.chromium.org/937513003/diff/240001/components/cronet/andro...
File
components/cronet/android/java/src/org/chromium/net/UrlRequestContextConfig.java
(right):

https://codereview.chromium.org/937513003/diff/240001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/UrlRequestContextConfig.java:108:
public UrlRequestContextConfig enableDataReductionProxy(boolean value,
On 2015/04/24 21:17:59, mmenke (Out 4-27 to 5-1) wrote:
> On 2015/04/24 21:13:51, bengr wrote:
> > On 2015/04/24 19:10:28, mmenke (Out 4-27 to 5-1) wrote:
> > > Wonder if we really need a value - if you don't want it, don't enable it
> (The
> > > ones above are a bit different, as default may change over time).
> > 
> > Probably not necessary immediately, but I wanted eventually to add support
to
> > turn it off/on over time.
> 
> Note that we don't support modifying the config after creation, and really
can't
> support it, so we'd need to use another method to enable/disable it on a live
> RequestContext than just creating a new config

Done.

[mmenke, 2015-05-05 21:45:52]

Sorry I haven't gotten back to this yet.  A bit of a blacklog.  Should get back
to it tomorrow.

[mmenke, 2015-05-07 20:22:19]

Sorry for being so slow on this one.

https://codereview.chromium.org/937513003/diff/280001/components/cronet/andro...
File components/cronet/android/cronet_data_reduction_proxy.cc (right):

https://codereview.chromium.org/937513003/diff/280001/components/cronet/andro...
components/cronet/android/cronet_data_reduction_proxy.cc:46: base::CommandLine*
command_line) {
On 2015/04/24 21:13:50, bengr wrote:
> On 2015/04/24 19:10:27, mmenke (Out 4-27 to 5-1) wrote:
> > Hrm...  This isn't threadsafe.  If we were creating two of these at once on
> > different threads, even with the same parameters, bad things could happen.
> > 
> > Also, if the DRP code relies on reading command line options as it needs
> access
> > to the parameters, we could also get into serious trouble.
> 
> This is here only for tests. I've added a TODO to not use the command line.
This
> is planned anyway.

Would suggest adding something to warn people away from enabling the DRP this
until its ready, but looks like your followup CL that just excludes it from the
build should be enough for that.

https://codereview.chromium.org/937513003/diff/280001/components/cronet/andro...
components/cronet/android/cronet_data_reduction_proxy.cc:73: :
task_runner_(task_runner) {
DCHECK(task_runner_->BelongsToCurrenThread());?

Since we're going to do both IO and UI stuff there, per class docs, and call
InitOnUIThread and SetKeyOnIO, seems like we're rather depending that we're
currently on it.

https://codereview.chromium.org/937513003/diff/280001/components/cronet/andro...
components/cronet/android/cronet_data_reduction_proxy.cc:88:
io_data_->InitOnUIThread(prefs_.get());
I'm assuming that elsewhere, SetKeyOnIO is generally called after
InitOnUIThread?  Should probably use the same order here.

Actually, I can't find the method DataReductionProxyIOData::InitOnUIThread. 
Does it still exist?

https://codereview.chromium.org/937513003/diff/280001/components/cronet/andro...
File components/cronet/android/cronet_data_reduction_proxy.h (right):

https://codereview.chromium.org/937513003/diff/280001/components/cronet/andro...
components/cronet/android/cronet_data_reduction_proxy.h:36: // configured the
Data Reduction Proxy to run both its UI and IO classes on
configured -> configures

https://codereview.chromium.org/937513003/diff/280001/components/cronet/andro...
components/cronet/android/cronet_data_reduction_proxy.h:41: // the
authentication key. The task_runner should be suitable for running
nit:  |task_runner|

https://codereview.chromium.org/937513003/diff/280001/components/cronet/andro...
File components/cronet/android/cronet_in_memory_pref_store.cc (right):

https://codereview.chromium.org/937513003/diff/280001/components/cronet/andro...
components/cronet/android/cronet_in_memory_pref_store.cc:41: DCHECK(value);
Need to include base/logging.h for DCHECK.

https://codereview.chromium.org/937513003/diff/280001/components/cronet/andro...
File components/cronet/android/cronet_library_loader.cc (right):

https://codereview.chromium.org/937513003/diff/280001/components/cronet/andro...
components/cronet/android/cronet_library_loader.cc:102:
base::CommandLine::Init(0, nullptr);
Please add a TODO about getting rid of this once the DRP no longer needs it for
configuration information.

https://codereview.chromium.org/937513003/diff/280001/components/cronet/andro...
File components/cronet/android/cronet_url_request_context_adapter.cc (right):

https://codereview.chromium.org/937513003/diff/280001/components/cronet/andro...
components/cronet/android/cronet_url_request_context_adapter.cc:155:
net::URLRequestContextBuilder context_builder;
Suggest a blank line here.

https://codereview.chromium.org/937513003/diff/280001/components/cronet/andro...
File components/cronet/android/cronet_url_request_context_adapter.h (right):

https://codereview.chromium.org/937513003/diff/280001/components/cronet/andro...
components/cronet/android/cronet_url_request_context_adapter.h:102:
scoped_ptr<CronetDataReductionProxy> data_reduction_proxy_;
BUG:  For proper teardown, we need to delete whatever object owns the URLFetcher
*before* the URLRequestContext.  Since the context owns the DRP's
NetworkDelegate, it has to be safe to delete this before the NetworkDelegate.

Alternatively, you could just cancel any probe requests before destroying the
context (Optionally deleting just the "UI" part of the data structure), and
everything will work out file.

https://codereview.chromium.org/937513003/diff/280001/components/cronet/andro...
File
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java
(right):

https://codereview.chromium.org/937513003/diff/280001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:113:
commandLineArgs);
Can you just do something like:

// Ensure native code is loaded before trying to start test server.
UrlRequestContext.createContext(this, config).shutdown();

Even if that doesn't work for some reason, think it's worth a comment what's
going on.

https://codereview.chromium.org/937513003/diff/280001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:140:
listener.mResponseInfo.getUrl());
Also think it's worth an explanation of just what this test is really doing.

https://codereview.chromium.org/937513003/diff/280001/components/data_reducti...
File components/data_reduction_proxy.gypi (right):

https://codereview.chromium.org/937513003/diff/280001/components/data_reducti...
components/data_reduction_proxy.gypi:52: 'pref_registry',
Hrm...pref_registry has a DEPS file that includes ui/base, which depends on
base_i18n, which cronet can not depend on.  Looks like that DEPS file isn't
actually needed, though.  Curious.

https://codereview.chromium.org/937513003/diff/280001/components/data_reducti...
components/data_reduction_proxy.gypi:85:
'data_reduction_proxy_core_common_small',
You've included this one twice.

https://codereview.chromium.org/937513003/diff/280001/components/data_reducti...
components/data_reduction_proxy.gypi:90: 'pref_registry',
data_reduction_proxy_proto and pref_registry are already in
"data_reduction_proxy_core_browser_deps".

https://codereview.chromium.org/937513003/diff/280001/components/data_reducti...
components/data_reduction_proxy.gypi:191: 'data_reduction_proxy_core_common',
Again, you have this twice.

https://codereview.chromium.org/937513003/diff/280001/components/data_reducti...
components/data_reduction_proxy.gypi:196: 'pref_registry',
data_reduction_proxy_proto and pref_registry are already in
"data_reduction_proxy_core_browser_deps".

[mmenke, 2015-05-07 20:39:52]

https://codereview.chromium.org/937513003/diff/280001/components/cronet/andro...
File components/cronet/android/cronet_url_request_context_adapter.h (right):

https://codereview.chromium.org/937513003/diff/280001/components/cronet/andro...
components/cronet/android/cronet_url_request_context_adapter.h:102:
scoped_ptr<CronetDataReductionProxy> data_reduction_proxy_;
Test suggestion:  In another test, use a URL that hangs eternally for the
flywheel probe URL.

MockUrlRequestJobFactory already supports such urls - use getMockUrlWithFailure
with a netError of -1.

[bengr, 2015-05-08 22:41:03]

https://codereview.chromium.org/937513003/diff/280001/components/cronet/andro...
File components/cronet/android/cronet_data_reduction_proxy.cc (right):

https://codereview.chromium.org/937513003/diff/280001/components/cronet/andro...
components/cronet/android/cronet_data_reduction_proxy.cc:46: base::CommandLine*
command_line) {
On 2015/05/07 20:22:18, mmenke wrote:
> On 2015/04/24 21:13:50, bengr wrote:
> > On 2015/04/24 19:10:27, mmenke (Out 4-27 to 5-1) wrote:
> > > Hrm...  This isn't threadsafe.  If we were creating two of these at once
on
> > > different threads, even with the same parameters, bad things could happen.
> > > 
> > > Also, if the DRP code relies on reading command line options as it needs
> > access
> > > to the parameters, we could also get into serious trouble.
> > 
> > This is here only for tests. I've added a TODO to not use the command line.
> This
> > is planned anyway.
> 
> Would suggest adding something to warn people away from enabling the DRP this
> until its ready, but looks like your followup CL that just excludes it from
the
> build should be enough for that.

Acknowledged.

https://codereview.chromium.org/937513003/diff/280001/components/cronet/andro...
components/cronet/android/cronet_data_reduction_proxy.cc:73: :
task_runner_(task_runner) {
On 2015/05/07 20:22:18, mmenke wrote:
> DCHECK(task_runner_->BelongsToCurrenThread());?
> 
> Since we're going to do both IO and UI stuff there, per class docs, and call
> InitOnUIThread and SetKeyOnIO, seems like we're rather depending that we're
> currently on it.

Done.

https://codereview.chromium.org/937513003/diff/280001/components/cronet/andro...
components/cronet/android/cronet_data_reduction_proxy.cc:88:
io_data_->InitOnUIThread(prefs_.get());
On 2015/05/07 20:22:18, mmenke wrote:
> I'm assuming that elsewhere, SetKeyOnIO is generally called after
> InitOnUIThread?  Should probably use the same order here.
> 
> Actually, I can't find the method DataReductionProxyIOData::InitOnUIThread. 
> Does it still exist?

This was removed recently.

https://codereview.chromium.org/937513003/diff/280001/components/cronet/andro...
File components/cronet/android/cronet_data_reduction_proxy.h (right):

https://codereview.chromium.org/937513003/diff/280001/components/cronet/andro...
components/cronet/android/cronet_data_reduction_proxy.h:36: // configured the
Data Reduction Proxy to run both its UI and IO classes on
On 2015/05/07 20:22:18, mmenke wrote:
> configured -> configures

Done.

https://codereview.chromium.org/937513003/diff/280001/components/cronet/andro...
components/cronet/android/cronet_data_reduction_proxy.h:41: // the
authentication key. The task_runner should be suitable for running
On 2015/05/07 20:22:18, mmenke wrote:
> nit:  |task_runner|

Done.

https://codereview.chromium.org/937513003/diff/280001/components/cronet/andro...
File components/cronet/android/cronet_in_memory_pref_store.cc (right):

https://codereview.chromium.org/937513003/diff/280001/components/cronet/andro...
components/cronet/android/cronet_in_memory_pref_store.cc:41: DCHECK(value);
On 2015/05/07 20:22:18, mmenke wrote:
> Need to include base/logging.h for DCHECK.

Done.

https://codereview.chromium.org/937513003/diff/280001/components/cronet/andro...
File components/cronet/android/cronet_library_loader.cc (right):

https://codereview.chromium.org/937513003/diff/280001/components/cronet/andro...
components/cronet/android/cronet_library_loader.cc:102:
base::CommandLine::Init(0, nullptr);
On 2015/05/07 20:22:18, mmenke wrote:
> Please add a TODO about getting rid of this once the DRP no longer needs it
for
> configuration information.

Done.

https://codereview.chromium.org/937513003/diff/280001/components/cronet/andro...
File components/cronet/android/cronet_url_request_context_adapter.cc (right):

https://codereview.chromium.org/937513003/diff/280001/components/cronet/andro...
components/cronet/android/cronet_url_request_context_adapter.cc:155:
net::URLRequestContextBuilder context_builder;
On 2015/05/07 20:22:18, mmenke wrote:
> Suggest a blank line here.

Done.

https://codereview.chromium.org/937513003/diff/280001/components/cronet/andro...
File components/cronet/android/cronet_url_request_context_adapter.h (right):

https://codereview.chromium.org/937513003/diff/280001/components/cronet/andro...
components/cronet/android/cronet_url_request_context_adapter.h:102:
scoped_ptr<CronetDataReductionProxy> data_reduction_proxy_;
On 2015/05/07 20:22:18, mmenke wrote:
> BUG:  For proper teardown, we need to delete whatever object owns the
URLFetcher
> *before* the URLRequestContext.  Since the context owns the DRP's
> NetworkDelegate, it has to be safe to delete this before the NetworkDelegate.
> 
> Alternatively, you could just cancel any probe requests before destroying the
> context (Optionally deleting just the "UI" part of the data structure), and
> everything will work out file.

Can you say a little more? data_reduction_proxy_ gets destroyed before context_,
and drp_ owns the DRPIOData, which owns the DRPConfig, which owns the
URLFetcher.

https://codereview.chromium.org/937513003/diff/280001/components/cronet/andro...
components/cronet/android/cronet_url_request_context_adapter.h:102:
scoped_ptr<CronetDataReductionProxy> data_reduction_proxy_;
On 2015/05/07 20:39:52, mmenke wrote:
> Test suggestion:  In another test, use a URL that hangs eternally for the
> flywheel probe URL.
> 
> MockUrlRequestJobFactory already supports such urls - use
getMockUrlWithFailure
> with a netError of -1.

What would this test? And mind if we postpone to a followup CL?

https://codereview.chromium.org/937513003/diff/280001/components/cronet/andro...
File
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java
(right):

https://codereview.chromium.org/937513003/diff/280001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:113:
commandLineArgs);
On 2015/05/07 20:22:18, mmenke wrote:
> Can you just do something like:
> 
> // Ensure native code is loaded before trying to start test server.
> UrlRequestContext.createContext(this, config).shutdown();
> 
> Even if that doesn't work for some reason, think it's worth a comment what's
> going on.

Done.

https://codereview.chromium.org/937513003/diff/280001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:140:
listener.mResponseInfo.getUrl());
On 2015/05/07 20:22:18, mmenke wrote:
> Also think it's worth an explanation of just what this test is really doing.

Done.

https://codereview.chromium.org/937513003/diff/280001/components/data_reducti...
File components/data_reduction_proxy.gypi (right):

https://codereview.chromium.org/937513003/diff/280001/components/data_reducti...
components/data_reduction_proxy.gypi:52: 'pref_registry',
On 2015/05/07 20:22:19, mmenke wrote:
> Hrm...pref_registry has a DEPS file that includes ui/base, which depends on
> base_i18n, which cronet can not depend on.  Looks like that DEPS file isn't
> actually needed, though.  Curious.

Acknowledged.

https://codereview.chromium.org/937513003/diff/280001/components/data_reducti...
components/data_reduction_proxy.gypi:85:
'data_reduction_proxy_core_common_small',
On 2015/05/07 20:22:19, mmenke wrote:
> You've included this one twice.

Done.

https://codereview.chromium.org/937513003/diff/280001/components/data_reducti...
components/data_reduction_proxy.gypi:90: 'pref_registry',
On 2015/05/07 20:22:18, mmenke wrote:
> data_reduction_proxy_proto and pref_registry are already in
> "data_reduction_proxy_core_browser_deps".

drp_proto is needed here.

https://codereview.chromium.org/937513003/diff/280001/components/data_reducti...
components/data_reduction_proxy.gypi:191: 'data_reduction_proxy_core_common',
On 2015/05/07 20:22:18, mmenke wrote:
> Again, you have this twice.

Done.

https://codereview.chromium.org/937513003/diff/280001/components/data_reducti...
components/data_reduction_proxy.gypi:196: 'pref_registry',
On 2015/05/07 20:22:19, mmenke wrote:
> data_reduction_proxy_proto and pref_registry are already in
> "data_reduction_proxy_core_browser_deps".

drp_proto is needed here.

[mmenke, 2015-05-12 18:05:46]

LGTM, just a nit.

https://codereview.chromium.org/937513003/diff/300001/components/cronet/andro...
File components/cronet/android/cronet_in_memory_pref_store.h (right):

https://codereview.chromium.org/937513003/diff/300001/components/cronet/andro...
components/cronet/android/cronet_in_memory_pref_store.h:35: //
PersistentPrefStore overrides:
nit:  Should use same comment style here as in line 27 (Don't care what style
you use)

[bengr, 2015-05-13 16:35:04]

https://codereview.chromium.org/937513003/diff/300001/components/cronet/andro...
File components/cronet/android/cronet_in_memory_pref_store.h (right):

https://codereview.chromium.org/937513003/diff/300001/components/cronet/andro...
components/cronet/android/cronet_in_memory_pref_store.h:35: //
PersistentPrefStore overrides:
On 2015/05/12 18:05:46, mmenke wrote:
> nit:  Should use same comment style here as in line 27 (Don't care what style
> you use)

Done.

[bengr, 2015-05-13 17:25:42]

The CQ bit was checked by bengr@chromium.org

[bengr, 2015-05-13 17:25:43]

The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org
Link to the patchset: https://codereview.chromium.org/937513003/#ps320001
(title: "Addressed nit")

[bengr, 2015-05-13 17:25:49]

The CQ bit was unchecked by bengr@chromium.org

[bengr, 2015-05-13 17:26:17]

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

[sclittle, 2015-05-13 17:39:09]

components/data_reduction_proxy LGTM

[bengr, 2015-05-13 17:49:12]

The CQ bit was checked by bengr@chromium.org

[commit-bot: I haz the power, 2015-05-13 17:50:24]

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

[commit-bot: I haz the power, 2015-05-13 17:56:42]

Committed patchset #11 (id:320001)

[commit-bot: I haz the power, 2015-05-13 17:57:56]

Patchset 11 (id:??) landed as
https://crrev.com/59cb6968d3bcb367ec97f6c3c88da08f7921e697
Cr-Commit-Position: refs/heads/master@{#329674}