Moved data reduction proxy initialization logic to ProfileImplIOData

Moved data reduction proxy initialization logic from ProfileIOData,
where it would be available to all profiles, including Incognito, to
ProfileImplIOData, where it would only be available to non-
Incognito profiles. 

In the process, (1) refactored and simplified DataReductionProxyUsageStats
so that it could live on the IO thread. (2) Added a keyed service called
DataReductionProxyChromeSettings, so chrome browsers on all platforms
could use the same pattern.

BUG=396695
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=288288

[bengr, 2014-07-25 02:08:44]

nyquist: chrome/android/*
thestig: chrome/browser/io_thread.*
marq: chrome/browser/net/spdyproxy/* and components/data_reduction_proxy*
willchan: chrome/browser/profiles/*

[bengr, 2014-07-25 20:52:16]

On 2014/07/25 02:08:44, bengr1 wrote:
> nyquist: chrome/android/*
> thestig: chrome/browser/io_thread.*
> marq: chrome/browser/net/spdyproxy/* and components/data_reduction_proxy*
> willchan: chrome/browser/profiles/*

Please hold off on reviewing until I resolve a few issues with this change.

[willchan no longer on Chromium, 2014-07-28 15:45:23]

Here's a first pass

https://codereview.chromium.org/412143009/diff/100001/chrome/browser/io_thread.h
File chrome/browser/io_thread.h (right):

https://codereview.chromium.org/412143009/diff/100001/chrome/browser/io_threa...
chrome/browser/io_thread.h:442: #if defined(SPDY_PROXY_AUTH_ORIGIN)
Why did this get moved down here? This is probably a bug. All these members are
UI thread objects. IOThread is a UI thread object that manages the IO thread.
Anything that is a member in IOThread will be destroyed on the UI thread.
IOThread::Globals is set of data that will be constructed and destroyed on the
IO thread.

Since these members were previously constructed in IOThread::InitAsync(), and
they're used by the NetworkDelegate, I suspect you want to keep them on the IO
thread rather than move them to the UI thread.

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

https://codereview.chromium.org/412143009/diff/100001/chrome/browser/net/spdy...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.cc:35:
g_browser_process->local_state(),
Can you pass this in rather than reaching into the global here? I prefer to keep
globals as close to main() as possible, rather than having lower layer code
reach into global state.

https://codereview.chromium.org/412143009/diff/100001/chrome/browser/net/spdy...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.cc:36:
ProfileManager::GetActiveUserProfile()->GetRequestContext());
Isn't this the same as the profile that's passed in? Why are you pulling it from
ProfileManager? Are they different?

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

https://codereview.chromium.org/412143009/diff/100001/chrome/browser/net/spdy...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.h:33: virtual
void InitDataReductionProxySettings(Profile* profile);
Why is this virtual? Is it overridden somewhere (I can't find it if so).

Also, can you pass in the exact parameters rather than the Profile*? I try to
scope the parameters as narrowly as possible, and Profile has a lot of
dependencies that it's best not to pull in. Lots of blundell@'s work with
componentization is undoing all these Profile* parameters that people pass
around.

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

https://codereview.chromium.org/412143009/diff/100001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl.cc:729: DCHECK(unavailable_callback);
Don't bother with the DCHECK, since the next line it'll just crash anyway.

https://codereview.chromium.org/412143009/diff/100001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl.cc:734: // Post to prevent a Profile
initialization loop.
I don't understand this comment. What's the loop?

https://codereview.chromium.org/412143009/diff/100001/chrome/browser/profiles...
File chrome/browser/profiles/profile_impl_io_data.h (right):

https://codereview.chromium.org/412143009/diff/100001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl_io_data.h:243: #if
defined(SPDY_PROXY_AUTH_ORIGIN)
I need to think further about the right layering here. All the member variables
are sensitive to destruction ordering issues, so if your destructors reach into
any other state, or the network delegate or something reaches into these members
during shutdown, we could have a bug. It looks like our ordering is starting to
get a little sloppy here =/

[bengr, 2014-07-28 20:29:04]

https://codereview.chromium.org/412143009/diff/100001/chrome/browser/io_thread.h
File chrome/browser/io_thread.h (right):

https://codereview.chromium.org/412143009/diff/100001/chrome/browser/io_threa...
chrome/browser/io_thread.h:442: #if defined(SPDY_PROXY_AUTH_ORIGIN)
On 2014/07/28 15:45:22, willchan wrote:
> Why did this get moved down here? This is probably a bug. All these members
are
> UI thread objects. IOThread is a UI thread object that manages the IO thread.
> Anything that is a member in IOThread will be destroyed on the UI thread.
> IOThread::Globals is set of data that will be constructed and destroyed on the
> IO thread.
> 
> Since these members were previously constructed in IOThread::InitAsync(), and
> they're used by the NetworkDelegate, I suspect you want to keep them on the IO
> thread rather than move them to the UI thread.

Done.

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

https://codereview.chromium.org/412143009/diff/100001/chrome/browser/net/spdy...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.cc:35:
g_browser_process->local_state(),
On 2014/07/28 15:45:22, willchan wrote:
> Can you pass this in rather than reaching into the global here? I prefer to
keep
> globals as close to main() as possible, rather than having lower layer code
> reach into global state.

Done.

https://codereview.chromium.org/412143009/diff/100001/chrome/browser/net/spdy...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.cc:36:
ProfileManager::GetActiveUserProfile()->GetRequestContext());
On 2014/07/28 15:45:22, willchan wrote:
> Isn't this the same as the profile that's passed in? Why are you pulling it
from
> ProfileManager? Are they different?

That's old cruft. Done.

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

https://codereview.chromium.org/412143009/diff/100001/chrome/browser/net/spdy...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.h:33: virtual
void InitDataReductionProxySettings(Profile* profile);
On 2014/07/28 15:45:22, willchan wrote:
> Why is this virtual? Is it overridden somewhere (I can't find it if so).
> 
> Also, can you pass in the exact parameters rather than the Profile*? I try to
> scope the parameters as narrowly as possible, and Profile has a lot of
> dependencies that it's best not to pull in. Lots of blundell@'s work with
> componentization is undoing all these Profile* parameters that people pass
> around.

Done.

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

https://codereview.chromium.org/412143009/diff/100001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl.cc:729: DCHECK(unavailable_callback);
On 2014/07/28 15:45:23, willchan wrote:
> Don't bother with the DCHECK, since the next line it'll just crash anyway.

Done.

https://codereview.chromium.org/412143009/diff/100001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl.cc:734: // Post to prevent a Profile
initialization loop.
On 2014/07/28 15:45:23, willchan wrote:
> I don't understand this comment. What's the loop?

The comment needs to change. Before changing to pass in the profile's
parameters, it did loop. Something was triggering the initialization of the
profile, which in turn was calling this stuff. With the change, Chrome crashes
if I don't post the task. It seems the RequestContext isn't set up.

https://codereview.chromium.org/412143009/diff/100001/chrome/browser/profiles...
File chrome/browser/profiles/profile_impl_io_data.h (right):

https://codereview.chromium.org/412143009/diff/100001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl_io_data.h:243: #if
defined(SPDY_PROXY_AUTH_ORIGIN)
On 2014/07/28 15:45:23, willchan wrote:
> I need to think further about the right layering here. All the member
variables
> are sensitive to destruction ordering issues, so if your destructors reach
into
> any other state, or the network delegate or something reaches into these
members
> during shutdown, we could have a bug. It looks like our ordering is starting
to
> get a little sloppy here =/

ok. Let me know what you conclude...

[willchan no longer on Chromium, 2014-07-29 00:26:57]

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

https://codereview.chromium.org/412143009/diff/100001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl.cc:734: // Post to prevent a Profile
initialization loop.
On 2014/07/28 20:29:04, bengr1 wrote:
> On 2014/07/28 15:45:23, willchan wrote:
> > I don't understand this comment. What's the loop?
> 
> The comment needs to change. Before changing to pass in the profile's
> parameters, it did loop. Something was triggering the initialization of the
> profile, which in turn was calling this stuff. With the change, Chrome crashes
> if I don't post the task. It seems the RequestContext isn't set up.

Can you paste the error?

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

https://codereview.chromium.org/412143009/diff/180001/android_webview/browser...
android_webview/browser/aw_browser_context.cc:105:
data_reduction_proxy_auth_request_handler_.reset(
Is this code running on the UI thread? It looks like the
DataReductionProxyParams is created in ProfileImplIOData in the other way. Is
there a way to align these so we don't have the DataReductionProxyFoo objects
created on different threads depending on which application we're running in?

I assume there's an IO thread equivalent of AwBrowserContext that we could stash
this in.

https://codereview.chromium.org/412143009/diff/180001/chrome/browser/io_threa...
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/412143009/diff/180001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:645: new DataReductionProxyParams(drp_flags));
Can you get rid of the using declaration above and fully specify this here? I'd
be on board with it if there were more uses, but since there only appears to be
this single one, it'll probably be easier for people to understand if they see
the namespace specified here.

https://codereview.chromium.org/412143009/diff/180001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:661: 
Can you delete this vertical whitespace? I generally don't like more than 2
blank lines since many many monitors are short on vertical space.

https://codereview.chromium.org/412143009/diff/180001/chrome/browser/net/spdy...
File
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings_factory.cc
(right):

https://codereview.chromium.org/412143009/diff/180001/chrome/browser/net/spdy...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings_factory.cc:67:
// Takes ownership of params.
You don't have to fix this, but you could get rid of this documentation and use
scoped_ptr<T>::Pass() to explicitly pass ownership instead.

https://codereview.chromium.org/412143009/diff/180001/chrome/browser/profiles...
File chrome/browser/profiles/profile_impl.h (right):

https://codereview.chromium.org/412143009/diff/180001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl.h:303: DataReductionProxyChromeSettings*
data_reduction_proxy_chrome_settings_;
The comments above say not to add any more. What do you need this for?

https://codereview.chromium.org/412143009/diff/180001/components/data_reducti...
File components/data_reduction_proxy/browser/data_reduction_proxy_params.h
(right):

https://codereview.chromium.org/412143009/diff/180001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_params.h:79:
DataReductionProxyParams(int flags);
This should be explicit.

https://codereview.chromium.org/412143009/diff/180001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_params.h:81:
DataReductionProxyParams(const DataReductionProxyParams& params);
What makes this necessary? We generally avoid copy constructors.

[bengr, 2014-07-29 21:29:53]

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

https://codereview.chromium.org/412143009/diff/180001/android_webview/browser...
android_webview/browser/aw_browser_context.cc:105:
data_reduction_proxy_auth_request_handler_.reset(
I don't see one. Maybe Selim knows?

On 2014/07/29 00:26:56, willchan wrote:
> Is this code running on the UI thread? It looks like the
> DataReductionProxyParams is created in ProfileImplIOData in the other way. Is
> there a way to align these so we don't have the DataReductionProxyFoo objects
> created on different threads depending on which application we're running in?
> 
> I assume there's an IO thread equivalent of AwBrowserContext that we could
stash
> this in.

https://codereview.chromium.org/412143009/diff/180001/chrome/browser/io_threa...
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/412143009/diff/180001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:645: new DataReductionProxyParams(drp_flags));
On 2014/07/29 00:26:56, willchan wrote:
> Can you get rid of the using declaration above and fully specify this here?
I'd
> be on board with it if there were more uses, but since there only appears to
be
> this single one, it'll probably be easier for people to understand if they see
> the namespace specified here.

Done.

https://codereview.chromium.org/412143009/diff/180001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:661: 
On 2014/07/29 00:26:56, willchan wrote:
> Can you delete this vertical whitespace? I generally don't like more than 2
> blank lines since many many monitors are short on vertical space.

Done.

https://codereview.chromium.org/412143009/diff/180001/chrome/browser/net/spdy...
File
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings_factory.cc
(right):

https://codereview.chromium.org/412143009/diff/180001/chrome/browser/net/spdy...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings_factory.cc:67:
// Takes ownership of params.
On 2014/07/29 00:26:56, willchan wrote:
> You don't have to fix this, but you could get rid of this documentation and
use
> scoped_ptr<T>::Pass() to explicitly pass ownership instead.

I won't fix this in this CL, but I tightened the code up a bit to be
a little less error prone.

https://codereview.chromium.org/412143009/diff/180001/chrome/browser/profiles...
File chrome/browser/profiles/profile_impl.h (right):

https://codereview.chromium.org/412143009/diff/180001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl.h:303: DataReductionProxyChromeSettings*
data_reduction_proxy_chrome_settings_;
Done. Removed.

On 2014/07/29 00:26:56, willchan wrote:
> The comments above say not to add any more. What do you need this for?

https://codereview.chromium.org/412143009/diff/180001/components/data_reducti...
File components/data_reduction_proxy/browser/data_reduction_proxy_params.h
(right):

https://codereview.chromium.org/412143009/diff/180001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_params.h:79:
DataReductionProxyParams(int flags);
On 2014/07/29 00:26:56, willchan wrote:
> This should be explicit.

Done.

https://codereview.chromium.org/412143009/diff/180001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_params.h:81:
DataReductionProxyParams(const DataReductionProxyParams& params);
On 2014/07/29 00:26:57, willchan wrote:
> What makes this necessary? We generally avoid copy constructors.

I can remove this, but I think it would require having DataReductionProxyParams
support WeakPtrs, passing WeakPtrs all over the place in Profile*, and passing
ownership of DRPParams object from the DataReductionProxyChromeSettings object
to ProfileImplIOData so it is still around after DRPSettings is destroyed.

A copy constructor was much simpler.

Let me know what you think I should do.

[bengr, 2014-07-31 17:48:46]

Addressed comments from willchan@.

sgurun@, please review android_webview/*

[willchan no longer on Chromium, 2014-07-31 22:31:35]

marq_: Are you planning on reviewing this? I'd like to get another eye on this,
since I don't own the data_reduction_proxy component, and I can't do as thorough
of a review on it.

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

https://codereview.chromium.org/412143009/diff/100001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl.cc:734: // Post to prevent a Profile
initialization loop.
On 2014/07/29 00:26:56, willchan wrote:
> On 2014/07/28 20:29:04, bengr1 wrote:
> > On 2014/07/28 15:45:23, willchan wrote:
> > > I don't understand this comment. What's the loop?
> > 
> > The comment needs to change. Before changing to pass in the profile's
> > parameters, it did loop. Something was triggering the initialization of the
> > profile, which in turn was calling this stuff. With the change, Chrome
crashes
> > if I don't post the task. It seems the RequestContext isn't set up.
> 
> Can you paste the error?

Ben and I chatted about this offline. The problem is that this is getting called
before the ProfileImplIOData::Handle::Init() call. This means that the
URLRequestContextGetter isn't initialized yet. The simple solution is to move it
after the Init() call.

https://codereview.chromium.org/412143009/diff/180001/components/data_reducti...
File components/data_reduction_proxy/browser/data_reduction_proxy_params.h
(right):

https://codereview.chromium.org/412143009/diff/180001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_params.h:81:
DataReductionProxyParams(const DataReductionProxyParams& params);
On 2014/07/29 21:29:52, bengr1 wrote:
> On 2014/07/29 00:26:57, willchan wrote:
> > What makes this necessary? We generally avoid copy constructors.
> 
> I can remove this, but I think it would require having
DataReductionProxyParams
> support WeakPtrs, passing WeakPtrs all over the place in Profile*, and passing
> ownership of DRPParams object from the DataReductionProxyChromeSettings object
> to ProfileImplIOData so it is still around after DRPSettings is destroyed.
> 
> A copy constructor was much simpler.
> 
> Let me know what you think I should do.

We chatted offline about this. The style guide prohibits copy constructors in
general, and suggests using a Clone() method instead. But long-term, this code
maybe should get refactored to just be a struct, since it's just packaging data
anyway.

https://codereview.chromium.org/412143009/diff/240001/android_webview/browser...
File android_webview/browser/net/aw_url_request_context_getter.cc (right):

https://codereview.chromium.org/412143009/diff/240001/android_webview/browser...
android_webview/browser/net/aw_url_request_context_getter.cc:199:
data_reduction_proxy_config_service_.release());
This should get fixed. AFAICT, this is what happens:

(a) AwURLRequestContextGetter gets created on the UI thread, which creates
DataReductionProxyConfigService, which lives on the IO thread.
(b) Right afterward, AwBrowserContext extracts the
DataReductionProxyConfigService* on the UI thread via
AwURLRequestContextGetter::GetDataReductionProxyConfigService(), and it passes
the object to DataReductionProxyConfigTracker, which lives on the UI thread.
(c) When AwURLRequestContextGetter initializes on the IO thread, it'll release
the DataReductionProxyConfigService member variable into the
URLRequestContextBuilder.

There are two problems:
(1) AwURLRequestContextGetter::GetDataReductionProxyConfigService() now returns
NULL. It's only ever safe to call at one particular point in the initialization.
This is very fragile, because there's no way for a reader to understand this at
first glance.
(2) DataReductionProxyConfigTracker is a UI thread object with a raw pointer
(DataReductionProxyConfigService*) to an IO thread object. And there's no thread
safety checking.

Here are my proposed fixes:
(i) Create the DataReductionProxyConfigService in
AwBrowserContext::PreMainMessageLoopRun() and pass it in via the constructor to
AwURLRequestContextGetter. This solves problem (1), since now you don't need the
getter just to expose it back to AwBrowserContext::PreMainMessageLoopRun().
(ii) DataReductionProxyConfigTracker shouldn't take a
DataReductionProxyConfigService*. Instead, it should take a base::Callback<bool,
const net::ProxyConfig&>. AwURLRequestContextGetter::PreMainMessageLoopRun()
should pass in base::Bind(&DataReductionProxyConfigService::UpdateProxyConfig,
base::Unretained(data_reduction_proxy_config_service)) to
DataReductionProxyConfigTracker. This reduces the scope, and loosens the
coupling between DataReductionProxyConfigTracker and
DataReductionProxyConfigService, and improves testability since you don't need a
full DataReductionProxyConfigService now for testing.
(iii) For extra safety, you could wrap the callback passed to
DataReductionProxyConfigService inside another callback that does
DCHECK(content::BrowserThread::GetMessageLoopProxyForThread(content::BrowserThread::IO)->BelongsToCurrentThread()));
This would make sure it always runs on the correct thread.

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

https://codereview.chromium.org/412143009/diff/240001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl.cc:623: &data_reduction_proxy_unavailable);
indentation off

https://codereview.chromium.org/412143009/diff/240001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl.cc:742: void
ProfileImpl::InitDataReductionProxyForIO(
There's no reason for this to be a member function. Remove it from the header
and put this in an anonymous namespace here in the .cc file.

https://codereview.chromium.org/412143009/diff/240001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl.cc:755: void
ProfileImpl::InitDataReductionProxySettings() {
Ditto, this doesn't need to be a member function.

https://codereview.chromium.org/412143009/diff/240001/components/data_reducti...
File
components/data_reduction_proxy/browser/data_reduction_proxy_auth_request_handler.h
(right):

https://codereview.chromium.org/412143009/diff/240001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_auth_request_handler.h:42:
base::MessageLoopProxy* io_thread_proxy);
Please use SingleThreadTaskRunner instead. MLP is deprecated.

https://codereview.chromium.org/412143009/diff/240001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_auth_request_handler.h:59:
// the standard behavior. Must be called on the UI Thread.
Since this object primarily lives on the IO thread, please name this function
SetKeyOnUI(). And add OnUI() for all member functions that are supposed to be on
the UI thread.

https://codereview.chromium.org/412143009/diff/240001/components/data_reducti...
File
components/data_reduction_proxy/browser/data_reduction_proxy_auth_request_handler_unittest.cc
(right):

https://codereview.chromium.org/412143009/diff/240001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_auth_request_handler_unittest.cc:93:
base::MessageLoop::current()->RunUntilIdle();
Use this instead:
https://code.google.com/p/chromium/codesearch#chromium/src/base/run_loop.h

https://codereview.chromium.org/412143009/diff/240001/components/data_reducti...
File components/data_reduction_proxy/browser/data_reduction_proxy_params.h
(right):

https://codereview.chromium.org/412143009/diff/240001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_params.h:86:
DataReductionProxyParams* Clone();
Return scoped_ptr<DataReductionProxyParams>. This self-documents that it's
transferring ownership.

https://codereview.chromium.org/412143009/diff/240001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_params.h:258: bool
configured_on_command_line_;
Please declare a private operator=. I don't think you need to define it since it
should never be called. But I think if you don't do this, the compiler will
generate a public one.

[sgurun-gerrit only, 2014-07-31 23:11:55]

https://codereview.chromium.org/412143009/diff/240001/android_webview/browser...
File android_webview/browser/net/aw_url_request_context_getter.cc (right):

https://codereview.chromium.org/412143009/diff/240001/android_webview/browser...
android_webview/browser/net/aw_url_request_context_getter.cc:199:
data_reduction_proxy_config_service_.release());
agreed with Will.  (thanks for doing such a detailed review!)

On 2014/07/31 22:31:34, willchan wrote:
> This should get fixed. AFAICT, this is what happens:
> 
> (a) AwURLRequestContextGetter gets created on the UI thread, which creates
> DataReductionProxyConfigService, which lives on the IO thread.
> (b) Right afterward, AwBrowserContext extracts the
> DataReductionProxyConfigService* on the UI thread via
> AwURLRequestContextGetter::GetDataReductionProxyConfigService(), and it passes
> the object to DataReductionProxyConfigTracker, which lives on the UI thread.
> (c) When AwURLRequestContextGetter initializes on the IO thread, it'll release
> the DataReductionProxyConfigService member variable into the
> URLRequestContextBuilder.
> 
> There are two problems:
> (1) AwURLRequestContextGetter::GetDataReductionProxyConfigService() now
returns
> NULL. It's only ever safe to call at one particular point in the
initialization.
> This is very fragile, because there's no way for a reader to understand this
at
> first glance.
> (2) DataReductionProxyConfigTracker is a UI thread object with a raw pointer
> (DataReductionProxyConfigService*) to an IO thread object. And there's no
thread
> safety checking.
> 
> Here are my proposed fixes:
> (i) Create the DataReductionProxyConfigService in
> AwBrowserContext::PreMainMessageLoopRun() and pass it in via the constructor
to
> AwURLRequestContextGetter. This solves problem (1), since now you don't need
the
> getter just to expose it back to AwBrowserContext::PreMainMessageLoopRun().
> (ii) DataReductionProxyConfigTracker shouldn't take a
> DataReductionProxyConfigService*. Instead, it should take a
base::Callback<bool,
> const net::ProxyConfig&>. AwURLRequestContextGetter::PreMainMessageLoopRun()
> should pass in base::Bind(&DataReductionProxyConfigService::UpdateProxyConfig,
> base::Unretained(data_reduction_proxy_config_service)) to
> DataReductionProxyConfigTracker. This reduces the scope, and loosens the
> coupling between DataReductionProxyConfigTracker and
> DataReductionProxyConfigService, and improves testability since you don't need
a
> full DataReductionProxyConfigService now for testing.
> (iii) For extra safety, you could wrap the callback passed to
> DataReductionProxyConfigService inside another callback that does
>
DCHECK(content::BrowserThread::GetMessageLoopProxyForThread(content::BrowserThread::IO)->BelongsToCurrentThread()));
> This would make sure it always runs on the correct thread.

[marq (ping after 24h), 2014-07-31 23:14:34]

On 2014/07/31 22:31:35, willchan wrote:
> marq_: Are you planning on reviewing this? I'd like to get another eye on
this,
> since I don't own the data_reduction_proxy component, and I can't do as
thorough
> of a review on it.

Looking at it now.

[willchan no longer on Chromium, 2014-07-31 23:17:58]

Sorry, didn't mean it in a "please look at this now". Just more of a, I
want to make sure someone responsible than me will eventually take a look :)

Please enjoy beer o'clock.


On Thu, Jul 31, 2014 at 4:14 PM, <marq@chromium.org> wrote:

> On 2014/07/31 22:31:35, willchan wrote:
>
>> marq_: Are you planning on reviewing this? I'd like to get another eye on
>>
> this,
>
>> since I don't own the data_reduction_proxy component, and I can't do as
>>
> thorough
>
>> of a review on it.
>>
>
> Looking at it now.
>
>
>
> https://codereview.chromium.org/412143009/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[marq (ping after 24h), 2014-07-31 23:54:33]

https://codereview.chromium.org/412143009/diff/240001/chrome/browser/io_threa...
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/412143009/diff/240001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:648: globals_->data_reduction_proxy_params.reset(
Why aren't the holdback and promoAllowed param flags set here, too?

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

https://codereview.chromium.org/412143009/diff/240001/chrome/browser/net/spdy...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.h:42: // get
the appropriate pref service.
Update the method comment for the new arguments.

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

https://codereview.chromium.org/412143009/diff/240001/chrome/browser/net/spdy...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings_factory.cc:63:
flags |= DataReductionProxyParams::kPromoAllowed;
The building-flags-from-params code is duplicated (somewhat) in io_thread.cc,
and (aside from code duplication being bad), it's not clear why there's more
than one place where this can happen. 

In fact, looking closer at this, why do we do this at all? This is calling a
bunch of static methods in DataReductionProxyParams and then using the results
of those calls to ... create a DataReductionProxyParams object? Why doesn't the
DataReductionProxyParams constructor just call its own static methods?

More generally, with io_thread setting up a global params object, when should
this method ever be called?

https://codereview.chromium.org/412143009/diff/240001/chrome/browser/net/spdy...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings_factory.cc:67:
// Takes ownership of params.
Comment doesn't really make sense now.

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

https://codereview.chromium.org/412143009/diff/240001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl.cc:742: void
ProfileImpl::InitDataReductionProxyForIO(
Nit: This doesn't actually initialize the data reduction proxy; it just
initializes some variables that get passed in to the io_data init. If this
weren't (I'm assuming) factored out to avoid a large #if block, it would
naturally just be inline code before the .Init call.

https://codereview.chromium.org/412143009/diff/240001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl.cc:748:
params->reset(data_reduction_proxy_chrome_settings->params()->Clone());
Params is cloned so there's a copy for the IO thread and a copy for the UI
thread, right? But isn't the params object immutable?

https://codereview.chromium.org/412143009/diff/240001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl.cc:758:
DataReductionProxyChromeSettingsFactory::GetForBrowserContext(this)->
Right, so this will use the params created in
DataReductionProxyChromeSettingsFactory::BuildServiceInstanceFor(). So what are
the global params used for?

https://codereview.chromium.org/412143009/diff/240001/components/data_reducti...
File
components/data_reduction_proxy/browser/data_reduction_proxy_auth_request_handler.h
(right):

https://codereview.chromium.org/412143009/diff/240001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_auth_request_handler.h:90:
void InitAuthenticationOnIOThread(
Symmetrically with Will's comments, if this is primary an IO-thread-dwelling
object, OnIO(Thread) can be assumed on any method that doesn't say otherwise, so
this can just be InitAuthentication().

[bengr, 2014-08-02 01:10:32]

https://codereview.chromium.org/412143009/diff/240001/android_webview/browser...
File android_webview/browser/net/aw_url_request_context_getter.cc (right):

https://codereview.chromium.org/412143009/diff/240001/android_webview/browser...
android_webview/browser/net/aw_url_request_context_getter.cc:199:
data_reduction_proxy_config_service_.release());
Thanks for going into so much detail. I didn't do iii. Where would I put the
definition of the callback that does the IO thread check? The config service is
in a component that doesn't depend on content.

On 2014/07/31 22:31:34, willchan wrote:
> This should get fixed. AFAICT, this is what happens:
> 
> (a) AwURLRequestContextGetter gets created on the UI thread, which creates
> DataReductionProxyConfigService, which lives on the IO thread.
> (b) Right afterward, AwBrowserContext extracts the
> DataReductionProxyConfigService* on the UI thread via
> AwURLRequestContextGetter::GetDataReductionProxyConfigService(), and it passes
> the object to DataReductionProxyConfigTracker, which lives on the UI thread.
> (c) When AwURLRequestContextGetter initializes on the IO thread, it'll release
> the DataReductionProxyConfigService member variable into the
> URLRequestContextBuilder.
> 
> There are two problems:
> (1) AwURLRequestContextGetter::GetDataReductionProxyConfigService() now
returns
> NULL. It's only ever safe to call at one particular point in the
initialization.
> This is very fragile, because there's no way for a reader to understand this
at
> first glance.
> (2) DataReductionProxyConfigTracker is a UI thread object with a raw pointer
> (DataReductionProxyConfigService*) to an IO thread object. And there's no
thread
> safety checking.
> 
> Here are my proposed fixes:
> (i) Create the DataReductionProxyConfigService in
> AwBrowserContext::PreMainMessageLoopRun() and pass it in via the constructor
to
> AwURLRequestContextGetter. This solves problem (1), since now you don't need
the
> getter just to expose it back to AwBrowserContext::PreMainMessageLoopRun().
> (ii) DataReductionProxyConfigTracker shouldn't take a
> DataReductionProxyConfigService*. Instead, it should take a
base::Callback<bool,
> const net::ProxyConfig&>. AwURLRequestContextGetter::PreMainMessageLoopRun()
> should pass in base::Bind(&DataReductionProxyConfigService::UpdateProxyConfig,
> base::Unretained(data_reduction_proxy_config_service)) to
> DataReductionProxyConfigTracker. This reduces the scope, and loosens the
> coupling between DataReductionProxyConfigTracker and
> DataReductionProxyConfigService, and improves testability since you don't need
a
> full DataReductionProxyConfigService now for testing.
> (iii) For extra safety, you could wrap the callback passed to
> DataReductionProxyConfigService inside another callback that does
>
DCHECK(content::BrowserThread::GetMessageLoopProxyForThread(content::BrowserThread::IO)->BelongsToCurrentThread()));
> This would make sure it always runs on the correct thread.

https://codereview.chromium.org/412143009/diff/240001/chrome/browser/io_threa...
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/412143009/diff/240001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:648: globals_->data_reduction_proxy_params.reset(
On 2014/07/31 23:54:32, marq_ wrote:
> Why aren't the holdback and promoAllowed param flags set here, too?

Done.

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

https://codereview.chromium.org/412143009/diff/240001/chrome/browser/net/spdy...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.h:42: // get
the appropriate pref service.
On 2014/07/31 23:54:32, marq_ wrote:
> Update the method comment for the new arguments.

Done.

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

https://codereview.chromium.org/412143009/diff/240001/chrome/browser/net/spdy...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings_factory.cc:63:
flags |= DataReductionProxyParams::kPromoAllowed;
On 2014/07/31 23:54:32, marq_ wrote:
> The building-flags-from-params code is duplicated (somewhat) in io_thread.cc,
> and (aside from code duplication being bad), it's not clear why there's more
> than one place where this can happen. 
> 
> In fact, looking closer at this, why do we do this at all? This is calling a
> bunch of static methods in DataReductionProxyParams and then using the results
> of those calls to ... create a DataReductionProxyParams object? Why doesn't
the
> DataReductionProxyParams constructor just call its own static methods?
> 
> More generally, with io_thread setting up a global params object, when should
> this method ever be called?

This should be used by each profile. The global setting in IOThread is for the
system context.

https://codereview.chromium.org/412143009/diff/240001/chrome/browser/net/spdy...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings_factory.cc:67:
// Takes ownership of params.
On 2014/07/31 23:54:32, marq_ wrote:
> Comment doesn't really make sense now.

Done.

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

https://codereview.chromium.org/412143009/diff/240001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl.cc:623: &data_reduction_proxy_unavailable);
On 2014/07/31 22:31:35, willchan wrote:
> indentation off

Done.

https://codereview.chromium.org/412143009/diff/240001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl.cc:742: void
ProfileImpl::InitDataReductionProxyForIO(
On 2014/07/31 23:54:32, marq_ wrote:
> Nit: This doesn't actually initialize the data reduction proxy; it just
> initializes some variables that get passed in to the io_data init. If this
> weren't (I'm assuming) factored out to avoid a large #if block, it would
> naturally just be inline code before the .Init call.

Done.

https://codereview.chromium.org/412143009/diff/240001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl.cc:742: void
ProfileImpl::InitDataReductionProxyForIO(
On 2014/07/31 22:31:34, willchan wrote:
> There's no reason for this to be a member function. Remove it from the header
> and put this in an anonymous namespace here in the .cc file.

Done.

https://codereview.chromium.org/412143009/diff/240001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl.cc:748:
params->reset(data_reduction_proxy_chrome_settings->params()->Clone());
On 2014/07/31 23:54:32, marq_ wrote:
> Params is cloned so there's a copy for the IO thread and a copy for the UI
> thread, right? But isn't the params object immutable?

The issue is lifetime. The profile's IO data outlives the keyed service.

https://codereview.chromium.org/412143009/diff/240001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl.cc:755: void
ProfileImpl::InitDataReductionProxySettings() {
On 2014/07/31 22:31:34, willchan wrote:
> Ditto, this doesn't need to be a member function.

Done.

https://codereview.chromium.org/412143009/diff/240001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl.cc:758:
DataReductionProxyChromeSettingsFactory::GetForBrowserContext(this)->
On 2014/07/31 23:54:32, marq_ wrote:
> Right, so this will use the params created in
> DataReductionProxyChromeSettingsFactory::BuildServiceInstanceFor(). So what
are
> the global params used for?

The global params are for the system context.

https://codereview.chromium.org/412143009/diff/240001/components/data_reducti...
File
components/data_reduction_proxy/browser/data_reduction_proxy_auth_request_handler.h
(right):

https://codereview.chromium.org/412143009/diff/240001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_auth_request_handler.h:42:
base::MessageLoopProxy* io_thread_proxy);
On 2014/07/31 22:31:35, willchan wrote:
> Please use SingleThreadTaskRunner instead. MLP is deprecated.

Done.

https://codereview.chromium.org/412143009/diff/240001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_auth_request_handler.h:59:
// the standard behavior. Must be called on the UI Thread.
On 2014/07/31 22:31:35, willchan wrote:
> Since this object primarily lives on the IO thread, please name this function
> SetKeyOnUI(). And add OnUI() for all member functions that are supposed to be
on
> the UI thread.

Done.

https://codereview.chromium.org/412143009/diff/240001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_auth_request_handler.h:90:
void InitAuthenticationOnIOThread(
On 2014/07/31 23:54:32, marq_ wrote:
> Symmetrically with Will's comments, if this is primary an IO-thread-dwelling
> object, OnIO(Thread) can be assumed on any method that doesn't say otherwise,
so
> this can just be InitAuthentication().

Done.

https://codereview.chromium.org/412143009/diff/240001/components/data_reducti...
File
components/data_reduction_proxy/browser/data_reduction_proxy_auth_request_handler_unittest.cc
(right):

https://codereview.chromium.org/412143009/diff/240001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_auth_request_handler_unittest.cc:93:
base::MessageLoop::current()->RunUntilIdle();
On 2014/07/31 22:31:35, willchan wrote:
> Use this instead:
> https://code.google.com/p/chromium/codesearch#chromium/src/base/run_loop.h

Done.

https://codereview.chromium.org/412143009/diff/240001/components/data_reducti...
File components/data_reduction_proxy/browser/data_reduction_proxy_params.h
(right):

https://codereview.chromium.org/412143009/diff/240001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_params.h:86:
DataReductionProxyParams* Clone();
On 2014/07/31 22:31:35, willchan wrote:
> Return scoped_ptr<DataReductionProxyParams>. This self-documents that it's
> transferring ownership.

Done.

https://codereview.chromium.org/412143009/diff/240001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_params.h:258: bool
configured_on_command_line_;
On 2014/07/31 22:31:35, willchan wrote:
> Please declare a private operator=. I don't think you need to define it since
it
> should never be called. But I think if you don't do this, the compiler will
> generate a public one.

Done.

[willchan no longer on Chromium, 2014-08-04 23:13:46]

I've reviewed the profiles and chrome/browser/net parts pretty thoroughly, but
not all the details in components/. Let me know if I need to review that, but
I'm getting a bit swamped right now.

I'm basically done here though if not.

https://codereview.chromium.org/412143009/diff/260001/chrome/browser/io_threa...
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/412143009/diff/260001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:642:
data_reduction_proxy::DataReductionProxyParams::kFallbackAllowed);
Is this an intentional behavior change? Specifically, kFallbackAllowed is now
not set by default. It looks like most of this change is a refactoring. In
general, it's best not to combine refactoring and behavior changes in the same
CL.

https://codereview.chromium.org/412143009/diff/260001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:645: IsIncludedInAlternativeFieldTrial())
Nit: Now that this is multi-line, please add braces.

https://codereview.chromium.org/412143009/diff/260001/chrome/browser/io_thread.h
File chrome/browser/io_thread.h (right):

https://codereview.chromium.org/412143009/diff/260001/chrome/browser/io_threa...
chrome/browser/io_thread.h:203: 
Any reason for this space? Most other places don't have that.

https://codereview.chromium.org/412143009/diff/260001/components/data_reducti...
File components/data_reduction_proxy/browser/data_reduction_proxy_params.cc
(right):

https://codereview.chromium.org/412143009/diff/260001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:488:
DataReductionProxyParams& DataReductionProxyParams::operator=(
What happens if you don't define this? Does anything invoke this? Do you get a
link error? If it gives you a link error, then it's fine to keep this. But if
there's no link error, please delete this.

[marq (ping after 24h), 2014-08-05 00:03:42]

LGTM, although the duplication of the Params object still bugs me.

[sgurun-gerrit only, 2014-08-05 01:26:36]

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

https://codereview.chromium.org/412143009/diff/260001/android_webview/browser...
android_webview/browser/aw_browser_context.cc:107:
scoped_ptr<data_reduction_proxy::DataReductionProxyConfigService>
nit: import name from data_reduction_proxy namespace to prevent repeating
data_reduction_proxy::

https://codereview.chromium.org/412143009/diff/260001/android_webview/browser...
android_webview/browser/aw_browser_context.cc:114: NULL /* Ignored on Android
*/)).Pass()));
8 line statement is not very readable. you can import
CreateSystemProxyConfigService to make it slightly less clunky.

