Data Reduction Proxy class ownership updates and Settings cleanup

Class ownership changes:
- DRPIOData is a container for IOData lifetime objects, and constructed first.
- DataSaverService is a container for Profile/UI based lifetime objects, and
  constructed second. It can take raw pointers to DRPIOData, since
  DataSaverService is destroyed before DRPIOData.
- DRPIOData (and its classes) then fed a WeakPtr to the DataSaverService.
- DataSaverService is (temporarily) owned by DRPSettings (primarily because
  DRPChromeSettings is the KeyedService) - this will swap in a future CL.
- During KeyedService shutdown, the WeakPtr is invalidated.

Removal of redundant functionality in the DRPSettings:
- I/O thread related functionality (Canary check, IP address change) now lives
  in the Config class, with calls into DataSaverService to use the URLFetcher
- the Settings class no longer is aware of Params, and instead uses the Config
  class for retrieving information - this results in Params construction
  moving from Settings -> DRP IO/Data
- Certain information which is exposed to the UI (via the Settings class) is
  now cached after being retrieved from the Config class (for thread safety)

BUG=452773
Committed: https://crrev.com/f917e431fd15598c0e28dbc9a39ac408272548a4
Cr-Commit-Position: refs/heads/master@{#317432}

[jeremyim, 2015-02-02 18:23:41]

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

[jeremyim, 2015-02-02 18:23:42]

PTAL. =)

[bengr, 2015-02-02 19:48:36]

On 2015/02/02 18:23:42, jeremyim wrote:
> PTAL. =)

Looking. For now, please change the CL description to provide more details about
what you changed, specifically, and why.

[bengr, 2015-02-03 21:51:58]

The formatting of your CL comment is off. I add my own line breaks when I update
from this interface.

https://codereview.chromium.org/893003002/diff/1/chrome/browser/net/spdyproxy...
File chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.h
(right):

https://codereview.chromium.org/893003002/diff/1/chrome/browser/net/spdyproxy...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.h:26: class
DataReductionProxyChromeSettings
Hmm, I guess I was expecting that you'd constuct a threadsafe config object for
settings construction and pass ownership of it to io_data and have setting
retain a WeakPtr. Please work through the construction/destruction ordering of
settings, profile, UI thread and IO thread to see if your approach is viable.

https://codereview.chromium.org/893003002/diff/1/chrome/browser/net/spdyproxy...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.h:30: //
Constructs a settings object with the given configuration parameters.
Update the comment.

https://codereview.chromium.org/893003002/diff/1/chrome/browser/net/spdyproxy...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.h:39:
data_reduction_proxy::DataReductionProxyIOData* io_data,
Hmm. I'm worried that initializing the settings object after io_data creation
may be too late. Does this work on Android, e.g.?

https://codereview.chromium.org/893003002/diff/20001/android_webview/browser/...
File android_webview/browser/aw_browser_context.cc (right):

https://codereview.chromium.org/893003002/diff/20001/android_webview/browser/...
android_webview/browser/aw_browser_context.cc:279:
data_reduction_proxy_io_data_->InitURLRequestContext(GetRequestContext());
Why can't this be part of drp_io_data construction?

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

https://codereview.chromium.org/893003002/diff/20001/chrome/browser/net/spdyp...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.cc:67:
io_data->config(),
Why not just pass in io_data? Then you wouldn't need both parameters. And it
would be easier to extend in the future.

https://codereview.chromium.org/893003002/diff/20001/chrome/browser/net/spdyp...
File chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.h
(right):

https://codereview.chromium.org/893003002/diff/20001/chrome/browser/net/spdyp...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.h:30: //
Constructs a settings object with the given configuration parameters.
What parameters?

https://codereview.chromium.org/893003002/diff/20001/chrome/browser/net/spdyp...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.h:32:
DataReductionProxyChromeSettings();
I would love to see this renamed as DataReductionProxyUIData, but probably not
in this CL.

Actually, DataSaverUIData would be even better.

https://codereview.chromium.org/893003002/diff/20001/chrome/browser/net/spdyp...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.h:41:
PrefService* local_state_prefs);
Do we still need local state?

https://codereview.chromium.org/893003002/diff/20001/chrome/browser/net/spdyp...
File
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings_factory.cc
(left):

https://codereview.chromium.org/893003002/diff/20001/chrome/browser/net/spdyp...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings_factory.cc:67:
if (DataReductionProxyParams::IsIncludedInAndroidOnePromoFieldTrial(
Where is this now?

https://codereview.chromium.org/893003002/diff/20001/chrome/browser/net/spdyp...
File chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc
(right):

https://codereview.chromium.org/893003002/diff/20001/chrome/browser/net/spdyp...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:49: return
ConvertUTF8ToJavaString(env, Settings()->PrimaryOrigin().spec());
I thought you were going to move us away from notions of "primary" and
"fallback" and towards HTTPS and HTTP or something like that.

https://codereview.chromium.org/893003002/diff/20001/chrome/browser/net/spdyp...
File
chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc
(right):

https://codereview.chromium.org/893003002/diff/20001/chrome/browser/net/spdyp...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:72:
static_cast<MockDataReductionProxyConfig*>(config_.get());
To avoid this dance, couldn't config_ just be a MockDataReductionProxyConfig* ?

https://codereview.chromium.org/893003002/diff/20001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

https://codereview.chromium.org/893003002/diff/20001/chrome/browser/profiles/...
chrome/browser/profiles/profile_impl_io_data.cc:182:
io_data_->data_reduction_proxy_io_data()->InitURLRequestContext(
I'd strongly prefer not adding to the interface between ProfileImplIOData and
DRPIOData.

https://codereview.chromium.org/893003002/diff/20001/components/data_reductio...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_bypass_protocol_unittest.cc
(right):

https://codereview.chromium.org/893003002/diff/20001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_bypass_protocol_unittest.cc:134:
base::MessageLoopProxy::current(),
Please put parameters together on the same line. Here and below.

https://codereview.chromium.org/893003002/diff/20001/components/data_reductio...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc
(right):

https://codereview.chromium.org/893003002/diff/20001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:90:
params_.reset(params.release());
params_ = params.Pass() ?

https://codereview.chromium.org/893003002/diff/20001/components/data_reductio...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_test_utils.cc
(right):

https://codereview.chromium.org/893003002/diff/20001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_test_utils.cc:27:
event_store) {}
end curly on new line.

https://codereview.chromium.org/893003002/diff/20001/components/data_reductio...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_test_utils.h
(right):

https://codereview.chromium.org/893003002/diff/20001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_test_utils.h:25:
MockDataReductionProxyConfig(
Thinking ahead a bit, I'd like to have Mock/Fake subclasses of all of our
objects so that we can construct a DRP stack with as much or as little mocked
out as we need for a test. This seems like a good start for config. Do you think
instead of having separate files for each mock class we should just have a
single data_reduction_proxy_test_utils?

https://codereview.chromium.org/893003002/diff/20001/components/data_reductio...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc
(right):

https://codereview.chromium.org/893003002/diff/20001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc:51:
if (client != Client::WEBVIEW_ANDROID) {
This is bad. Please have each platform inject its configuration options.

https://codereview.chromium.org/893003002/diff/20001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc:58:
#if defined(OS_ANDROID)
Likewise, as much as possible I'd like to keep our component clean of platform
specialization.

https://codereview.chromium.org/893003002/diff/20001/components/data_reductio...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h
(right):

https://codereview.chromium.org/893003002/diff/20001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h:79:
DataReductionProxyConfig* config() const { return config_.get(); }
Put return on new line.

https://codereview.chromium.org/893003002/diff/20001/components/data_reductio...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.cc
(right):

https://codereview.chromium.org/893003002/diff/20001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.cc:83:
alternative_allowed_ = config_->params()->alternative_allowed();
Why do we have these members if they are directly copied from config?

https://codereview.chromium.org/893003002/diff/20001/components/data_reductio...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.h
(right):

https://codereview.chromium.org/893003002/diff/20001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.h:134:
bool Allowed() const { return allowed_; }
Move return to new line here and below.

[jeremyim, 2015-02-04 01:30:13]

Patchset #3 (id:40001) has been deleted

[jeremyim, 2015-02-04 01:31:21]

https://codereview.chromium.org/893003002/diff/20001/android_webview/browser/...
File android_webview/browser/aw_browser_context.cc (right):

https://codereview.chromium.org/893003002/diff/20001/android_webview/browser/...
android_webview/browser/aw_browser_context.cc:279:
data_reduction_proxy_io_data_->InitURLRequestContext(GetRequestContext());
On 2015/02/03 21:51:57, bengr wrote:
> Why can't this be part of drp_io_data construction?

It can be for AW, but not for Chrome. Comment added on
DataReductionProxyIOData::Initialize

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

https://codereview.chromium.org/893003002/diff/20001/chrome/browser/net/spdyp...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.cc:67:
io_data->config(),
On 2015/02/03 21:51:57, bengr wrote:
> Why not just pass in io_data? Then you wouldn't need both parameters. And it
> would be easier to extend in the future.

Done.

https://codereview.chromium.org/893003002/diff/20001/chrome/browser/net/spdyp...
File chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.h
(right):

https://codereview.chromium.org/893003002/diff/20001/chrome/browser/net/spdyp...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.h:30: //
Constructs a settings object with the given configuration parameters.
On 2015/02/03 21:51:57, bengr wrote:
> What parameters?

Done.

https://codereview.chromium.org/893003002/diff/20001/chrome/browser/net/spdyp...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.h:32:
DataReductionProxyChromeSettings();
On 2015/02/03 21:51:57, bengr wrote:
> I would love to see this renamed as DataReductionProxyUIData, but probably not
> in this CL.
> 
> Actually, DataSaverUIData would be even better.

Acknowledged.

https://codereview.chromium.org/893003002/diff/20001/chrome/browser/net/spdyp...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.h:41:
PrefService* local_state_prefs);
On 2015/02/03 21:51:58, bengr wrote:
> Do we still need local state?

Done.

https://codereview.chromium.org/893003002/diff/20001/chrome/browser/net/spdyp...
File
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings_factory.cc
(left):

https://codereview.chromium.org/893003002/diff/20001/chrome/browser/net/spdyp...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings_factory.cc:67:
if (DataReductionProxyParams::IsIncludedInAndroidOnePromoFieldTrial(
On 2015/02/03 21:51:58, bengr wrote:
> Where is this now?

This now lives in CreateDataReductionProxyChromeIOData

https://codereview.chromium.org/893003002/diff/20001/chrome/browser/net/spdyp...
File chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc
(right):

https://codereview.chromium.org/893003002/diff/20001/chrome/browser/net/spdyp...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:49: return
ConvertUTF8ToJavaString(env, Settings()->PrimaryOrigin().spec());
On 2015/02/03 21:51:58, bengr wrote:
> I thought you were going to move us away from notions of "primary" and
> "fallback" and towards HTTPS and HTTP or something like that.

This will take place prior to Params removal to keep the CL's as focused as
possible.

https://codereview.chromium.org/893003002/diff/20001/chrome/browser/net/spdyp...
File
chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc
(right):

https://codereview.chromium.org/893003002/diff/20001/chrome/browser/net/spdyp...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:72:
static_cast<MockDataReductionProxyConfig*>(config_.get());
On 2015/02/03 21:51:58, bengr wrote:
> To avoid this dance, couldn't config_ just be a MockDataReductionProxyConfig*
?

It actually already is... Oops! Done.

https://codereview.chromium.org/893003002/diff/20001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

https://codereview.chromium.org/893003002/diff/20001/chrome/browser/profiles/...
chrome/browser/profiles/profile_impl_io_data.cc:182:
io_data_->data_reduction_proxy_io_data()->InitURLRequestContext(
On 2015/02/03 21:51:58, bengr wrote:
> I'd strongly prefer not adding to the interface between ProfileImplIOData and
> DRPIOData.

Per comment on Initialize: GetRequestContext() requires a ProfileImplIOData, but
initializing a ProfileImplIOData requires a DRPIOData (to inject the DRP network
delegate), so we need some means of breaking the cycle.

https://codereview.chromium.org/893003002/diff/20001/components/data_reductio...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_bypass_protocol_unittest.cc
(right):

https://codereview.chromium.org/893003002/diff/20001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_bypass_protocol_unittest.cc:134:
base::MessageLoopProxy::current(),
On 2015/02/03 21:51:58, bengr wrote:
> Please put parameters together on the same line. Here and below.

Done.

https://codereview.chromium.org/893003002/diff/20001/components/data_reductio...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc
(right):

https://codereview.chromium.org/893003002/diff/20001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:90:
params_.reset(params.release());
On 2015/02/03 21:51:58, bengr wrote:
> params_ = params.Pass() ?

Done.

https://codereview.chromium.org/893003002/diff/20001/components/data_reductio...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_test_utils.cc
(right):

https://codereview.chromium.org/893003002/diff/20001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_test_utils.cc:27:
event_store) {}
On 2015/02/03 21:51:58, bengr wrote:
> end curly on new line.

Done.

https://codereview.chromium.org/893003002/diff/20001/components/data_reductio...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_test_utils.h
(right):

https://codereview.chromium.org/893003002/diff/20001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_test_utils.h:25:
MockDataReductionProxyConfig(
On 2015/02/03 21:51:58, bengr wrote:
> Thinking ahead a bit, I'd like to have Mock/Fake subclasses of all of our
> objects so that we can construct a DRP stack with as much or as little mocked
> out as we need for a test. This seems like a good start for config. Do you
think
> instead of having separate files for each mock class we should just have a
> single data_reduction_proxy_test_utils?

Done.

https://codereview.chromium.org/893003002/diff/20001/components/data_reductio...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc
(right):

https://codereview.chromium.org/893003002/diff/20001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc:51:
if (client != Client::WEBVIEW_ANDROID) {
On 2015/02/03 21:51:58, bengr wrote:
> This is bad. Please have each platform inject its configuration options. 

Done.

https://codereview.chromium.org/893003002/diff/20001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc:58:
#if defined(OS_ANDROID)
On 2015/02/03 21:51:58, bengr wrote:
> Likewise, as much as possible I'd like to keep our component clean of platform
> specialization.

Done.

https://codereview.chromium.org/893003002/diff/20001/components/data_reductio...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h
(right):

https://codereview.chromium.org/893003002/diff/20001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h:79:
DataReductionProxyConfig* config() const { return config_.get(); }
On 2015/02/03 21:51:58, bengr wrote:
> Put return on new line.

Done.

https://codereview.chromium.org/893003002/diff/20001/components/data_reductio...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.cc
(right):

https://codereview.chromium.org/893003002/diff/20001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.cc:83:
alternative_allowed_ = config_->params()->alternative_allowed();
On 2015/02/03 21:51:58, bengr wrote:
> Why do we have these members if they are directly copied from config?

In a world where Config does all of its operations on the IO thread, but
settings delivers its values on the UI thread, and Config can have its values
change, UpdateConfigValues will eventually receive a copy of the desired values
(either through a Callback or as an Observer) and not call into Config directly
to receive its values.

https://codereview.chromium.org/893003002/diff/20001/components/data_reductio...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.h
(right):

https://codereview.chromium.org/893003002/diff/20001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.h:134:
bool Allowed() const { return allowed_; }
On 2015/02/03 21:51:58, bengr wrote:
> Move return to new line here and below.

Done.

[bengr, 2015-02-05 00:46:02]

https://codereview.chromium.org/893003002/diff/20001/components/data_reductio...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.cc
(right):

https://codereview.chromium.org/893003002/diff/20001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.cc:83:
alternative_allowed_ = config_->params()->alternative_allowed();
On 2015/02/04 01:31:21, jeremyim wrote:
> On 2015/02/03 21:51:58, bengr wrote:
> > Why do we have these members if they are directly copied from config?
> 
> In a world where Config does all of its operations on the IO thread, but
> settings delivers its values on the UI thread, and Config can have its values
> change, UpdateConfigValues will eventually receive a copy of the desired
values
> (either through a Callback or as an Observer) and not call into Config
directly
> to receive its values.

Acknowledged.

https://codereview.chromium.org/893003002/diff/80001/chrome/browser/net/spdyp...
File chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.cc
(right):

https://codereview.chromium.org/893003002/diff/80001/chrome/browser/net/spdyp...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.cc:66: #if
defined(OS_ANDROID)
This platform specialization will go away in a future CL, right? If so, add a
TODO(jeremyim) explaining

https://codereview.chromium.org/893003002/diff/80001/components/data_reductio...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_test_utils.h
(right):

https://codereview.chromium.org/893003002/diff/80001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_test_utils.h:26:
scoped_refptr<base::SingleThreadTaskRunner> network_task_runner,
I think this can be forward declared instead of included.

https://codereview.chromium.org/893003002/diff/80001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_test_utils.h:27:
net::NetLog* net_log,
Forward declare NetLog, Configurator, EventStore, NetworkInterfaceList.

https://codereview.chromium.org/893003002/diff/80001/components/data_reductio...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h
(right):

https://codereview.chromium.org/893003002/diff/80001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h:86:
virtual DataReductionProxyConfig* config() const;
Why did these all become virtuals?

[jeremyim, 2015-02-05 00:59:52]

https://codereview.chromium.org/893003002/diff/80001/chrome/browser/net/spdyp...
File chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.cc
(right):

https://codereview.chromium.org/893003002/diff/80001/chrome/browser/net/spdyp...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.cc:66: #if
defined(OS_ANDROID)
On 2015/02/05 00:46:02, bengr wrote:
> This platform specialization will go away in a future CL, right? If so, add a
> TODO(jeremyim) explaining

This specialization won't go away, since it's the Android One promo.

https://codereview.chromium.org/893003002/diff/80001/components/data_reductio...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_test_utils.h
(right):

https://codereview.chromium.org/893003002/diff/80001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_test_utils.h:26:
scoped_refptr<base::SingleThreadTaskRunner> network_task_runner,
On 2015/02/05 00:46:02, bengr wrote:
> I think this can be forward declared instead of included.

Done.

https://codereview.chromium.org/893003002/diff/80001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_test_utils.h:27:
net::NetLog* net_log,
On 2015/02/05 00:46:02, bengr wrote:
> Forward declare NetLog, Configurator, EventStore, NetworkInterfaceList. 

Done.

NetworkInterfaceList is a typedef, and probably shouldn't be forward declared.

https://codereview.chromium.org/893003002/diff/80001/components/data_reductio...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h
(right):

https://codereview.chromium.org/893003002/diff/80001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h:86:
virtual DataReductionProxyConfig* config() const;
On 2015/02/05 00:46:02, bengr wrote:
> Why did these all become virtuals?

Done.

[bengr, 2015-02-05 01:35:12]

lgtm

https://codereview.chromium.org/893003002/diff/80001/chrome/browser/net/spdyp...
File chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.cc
(right):

https://codereview.chromium.org/893003002/diff/80001/chrome/browser/net/spdyp...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.cc:66: #if
defined(OS_ANDROID)
On 2015/02/05 00:59:52, jeremyim wrote:
> On 2015/02/05 00:46:02, bengr wrote:
> > This platform specialization will go away in a future CL, right? If so, add
a
> > TODO(jeremyim) explaining
> 
> This specialization won't go away, since it's the Android One promo.

Fine.

[jeremyim, 2015-02-05 01:37:24]

jeremyim@chromium.org changed reviewers:
+ mmenke@chromium.org, sgurun@chromium.org

[jeremyim, 2015-02-05 01:37:24]

PTAL =)

mmenke => profile_impl_io_data.cc
sgurun => aw_browser_context.cc

[jeremyim, 2015-02-05 01:39:10]

New patchsets have been uploaded after l-g-t-m from bengr@chromium.org

[mmenke, 2015-02-05 15:40:54]

https://codereview.chromium.org/893003002/diff/140001/chrome/browser/profiles...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

https://codereview.chromium.org/893003002/diff/140001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl_io_data.cc:183:
profile_->GetRequestContext());
This circular dependency seems weird and ugly....

The context getter can't be initialized without the io_data, but the io_data
can't be initialized without the getter, and getting the getter requires the
io_data already be set.

Also, more generally...Is the DRP using URLFetchers on the IO thread or the UI
thread?  If it's the UI thread class that's using them, shutdown should be fine,
but if it's the IO thread class that uses them, that can lead to crashes, I
believe, if a request is ongoing during profile destruction.

[jeremyim, 2015-02-05 17:47:59]

https://codereview.chromium.org/893003002/diff/140001/chrome/browser/profiles...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

https://codereview.chromium.org/893003002/diff/140001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl_io_data.cc:183:
profile_->GetRequestContext());
On 2015/02/05 15:40:53, mmenke wrote:
> This circular dependency seems weird and ugly....
> 
> The context getter can't be initialized without the io_data, but the io_data
> can't be initialized without the getter, and getting the getter requires the
> io_data already be set.
> 
> Also, more generally...Is the DRP using URLFetchers on the IO thread or the UI
> thread?  If it's the UI thread class that's using them, shutdown should be
fine,
> but if it's the IO thread class that uses them, that can lead to crashes, I
> believe, if a request is ongoing during profile destruction.

We were using URLFetchers on the IO thread, but this change was moving it to use
an IO data class. I'll look into the object dependencies some more; thanks!

[mmenke, 2015-02-05 17:51:24]

On 2015/02/05 17:47:59, jeremyim wrote:
>
https://codereview.chromium.org/893003002/diff/140001/chrome/browser/profiles...
> File chrome/browser/profiles/profile_impl_io_data.cc (right):
> 
>
https://codereview.chromium.org/893003002/diff/140001/chrome/browser/profiles...
> chrome/browser/profiles/profile_impl_io_data.cc:183:
> profile_->GetRequestContext());
> On 2015/02/05 15:40:53, mmenke wrote:
> > This circular dependency seems weird and ugly....
> > 
> > The context getter can't be initialized without the io_data, but the io_data
> > can't be initialized without the getter, and getting the getter requires the
> > io_data already be set.
> > 
> > Also, more generally...Is the DRP using URLFetchers on the IO thread or the
UI
> > thread?  If it's the UI thread class that's using them, shutdown should be
> fine,
> > but if it's the IO thread class that uses them, that can lead to crashes, I
> > believe, if a request is ongoing during profile destruction.
> 
> We were using URLFetchers on the IO thread, but this change was moving it to
use
> an IO data class. I'll look into the object dependencies some more; thanks!

The shutdown problem is this:

Destroying the fetcher results in posting a task to delete the URLRequest.  Then
we delete the URLRequestContext before that task runs (While continuing to tear
down the ProfileIOData).  Can't delete the URLRequestContext before the
URLRequest is deleted.  Sorry I didn't catch this earlier.

[mmenke, 2015-02-07 02:03:34]

On 2015/02/05 17:51:24, mmenke wrote:
> On 2015/02/05 17:47:59, jeremyim wrote:
> >
>
https://codereview.chromium.org/893003002/diff/140001/chrome/browser/profiles...
> > File chrome/browser/profiles/profile_impl_io_data.cc (right):
> > 
> >
>
https://codereview.chromium.org/893003002/diff/140001/chrome/browser/profiles...
> > chrome/browser/profiles/profile_impl_io_data.cc:183:
> > profile_->GetRequestContext());
> > On 2015/02/05 15:40:53, mmenke wrote:
> > > This circular dependency seems weird and ugly....
> > > 
> > > The context getter can't be initialized without the io_data, but the
io_data
> > > can't be initialized without the getter, and getting the getter requires
the
> > > io_data already be set.
> > > 
> > > Also, more generally...Is the DRP using URLFetchers on the IO thread or
the
> UI
> > > thread?  If it's the UI thread class that's using them, shutdown should be
> > fine,
> > > but if it's the IO thread class that uses them, that can lead to crashes,
I
> > > believe, if a request is ongoing during profile destruction.
> > 
> > We were using URLFetchers on the IO thread, but this change was moving it to
> use
> > an IO data class. I'll look into the object dependencies some more; thanks!
> 
> The shutdown problem is this:
> 
> Destroying the fetcher results in posting a task to delete the URLRequest. 
Then
> we delete the URLRequestContext before that task runs (While continuing to
tear
> down the ProfileIOData).  Can't delete the URLRequestContext before the
> URLRequest is deleted.  Sorry I didn't catch this earlier.

And...looks like I was wrong about crashing (Sorry about that).  Still concerned
about ownership, though.

[jeremyim, 2015-02-13 23:14:12]

Patchset #8 (id:160001) has been deleted

[jeremyim, 2015-02-13 23:30:26]

A pretty big round of updates, after rebasing to head to grab some recent DRP
changes, and playing around with ownership.

bengr: Not sure if you want to Not LGTM until you have a chance to review it
further. =)

On 2015/02/07 02:03:34, mmenke wrote:
> On 2015/02/05 17:51:24, mmenke wrote:
> > On 2015/02/05 17:47:59, jeremyim wrote:
> > >
> >
>
https://codereview.chromium.org/893003002/diff/140001/chrome/browser/profiles...
> > > File chrome/browser/profiles/profile_impl_io_data.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/893003002/diff/140001/chrome/browser/profiles...
> > > chrome/browser/profiles/profile_impl_io_data.cc:183:
> > > profile_->GetRequestContext());
> > > On 2015/02/05 15:40:53, mmenke wrote:
> > > > This circular dependency seems weird and ugly....
> > > > 
> > > > The context getter can't be initialized without the io_data, but the
> io_data
> > > > can't be initialized without the getter, and getting the getter requires
> the
> > > > io_data already be set.
> > > > 
> > > > Also, more generally...Is the DRP using URLFetchers on the IO thread or
> the
> > UI
> > > > thread?  If it's the UI thread class that's using them, shutdown should
be
> > > fine,
> > > > but if it's the IO thread class that uses them, that can lead to
crashes,
> I
> > > > believe, if a request is ongoing during profile destruction.
> > > 
> > > We were using URLFetchers on the IO thread, but this change was moving it
to
> > use
> > > an IO data class. I'll look into the object dependencies some more;
thanks!
> > 
> > The shutdown problem is this:
> > 
> > Destroying the fetcher results in posting a task to delete the URLRequest. 
> Then
> > we delete the URLRequestContext before that task runs (While continuing to
> tear
> > down the ProfileIOData).  Can't delete the URLRequestContext before the
> > URLRequest is deleted.  Sorry I didn't catch this earlier.
> 
> And...looks like I was wrong about crashing (Sorry about that).  Still
concerned
> about ownership, though.