https://codereview.chromium.org/412143009/diff/260001/android_webview/browser...
android_webview/browser/aw_browser_context.cc:117:
base::Bind(&data_reduction_proxy::DataReductionProxyConfigService::
drop data_reduction_proxy after addressing the comment above.

https://codereview.chromium.org/412143009/diff/260001/android_webview/browser...
File android_webview/browser/net/aw_url_request_context_getter.cc (right):

https://codereview.chromium.org/412143009/diff/260001/android_webview/browser...
android_webview/browser/net/aw_url_request_context_getter.cc:229: if
(data_reduction_proxy_settings) {
is it an error if data_reduction_proxy_settings does not exist here? why do we
want to silently drop it?

https://codereview.chromium.org/412143009/diff/260001/android_webview/browser...
File android_webview/browser/net/aw_url_request_context_getter.h (right):

https://codereview.chromium.org/412143009/diff/260001/android_webview/browser...
android_webview/browser/net/aw_url_request_context_getter.h:40:
scoped_ptr<data_reduction_proxy::DataReductionProxyConfigService>
use using to reduce clunkiness in names while reading them

https://codereview.chromium.org/412143009/diff/260001/android_webview/native/...
File android_webview/native/aw_contents_statics.cc (right):

https://codereview.chromium.org/412143009/diff/260001/android_webview/native/...
android_webview/native/aw_contents_statics.cc:68:
data_reduction_proxy::kProtocolVersion);
log an error if auth_request_handler does not exist. this can help to debug why
drp is not working/

[bengr, 2014-08-05 02:35:07]

thestig and nyquist, PTAL

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

https://codereview.chromium.org/412143009/diff/260001/android_webview/browser...
android_webview/browser/aw_browser_context.cc:107:
scoped_ptr<data_reduction_proxy::DataReductionProxyConfigService>
On 2014/08/05 01:26:34, sgurun wrote:
> nit: import name from data_reduction_proxy namespace to prevent repeating
> data_reduction_proxy::

Done.

https://codereview.chromium.org/412143009/diff/260001/android_webview/browser...
android_webview/browser/aw_browser_context.cc:114: NULL /* Ignored on Android
*/)).Pass()));
On 2014/08/05 01:26:33, sgurun wrote:
> 8 line statement is not very readable. you can import
> CreateSystemProxyConfigService to make it slightly less clunky.