[bengr, 2015-02-13 23:39:51]

On 2015/02/13 23:30:26, jeremyim wrote:
> A pretty big round of updates, after rebasing to head to grab some recent DRP
> changes, and playing around with ownership.
> 
> bengr: Not sure if you want to Not LGTM until you have a chance to review it
> further. =)
> 
> On 2015/02/07 02:03:34, mmenke wrote:
> > On 2015/02/05 17:51:24, mmenke wrote:
> > > On 2015/02/05 17:47:59, jeremyim wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/893003002/diff/140001/chrome/browser/profiles...
> > > > File chrome/browser/profiles/profile_impl_io_data.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/893003002/diff/140001/chrome/browser/profiles...
> > > > chrome/browser/profiles/profile_impl_io_data.cc:183:
> > > > profile_->GetRequestContext());
> > > > On 2015/02/05 15:40:53, mmenke wrote:
> > > > > This circular dependency seems weird and ugly....
> > > > > 
> > > > > The context getter can't be initialized without the io_data, but the
> > io_data
> > > > > can't be initialized without the getter, and getting the getter
requires
> > the
> > > > > io_data already be set.
> > > > > 
> > > > > Also, more generally...Is the DRP using URLFetchers on the IO thread
or
> > the
> > > UI
> > > > > thread?  If it's the UI thread class that's using them, shutdown
should
> be
> > > > fine,
> > > > > but if it's the IO thread class that uses them, that can lead to
> crashes,
> > I
> > > > > believe, if a request is ongoing during profile destruction.
> > > > 
> > > > We were using URLFetchers on the IO thread, but this change was moving
it
> to
> > > use
> > > > an IO data class. I'll look into the object dependencies some more;
> thanks!
> > > 
> > > The shutdown problem is this:
> > > 
> > > Destroying the fetcher results in posting a task to delete the URLRequest.