That's a static method not a class so it would only save the net::.

https://codereview.chromium.org/412143009/diff/260001/android_webview/browser...
android_webview/browser/aw_browser_context.cc:117:
base::Bind(&data_reduction_proxy::DataReductionProxyConfigService::
On 2014/08/05 01:26:33, sgurun wrote:
> drop data_reduction_proxy after addressing the comment above.

Done.

https://codereview.chromium.org/412143009/diff/260001/android_webview/browser...
File android_webview/browser/net/aw_url_request_context_getter.cc (right):

https://codereview.chromium.org/412143009/diff/260001/android_webview/browser...
android_webview/browser/net/aw_url_request_context_getter.cc:229: if
(data_reduction_proxy_settings) {
On 2014/08/05 01:26:35, sgurun wrote:
> is it an error if data_reduction_proxy_settings does not exist here? why do we
> want to silently drop it?

You are right. Done.

https://codereview.chromium.org/412143009/diff/260001/android_webview/browser...
File android_webview/browser/net/aw_url_request_context_getter.h (right):

https://codereview.chromium.org/412143009/diff/260001/android_webview/browser...
android_webview/browser/net/aw_url_request_context_getter.h:40:
scoped_ptr<data_reduction_proxy::DataReductionProxyConfigService>
On 2014/08/05 01:26:35, sgurun wrote:
> use using to reduce clunkiness in names while reading them

People are split on whether "using" is helpful, particularly in headers. It
removes context. I'm leaving as is.

https://codereview.chromium.org/412143009/diff/260001/android_webview/native/...
File android_webview/native/aw_contents_statics.cc (right):

https://codereview.chromium.org/412143009/diff/260001/android_webview/native/...
android_webview/native/aw_contents_statics.cc:68:
data_reduction_proxy::kProtocolVersion);
On 2014/08/05 01:26:36, sgurun wrote:
> log an error if auth_request_handler does not exist. this can help to debug
why
> drp is not working/

Done.

https://codereview.chromium.org/412143009/diff/260001/chrome/browser/io_threa...
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/412143009/diff/260001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:642:
data_reduction_proxy::DataReductionProxyParams::kFallbackAllowed);
On 2014/08/04 23:13:46, willchan wrote:
> Is this an intentional behavior change? Specifically, kFallbackAllowed is now
> not set by default. It looks like most of this change is a refactoring. In
> general, it's best not to combine refactoring and behavior changes in the same
> CL.

This is intentional. I believe this change is first introduced in a change that
landed after I last rebased. I'll confirm. If not, are you ok with me landing it
in this CL?

https://codereview.chromium.org/412143009/diff/260001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:645: IsIncludedInAlternativeFieldTrial())
On 2014/08/04 23:13:46, willchan wrote:
> Nit: Now that this is multi-line, please add braces.

Done.

https://codereview.chromium.org/412143009/diff/260001/chrome/browser/io_thread.h
File chrome/browser/io_thread.h (right):

https://codereview.chromium.org/412143009/diff/260001/chrome/browser/io_threa...
chrome/browser/io_thread.h:203: 
On 2014/08/04 23:13:46, willchan wrote:
> Any reason for this space? Most other places don't have that.

Done.

https://codereview.chromium.org/412143009/diff/260001/components/data_reducti...
File components/data_reduction_proxy/browser/data_reduction_proxy_params.cc
(right):

https://codereview.chromium.org/412143009/diff/260001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:488:
DataReductionProxyParams& DataReductionProxyParams::operator=(
On 2014/08/04 23:13:46, willchan wrote:
> What happens if you don't define this? Does anything invoke this? Do you get a
> link error? If it gives you a link error, then it's fine to keep this. But if
> there's no link error, please delete this.

No link error. Interesting. Done.

[Lei Zhang, 2014-08-05 02:52:23]

I'm delegating c/b/io_thread* OWNERship to some networking folks. [1] So taking
myself off this CL for now. If you need me to review any other part of chrome/,
feel free to add me back.

[1] https://codereview.chromium.org/439883002/

[willchan no longer on Chromium, 2014-08-05 03:03:52]

LGTM

https://codereview.chromium.org/412143009/diff/260001/android_webview/browser...
File android_webview/browser/net/aw_url_request_context_getter.h (right):

https://codereview.chromium.org/412143009/diff/260001/android_webview/browser...
android_webview/browser/net/aw_url_request_context_getter.h:40:
scoped_ptr<data_reduction_proxy::DataReductionProxyConfigService>
On 2014/08/05 02:35:07, bengr1 wrote:
> On 2014/08/05 01:26:35, sgurun wrote:
> > use using to reduce clunkiness in names while reading them
> 
> People are split on whether "using" is helpful, particularly in headers. It
> removes context. I'm leaving as is.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Namesp...
covers this. It's disallowed in header files unless scoped within functions,
methods or classes. And only using-declarations are allowed to bring specific
symbols in. You can't bring in the entire namespace via a using-directive.

https://codereview.chromium.org/412143009/diff/260001/chrome/browser/io_threa...
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/412143009/diff/260001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:642:
data_reduction_proxy::DataReductionProxyParams::kFallbackAllowed);
On 2014/08/05 02:35:07, bengr1 wrote:
> On 2014/08/04 23:13:46, willchan wrote:
> > Is this an intentional behavior change? Specifically, kFallbackAllowed is
now
> > not set by default. It looks like most of this change is a refactoring. In
> > general, it's best not to combine refactoring and behavior changes in the
same
> > CL.
> 
> This is intentional. I believe this change is first introduced in a change
that
> landed after I last rebased. I'll confirm. If not, are you ok with me landing
it
> in this CL?