> > Then
> > > we delete the URLRequestContext before that task runs (While continuing to
> > tear
> > > down the ProfileIOData).  Can't delete the URLRequestContext before the
> > > URLRequest is deleted.  Sorry I didn't catch this earlier.
> > 
> > And...looks like I was wrong about crashing (Sorry about that).  Still
> concerned
> > about ownership, though.

not lgtm, just in case you can't help yourself from waiting for my review. :)

[jeremyim, 2015-02-15 06:02:10]

Patchset #9 (id:200001) has been deleted

[mmenke, 2015-02-17 17:03:26]

profiles/ LGTM

https://codereview.chromium.org/893003002/diff/220001/chrome/browser/profiles...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

https://codereview.chromium.org/893003002/diff/220001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl_io_data.cc:168: ChromeNetLog* const net_log
= g_browser_process->io_thread()->net_log();
optional:  Suggest just inlining this, now that you're not passing it to both
objects.

[bengr, 2015-02-17 18:10:24]

https://codereview.chromium.org/893003002/diff/220001/android_webview/browser...
File android_webview/browser/aw_browser_context.cc (right):

https://codereview.chromium.org/893003002/diff/220001/android_webview/browser...
android_webview/browser/aw_browser_context.cc:368:
DCHECK(GetDataReductionProxySettings()->SaverService());
Cute, but I'd still call this "DataSaverService() or "data_saver_service()."