Oh, if it's from a rebase, that's fine. If not, I think you should probably make
that change separately. If you pull that change out to a separate CL, I'm happy
to approve that one quickly (just ping me on IM).

https://codereview.chromium.org/412143009/diff/260001/components/data_reducti...
File components/data_reduction_proxy/browser/data_reduction_proxy_params.cc
(right):

https://codereview.chromium.org/412143009/diff/260001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:488:
DataReductionProxyParams& DataReductionProxyParams::operator=(
On 2014/08/05 02:35:07, bengr1 wrote:
> On 2014/08/04 23:13:46, willchan wrote:
> > What happens if you don't define this? Does anything invoke this? Do you get
a
> > link error? If it gives you a link error, then it's fine to keep this. But
if
> > there's no link error, please delete this.
> 
> No link error. Interesting. Done.

DISALLOW_COPY_AND_ASSIGN() just declares the copy constructor and operator=. It
never provides a definition, because no one ever calls it. Declarations don't
need definitions, unless it's actually used.

[willchan no longer on Chromium, 2014-08-05 03:04:06]

My LGTM also covers c/b/io_thread*

[chromium-reviews, 2014-08-05 04:35:24]

Aw lgtm
On Aug 4, 2014 8:03 PM, <willchan@chromium.org> wrote:

> LGTM
>
>
> https://codereview.chromium.org/412143009/diff/260001/
> android_webview/browser/net/aw_url_request_context_getter.h
> File android_webview/browser/net/aw_url_request_context_getter.h
> (right):
>
> https://codereview.chromium.org/412143009/diff/260001/
> android_webview/browser/net/aw_url_request_context_getter.h#newcode40
> android_webview/browser/net/aw_url_request_context_getter.h:40:
> scoped_ptr<data_reduction_proxy::DataReductionProxyConfigService>
> On 2014/08/05 02:35:07, bengr1 wrote:
>
>> On 2014/08/05 01:26:35, sgurun wrote:
>> > use using to reduce clunkiness in names while reading them
>>
>
>  People are split on whether "using" is helpful, particularly in
>>
> headers. It
>
>> removes context. I'm leaving as is.
>>
>
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=
> Namespaces#Namespaces
> covers this. It's disallowed in header files unless scoped within
> functions, methods or classes. And only using-declarations are allowed
> to bring specific symbols in. You can't bring in the entire namespace
> via a using-directive.
>
> https://codereview.chromium.org/412143009/diff/260001/
> chrome/browser/io_thread.cc
> File chrome/browser/io_thread.cc (right):
>
> https://codereview.chromium.org/412143009/diff/260001/
> chrome/browser/io_thread.cc#newcode642
> chrome/browser/io_thread.cc:642:
> data_reduction_proxy::DataReductionProxyParams::kFallbackAllowed);
> On 2014/08/05 02:35:07, bengr1 wrote:
>
>> On 2014/08/04 23:13:46, willchan wrote:
>> > Is this an intentional behavior change? Specifically,
>>
> kFallbackAllowed is now
>
>> > not set by default. It looks like most of this change is a
>>
> refactoring. In
>
>> > general, it's best not to combine refactoring and behavior changes
>>
> in the same
>
>> > CL.
>>
>
>  This is intentional. I believe this change is first introduced in a
>>
> change that
>
>> landed after I last rebased. I'll confirm. If not, are you ok with me
>>
> landing it
>
>> in this CL?
>>
>
> Oh, if it's from a rebase, that's fine. If not, I think you should
> probably make that change separately. If you pull that change out to a
> separate CL, I'm happy to approve that one quickly (just ping me on IM).
>
> https://codereview.chromium.org/412143009/diff/260001/
> components/data_reduction_proxy/browser/data_reduction_proxy_params.cc
> File
> components/data_reduction_proxy/browser/data_reduction_proxy_params.cc
> (right):
>
> https://codereview.chromium.org/412143009/diff/260001/
> components/data_reduction_proxy/browser/data_reduction_
> proxy_params.cc#newcode488
> components/data_reduction_proxy/browser/data_reduction_
> proxy_params.cc:488:
> DataReductionProxyParams& DataReductionProxyParams::operator=(
> On 2014/08/05 02:35:07, bengr1 wrote:
>
>> On 2014/08/04 23:13:46, willchan wrote:
>> > What happens if you don't define this? Does anything invoke this? Do
>>
> you get a
>
>> > link error? If it gives you a link error, then it's fine to keep
>>
> this. But if
>
>> > there's no link error, please delete this.
>>
>
>  No link error. Interesting. Done.
>>
>
> DISALLOW_COPY_AND_ASSIGN() just declares the copy constructor and
> operator=. It never provides a definition, because no one ever calls it.
> Declarations don't need definitions, unless it's actually used.
>
> https://codereview.chromium.org/412143009/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sgurun-gerrit only, 2014-08-05 17:11:26]

On 2014/08/05 04:35:24, chromium-reviews wrote:
> Aw lgtm
> On Aug 4, 2014 8:03 PM, <mailto:willchan@chromium.org> wrote:
> 
> > LGTM
> >
> >
> > https://codereview.chromium.org/412143009/diff/260001/
> > android_webview/browser/net/aw_url_request_context_getter.h
> > File android_webview/browser/net/aw_url_request_context_getter.h
> > (right):
> >
> > https://codereview.chromium.org/412143009/diff/260001/
> > android_webview/browser/net/aw_url_request_context_getter.h#newcode40
> > android_webview/browser/net/aw_url_request_context_getter.h:40:
> > scoped_ptr<data_reduction_proxy::DataReductionProxyConfigService>
> > On 2014/08/05 02:35:07, bengr1 wrote:
> >
> >> On 2014/08/05 01:26:35, sgurun wrote:
> >> > use using to reduce clunkiness in names while reading them
> >>
> >
> >  People are split on whether "using" is helpful, particularly in
> >>
> > headers. It
> >
> >> removes context. I'm leaving as is.
> >>
> >
> > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=
> > Namespaces#Namespaces
> > covers this. It's disallowed in header files unless scoped within
> > functions, methods or classes. And only using-declarations are allowed
> > to bring specific symbols in. You can't bring in the entire namespace
> > via a using-directive.
> >
> > https://codereview.chromium.org/412143009/diff/260001/
> > chrome/browser/io_thread.cc
> > File chrome/browser/io_thread.cc (right):
> >
> > https://codereview.chromium.org/412143009/diff/260001/
> > chrome/browser/io_thread.cc#newcode642
> > chrome/browser/io_thread.cc:642:
> > data_reduction_proxy::DataReductionProxyParams::kFallbackAllowed);
> > On 2014/08/05 02:35:07, bengr1 wrote:
> >
> >> On 2014/08/04 23:13:46, willchan wrote:
> >> > Is this an intentional behavior change? Specifically,
> >>
> > kFallbackAllowed is now
> >
> >> > not set by default. It looks like most of this change is a
> >>
> > refactoring. In
> >
> >> > general, it's best not to combine refactoring and behavior changes
> >>
> > in the same
> >
> >> > CL.
> >>
> >
> >  This is intentional. I believe this change is first introduced in a
> >>
> > change that
> >
> >> landed after I last rebased. I'll confirm. If not, are you ok with me
> >>
> > landing it
> >
> >> in this CL?
> >>
> >
> > Oh, if it's from a rebase, that's fine. If not, I think you should
> > probably make that change separately. If you pull that change out to a
> > separate CL, I'm happy to approve that one quickly (just ping me on IM).
> >
> > https://codereview.chromium.org/412143009/diff/260001/
> > components/data_reduction_proxy/browser/data_reduction_proxy_params.cc
> > File
> > components/data_reduction_proxy/browser/data_reduction_proxy_params.cc
> > (right):
> >
> > https://codereview.chromium.org/412143009/diff/260001/
> > components/data_reduction_proxy/browser/data_reduction_
> > proxy_params.cc#newcode488
> > components/data_reduction_proxy/browser/data_reduction_
> > proxy_params.cc:488:
> > DataReductionProxyParams& DataReductionProxyParams::operator=(
> > On 2014/08/05 02:35:07, bengr1 wrote:
> >
> >> On 2014/08/04 23:13:46, willchan wrote:
> >> > What happens if you don't define this? Does anything invoke this? Do
> >>
> > you get a
> >
> >> > link error? If it gives you a link error, then it's fine to keep
> >>
> > this. But if
> >
> >> > there's no link error, please delete this.
> >>
> >
> >  No link error. Interesting. Done.
> >>
> >
> > DISALLOW_COPY_AND_ASSIGN() just declares the copy constructor and
> > operator=. It never provides a definition, because no one ever calls it.
> > Declarations don't need definitions, unless it's actually used.
> >
> > https://codereview.chromium.org/412143009/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

aw lgtm

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

The CQ bit was checked by bengr@chromium.org

[commit-bot: I haz the power, 2014-08-06 01:15:56]

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

[bengr, 2014-08-06 01:34:15]

The CQ bit was unchecked by bengr@chromium.org

[sgurun-gerrit only, 2014-08-06 21:46:36]

On 2014/08/06 01:34:15, bengr1 wrote:
> The CQ bit was unchecked by mailto:bengr@chromium.org

looking at the crash: the check in proxy_service.cc:908 fails, 


// static
ProxyService* ProxyService::CreateUsingSystemProxyResolver(
    ProxyConfigService* proxy_config_service,
    size_t num_pac_threads,
    NetLog* net_log) {
  DCHECK(proxy_config_service);


this means data_reduction_proxy_config_service_ is null, in
aw_url_request_context_getter
 #if defined(SPDY_PROXY_AUTH_ORIGIN)
 196   if (data_reduction_proxy_config_service_.get()) {
 197     builder.set_proxy_config_service(
 198         data_reduction_proxy_config_service_.release());
 199   }
 200 #endif


is SPDY_PROXY_AUTH_ORIGIN defined for android_Webview? If it is not defined, and
if undefined configuration is not meaningful (fails all tests) then do we need
to maintain it? it is quite confusing to me.

[sgurun-gerrit only, 2014-08-07 00:48:11]

https://codereview.chromium.org/412143009/diff/340001/android_webview/browser...
File android_webview/browser/net/aw_url_request_context_getter.cc (right):

https://codereview.chromium.org/412143009/diff/340001/android_webview/browser...
android_webview/browser/net/aw_url_request_context_getter.cc:228: #if
defined(SPDY_PROXY_AUTH_ORIGIN)
do you still want to keep this?

[bengr, 2014-08-07 00:49:38]

https://codereview.chromium.org/412143009/diff/340001/android_webview/browser...
File android_webview/browser/net/aw_url_request_context_getter.cc (right):

https://codereview.chromium.org/412143009/diff/340001/android_webview/browser...
android_webview/browser/net/aw_url_request_context_getter.cc:228: #if
defined(SPDY_PROXY_AUTH_ORIGIN)
On 2014/08/07 00:48:11, sgurun wrote:
> do you still want to keep this?

For now yes.

[sgurun-gerrit only, 2014-08-07 00:51:12]

aw still lgtm

[bengr, 2014-08-07 01:52:35]

The CQ bit was checked by bengr@chromium.org

[bengr, 2014-08-07 01:52:48]

The CQ bit was unchecked by bengr@chromium.org

[nyquist, 2014-08-07 04:19:24]

looks like the PrefMemberBase::VerifyValuePrefName DCHECK is failing on Android.

[nyquist, 2014-08-07 17:01:52]

Sorry for being unclear in my previous comment.

This is the check failure: 
Check failed: !pref_name_.empty().


 0031b9a9  subtle::PrefMemberBase::VerifyValuePrefName() const+52               
                                                                                
                                                        libgcc2.c:0
  006efaed  subtle::PrefMemberBase::VerifyPref() const+4                        
                                                                                
                                                         libgcc2.c:0
  0031ba2d  PrefMember<bool>::GetValue() const+16                               
                                                                                
                                                         libgcc2.c:0
  003d377f  ProfileIOData::IsDataReductionProxyEnabled() const+18               
                                                                                
                                                         libgcc2.c:0
  003d9fe5 
ChromeResourceDispatcherHostDelegate::AppendStandardResourceThrottles(net::URLRequest*,
content::ResourceContext*, content::ResourceType,
ScopedVector<content::ResourceThrottle>*)+36                         libgcc2.c:0
  003da3ff 
ChromeResourceDispatcherHostDelegate::RequestBeginning(net::URLRequest*,
content::ResourceContext*, content::AppCacheService*, content::ResourceType,
int, int, ScopedVector<content::ResourceThrottle>*)+326  libgcc2.c:0
  017db6f9 
content::ResourceDispatcherHostImpl::CreateResourceHandler(net::URLRequest*,
ResourceHostMsg_Request const&, IPC::Message*, int, int, int,
content::ResourceContext*)+440                                      libgcc2.c:0
  017df31f  content::ResourceDispatcherHostImpl::BeginRequest(int,
ResourceHostMsg_Request const&, IPC::Message*, int)+1626                        
                                                                      
libgcc2.c:0
  017df427  content::ResourceDispatcherHostImpl::OnRequestResource(int, int,
ResourceHostMsg_Request const&)+10                                              
                                                             libgcc2.c:0
  017dbd29  content::ResourceDispatcherHostImpl::OnMessageReceived(IPC::Message
const&, content::ResourceMessageFilter*)+220                                    
                                                          libgcc2.c:0
  016f4751 
content::BrowserMessageFilter::Internal::DispatchMessage(IPC::Message const&)+28
                                                                                
                                             libgcc2.c:0
  016f48cf 
content::BrowserMessageFilter::Internal::OnMessageReceived(IPC::Message
const&)+290                                                                     
                                                      libgcc2.c:0
  0070f55b  IPC::(anonymous
namespace)::TryFiltersImpl(std::vector<IPC::MessageFilter*,
std::allocator<IPC::MessageFilter*> >&, IPC::Message const&)+30                 
                                                  libgcc2.c:0
  0070f7f3  IPC::MessageFilterRouter::TryFilters(IPC::Message const&)+94        
                                                                                
                                                         libgcc2.c:0
  00708c91  IPC::ChannelProxy::Context::TryFilters(IPC::Message const&)+56      
                                                                                
                                                         libgcc2.c:0
  00708d5f  IPC::ChannelProxy::Context::OnMessageReceived(IPC::Message const&)+6
                                                                                
                                                         libgcc2.c:0
  0070a509  IPC::internal::ChannelReader::DispatchInputData(char const*,
int)+496                                                                        
                                                                 libgcc2.c:0
  0070a617  IPC::internal::ChannelReader::ProcessIncomingMessages()+98          
                                                                                
                                                         libgcc2.c:0
  007076f7  IPC::ChannelPosix::OnFileCanReadWithoutBlocking(int)+486            
                                                                                
                                                         libgcc2.c:0
  00221e05 
base::MessagePumpLibevent::FileDescriptorWatcher::OnFileCanReadWithoutBlocking(int,
base::MessagePumpLibevent*)+26                                                  
                                          libgcc2.c:0
  00221f45  base::MessagePumpLibevent::OnLibeventNotification(int, short,
void*)+192                                                                      
                                                                libgcc2.c:0
  002b08e1  event_base_loop+608                                                 
                                                                                
                                                         libgcc2.c:0
  002212ef  base::MessagePumpLibevent::Run(base::MessagePump::Delegate*)+254    
                                                                                
                                                         libgcc2.c:0
  0024ec27  base::MessageLoop::RunHandler()+82                                  
                                                                                
                                                         libgcc2.c:0
  0025bd9d  base::RunLoop::Run()+12                                             
                                 


See the stdio of 'Run stack tool with logcat dump' step for details:
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tri...

[bengr, 2014-08-08 02:02:54]

The CQ bit was checked by bengr@chromium.org

[commit-bot: I haz the power, 2014-08-08 02:05:38]

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

[commit-bot: I haz the power, 2014-08-08 06:55:31]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-08 09:50:53]

Change committed as 288288