https://codereview.chromium.org/893003002/diff/220001/android_webview/browser...
File android_webview/browser/aw_browser_context.h (right):

https://codereview.chromium.org/893003002/diff/220001/android_webview/browser...
android_webview/browser/aw_browser_context.h:36: class DataSaverService;
I guess we can start making the name change, though this will be a long slog.
Let's discuss offline.

https://codereview.chromium.org/893003002/diff/220001/android_webview/browser...
android_webview/browser/aw_browser_context.h:153:
scoped_ptr<AwSSLHostStateDelegate> ssl_host_state_delegate_;
This probably shouldn't be in this CL.

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

https://codereview.chromium.org/893003002/diff/220001/chrome/browser/net/spdy...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.h:41: void
Shutdown() override;
Add comments that say what these overrides override.

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

https://codereview.chromium.org/893003002/diff/220001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:94:
void DataReductionProxyConfig::SetProxyPrefsOnIOThread(bool enabled,
What does this have to do with prefs?

https://codereview.chromium.org/893003002/diff/220001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:276:
if (!ui_task_runner_->BelongsToCurrentThread()) {
This construct bugs me. Why do we not know which thread this will come in on?

https://codereview.chromium.org/893003002/diff/220001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_test_utils.h
(right):

https://codereview.chromium.org/893003002/diff/220001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_test_utils.h:5:
#ifndef
COMPONENTS_DATA_REDUCTION_PROXY_CORE_BROWSER_DATA_REDUCTION_PROXY_TEST_UTILS_H_
What do you think about landing this as a separate CL first?

https://codereview.chromium.org/893003002/diff/220001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_test_utils.h:65:
class DataReductionProxyTestContext {
Add comments to every class, enum, non-trivial method, and member.

https://codereview.chromium.org/893003002/diff/220001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_test_utils.h:67:
enum TestContextOptions {
In particular, I have no idea how to use these options.

https://codereview.chromium.org/893003002/diff/220001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_test_utils.h:145:
}  // namespace data_reduction_proxy
Add a blank line below

[jeremyim, 2015-02-17 22:07:49]

New patchsets have been uploaded after l-g-t-m from mmenke@chromium.org

[jeremyim, 2015-02-17 22:09:33]

Patchset #10 (id:240001) has been deleted

[jeremyim, 2015-02-19 20:15:28]

Patchset #10 (id:260001) has been deleted

[jeremyim, 2015-02-19 21:03:44]

https://codereview.chromium.org/893003002/diff/220001/android_webview/browser...
File android_webview/browser/aw_browser_context.cc (right):

https://codereview.chromium.org/893003002/diff/220001/android_webview/browser...
android_webview/browser/aw_browser_context.cc:368:
DCHECK(GetDataReductionProxySettings()->SaverService());
On 2015/02/17 18:10:24, bengr wrote:
> Cute, but I'd still call this "DataSaverService() or "data_saver_service()."

Done.

https://codereview.chromium.org/893003002/diff/220001/android_webview/browser...
File android_webview/browser/aw_browser_context.h (right):

https://codereview.chromium.org/893003002/diff/220001/android_webview/browser...
android_webview/browser/aw_browser_context.h:36: class DataSaverService;
On 2015/02/17 18:10:24, bengr wrote:
> I guess we can start making the name change, though this will be a long slog.
> Let's discuss offline.

Acknowledged.

https://codereview.chromium.org/893003002/diff/220001/android_webview/browser...
android_webview/browser/aw_browser_context.h:153:
scoped_ptr<AwSSLHostStateDelegate> ssl_host_state_delegate_;
On 2015/02/17 18:10:24, bengr wrote:
> This probably shouldn't be in this CL.

Done.

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

https://codereview.chromium.org/893003002/diff/220001/chrome/browser/net/spdy...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.h:41: void
Shutdown() override;
On 2015/02/17 18:10:24, bengr wrote:
> Add comments that say what these overrides override.

Done.

https://codereview.chromium.org/893003002/diff/220001/chrome/browser/profiles...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

https://codereview.chromium.org/893003002/diff/220001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl_io_data.cc:168: ChromeNetLog* const net_log
= g_browser_process->io_thread()->net_log();
On 2015/02/17 17:03:25, mmenke wrote:
> optional:  Suggest just inlining this, now that you're not passing it to both
> objects.

Done.

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

https://codereview.chromium.org/893003002/diff/220001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:94:
void DataReductionProxyConfig::SetProxyPrefsOnIOThread(bool enabled,
On 2015/02/17 18:10:24, bengr wrote:
> What does this have to do with prefs?

Done.

https://codereview.chromium.org/893003002/diff/220001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:276:
if (!ui_task_runner_->BelongsToCurrentThread()) {
On 2015/02/17 18:10:24, bengr wrote:
> This construct bugs me. Why do we not know which thread this will come in on?

Done.

https://codereview.chromium.org/893003002/diff/220001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_test_utils.h
(right):

https://codereview.chromium.org/893003002/diff/220001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_test_utils.h:5:
#ifndef
COMPONENTS_DATA_REDUCTION_PROXY_CORE_BROWSER_DATA_REDUCTION_PROXY_TEST_UTILS_H_
On 2015/02/17 18:10:24, bengr wrote:
> What do you think about landing this as a separate CL first?

Done.

https://codereview.chromium.org/893003002/diff/220001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_test_utils.h:65:
class DataReductionProxyTestContext {
On 2015/02/17 18:10:24, bengr wrote:
> Add comments to every class, enum, non-trivial method, and member.

Done.

https://codereview.chromium.org/893003002/diff/220001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_test_utils.h:67:
enum TestContextOptions {
On 2015/02/17 18:10:24, bengr wrote:
> In particular, I have no idea how to use these options.

Done.

https://codereview.chromium.org/893003002/diff/220001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_test_utils.h:145:
}  // namespace data_reduction_proxy
On 2015/02/17 18:10:24, bengr wrote:
> Add a blank line below

Done.

[bengr, 2015-02-20 00:00:37]

https://codereview.chromium.org/893003002/diff/280001/android_webview/browser...
File android_webview/browser/aw_browser_context.cc (right):

https://codereview.chromium.org/893003002/diff/280001/android_webview/browser...
android_webview/browser/aw_browser_context.cc:369:
DCHECK(GetDataReductionProxySettings()->data_reduction_proxy_service());
nit: You could introduce a local to hold GetDRPSettings()->d_r_p_service()

https://codereview.chromium.org/893003002/diff/280001/chrome/browser/net/spdy...
File chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.cc
(right):

https://codereview.chromium.org/893003002/diff/280001/chrome/browser/net/spdy...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.cc:36: int
flags = DataReductionProxyParams::kAllowed;
nit: Combine with lines 37 and 38 to OR three flags.

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

https://codereview.chromium.org/893003002/diff/280001/chrome/browser/net/spdy...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.cc:86:
net::URLRequestContextGetter* request_context,
request_context_getter

https://codereview.chromium.org/893003002/diff/280001/chrome/browser/net/spdy...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.cc:89: // On
mobile we write data reduction proxy prefs directly to the pref service.
"Data Reduction Proxy," everywhere.

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

https://codereview.chromium.org/893003002/diff/280001/chrome/browser/net/spdy...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.h:49:
net::URLRequestContextGetter* request_context,
nit: request_context_getter

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

https://codereview.chromium.org/893003002/diff/280001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:157:
if (!io_task_runner_->BelongsToCurrentThread()) {
Don't we know which thread this is coming in on? (I hate this construct.)

https://codereview.chromium.org/893003002/diff/280001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:279:
void DataReductionProxyConfig::StartProbe() {
Shouldn't we verify that this is called on the UI thread?

https://codereview.chromium.org/893003002/diff/280001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:284:
event_store_->BeginCanaryRequest(bound_net_log_, params_->probe_url());
"Canary" and "Probe" are used to mean the same thing. Please change the code to
use one or the other, but not both.

https://codereview.chromium.org/893003002/diff/280001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:285:
}
Remove curly braces

https://codereview.chromium.org/893003002/diff/280001/components/data_reducti...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h
(right):

https://codereview.chromium.org/893003002/diff/280001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:184:
base::WeakPtr<DataReductionProxyService> data_reduction_proxy_service_;
Comment on what this member is.

https://codereview.chromium.org/893003002/diff/280001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_test_utils.cc
(right):

https://codereview.chromium.org/893003002/diff/280001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_test_utils.cc:24:
: DataReductionProxyConfig(task_runner,
nit: If you move task_runner to the next line, you can fit in more stuff per
line.

https://codereview.chromium.org/893003002/diff/280001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_test_utils.cc:78:
: TestDataReductionProxyConfig(params_flags,
nit: likewise, with params_flags.

https://codereview.chromium.org/893003002/diff/280001/components/data_reducti...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h
(right):

https://codereview.chromium.org/893003002/diff/280001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h:35:
// construct information about the DNS names used by the proxy, and allowable
Is construct the right word?

https://codereview.chromium.org/893003002/diff/280001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.h
(right):

https://codereview.chromium.org/893003002/diff/280001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.h:153:
DataReductionProxyIOData* data_reduction_proxy_io_data_;
You should probably comment this must outlive this DRPNetworkDelegate. Or you
can write one comment for all of these raw pointers.

https://codereview.chromium.org/893003002/diff/280001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_service.cc
(right):

https://codereview.chromium.org/893003002/diff/280001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_service.cc:50:
const net::URLFetcher* source) {
Can this function be const?

https://codereview.chromium.org/893003002/diff/280001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_service.cc:75:
FetcherResponseCallback fetcher_callback) {
Bad indentation.

https://codereview.chromium.org/893003002/diff/280001/components/data_reducti...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_service.h
(right):

https://codereview.chromium.org/893003002/diff/280001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_service.h:37:
class DataReductionProxyService : public base::NonThreadSafe,
Class comment.

https://codereview.chromium.org/893003002/diff/280001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_service.h:49:
~DataReductionProxyService() override;
Add a blank line above.

https://codereview.chromium.org/893003002/diff/280001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_service.h:100:
}  // namespace data_reduction_proxy
Add a blank line below.

https://codereview.chromium.org/893003002/diff/280001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings_test_utils.h
(right):

https://codereview.chromium.org/893003002/diff/280001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings_test_utils.h:31:
MockDataReductionProxySettings<C>() : C() {}
Move close curly to next line.

https://codereview.chromium.org/893003002/diff/280001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings_test_utils.h:66:
bool expected_at_startup);
Add blank line below.

https://codereview.chromium.org/893003002/diff/280001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_test_utils.cc
(right):

https://codereview.chromium.org/893003002/diff/280001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_test_utils.cc:154:
return service_pointer_;
Do we need the word "pointer" ?

[jeremyim, 2015-02-20 02:17:17]

https://codereview.chromium.org/893003002/diff/280001/android_webview/browser...
File android_webview/browser/aw_browser_context.cc (right):

https://codereview.chromium.org/893003002/diff/280001/android_webview/browser...
android_webview/browser/aw_browser_context.cc:369:
DCHECK(GetDataReductionProxySettings()->data_reduction_proxy_service());
On 2015/02/20 00:00:36, bengr wrote:
> nit: You could introduce a local to hold GetDRPSettings()->d_r_p_service()

Done.

https://codereview.chromium.org/893003002/diff/280001/chrome/browser/net/spdy...
File chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.cc
(right):

https://codereview.chromium.org/893003002/diff/280001/chrome/browser/net/spdy...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.cc:36: int
flags = DataReductionProxyParams::kAllowed;
On 2015/02/20 00:00:36, bengr wrote:
> nit: Combine with lines 37 and 38 to OR three flags.

Done.

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

https://codereview.chromium.org/893003002/diff/280001/chrome/browser/net/spdy...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.cc:86:
net::URLRequestContextGetter* request_context,
On 2015/02/20 00:00:36, bengr wrote:
> request_context_getter

Done.

https://codereview.chromium.org/893003002/diff/280001/chrome/browser/net/spdy...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.cc:89: // On
mobile we write data reduction proxy prefs directly to the pref service.
On 2015/02/20 00:00:36, bengr wrote:
> "Data Reduction Proxy," everywhere.

Done.

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

https://codereview.chromium.org/893003002/diff/280001/chrome/browser/net/spdy...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.h:49:
net::URLRequestContextGetter* request_context,
On 2015/02/20 00:00:36, bengr wrote:
> nit: request_context_getter

Done.

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

https://codereview.chromium.org/893003002/diff/280001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:157:
if (!io_task_runner_->BelongsToCurrentThread()) {
On 2015/02/20 00:00:36, bengr wrote:
> Don't we know which thread this is coming in on? (I hate this construct.)

We know that it comes in on the UI thread. The alternative to this construct is
to have HandleProbeResponse post the results to HandleProbeResponseOnIOThread
which contains the rest of this method.

https://codereview.chromium.org/893003002/diff/280001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:279:
void DataReductionProxyConfig::StartProbe() {
On 2015/02/20 00:00:36, bengr wrote:
> Shouldn't we verify that this is called on the UI thread?

Done.

The check on the data_reduction_proxy_service_ on 282 would have done it anyway.

https://codereview.chromium.org/893003002/diff/280001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:284:
event_store_->BeginCanaryRequest(bound_net_log_, params_->probe_url());
On 2015/02/20 00:00:36, bengr wrote:
> "Canary" and "Probe" are used to mean the same thing. Please change the code
to
> use one or the other, but not both.

Acknowledged.

The code is already split between the two... Bug filed for making it consistent;
otherwise it would add a lot of code churn in this CL.

https://codereview.chromium.org/893003002/diff/280001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:285:
}
On 2015/02/20 00:00:36, bengr wrote:
> Remove curly braces

Done.

https://codereview.chromium.org/893003002/diff/280001/components/data_reducti...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h
(right):

https://codereview.chromium.org/893003002/diff/280001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:184:
base::WeakPtr<DataReductionProxyService> data_reduction_proxy_service_;
On 2015/02/20 00:00:36, bengr wrote:
> Comment on what this member is.

Done.

https://codereview.chromium.org/893003002/diff/280001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_test_utils.cc
(right):

https://codereview.chromium.org/893003002/diff/280001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_test_utils.cc:24:
: DataReductionProxyConfig(task_runner,
On 2015/02/20 00:00:36, bengr wrote:
> nit: If you move task_runner to the next line, you can fit in more stuff per
> line.

Done.

https://codereview.chromium.org/893003002/diff/280001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_test_utils.cc:78:
: TestDataReductionProxyConfig(params_flags,
On 2015/02/20 00:00:36, bengr wrote:
> nit: likewise, with params_flags.

Done.

https://codereview.chromium.org/893003002/diff/280001/components/data_reducti...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h
(right):

https://codereview.chromium.org/893003002/diff/280001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h:35:
// construct information about the DNS names used by the proxy, and allowable
On 2015/02/20 00:00:36, bengr wrote:
> Is construct the right word?

Done.

https://codereview.chromium.org/893003002/diff/280001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.h
(right):

https://codereview.chromium.org/893003002/diff/280001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.h:153:
DataReductionProxyIOData* data_reduction_proxy_io_data_;
On 2015/02/20 00:00:37, bengr wrote:
> You should probably comment this must outlive this DRPNetworkDelegate. Or you
> can write one comment for all of these raw pointers.

Done.

https://codereview.chromium.org/893003002/diff/280001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_service.cc
(right):

https://codereview.chromium.org/893003002/diff/280001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_service.cc:50:
const net::URLFetcher* source) {
On 2015/02/20 00:00:37, bengr wrote:
> Can this function be const?

No, because it is an override.

https://codereview.chromium.org/893003002/diff/280001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_service.cc:75:
FetcherResponseCallback fetcher_callback) {
On 2015/02/20 00:00:37, bengr wrote:
> Bad indentation.

Done.

https://codereview.chromium.org/893003002/diff/280001/components/data_reducti...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_service.h
(right):

https://codereview.chromium.org/893003002/diff/280001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_service.h:37:
class DataReductionProxyService : public base::NonThreadSafe,
On 2015/02/20 00:00:37, bengr wrote:
> Class comment.

Done.

https://codereview.chromium.org/893003002/diff/280001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_service.h:49:
~DataReductionProxyService() override;
On 2015/02/20 00:00:37, bengr wrote:
> Add a blank line above.

Done.

https://codereview.chromium.org/893003002/diff/280001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_service.h:100:
}  // namespace data_reduction_proxy
On 2015/02/20 00:00:37, bengr wrote:
> Add a blank line below.

Done.

https://codereview.chromium.org/893003002/diff/280001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings_test_utils.h
(right):

https://codereview.chromium.org/893003002/diff/280001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings_test_utils.h:31:
MockDataReductionProxySettings<C>() : C() {}
On 2015/02/20 00:00:37, bengr wrote:
> Move close curly to next line.

Done.

https://codereview.chromium.org/893003002/diff/280001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings_test_utils.h:66:
bool expected_at_startup);
On 2015/02/20 00:00:37, bengr wrote:
> Add blank line below.

Done.

https://codereview.chromium.org/893003002/diff/280001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_test_utils.cc
(right):

https://codereview.chromium.org/893003002/diff/280001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_test_utils.cc:154:
return service_pointer_;
On 2015/02/20 00:00:37, bengr wrote:
> Do we need the word "pointer" ?

Done.

[jeremyim, 2015-02-20 05:40:46]

Patchset #11 (id:300001) has been deleted

[jeremyim, 2015-02-20 17:40:39]

jeremyim@chromium.org changed reviewers:
+ tbansal@google.com

[jeremyim, 2015-02-20 17:42:47]

+tbansal for the rebase which included his QUIC work in the following files:
aw_browser_context.cc
data_reduction_proxy_chrome_io_data.*
data_reduction_proxy_chrome_settings.*
profile_impl_io_data.cc
data_reduction_proxy_io_data.*
data_reduction_proxy_settings.*

[tbansal1, 2015-02-20 18:06:32]

On 2015/02/20 17:42:47, jeremyim wrote:
> +tbansal for the rebase which included his QUIC work in the following files:
> aw_browser_context.cc
> data_reduction_proxy_chrome_io_data.*
> data_reduction_proxy_chrome_settings.*
> profile_impl_io_data.cc
> data_reduction_proxy_io_data.*
> data_reduction_proxy_settings.*

lgtm

Thanks for doing this.

[bengr, 2015-02-20 20:26:45]

lgtm, but please address the one comment.

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

https://codereview.chromium.org/893003002/diff/280001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:157:
if (!io_task_runner_->BelongsToCurrentThread()) {
On 2015/02/20 02:17:17, jeremyim wrote:
> On 2015/02/20 00:00:36, bengr wrote:
> > Don't we know which thread this is coming in on? (I hate this construct.)
> 
> We know that it comes in on the UI thread. The alternative to this construct
is
> to have HandleProbeResponse post the results to HandleProbeResponseOnIOThread
> which contains the rest of this method.

I prefer that because it is more explicit. As written, it isn't clear.

https://codereview.chromium.org/893003002/diff/280001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_service.cc
(right):

https://codereview.chromium.org/893003002/diff/280001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_service.cc:50:
const net::URLFetcher* source) {
On 2015/02/20 02:17:17, jeremyim wrote:
> On 2015/02/20 00:00:37, bengr wrote:
> > Can this function be const?
> 
> No, because it is an override.

Acknowledged.

[jeremyim, 2015-02-20 21:03:02]

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

https://codereview.chromium.org/893003002/diff/280001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:157:
if (!io_task_runner_->BelongsToCurrentThread()) {
On 2015/02/20 20:26:45, bengr wrote:
> On 2015/02/20 02:17:17, jeremyim wrote:
> > On 2015/02/20 00:00:36, bengr wrote:
> > > Don't we know which thread this is coming in on? (I hate this construct.)
> > 
> > We know that it comes in on the UI thread. The alternative to this construct
> is
> > to have HandleProbeResponse post the results to
HandleProbeResponseOnIOThread
> > which contains the rest of this method.
> 
> I prefer that because it is more explicit. As written, it isn't clear.

Done.

[sgurun-gerrit only, 2015-02-20 21:53:18]

On 2015/02/20 21:03:02, jeremyim wrote:
>
https://codereview.chromium.org/893003002/diff/280001/components/data_reducti...
> File
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc
> (right):
> 
>
https://codereview.chromium.org/893003002/diff/280001/components/data_reducti...
>
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:157:
> if (!io_task_runner_->BelongsToCurrentThread()) {
> On 2015/02/20 20:26:45, bengr wrote:
> > On 2015/02/20 02:17:17, jeremyim wrote:
> > > On 2015/02/20 00:00:36, bengr wrote:
> > > > Don't we know which thread this is coming in on? (I hate this
construct.)
> > > 
> > > We know that it comes in on the UI thread. The alternative to this
construct
> > is
> > > to have HandleProbeResponse post the results to
> HandleProbeResponseOnIOThread
> > > which contains the rest of this method.
> > 
> > I prefer that because it is more explicit. As written, it isn't clear.
> 
> Done.

[sgurun-gerrit only, 2015-02-20 21:53:18]

On 2015/02/20 21:03:02, jeremyim wrote:
>
https://codereview.chromium.org/893003002/diff/280001/components/data_reducti...
> File
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc
> (right):
> 
>
https://codereview.chromium.org/893003002/diff/280001/components/data_reducti...
>
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:157:
> if (!io_task_runner_->BelongsToCurrentThread()) {
> On 2015/02/20 20:26:45, bengr wrote:
> > On 2015/02/20 02:17:17, jeremyim wrote:
> > > On 2015/02/20 00:00:36, bengr wrote:
> > > > Don't we know which thread this is coming in on? (I hate this
construct.)
> > > 
> > > We know that it comes in on the UI thread. The alternative to this
construct
> > is
> > > to have HandleProbeResponse post the results to
> HandleProbeResponseOnIOThread
> > > which contains the rest of this method.
> > 
> > I prefer that because it is more explicit. As written, it isn't clear.
> 
> Done.

[sgurun-gerrit only, 2015-02-20 21:53:28]

https://codereview.chromium.org/893003002/diff/340001/android_webview/browser...
File android_webview/browser/aw_browser_context.cc (right):

https://codereview.chromium.org/893003002/diff/340001/android_webview/browser...
android_webview/browser/aw_browser_context.cc:41: class
DataReductionProxyConfigurator;
remove.

[jeremyim, 2015-02-20 22:03:16]

New patchsets have been uploaded after l-g-t-m from
tbansal@chromium.org,bengr@chromium.org

[jeremyim, 2015-02-20 22:04:14]

https://codereview.chromium.org/893003002/diff/340001/android_webview/browser...
File android_webview/browser/aw_browser_context.cc (right):

https://codereview.chromium.org/893003002/diff/340001/android_webview/browser...
android_webview/browser/aw_browser_context.cc:41: class
DataReductionProxyConfigurator;
On 2015/02/20 21:53:28, sgurun wrote:
> remove.

Done.

[sgurun-gerrit only, 2015-02-20 22:06:22]

On 2015/02/20 22:04:14, jeremyim wrote:
>
https://codereview.chromium.org/893003002/diff/340001/android_webview/browser...
> File android_webview/browser/aw_browser_context.cc (right):
> 
>
https://codereview.chromium.org/893003002/diff/340001/android_webview/browser...
> android_webview/browser/aw_browser_context.cc:41: class
> DataReductionProxyConfigurator;
> On 2015/02/20 21:53:28, sgurun wrote:
> > remove.
> 
> Done.

aw lgtm

[jeremyim, 2015-02-20 22:07:54]

The CQ bit was checked by jeremyim@chromium.org

[commit-bot: I haz the power, 2015-02-20 22:08:20]

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

[jeremyim, 2015-02-20 22:08:54]

Thank you everyone for your feedback and comments!

[commit-bot: I haz the power, 2015-02-20 22:49:57]

Committed patchset #13 (id:360001)

[commit-bot: I haz the power, 2015-02-20 22:50:33]

Patchset 13 (id:??) landed as
https://crrev.com/f917e431fd15598c0e28dbc9a39ac408272548a4
Cr-Commit-Position: refs/heads/master@{#317432}