Add data reduction functionality to all platforms.

Add data reduction functionality to all platforms.

* Remove all #if defined guards from data reduction proxy functionality.
* Hard code proxy urls in params rather than providing them in gyp files.
* Add DataCompressionProxyRollout field trial to all platforms.
* Pass in bypass duration into UpdateRetryInfoOnFallback rather than computing
it based on SPDY_PROXY_AUTH_ORIGIN. Remove unused UMAs.

BUG=384394, 384397
Committed: https://crrev.com/88ca41196a5d2db1554d3848af0fe5038873a937
Cr-Commit-Position: refs/heads/master@{#296122}

[bengr, 2014-07-11 20:42:54]

https://codereview.chromium.org/382313003/diff/1/chrome/browser/android/inter...
File chrome/browser/android/intercept_download_resource_throttle.cc (left):

https://codereview.chromium.org/382313003/diff/1/chrome/browser/android/inter...
chrome/browser/android/intercept_download_resource_throttle.cc:51: #if
defined(SPDY_PROXY_AUTH_ORIGIN)
Currently, Android apps built from solely the chromium code will not have
authentication credentials. If you remove these #ifdefs, then you must also not
configure the data reduction proxy if the key is not set.

https://codereview.chromium.org/382313003/diff/1/chrome/browser/io_thread.cc
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/382313003/diff/1/chrome/browser/io_thread.cc#...
chrome/browser/io_thread.cc:623: if
(DataReductionProxyParams::IsIncludedInFieldTrial())
All of this stuff should be part of a data reduction proxy initialization API
that's used by IOThread and ProfileIOData. This API should be compatible with
Android Webview's use.

https://codereview.chromium.org/382313003/diff/1/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/382313003/diff/1/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_io_data.cc:519: #if defined(OS_ANDROID)
I think we need an API for initializing the proxy and everything between this
#if and the #endif should be in one function of that API.

Also, followup CLs should move Android and iOS to do what desktop does. Add a
TODO.

https://codereview.chromium.org/382313003/diff/1/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_io_data.cc:545:
proxySettings->SetDataReductionProxyEnabled(true);
Obviously, this must be removed and hooked to UI.

https://codereview.chromium.org/382313003/diff/1/components/data_reduction_pr...
File components/data_reduction_proxy/browser/data_reduction_proxy_metrics.cc
(right):

https://codereview.chromium.org/382313003/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/browser/data_reduction_proxy_metrics.cc:299:
DataReductionProxyParams params(
These params should be the same params that are passed to create the DRPSettings
object. They should not be a copy.

https://codereview.chromium.org/382313003/diff/1/components/data_reduction_pr...
File components/data_reduction_proxy/browser/data_reduction_proxy_params.cc
(left):

https://codereview.chromium.org/382313003/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:334:
return SPDY_PROXY_AUTH_ORIGIN;
There are lots of checks and tests for cases where these values are not defined.
That logic should all be removed.

https://codereview.chromium.org/382313003/diff/1/net/http/http_network_transa...
File net/http/http_network_transaction.cc (left):

https://codereview.chromium.org/382313003/diff/1/net/http/http_network_transa...
net/http/http_network_transaction.cc:66: #include <algorithm>
is this not needed?

https://codereview.chromium.org/382313003/diff/1/net/proxy/proxy_list.cc
File net/proxy/proxy_list.cc (left):

https://codereview.chromium.org/382313003/diff/1/net/proxy/proxy_list.cc#oldc...
net/proxy/proxy_list.cc:209: #if defined(SPDY_PROXY_AUTH_ORIGIN)
I would suggest keeping line 215 and removing the randomization logic here. Have
the DRP-related caller do the randomization and pass it into |retry_delay|.

https://codereview.chromium.org/382313003/diff/1/net/proxy/proxy_server.cc
File net/proxy/proxy_server.cc (left):

https://codereview.chromium.org/382313003/diff/1/net/proxy/proxy_server.cc#ol...
net/proxy/proxy_server.cc:228:
HostPortPair::FromURL(GURL(DATA_REDUCTION_DEV_HOST)));
We should deprecate and remove ProxyServer::isDataReductionProxy()

https://codereview.chromium.org/382313003/diff/1/net/proxy/proxy_service.cc
File net/proxy/proxy_service.cc (left):

https://codereview.chromium.org/382313003/diff/1/net/proxy/proxy_service.cc#o...
net/proxy/proxy_service.cc:1211: if
(result->proxy_server().isDataReductionProxy()) {
We should embed this logic into ProxyService instead of it being part of
ProxyService.

https://codereview.chromium.org/382313003/diff/1/net/socket/ssl_client_socket...
File net/socket/ssl_client_socket_pool.cc (left):

https://codereview.chromium.org/382313003/diff/1/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_pool.cc:377: UMA_HISTOGRAM_CUSTOM_TIMES(
I don't think we need this UMA anymore. Ask around.

https://codereview.chromium.org/382313003/diff/1/net/spdy/spdy_session.cc
File net/spdy/spdy_session.cc (left):

https://codereview.chromium.org/382313003/diff/1/net/spdy/spdy_session.cc#old...
net/spdy/spdy_session.cc:679:
UMA_HISTOGRAM_BOOLEAN("Net.SpdySessions_DataReductionProxy",
I don't think we need this UMA anymore. Ask around.

[Not at Google. Contact bengr, 2014-07-15 01:45:14]

The CQ bit was checked by kundaji@chromium.org

[Not at Google. Contact bengr, 2014-07-15 01:45:27]

The CQ bit was unchecked by kundaji@chromium.org

[Not at Google. Contact bengr, 2014-07-15 01:45:53]

The CQ bit was checked by kundaji@chromium.org

[Not at Google. Contact bengr, 2014-07-15 01:46:21]

The CQ bit was unchecked by kundaji@chromium.org

[bengr, 2014-07-15 16:10:42]

On 2014/07/15 01:46:21, kundaji wrote:
> The CQ bit was unchecked by mailto:kundaji@chromium.org

The win_blink_dbg failure is likely due to this change.

[bengr, 2014-07-15 16:38:17]

https://codereview.chromium.org/382313003/diff/40001/chrome/browser/io_thread.cc
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/382313003/diff/40001/chrome/browser/io_thread...
chrome/browser/io_thread.cc:623: if
(DataReductionProxyParams::IsIncludedInFieldTrial())
Can you gate the instantiation of everything related to the data reduction proxy
on this?

https://codereview.chromium.org/382313003/diff/40001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/382313003/diff/40001/chrome/browser/profiles/...
chrome/browser/profiles/profile_io_data.cc:531: DataReductionProxySettings*
proxySettings =
proxy_settings

https://codereview.chromium.org/382313003/diff/40001/chrome/browser/profiles/...
chrome/browser/profiles/profile_io_data.cc:533:
proxySettings->SetDataReductionProxyUsageStats(usage_stats);
Who owns proxy_settings? This is a memory leak.

https://codereview.chromium.org/382313003/diff/40001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_io_data.h (right):

https://codereview.chromium.org/382313003/diff/40001/chrome/browser/profiles/...
chrome/browser/profiles/profile_io_data.h:336: void
SetupDataReductionProxyOnUIThread(Profile* profile,
Why the name change? Also, please add a comment describing what this function
does. While you're here, please add comments to all data reduction proxy
functions.

https://codereview.chromium.org/382313003/diff/40001/components/data_reductio...
File components/data_reduction_proxy/browser/data_reduction_proxy_params.cc
(right):

https://codereview.chromium.org/382313003/diff/40001/components/data_reductio...
components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:325:
return "http://proxy-dev.googlezip.net:80/";
Add a TODO that this class and its tests should be cleaned up. E.g., we no
longer needed to check if these names are defined.

https://codereview.chromium.org/382313003/diff/40001/components/data_reductio...
File components/data_reduction_proxy/common/data_reduction_proxy_headers.cc
(right):

https://codereview.chromium.org/382313003/diff/40001/components/data_reductio...
components/data_reduction_proxy/common/data_reduction_proxy_headers.cc:55:
proxy_info->bypass_duration = TimeDelta::FromMilliseconds(
This won't work. Leave this one as is and move this randomization code to:
https://code.google.com/p/chromium/codesearch#chromium/src/components/data_re...

https://codereview.chromium.org/382313003/diff/40001/net/proxy/proxy_list.cc
File net/proxy/proxy_list.cc (right):

https://codereview.chromium.org/382313003/diff/40001/net/proxy/proxy_list.cc#...
net/proxy/proxy_list.cc:207: DCHECK(retry_delay != base::TimeDelta());
I think this is wrong too. 

I think there are three cases and you'll have to update the code accordingly:

(1) If the data reduction proxy sent a response that results in bypass,
retry_delay will be non-zero, and this function should use that delay.

(2) If a network error results in a bypass and the proxy being bypassed is the
data reduction proxy. retry_delay should be a time between one and five minutes.


(3) If a network error results in a bypass and the proxy being bypassed is not
the data reduction proxy. retry_delay should be five minutes. 

For (2) and (3) the values of retry_delay should be passed into this method, I
suppose. For (1), the values must be passed in.

https://codereview.chromium.org/382313003/diff/40001/net/proxy/proxy_server.h
File net/proxy/proxy_server.h (left):

https://codereview.chromium.org/382313003/diff/40001/net/proxy/proxy_server.h...
net/proxy/proxy_server.h:161: bool isDataReductionProxy() const;
Add a TODO and file a bug to deprecate these. They should not be used.

https://codereview.chromium.org/382313003/diff/40001/net/proxy/proxy_service.cc
File net/proxy/proxy_service.cc (left):

https://codereview.chromium.org/382313003/diff/40001/net/proxy/proxy_service....
net/proxy/proxy_service.cc:1211: if
(result->proxy_server().isDataReductionProxy()) {
Pass in the name of the proxy server at ProxyService construction and get rid of
this function.

https://codereview.chromium.org/382313003/diff/40001/net/proxy/proxy_service....
net/proxy/proxy_service.cc:1216: } else if
(result->proxy_server().isDataReductionProxyFallback()) {
Pass in the name of the proxy server at ProxyService construction and get rid of
this function.

[Not at Google. Contact bengr, 2014-07-16 22:41:35]

Created a spreadsheet to track issues:
https://docs.google.com/a/google.com/spreadsheets/d/13ab655HByKJjh427gsD3TIYd...

https://codereview.chromium.org/382313003/diff/1/chrome/browser/android/inter...
File chrome/browser/android/intercept_download_resource_throttle.cc (left):

https://codereview.chromium.org/382313003/diff/1/chrome/browser/android/inter...
chrome/browser/android/intercept_download_resource_throttle.cc:51: #if
defined(SPDY_PROXY_AUTH_ORIGIN)
On 2014/07/11 20:42:53, bengr1 wrote:
> Currently, Android apps built from solely the chromium code will not have
> authentication credentials. If you remove these #ifdefs, then you must also
not
> configure the data reduction proxy if the key is not set.

Discussed in person. Let's do the configuration regardless of the key. We could
remove the UI which allows DRP to be switched on if key is absent.

https://codereview.chromium.org/382313003/diff/1/chrome/browser/io_thread.cc
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/382313003/diff/1/chrome/browser/io_thread.cc#...
chrome/browser/io_thread.cc:623: if
(DataReductionProxyParams::IsIncludedInFieldTrial())
On 2014/07/11 20:42:53, bengr1 wrote:
> All of this stuff should be part of a data reduction proxy initialization API
> that's used by IOThread and ProfileIOData. This API should be compatible with
> Android Webview's use.

We are creating instances of 3 different classes here: DataReductionProxyParam,
DataReductionProxyAuthRequestHandler, and DataReductionProxyUsageStats. They
should probably be bundled within a single class. Will leave this as is for now
to keep this cl small.

https://codereview.chromium.org/382313003/diff/1/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/382313003/diff/1/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_io_data.cc:519: #if defined(OS_ANDROID)
On 2014/07/11 20:42:53, bengr1 wrote:
> I think we need an API for initializing the proxy and everything between this
> #if and the #endif should be in one function of that API.
> 
> Also, followup CLs should move Android and iOS to do what desktop does. Add a
> TODO.

Done.

https://codereview.chromium.org/382313003/diff/1/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_io_data.cc:545:
proxySettings->SetDataReductionProxyEnabled(true);
On 2014/07/11 20:42:53, bengr1 wrote:
> Obviously, this must be removed and hooked to UI.

Removed

https://codereview.chromium.org/382313003/diff/1/components/data_reduction_pr...
File components/data_reduction_proxy/browser/data_reduction_proxy_metrics.cc
(right):

https://codereview.chromium.org/382313003/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/browser/data_reduction_proxy_metrics.cc:299:
DataReductionProxyParams params(
On 2014/07/11 20:42:53, bengr1 wrote:
> These params should be the same params that are passed to create the
DRPSettings
> object. They should not be a copy.

Add a TODO and an entry in the sheet.

https://codereview.chromium.org/382313003/diff/1/components/data_reduction_pr...
File components/data_reduction_proxy/browser/data_reduction_proxy_params.cc
(left):

https://codereview.chromium.org/382313003/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:334:
return SPDY_PROXY_AUTH_ORIGIN;
On 2014/07/11 20:42:53, bengr1 wrote:
> There are lots of checks and tests for cases where these values are not
defined.
> That logic should all be removed.

I think I have removed it all.

https://codereview.chromium.org/382313003/diff/1/net/http/http_network_transa...
File net/http/http_network_transaction.cc (left):

https://codereview.chromium.org/382313003/diff/1/net/http/http_network_transa...
net/http/http_network_transaction.cc:66: #include <algorithm>
On 2014/07/11 20:42:53, bengr1 wrote:
> is this not needed?

Not sure what it is used for. I am able to build and run fine without it.

https://codereview.chromium.org/382313003/diff/1/net/proxy/proxy_list.cc
File net/proxy/proxy_list.cc (left):

https://codereview.chromium.org/382313003/diff/1/net/proxy/proxy_list.cc#oldc...
net/proxy/proxy_list.cc:209: #if defined(SPDY_PROXY_AUTH_ORIGIN)
On 2014/07/11 20:42:53, bengr1 wrote:
> I would suggest keeping line 215 and removing the randomization logic here.
Have
> the DRP-related caller do the randomization and pass it into |retry_delay|.

Done.

https://codereview.chromium.org/382313003/diff/40001/chrome/browser/io_thread.cc
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/382313003/diff/40001/chrome/browser/io_thread...
chrome/browser/io_thread.cc:623: if
(DataReductionProxyParams::IsIncludedInFieldTrial())
On 2014/07/15 16:38:16, bengr1 wrote:
> Can you gate the instantiation of everything related to the data reduction
proxy
> on this?
Added TODO and an entry in spreadsheet.

https://codereview.chromium.org/382313003/diff/40001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/382313003/diff/40001/chrome/browser/profiles/...
chrome/browser/profiles/profile_io_data.cc:531: DataReductionProxySettings*
proxySettings =
On 2014/07/15 16:38:16, bengr1 wrote:
> proxy_settings

Done.

https://codereview.chromium.org/382313003/diff/40001/chrome/browser/profiles/...
chrome/browser/profiles/profile_io_data.cc:533:
proxySettings->SetDataReductionProxyUsageStats(usage_stats);
On 2014/07/15 16:38:16, bengr1 wrote:
> Who owns proxy_settings? This is a memory leak.

Assigned to scoped_ptr.

https://codereview.chromium.org/382313003/diff/40001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_io_data.h (right):

https://codereview.chromium.org/382313003/diff/40001/chrome/browser/profiles/...
chrome/browser/profiles/profile_io_data.h:336: void
SetupDataReductionProxyOnUIThread(Profile* profile,
On 2014/07/15 16:38:16, bengr1 wrote:
> Why the name change? Also, please add a comment describing what this function
> does. While you're here, please add comments to all data reduction proxy
> functions.

Added comments.

Method now sets params as well as usage_stats.

https://codereview.chromium.org/382313003/diff/40001/components/data_reductio...
File components/data_reduction_proxy/browser/data_reduction_proxy_params.cc
(right):

https://codereview.chromium.org/382313003/diff/40001/components/data_reductio...
components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:325:
return "http://proxy-dev.googlezip.net:80/";
On 2014/07/15 16:38:17, bengr1 wrote:
> Add a TODO that this class and its tests should be cleaned up. E.g., we no
> longer needed to check if these names are defined.

Added todo and an entry in sheet.

https://codereview.chromium.org/382313003/diff/40001/components/data_reductio...
File components/data_reduction_proxy/common/data_reduction_proxy_headers.cc
(right):

https://codereview.chromium.org/382313003/diff/40001/components/data_reductio...
components/data_reduction_proxy/common/data_reduction_proxy_headers.cc:55:
proxy_info->bypass_duration = TimeDelta::FromMilliseconds(
On 2014/07/15 16:38:17, bengr1 wrote:
> This won't work. Leave this one as is and move this randomization code to:
>
https://code.google.com/p/chromium/codesearch#chromium/src/components/data_re...
> 

Done.

https://codereview.chromium.org/382313003/diff/40001/net/proxy/proxy_list.cc
File net/proxy/proxy_list.cc (right):

https://codereview.chromium.org/382313003/diff/40001/net/proxy/proxy_list.cc#...
net/proxy/proxy_list.cc:207: DCHECK(retry_delay != base::TimeDelta());
On 2014/07/15 16:38:17, bengr1 wrote:
> I think this is wrong too. 
> 
> I think there are three cases and you'll have to update the code accordingly:
> 
> (1) If the data reduction proxy sent a response that results in bypass,
> retry_delay will be non-zero, and this function should use that delay.
> 
> (2) If a network error results in a bypass and the proxy being bypassed is the
> data reduction proxy. retry_delay should be a time between one and five
minutes.
> 
> 
> (3) If a network error results in a bypass and the proxy being bypassed is not
> the data reduction proxy. retry_delay should be five minutes. 
> 
> For (2) and (3) the values of retry_delay should be passed into this method, I
> suppose. For (1), the values must be passed in.

Fixed now.

https://codereview.chromium.org/382313003/diff/40001/net/proxy/proxy_server.h
File net/proxy/proxy_server.h (left):

https://codereview.chromium.org/382313003/diff/40001/net/proxy/proxy_server.h...
net/proxy/proxy_server.h:161: bool isDataReductionProxy() const;
On 2014/07/15 16:38:17, bengr1 wrote:
> Add a TODO and file a bug to deprecate these. They should not be used.

Removed these methods.

https://codereview.chromium.org/382313003/diff/40001/net/proxy/proxy_service.cc
File net/proxy/proxy_service.cc (left):

https://codereview.chromium.org/382313003/diff/40001/net/proxy/proxy_service....
net/proxy/proxy_service.cc:1211: if
(result->proxy_server().isDataReductionProxy()) {
On 2014/07/15 16:38:17, bengr1 wrote:
> Pass in the name of the proxy server at ProxyService construction and get rid
of
> this function.

Done.

https://codereview.chromium.org/382313003/diff/40001/net/proxy/proxy_service....
net/proxy/proxy_service.cc:1216: } else if
(result->proxy_server().isDataReductionProxyFallback()) {
On 2014/07/15 16:38:17, bengr1 wrote:
> Pass in the name of the proxy server at ProxyService construction and get rid
of
> this function.

Done.

[bengr, 2014-07-17 00:11:08]

https://codereview.chromium.org/382313003/diff/120001/chrome/browser/chrome_b...
File chrome/browser/chrome_browser_field_trials.cc (right):

https://codereview.chromium.org/382313003/diff/120001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_field_trials.cc:36: // Base function used by all
data reduction proxy field trials. A trial is
We might be able to remove all of this.

https://codereview.chromium.org/382313003/diff/120001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_field_trials.cc:52: // Experiment enabled until
Jan 1, 2015. By default, disabled.
We should change the expiration date to something much farther in the future.

https://codereview.chromium.org/382313003/diff/120001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_field_trials.cc:70: // Governs the rollout of the
compression proxy for Chrome on mobile
all platforms, right?

https://codereview.chromium.org/382313003/diff/120001/chrome/common/variation...
File chrome/common/variations/uniformity_field_trials.cc (right):

https://codereview.chromium.org/382313003/diff/120001/chrome/common/variation...
chrome/common/variations/uniformity_field_trials.cc:12: #include
"chrome/common/chrome_version_info.h"
Why is this needed?

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

https://codereview.chromium.org/382313003/diff/120001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:244: if
(fallback_allowed()) {
no need for these curlies.

https://codereview.chromium.org/382313003/diff/120001/components/data_reducti...
File components/data_reduction_proxy/browser/data_reduction_proxy_protocol.cc
(right):

https://codereview.chromium.org/382313003/diff/120001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_protocol.cc:60: 
remove blank line.

https://codereview.chromium.org/382313003/diff/120001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_protocol.cc:98: 
remove blank line

https://codereview.chromium.org/382313003/diff/120001/components/data_reducti...
File components/data_reduction_proxy/common/data_reduction_proxy_headers.cc
(right):

https://codereview.chromium.org/382313003/diff/120001/components/data_reducti...
components/data_reduction_proxy/common/data_reduction_proxy_headers.cc:172: void
SetRandBypassDuration(DataReductionProxyInfo* data_reduction_proxy_info) {
This can be in an anonymous namespace.

https://codereview.chromium.org/382313003/diff/120001/components/data_reducti...
File components/data_reduction_proxy/common/data_reduction_proxy_headers.h
(right):

https://codereview.chromium.org/382313003/diff/120001/components/data_reducti...
components/data_reduction_proxy/common/data_reduction_proxy_headers.h:54: bool
ParseHeadersAndSetProxyInfoBypassDuration(
I'd save function renaming for another CL.

https://codereview.chromium.org/382313003/diff/120001/components/data_reducti...
components/data_reduction_proxy/common/data_reduction_proxy_headers.h:59: // Set
a random bypass duration between 1 and 5 minutes.
Sets

[Not at Google. Contact bengr, 2014-07-17 18:10:22]

https://codereview.chromium.org/382313003/diff/120001/chrome/browser/chrome_b...
File chrome/browser/chrome_browser_field_trials.cc (right):

https://codereview.chromium.org/382313003/diff/120001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_field_trials.cc:36: // Base function used by all
data reduction proxy field trials. A trial is
On 2014/07/17 00:11:07, bengr1 wrote:
> We might be able to remove all of this.

Discussed in person. Will figure out rollout procedure.

https://codereview.chromium.org/382313003/diff/120001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_field_trials.cc:52: // Experiment enabled until
Jan 1, 2015. By default, disabled.
On 2014/07/17 00:11:07, bengr1 wrote:
> We should change the expiration date to something much farther in the future.

Changed to 2020.

https://codereview.chromium.org/382313003/diff/120001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_field_trials.cc:70: // Governs the rollout of the
compression proxy for Chrome on mobile
On 2014/07/17 00:11:07, bengr1 wrote:
> all platforms, right?

Done.

https://codereview.chromium.org/382313003/diff/120001/chrome/common/variation...
File chrome/common/variations/uniformity_field_trials.cc (right):

https://codereview.chromium.org/382313003/diff/120001/chrome/common/variation...
chrome/common/variations/uniformity_field_trials.cc:12: #include
"chrome/common/chrome_version_info.h"
On 2014/07/17 00:11:08, bengr1 wrote:
> Why is this needed?

Done.

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

https://codereview.chromium.org/382313003/diff/120001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:244: if
(fallback_allowed()) {
On 2014/07/17 00:11:08, bengr1 wrote:
> no need for these curlies.

Done.

https://codereview.chromium.org/382313003/diff/120001/components/data_reducti...
File components/data_reduction_proxy/browser/data_reduction_proxy_protocol.cc
(right):

https://codereview.chromium.org/382313003/diff/120001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_protocol.cc:60: 
On 2014/07/17 00:11:08, bengr1 wrote:
> remove blank line.

Done.

https://codereview.chromium.org/382313003/diff/120001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_protocol.cc:98: 
On 2014/07/17 00:11:08, bengr1 wrote:
> remove blank line

Done.

https://codereview.chromium.org/382313003/diff/120001/components/data_reducti...
File components/data_reduction_proxy/common/data_reduction_proxy_headers.cc
(right):

https://codereview.chromium.org/382313003/diff/120001/components/data_reducti...
components/data_reduction_proxy/common/data_reduction_proxy_headers.cc:172: void
SetRandBypassDuration(DataReductionProxyInfo* data_reduction_proxy_info) {
On 2014/07/17 00:11:08, bengr1 wrote:
> This can be in an anonymous namespace.

Done. Removed declaration from .h.

https://codereview.chromium.org/382313003/diff/120001/components/data_reducti...
File components/data_reduction_proxy/common/data_reduction_proxy_headers.h
(right):

https://codereview.chromium.org/382313003/diff/120001/components/data_reducti...
components/data_reduction_proxy/common/data_reduction_proxy_headers.h:54: bool
ParseHeadersAndSetProxyInfoBypassDuration(
On 2014/07/17 00:11:08, bengr1 wrote:
> I'd save function renaming for another CL.

These 2 methods are only used locally, so thought I'd go ahead in this cl.
Please let me know if you have a strong preference otherwise, and I'll revert
it.

https://codereview.chromium.org/382313003/diff/120001/components/data_reducti...
components/data_reduction_proxy/common/data_reduction_proxy_headers.h:59: // Set
a random bypass duration between 1 and 5 minutes.
On 2014/07/17 00:11:08, bengr1 wrote:
> Sets

Done. Moved comment to .cc.

[Not at Google. Contact bengr, 2014-07-17 19:13:32]

thestig:
  chrome/browser/android/intercept_download_resource_throttle.cc
  chrome/browser/chrome_browser_field_trials.cc
  chrome/browser/chrome_browser_field_trials_mobile.cc
  chrome/browser/io_thread.cc
  chrome/browser/profiles/profile.cc
  chrome/browser/profiles/profile_io_data.cc
  chrome/browser/profiles/profile_io_data.h
asvitkine:
  chrome/browser/chrome_browser_field_trials.cc
  chrome/browser/chrome_browser_field_trials_mobile.cc
bengr:
  components/data_reduction_proxy/browser/data_reduction_proxy_metrics.cc
  components/data_reduction_proxy/browser/data_reduction_proxy_params.cc
  components/data_reduction_proxy/browser/data_reduction_proxy_params.h
  components/data_reduction_proxy/common/data_reduction_proxy_headers.cc
  components/data_reduction_proxy/common/data_reduction_proxy_headers.h
 
components/data_reduction_proxy/common/data_reduction_proxy_headers_unittest.cc
mef:
  net/http/http_log_util.cc
  net/http/http_log_util_unittest.cc
  net/http/http_network_transaction.cc
  net/proxy/proxy_list.cc
  net/proxy/proxy_server.cc
  net/proxy/proxy_server.h
  net/proxy/proxy_service.cc
  net/proxy/proxy_service.h
  net/socket/ssl_client_socket_pool.cc
  net/spdy/spdy_session.cc

[Lei Zhang, 2014-07-17 19:28:44]

Please use bullet points in the CL description rather than a paragraph.

Happy to see #ifdefs getting removed.

chrome/ sans chrome/browser/chrome_browser_field_trials* lgtm.

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

https://codereview.chromium.org/382313003/diff/220001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:650: 
nit: no random newlines please

[Not at Google. Contact bengr, 2014-07-17 20:16:22]

Changed description to use bullet points.

[Not at Google. Contact bengr, 2014-07-18 17:33:57]

Did not notice this comment earlier. Removed newline.

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

https://codereview.chromium.org/382313003/diff/220001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:650: 
On 2014/07/17 19:28:44, Lei Zhang wrote:
> nit: no random newlines please

Done.

[bengr, 2014-07-18 19:41:44]

https://codereview.chromium.org/382313003/diff/260001/chrome/browser/chrome_b...
File chrome/browser/chrome_browser_field_trials.cc (right):

https://codereview.chromium.org/382313003/diff/260001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_field_trials.cc:41: void
DataCompressionProxyBaseFieldTrial(
Confirm with asvitkine@ that all of this is needed.

https://codereview.chromium.org/382313003/diff/260001/chrome/browser/profiles...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/382313003/diff/260001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.cc:522:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
Do we expect all the params to be non-NULL? If so, we should DCHECK.

https://codereview.chromium.org/382313003/diff/260001/chrome/browser/profiles...
File chrome/browser/profiles/profile_io_data.h (right):

https://codereview.chromium.org/382313003/diff/260001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.h:346: // from io thread globals.
for given --> for the given

params --> |params|
usage_stats -> |usage_stats| 
io thread globals. --> |IOThread::Globals|.

In general, I prefer using || to separate code from text. I'm not sure what the
owners here prefer.

https://codereview.chromium.org/382313003/diff/260001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.h:350: // Setup data reduction proxy for
given profile, params, and usage_stats.
Say something about ownership. Is ownership retained by the caller?

https://codereview.chromium.org/382313003/diff/260001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.h:624:
scoped_ptr<data_reduction_proxy::DataReductionProxySettings>
Shouldn't this only be declared for non-Android/iOS?

https://codereview.chromium.org/382313003/diff/260001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.h:625: data_reduction_proxy_settings_;
Optional: Style isn't clear on whether the second line of a variable declaration
should be indented. I prefer not.

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

https://codereview.chromium.org/382313003/diff/260001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:332:
return "http://proxy-dev.googlezip.net:80/";
I would define all of these as const char[]s in an anonymous namespace at the
top of the file. It will make them much easier to change later, if needed.

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

https://codereview.chromium.org/382313003/diff/260001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_params.h:183:
virtual std::string GetDefaultDevOrigin() const;
These shouldn't be public.

https://codereview.chromium.org/382313003/diff/260001/components/data_reducti...
File components/data_reduction_proxy/common/data_reduction_proxy_headers.cc
(right):

https://codereview.chromium.org/382313003/diff/260001/components/data_reducti...
components/data_reduction_proxy/common/data_reduction_proxy_headers.cc:27: void
SetRandBypassDuration(DataReductionProxyInfo* data_reduction_proxy_info) {
DCHECK that this isn't NULL.

https://codereview.chromium.org/382313003/diff/260001/components/data_reducti...
components/data_reduction_proxy/common/data_reduction_proxy_headers.cc:27: void
SetRandBypassDuration(DataReductionProxyInfo* data_reduction_proxy_info) {
I still think this can be a base:TimeDelta*

https://codereview.chromium.org/382313003/diff/260001/components/data_reducti...
components/data_reduction_proxy/common/data_reduction_proxy_headers.cc:82: if
(ParseHeadersAndSetProxyInfoBypassDuration(
You're in a file with the name "headers" it it, so it is implied that you are
getting the duration from the headers. No need to change the name.

https://codereview.chromium.org/382313003/diff/260001/components/data_reducti...
File components/data_reduction_proxy/common/data_reduction_proxy_headers.h
(right):

https://codereview.chromium.org/382313003/diff/260001/components/data_reducti...
components/data_reduction_proxy/common/data_reduction_proxy_headers.h:57:
DataReductionProxyInfo* proxy_info);
Why did this parameter change?

https://codereview.chromium.org/382313003/diff/260001/net/proxy/proxy_service.cc
File net/proxy/proxy_service.cc (right):

https://codereview.chromium.org/382313003/diff/260001/net/proxy/proxy_service...
net/proxy/proxy_service.cc:1209: const net::ProxyServer& proxy_server =
result->proxy_server();
Instead of doing it this way, it would probably be better to embed a Callback or
delegate. Add a TODO to do it that way.

For example, you could do:

callback_.Run(result);

Then ProxyService wouldn't need to know what a data reduction proxy is.

[Not at Google. Contact bengr, 2014-07-21 17:46:26]

https://codereview.chromium.org/382313003/diff/260001/chrome/browser/chrome_b...
File chrome/browser/chrome_browser_field_trials.cc (right):

https://codereview.chromium.org/382313003/diff/260001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_field_trials.cc:41: void
DataCompressionProxyBaseFieldTrial(
On 2014/07/18 19:41:43, bengr1 wrote:
> Confirm with asvitkine@ that all of this is needed.

@asvitkine: Can you please confirm.

https://codereview.chromium.org/382313003/diff/260001/chrome/browser/profiles...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/382313003/diff/260001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.cc:522:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
On 2014/07/18 19:41:43, bengr1 wrote:
> Do we expect all the params to be non-NULL? If so, we should DCHECK.

Done.

https://codereview.chromium.org/382313003/diff/260001/chrome/browser/profiles...
File chrome/browser/profiles/profile_io_data.h (right):

https://codereview.chromium.org/382313003/diff/260001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.h:346: // from io thread globals.
On 2014/07/18 19:41:44, bengr1 wrote:
> for given --> for the given
> 
> params --> |params|
> usage_stats -> |usage_stats| 
> io thread globals. --> |IOThread::Globals|.
> 
> In general, I prefer using || to separate code from text. I'm not sure what
the
> owners here prefer.

Done.

https://codereview.chromium.org/382313003/diff/260001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.h:350: // Setup data reduction proxy for
given profile, params, and usage_stats.
On 2014/07/18 19:41:44, bengr1 wrote:
> Say something about ownership. Is ownership retained by the caller?

As discussed, I no longer instantiate the settings because it causes threading
issues, so this hasn't changed.

https://codereview.chromium.org/382313003/diff/260001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.h:624:
scoped_ptr<data_reduction_proxy::DataReductionProxySettings>
On 2014/07/18 19:41:44, bengr1 wrote:
> Shouldn't this only be declared for non-Android/iOS?

Removed.

https://codereview.chromium.org/382313003/diff/260001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.h:625: data_reduction_proxy_settings_;
On 2014/07/18 19:41:44, bengr1 wrote:
> Optional: Style isn't clear on whether the second line of a variable
declaration
> should be indented. I prefer not.

Removed.

I thought this would be treated like any other statement, and so next line would
indent?

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

https://codereview.chromium.org/382313003/diff/260001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:332:
return "http://proxy-dev.googlezip.net:80/";
On 2014/07/18 19:41:44, bengr1 wrote:
> I would define all of these as const char[]s in an anonymous namespace at the
> top of the file. It will make them much easier to change later, if needed.

Done.

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

https://codereview.chromium.org/382313003/diff/260001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_params.h:183:
virtual std::string GetDefaultDevOrigin() const;
On 2014/07/18 19:41:44, bengr1 wrote:
> These shouldn't be public.

We need to access this from io_thread.cc for now. Should go away once we use the
callback in proxy_service. Added TODO.

https://codereview.chromium.org/382313003/diff/260001/components/data_reducti...
File components/data_reduction_proxy/common/data_reduction_proxy_headers.cc
(right):

https://codereview.chromium.org/382313003/diff/260001/components/data_reducti...
components/data_reduction_proxy/common/data_reduction_proxy_headers.cc:27: void
SetRandBypassDuration(DataReductionProxyInfo* data_reduction_proxy_info) {
On 2014/07/18 19:41:44, bengr1 wrote:
> DCHECK that this isn't NULL.

Done.

https://codereview.chromium.org/382313003/diff/260001/components/data_reducti...
components/data_reduction_proxy/common/data_reduction_proxy_headers.cc:27: void
SetRandBypassDuration(DataReductionProxyInfo* data_reduction_proxy_info) {
On 2014/07/18 19:41:44, bengr1 wrote:
> I still think this can be a base:TimeDelta*

Done.

https://codereview.chromium.org/382313003/diff/260001/components/data_reducti...
components/data_reduction_proxy/common/data_reduction_proxy_headers.cc:82: if
(ParseHeadersAndSetProxyInfoBypassDuration(
On 2014/07/18 19:41:44, bengr1 wrote:
> You're in a file with the name "headers" it it, so it is implied that you are
> getting the duration from the headers. No need to change the name.

My attempt was to make it less ambiguous. I had initially assumed that the
return value would be the bypass duration. Also, wasn't obvious to me that it
was being parsed from the header.

If you have a strong preference for the original, I can revert:)

https://codereview.chromium.org/382313003/diff/260001/components/data_reducti...
File components/data_reduction_proxy/common/data_reduction_proxy_headers.h
(right):

https://codereview.chromium.org/382313003/diff/260001/components/data_reducti...
components/data_reduction_proxy/common/data_reduction_proxy_headers.h:57:
DataReductionProxyInfo* proxy_info);
On 2014/07/18 19:41:44, bengr1 wrote:
> Why did this parameter change?

Reverted.

Had initially changed to dereference proxy_info once here, instead of repeating
&...->bypass_duration.

https://codereview.chromium.org/382313003/diff/260001/net/proxy/proxy_service.cc
File net/proxy/proxy_service.cc (right):

https://codereview.chromium.org/382313003/diff/260001/net/proxy/proxy_service...
net/proxy/proxy_service.cc:1209: const net::ProxyServer& proxy_server =
result->proxy_server();
On 2014/07/18 19:41:44, bengr1 wrote:
> Instead of doing it this way, it would probably be better to embed a Callback
or
> delegate. Add a TODO to do it that way.
> 
> For example, you could do:
> 
> callback_.Run(result);
> 
> Then ProxyService wouldn't need to know what a data reduction proxy is.

Good idea. I went down the path of trying to do it in this cl. But it caused
some cascading changes which makes the cl too big. Added TODO.

[Alexei Svitkine (slow), 2014-07-21 20:25:36]

https://codereview.chromium.org/382313003/diff/300001/chrome/browser/chrome_b...
File chrome/browser/chrome_browser_field_trials.cc (right):

https://codereview.chromium.org/382313003/diff/300001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_field_trials.cc:40: // a client-side configuration
is used.
I guess this is still OK if you really want to support a default percentage on
first run after install. Otherwise, I'd still advise doing things entirely from
the server-side.

https://codereview.chromium.org/382313003/diff/300001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_field_trials.cc:42: const char* trial_name,
Nit: Might as well make this a const std::string& here - no advantage to keeping
it a C string since it will be converted below anyway.

https://codereview.chromium.org/382313003/diff/300001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_field_trials.cc:52: // Experiment enabled until
Jan 1, 2020. By default, disabled.
2020 is too far away. How about 2016 for now?

https://codereview.chromium.org/382313003/diff/300001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_field_trials.cc:76: // Governs the rollout of the
_promo_ for the compression proxy
Do we have the promo UI on all platforms already?

https://codereview.chromium.org/382313003/diff/300001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_field_trials.cc:94:
DataCompressionProxyFieldTrials();
Nit: Rename to SetupDataCompressionProxyFieldTrials().

https://codereview.chromium.org/382313003/diff/300001/components/data_reducti...
File components/data_reduction_proxy/common/data_reduction_proxy_headers.cc
(right):

https://codereview.chromium.org/382313003/diff/300001/components/data_reducti...
components/data_reduction_proxy/common/data_reduction_proxy_headers.cc:30:
base::RandInt(1 * 60 * 1000, 5 * 60 * 1000));
Nit: How about:

const int64 delta_ms = base::RandInt(
    base::TimeDelta::FromMinutes(1).InMilliseconds(),
    base::TimeDelta::FromMinutes(5).InMilliseconds());
return TimeDelta::FromMilliseconds(delta_ms);

That way, readers don't have to do the mental arithmetic to confirm the logic
here is sound.

Also, the function should just return the base::TimeDelta rather than set it as
an out param. (A TimeDelta is just an int64).

https://codereview.chromium.org/382313003/diff/300001/components/data_reducti...
File
components/data_reduction_proxy/common/data_reduction_proxy_headers_unittest.cc
(right):

https://codereview.chromium.org/382313003/diff/300001/components/data_reducti...
components/data_reduction_proxy/common/data_reduction_proxy_headers_unittest.cc:196:
EXPECT_TRUE(
Nit: Indentation is off.

https://codereview.chromium.org/382313003/diff/300001/net/proxy/proxy_service.cc
File net/proxy/proxy_service.cc (right):

https://codereview.chromium.org/382313003/diff/300001/net/proxy/proxy_service...
net/proxy/proxy_service.cc:1413: 
Nit: Remove extra line.

https://codereview.chromium.org/382313003/diff/300001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_pool.cc (left):

https://codereview.chromium.org/382313003/diff/300001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_pool.cc:378:
"Net.SSL_Connection_Latency_DataReductionProxy",
Can you mark this histogram as deprecated in histograms.xml given that it's not
no longer recorded?

https://codereview.chromium.org/382313003/diff/300001/net/spdy/spdy_session.cc
File net/spdy/spdy_session.cc (left):

https://codereview.chromium.org/382313003/diff/300001/net/spdy/spdy_session.c...
net/spdy/spdy_session.cc:682:
UMA_HISTOGRAM_BOOLEAN("Net.SpdySessions_DataReductionProxy",
Can you mark this histogram as deprecated in histograms.xml given that it's not
no longer recorded?

[mef, 2014-07-21 20:58:27]

+rsleevi

Removal of #if defined(SPDY_PROXY_AUTH_ORIGIN) from net/ seems a bit drastic as
it affects all platforms and all embedders. 

Adding Ryan to comment.

[bengr, 2014-07-21 21:08:30]

On 2014/07/21 20:58:27, mef wrote:
> +rsleevi
> 
> Removal of #if defined(SPDY_PROXY_AUTH_ORIGIN) from net/ seems a bit drastic
as
> it affects all platforms and all embedders. 
> 
> Adding Ryan to comment.

We're happy to guard the data reduction proxy logic with #if
defined(DATA_REDUCTION_PROXY) if needed, but the hope was that we'd remove all
data reduction proxy from net/ by M39. I think the only substantial data
reduction proxy logic that's still in net is the UMA in ProxyService. I hope
Rohit will have a follow-on CL to embed that logic.

[Not at Google. Contact bengr, 2014-07-21 21:45:09]

Thanks for the review!

https://codereview.chromium.org/382313003/diff/300001/chrome/browser/chrome_b...
File chrome/browser/chrome_browser_field_trials.cc (right):

https://codereview.chromium.org/382313003/diff/300001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_field_trials.cc:40: // a client-side configuration
is used.
On 2014/07/21 20:25:35, Alexei Svitkine wrote:
> I guess this is still OK if you really want to support a default percentage on
> first run after install. Otherwise, I'd still advise doing things entirely
from
> the server-side.

Acknowledged.

https://codereview.chromium.org/382313003/diff/300001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_field_trials.cc:42: const char* trial_name,
On 2014/07/21 20:25:35, Alexei Svitkine wrote:
> Nit: Might as well make this a const std::string& here - no advantage to
keeping
> it a C string since it will be converted below anyway.

Done.

https://codereview.chromium.org/382313003/diff/300001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_field_trials.cc:52: // Experiment enabled until
Jan 1, 2020. By default, disabled.
On 2014/07/21 20:25:35, Alexei Svitkine wrote:
> 2020 is too far away. How about 2016 for now?

Done.

https://codereview.chromium.org/382313003/diff/300001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_field_trials.cc:76: // Governs the rollout of the
_promo_ for the compression proxy
On 2014/07/21 20:25:35, Alexei Svitkine wrote:
> Do we have the promo UI on all platforms already?

No. Currently, this is not used on desktop and ChromeOS.

https://codereview.chromium.org/382313003/diff/300001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_field_trials.cc:94:
DataCompressionProxyFieldTrials();
On 2014/07/21 20:25:35, Alexei Svitkine wrote:
> Nit: Rename to SetupDataCompressionProxyFieldTrials().

Done.

https://codereview.chromium.org/382313003/diff/300001/components/data_reducti...
File components/data_reduction_proxy/common/data_reduction_proxy_headers.cc
(right):

https://codereview.chromium.org/382313003/diff/300001/components/data_reducti...
components/data_reduction_proxy/common/data_reduction_proxy_headers.cc:30:
base::RandInt(1 * 60 * 1000, 5 * 60 * 1000));
On 2014/07/21 20:25:35, Alexei Svitkine wrote:
> Nit: How about:
> 
> const int64 delta_ms = base::RandInt(
>     base::TimeDelta::FromMinutes(1).InMilliseconds(),
>     base::TimeDelta::FromMinutes(5).InMilliseconds());
> return TimeDelta::FromMilliseconds(delta_ms);
> 
> That way, readers don't have to do the mental arithmetic to confirm the logic
> here is sound.
> 
> Also, the function should just return the base::TimeDelta rather than set it
as
> an out param. (A TimeDelta is just an int64).

Done.

https://codereview.chromium.org/382313003/diff/300001/components/data_reducti...
File
components/data_reduction_proxy/common/data_reduction_proxy_headers_unittest.cc
(right):

https://codereview.chromium.org/382313003/diff/300001/components/data_reducti...
components/data_reduction_proxy/common/data_reduction_proxy_headers_unittest.cc:196:
EXPECT_TRUE(
On 2014/07/21 20:25:35, Alexei Svitkine wrote:
> Nit: Indentation is off.

Done.

https://codereview.chromium.org/382313003/diff/300001/net/proxy/proxy_service.cc
File net/proxy/proxy_service.cc (right):

https://codereview.chromium.org/382313003/diff/300001/net/proxy/proxy_service...
net/proxy/proxy_service.cc:1413: 
On 2014/07/21 20:25:35, Alexei Svitkine wrote:
> Nit: Remove extra line.

Done.

https://codereview.chromium.org/382313003/diff/300001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_pool.cc (left):

https://codereview.chromium.org/382313003/diff/300001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_pool.cc:378:
"Net.SSL_Connection_Latency_DataReductionProxy",
On 2014/07/21 20:25:35, Alexei Svitkine wrote:
> Can you mark this histogram as deprecated in histograms.xml given that it's
not
> no longer recorded?

Done.

https://codereview.chromium.org/382313003/diff/300001/net/spdy/spdy_session.cc
File net/spdy/spdy_session.cc (left):

https://codereview.chromium.org/382313003/diff/300001/net/spdy/spdy_session.c...
net/spdy/spdy_session.cc:682:
UMA_HISTOGRAM_BOOLEAN("Net.SpdySessions_DataReductionProxy",
On 2014/07/21 20:25:35, Alexei Svitkine wrote:
> Can you mark this histogram as deprecated in histograms.xml given that it's
not
> no longer recorded?

Done.

[bengr, 2014-07-21 22:45:16]

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

https://codereview.chromium.org/382313003/diff/320001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:638: if
(DataReductionProxyParams::IsIncludedInFieldTrial()) {
Please test that the code works when not part of the field trial.

https://codereview.chromium.org/382313003/diff/320001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:642: if
(DataReductionProxyParams::IsIncludedInAlternativeFieldTrial())
We are accumulating a lot of code here. Would you mind factoring it into a
method called something like EmbedDataReductionProxy()?

https://codereview.chromium.org/382313003/diff/320001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:1102:
globals_->system_proxy_service->SetDataReductionProxyOrigins(
Add a comment that this is temporary until the histogram code is factored out of
Proxy{Server|Service}

Also, why can't you use origin() and fallback_origin()?

https://codereview.chromium.org/382313003/diff/320001/chrome/browser/profiles...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/382313003/diff/320001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.cc:1052:
io_thread_globals->data_reduction_proxy_params->GetDefaultOrigin(),
origin() and fallback_origin()?

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

https://codereview.chromium.org/382313003/diff/320001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_params.h:181: //
Returns the corresponding string from preprocessor constants if defined,
I don't think these need to be public.

https://codereview.chromium.org/382313003/diff/320001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_params.h:183: //
TODO(kundaji): Make this private when it is no longer access from IOThread.
accessed

https://codereview.chromium.org/382313003/diff/320001/components/data_reducti...
File components/data_reduction_proxy/common/data_reduction_proxy_headers.cc
(right):

https://codereview.chromium.org/382313003/diff/320001/components/data_reducti...
components/data_reduction_proxy/common/data_reduction_proxy_headers.cc:24:
namespace {
nit: this doesn't have to be inside data_reduction_proxy

https://codereview.chromium.org/382313003/diff/320001/components/data_reducti...
components/data_reduction_proxy/common/data_reduction_proxy_headers.cc:27:
base::TimeDelta GetRandBypassDuration() {
Suggest changing name to GetDefaultBypassDuration()

https://codereview.chromium.org/382313003/diff/320001/components/data_reducti...
components/data_reduction_proxy/common/data_reduction_proxy_headers.cc:58: //
Server deferred to us to choose a duration. Default to a range from
range from... -> random duration between one and five minutes

https://codereview.chromium.org/382313003/diff/320001/components/data_reducti...
components/data_reduction_proxy/common/data_reduction_proxy_headers.cc:144: if
(headers->response_code() == net::HTTP_INTERNAL_SERVER_ERROR ||
Coordinate with megjablon when landing.

https://codereview.chromium.org/382313003/diff/320001/net/proxy/proxy_list.cc
File net/proxy/proxy_list.cc (right):

https://codereview.chromium.org/382313003/diff/320001/net/proxy/proxy_list.cc...
net/proxy/proxy_list.cc:169: UpdateRetryInfoOnFallback(proxy_retry_info,
TimeDelta::FromMinutes(5), true,
Add a comment that say by default proxies are not retried for 5 minutes.

[Not at Google. Contact bengr, 2014-07-22 00:12:10]

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

https://codereview.chromium.org/382313003/diff/320001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:638: if
(DataReductionProxyParams::IsIncludedInFieldTrial()) {
On 2014/07/21 22:45:14, bengr1 wrote:
> Please test that the code works when not part of the field trial.

Good catch. Made other flags conditional on kAllowed.

https://codereview.chromium.org/382313003/diff/320001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:642: if
(DataReductionProxyParams::IsIncludedInAlternativeFieldTrial())
On 2014/07/21 22:45:14, bengr1 wrote:
> We are accumulating a lot of code here. Would you mind factoring it into a
> method called something like EmbedDataReductionProxy()?

Done.

https://codereview.chromium.org/382313003/diff/320001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:1102:
globals_->system_proxy_service->SetDataReductionProxyOrigins(
On 2014/07/21 22:45:14, bengr1 wrote:
> Add a comment that this is temporary until the histogram code is factored out
of
> Proxy{Server|Service}
> 
> Also, why can't you use origin() and fallback_origin()?

Acknowledged.

https://codereview.chromium.org/382313003/diff/320001/chrome/browser/profiles...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/382313003/diff/320001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.cc:1052:
io_thread_globals->data_reduction_proxy_params->GetDefaultOrigin(),
On 2014/07/21 22:45:14, bengr1 wrote:
> origin() and fallback_origin()? 

Done.

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

https://codereview.chromium.org/382313003/diff/320001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_params.h:181: //
Returns the corresponding string from preprocessor constants if defined,
On 2014/07/21 22:45:14, bengr1 wrote:
> I don't think these need to be public.

Done.

https://codereview.chromium.org/382313003/diff/320001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_params.h:183: //
TODO(kundaji): Make this private when it is no longer access from IOThread.
On 2014/07/21 22:45:14, bengr1 wrote:
> accessed

Done.

https://codereview.chromium.org/382313003/diff/320001/components/data_reducti...
File components/data_reduction_proxy/common/data_reduction_proxy_headers.cc
(right):

https://codereview.chromium.org/382313003/diff/320001/components/data_reducti...
components/data_reduction_proxy/common/data_reduction_proxy_headers.cc:24:
namespace {
On 2014/07/21 22:45:15, bengr1 wrote:
> nit: this doesn't have to be inside data_reduction_proxy

Done.

https://codereview.chromium.org/382313003/diff/320001/components/data_reducti...
components/data_reduction_proxy/common/data_reduction_proxy_headers.cc:27:
base::TimeDelta GetRandBypassDuration() {
On 2014/07/21 22:45:14, bengr1 wrote:
> Suggest changing name to GetDefaultBypassDuration()

Done.

https://codereview.chromium.org/382313003/diff/320001/components/data_reducti...
components/data_reduction_proxy/common/data_reduction_proxy_headers.cc:58: //
Server deferred to us to choose a duration. Default to a range from
On 2014/07/21 22:45:16, bengr1 wrote:
> range from... -> random duration between one and five minutes

Done.

https://codereview.chromium.org/382313003/diff/320001/components/data_reducti...
components/data_reduction_proxy/common/data_reduction_proxy_headers.cc:144: if
(headers->response_code() == net::HTTP_INTERNAL_SERVER_ERROR ||
On 2014/07/21 22:45:15, bengr1 wrote:
> Coordinate with megjablon when landing.

Acknowledged.

https://codereview.chromium.org/382313003/diff/320001/net/proxy/proxy_list.cc
File net/proxy/proxy_list.cc (right):

https://codereview.chromium.org/382313003/diff/320001/net/proxy/proxy_list.cc...
net/proxy/proxy_list.cc:169: UpdateRetryInfoOnFallback(proxy_retry_info,
TimeDelta::FromMinutes(5), true,
On 2014/07/21 22:45:16, bengr1 wrote:
> Add a comment that say by default proxies are not retried for 5 minutes.

Done.

[bengr, 2014-07-22 01:06:42]

lgtm

[Ryan Sleevi, 2014-07-22 01:56:31]

Not LGTM.

Please do not land this in this form.
Please copy me on any CLs related to this code.

Kundaji, I understand you're sort of caught in the middle, so I apologize for
blocking this CL, as I'm sure it's frustrating. Please don't take it personally,
and please don't appreciate my terseness as anything more than brevity restating
issues that have been hashed and rehashed in the past regarding the interaction
of the data reduction proxy's integration.

I appreciate the effort to remove the #ifdefs, and agree, that's a long-term
goal. But we have a series of more pressing issues that the #ifdef's are
symptomatic of that need resolution first.



Ben, I thought we had a good understanding that this code would not be landing
until the (data reduction proxy) team worked to undo the many #ifdefs that were
added and that broke the various layering and encapsulation goals.

At present, these #ifdefs serve as signposts to damage imposed, and to
limitations of the API. My understanding was that the (data reduction proxy)
team was going to be focusing engineering effort to clean these up and to make
sure that we had sufficient abstractions.

To further layer on (although I see this as a small part of an overall picture),
I don't see any launch bugs with a chrome-security OK. I understand Matt was
looking to complete the survey, but that's a small part of the overall work to
get something launched.

https://codereview.chromium.org/382313003/diff/360001/chrome/browser/chrome_b...
File chrome/browser/chrome_browser_field_trials.cc (right):

https://codereview.chromium.org/382313003/diff/360001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_field_trials.cc:40: // a client-side configuration
is used.
NACK on this. Use Finch, control it by the server. We need a killbit for this
level of experimentation.

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

https://codereview.chromium.org/382313003/diff/360001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:544: int drp_flags = 0;
drop the drp_. Don't abbreviate like that, but this isn't necessary.

https://codereview.chromium.org/382313003/diff/360001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:1109: // Proxy{Server|Service}.
Based on the team, I have trouble believing this.

At a minimum, file a bug, and note the bug, and set a Release-Block label with
an M-Stone target. Refactoring/cleanup needs to be prioritized.

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

https://codereview.chromium.org/382313003/diff/360001/chrome/browser/io_threa...
chrome/browser/io_thread.h:311: void
SetupDataReductionProxy(ChromeNetworkDelegate* network_delegate);
The Data Reduction Proxy is inherently privacy sensitive. I'm dismayed by the
fact that Incognito is being routed through it. This absolutely should not be
part of the IOThread Globals.

https://codereview.chromium.org/382313003/diff/360001/chrome/browser/profiles...
File chrome/browser/profiles/profile.h (right):

https://codereview.chromium.org/382313003/diff/360001/chrome/browser/profiles...
chrome/browser/profiles/profile.h:15: #include
"components/data_reduction_proxy/browser/data_reduction_proxy_settings.h"
You don't update anything else in this .h, so I cannot help but feel this is
unnecessary.

Given the amount of forward declares in this file, it should have been a clear
signal that more #includes are bad. 

http://www.chromium.org/developers/coding-style/cpp-dos-and-donts

https://codereview.chromium.org/382313003/diff/360001/chrome/browser/profiles...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/382313003/diff/360001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.cc:145: using
data_reduction_proxy::DataReductionProxyUsageStats;
General consensus is that such using statements are bad, and hinder readability.

Please do not add these, and please consider removing the one that already
exists.

https://codereview.chromium.org/382313003/diff/360001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.cc:529:
DataReductionProxySettingsAndroid* proxySettingsAndroid =
STYLE: This variable name is wrong. Please fix to conform to Chrome style in a
separate CL.

proxy_settings_android

https://codereview.chromium.org/382313003/diff/360001/chrome/browser/profiles...
File chrome/browser/profiles/profile_io_data.h (right):

https://codereview.chromium.org/382313003/diff/360001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.h:346: // |usage_stats| from
|IOThread::Globals| on |IOThread|.
1) Only use || around variables. Not class names. (|IOThread::Globals| and
|IOThread| are both incorrect)
2) Be careful about describing too much of the implementation. Does this enable
the DataReductionProxy? Configure it to be enabled? What shared state? Etc.

https://codereview.chromium.org/382313003/diff/360001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.h:354:
data_reduction_proxy::DataReductionProxyUsageStats* usage_stats);
1) Reformat this. Your style is non-compliant. "git cl format' can help
2) Don't use || around UIThread

Use "the UI thread" and "the IO thread" to refer to these concepts.

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

https://codereview.chromium.org/382313003/diff/360001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:27: const
char kDefaultWarmupUrl[] = "http://www.gstatic.com/generate_204";
95% certain that these do not belong in //components.

If you're going to have Google-specific endpoints, please use //google_apis or
let the embedder supply them.

If that makes things difficult, it highlights that the API needs reconsidering
if it is to be part of //components.

https://codereview.chromium.org/382313003/diff/360001/net/http/http_log_util.cc
File net/http/http_log_util.cc (right):

https://codereview.chromium.org/382313003/diff/360001/net/http/http_log_util....
net/http/http_log_util.cc:37: void ElideChromeProxyDirective(const std::string&
header_value,
Absolutely cannot support moving this out of the #ifdef.

https://codereview.chromium.org/382313003/diff/360001/net/http/http_log_util....
net/http/http_log_util.cc:58: if (!base::strcasecmp(header.c_str(),
"chrome-proxy")) {
Nor this.

https://codereview.chromium.org/382313003/diff/360001/net/proxy/proxy_list.cc
File net/proxy/proxy_list.cc (right):

https://codereview.chromium.org/382313003/diff/360001/net/proxy/proxy_list.cc...
net/proxy/proxy_list.cc:207: DCHECK(retry_delay != base::TimeDelta());
No documentation/comment that I can tell why you're removing this.

https://codereview.chromium.org/382313003/diff/360001/net/proxy/proxy_service.h
File net/proxy/proxy_service.h (right):

https://codereview.chromium.org/382313003/diff/360001/net/proxy/proxy_service...
net/proxy/proxy_service.h:223: const GURL&
data_reduction_default_fallback_origin);
Sorry, this is a layering violation.

[bengr, 2014-07-22 07:20:05]

On 2014/07/22 01:56:31, Ryan Sleevi wrote:
> Not LGTM.
> 
> Please do not land this in this form.
> Please copy me on any CLs related to this code.
> 
> Kundaji, I understand you're sort of caught in the middle, so I apologize for
> blocking this CL, as I'm sure it's frustrating. Please don't take it
personally,
> and please don't appreciate my terseness as anything more than brevity
restating
> issues that have been hashed and rehashed in the past regarding the
interaction
> of the data reduction proxy's integration.
> 
> I appreciate the effort to remove the #ifdefs, and agree, that's a long-term
> goal. But we have a series of more pressing issues that the #ifdef's are
> symptomatic of that need resolution first.
> 
> 
> 
> Ben, I thought we had a good understanding that this code would not be landing
> until the (data reduction proxy) team worked to undo the many #ifdefs that
were
> added and that broke the various layering and encapsulation goals.
> 
> At present, these #ifdefs serve as signposts to damage imposed, and to
> limitations of the API. My understanding was that the (data reduction proxy)
> team was going to be focusing engineering effort to clean these up and to make
> sure that we had sufficient abstractions.
> 
> To further layer on (although I see this as a small part of an overall
picture),
> I don't see any launch bugs with a chrome-security OK. I understand Matt was
> looking to complete the survey, but that's a small part of the overall work to
> get something launched.

Ryan, the understanding was to move the data reduction proxy code out of net,
which I went to great pains to do. The result is that our logic is in a
component, and almost all of our proxy specific logic is embedded where needed
in net, e.g., via the ChromeNetworkDelegate. You are right that some layering
issues remain, but these are much smaller than the ones we've addressed already,
and we are continuing to address them. Rohit is removing one of these with this
CL. That one sets the default proxy retry map expiration time to 1-5 minutes for
the data reduction proxy and 5 minutes for all other proxies. A more bothersome
violation is that some of our UMA lives in ProxyService and should instead be
embedded. We are happy to fix that in this CL, but thought we'd postpone to a
follow-up CL to keep this one manageable. If you want that one to land first, we
can do that too. There are other #ifdefs that were removed, but those limited
the code to the Android and iOS platforms. Removing those #ifdefs IMHO makes
chromium better because it reduces the amount of platform-specific logic that
needs to be tracked separately.

That said, if you think that we are further breaking the layering and
abstractions of chromium with this CL, then please point out where we've gone
wrong and we'll fix those in this CL. That was not our intention.

The launch bug is https://code.google.com/p/chromium/issues/detail?id=375484 and
sidv is the owner. We can bring up any launch related concerns with him.






> 
>
https://codereview.chromium.org/382313003/diff/360001/chrome/browser/chrome_b...
> File chrome/browser/chrome_browser_field_trials.cc (right):
> 
>
https://codereview.chromium.org/382313003/diff/360001/chrome/browser/chrome_b...
> chrome/browser/chrome_browser_field_trials.cc:40: // a client-side
configuration
> is used.
> NACK on this. Use Finch, control it by the server. We need a killbit for this
> level of experimentation.
> 
>
https://codereview.chromium.org/382313003/diff/360001/chrome/browser/io_threa...
> File chrome/browser/io_thread.cc (right):
> 
>
https://codereview.chromium.org/382313003/diff/360001/chrome/browser/io_threa...
> chrome/browser/io_thread.cc:544: int drp_flags = 0;
> drop the drp_. Don't abbreviate like that, but this isn't necessary.
> 
>
https://codereview.chromium.org/382313003/diff/360001/chrome/browser/io_threa...
> chrome/browser/io_thread.cc:1109: // Proxy{Server|Service}.
> Based on the team, I have trouble believing this.
> 
> At a minimum, file a bug, and note the bug, and set a Release-Block label with
> an M-Stone target. Refactoring/cleanup needs to be prioritized.
> 
>
https://codereview.chromium.org/382313003/diff/360001/chrome/browser/io_thread.h
> File chrome/browser/io_thread.h (right):
> 
>
https://codereview.chromium.org/382313003/diff/360001/chrome/browser/io_threa...
> chrome/browser/io_thread.h:311: void
> SetupDataReductionProxy(ChromeNetworkDelegate* network_delegate);
> The Data Reduction Proxy is inherently privacy sensitive. I'm dismayed by the
> fact that Incognito is being routed through it. This absolutely should not be
> part of the IOThread Globals.
> 
>
https://codereview.chromium.org/382313003/diff/360001/chrome/browser/profiles...
> File chrome/browser/profiles/profile.h (right):
> 
>
https://codereview.chromium.org/382313003/diff/360001/chrome/browser/profiles...
> chrome/browser/profiles/profile.h:15: #include
> "components/data_reduction_proxy/browser/data_reduction_proxy_settings.h"
> You don't update anything else in this .h, so I cannot help but feel this is
> unnecessary.
> 
> Given the amount of forward declares in this file, it should have been a clear
> signal that more #includes are bad. 
> 
> http://www.chromium.org/developers/coding-style/cpp-dos-and-donts
> 
>
https://codereview.chromium.org/382313003/diff/360001/chrome/browser/profiles...
> File chrome/browser/profiles/profile_io_data.cc (right):
> 
>
https://codereview.chromium.org/382313003/diff/360001/chrome/browser/profiles...
> chrome/browser/profiles/profile_io_data.cc:145: using
> data_reduction_proxy::DataReductionProxyUsageStats;
> General consensus is that such using statements are bad, and hinder
readability.
> 
> Please do not add these, and please consider removing the one that already
> exists.
> 
>
https://codereview.chromium.org/382313003/diff/360001/chrome/browser/profiles...
> chrome/browser/profiles/profile_io_data.cc:529:
> DataReductionProxySettingsAndroid* proxySettingsAndroid =
> STYLE: This variable name is wrong. Please fix to conform to Chrome style in a
> separate CL.
> 
> proxy_settings_android
> 
>
https://codereview.chromium.org/382313003/diff/360001/chrome/browser/profiles...
> File chrome/browser/profiles/profile_io_data.h (right):
> 
>
https://codereview.chromium.org/382313003/diff/360001/chrome/browser/profiles...
> chrome/browser/profiles/profile_io_data.h:346: // |usage_stats| from
> |IOThread::Globals| on |IOThread|.
> 1) Only use || around variables. Not class names. (|IOThread::Globals| and
> |IOThread| are both incorrect)
> 2) Be careful about describing too much of the implementation. Does this
enable
> the DataReductionProxy? Configure it to be enabled? What shared state? Etc.
> 
>
https://codereview.chromium.org/382313003/diff/360001/chrome/browser/profiles...
> chrome/browser/profiles/profile_io_data.h:354:
> data_reduction_proxy::DataReductionProxyUsageStats* usage_stats);
> 1) Reformat this. Your style is non-compliant. "git cl format' can help
> 2) Don't use || around UIThread
> 
> Use "the UI thread" and "the IO thread" to refer to these concepts.
> 
>
https://codereview.chromium.org/382313003/diff/360001/components/data_reducti...
> File components/data_reduction_proxy/browser/data_reduction_proxy_params.cc
> (right):
> 
>
https://codereview.chromium.org/382313003/diff/360001/components/data_reducti...
> components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:27:
const
> char kDefaultWarmupUrl[] = "http://www.gstatic.com/generate_204";
> 95% certain that these do not belong in //components.
> 
> If you're going to have Google-specific endpoints, please use //google_apis or
> let the embedder supply them.
> 
> If that makes things difficult, it highlights that the API needs reconsidering
> if it is to be part of //components.
> 
>
https://codereview.chromium.org/382313003/diff/360001/net/http/http_log_util.cc
> File net/http/http_log_util.cc (right):
> 
>
https://codereview.chromium.org/382313003/diff/360001/net/http/http_log_util....
> net/http/http_log_util.cc:37: void ElideChromeProxyDirective(const
std::string&
> header_value,
> Absolutely cannot support moving this out of the #ifdef.
> 
>
https://codereview.chromium.org/382313003/diff/360001/net/http/http_log_util....
> net/http/http_log_util.cc:58: if (!base::strcasecmp(header.c_str(),
> "chrome-proxy")) {
> Nor this.
> 
> https://codereview.chromium.org/382313003/diff/360001/net/proxy/proxy_list.cc
> File net/proxy/proxy_list.cc (right):
> 
>
https://codereview.chromium.org/382313003/diff/360001/net/proxy/proxy_list.cc...
> net/proxy/proxy_list.cc:207: DCHECK(retry_delay != base::TimeDelta());
> No documentation/comment that I can tell why you're removing this.
> 
>
https://codereview.chromium.org/382313003/diff/360001/net/proxy/proxy_service.h
> File net/proxy/proxy_service.h (right):
> 
>
https://codereview.chromium.org/382313003/diff/360001/net/proxy/proxy_service...
> net/proxy/proxy_service.h:223: const GURL&
> data_reduction_default_fallback_origin);
> Sorry, this is a layering violation.

[Ryan Sleevi, 2014-07-22 08:22:25]

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

https://codereview.chromium.org/382313003/diff/360001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:562:
base::Bind(data_reduction_proxy::OnResolveProxyHandler));
As mentioned in past discussions, there are concerns about whether or not the
ChromeNetworkDelegate is the right layer to do this hooking. The interaction
with extensions, among other things, which you don't have to worry about on
mobile.

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

https://codereview.chromium.org/382313003/diff/360001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:27: const
char kDefaultWarmupUrl[] = "http://www.gstatic.com/generate_204";
On 2014/07/22 01:56:31, Ryan Sleevi wrote:
> 95% certain that these do not belong in //components.
> 
> If you're going to have Google-specific endpoints, please use //google_apis or
> let the embedder supply them.
> 
> If that makes things difficult, it highlights that the API needs reconsidering
> if it is to be part of //components.

Further red-flags: The number of HTTP URLs here for Google services.

Minimally, this requires documentation.

https://codereview.chromium.org/382313003/diff/360001/components/data_reducti...
File components/data_reduction_proxy/common/data_reduction_proxy_headers.cc
(right):

https://codereview.chromium.org/382313003/diff/360001/components/data_reducti...
components/data_reduction_proxy/common/data_reduction_proxy_headers.cc:32: }  //
namespace anonymous
1) line break after this
2) It's not // namespace anonymous. Just omit the comment.

https://codereview.chromium.org/382313003/diff/360001/components/data_reducti...
components/data_reduction_proxy/common/data_reduction_proxy_headers.cc:41:
std::string name = "chrome-proxy";
You're adding a new header into the exposed web platform. That it's already on
mobile is an unfortunate thing, but we shouldn't be doing it in Chrome just
because.

https://codereview.chromium.org/382313003/diff/360001/components/data_reducti...
components/data_reduction_proxy/common/data_reduction_proxy_headers.cc:126:
net::ProxyService::DataReductionProxyBypassType
1) linebreak between these constants.
2) Follow
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar...
3) function-level or unnamed namespace level. These are inviting ODR violations
because they're neither.

https://codereview.chromium.org/382313003/diff/360001/components/data_reducti...
File components/data_reduction_proxy/common/data_reduction_proxy_headers.h
(right):

https://codereview.chromium.org/382313003/diff/360001/components/data_reducti...
components/data_reduction_proxy/common/data_reduction_proxy_headers.h:12:
#include "net/http/http_response_headers.h"
Forward declare

https://codereview.chromium.org/382313003/diff/360001/components/data_reducti...
components/data_reduction_proxy/common/data_reduction_proxy_headers.h:13:
#include "net/proxy/proxy_service.h"
Forward Declare

https://codereview.chromium.org/382313003/diff/360001/components/data_reducti...
components/data_reduction_proxy/common/data_reduction_proxy_headers.h:15: using
net::ProxyService;
Do not use a Using directive in headers -
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Namesp...

https://codereview.chromium.org/382313003/diff/360001/net/http/http_log_util.cc
File net/http/http_log_util.cc (right):

https://codereview.chromium.org/382313003/diff/360001/net/http/http_log_util....
net/http/http_log_util.cc:37: void ElideChromeProxyDirective(const std::string&
header_value,
On 2014/07/22 01:56:31, Ryan Sleevi wrote:
> Absolutely cannot support moving this out of the #ifdef.

This is one aspect we've talked about in the past. Eliding such proxy directives
does not belong in //net. The naming alone is a dead give-away.

Redacting such data - including from information like NetLog - really does not
belong in //net.

https://codereview.chromium.org/382313003/diff/360001/net/http/http_log_util....
net/http/http_log_util.cc:58: if (!base::strcasecmp(header.c_str(),
"chrome-proxy")) {
On 2014/07/22 01:56:31, Ryan Sleevi wrote:
> Nor this.

1) //net knowing about chrome-proxy
2) Introducing chrome-proxy as a header
3) Whitewashing the NetLog from //net based on embedder needs.

https://codereview.chromium.org/382313003/diff/360001/net/proxy/proxy_list.cc
File net/proxy/proxy_list.cc (right):

https://codereview.chromium.org/382313003/diff/360001/net/proxy/proxy_list.cc...
net/proxy/proxy_list.cc:171: ProxyServer(), net_log);
This needs to be separated and dealt with first, independently.

https://codereview.chromium.org/382313003/diff/360001/net/proxy/proxy_service.cc
File net/proxy/proxy_service.cc (right):

https://codereview.chromium.org/382313003/diff/360001/net/proxy/proxy_service...
net/proxy/proxy_service.cc:1226: }
To be clear: A callback is (almost certainly) the wrong approach, because you're
still embedding higher level logic here.

A better question is whether composition/wrapping the ProxyService at the
//components layer is suitable.

https://codereview.chromium.org/382313003/diff/360001/net/proxy/proxy_service...
net/proxy/proxy_service.cc:1412:
net::HostPortPair::FromURL(data_reduction_default_fallback_origin);
This very concept - of two origins for data reduction proxy - don't belong in
//net.

[Alexei Svitkine (slow), 2014-07-22 13:48:24]

https://codereview.chromium.org/382313003/diff/300001/chrome/browser/chrome_b...
File chrome/browser/chrome_browser_field_trials.cc (right):

https://codereview.chromium.org/382313003/diff/300001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_field_trials.cc:76: // Governs the rollout of the
_promo_ for the compression proxy
On 2014/07/21 21:45:09, kundaji wrote:
> On 2014/07/21 20:25:35, Alexei Svitkine wrote:
> > Do we have the promo UI on all platforms already?
> 
> No. Currently, this is not used on desktop and ChromeOS.

Then it seems incorrect to set up a trial with 10% active group for this on all
platforms here, given that it's not implemented everywhere.

[bengr, 2014-07-22 18:26:55]

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

https://codereview.chromium.org/382313003/diff/360001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:562:
base::Bind(data_reduction_proxy::OnResolveProxyHandler));
On 2014/07/22 08:22:24, Ryan Sleevi wrote:
> As mentioned in past discussions, there are concerns about whether or not the
> ChromeNetworkDelegate is the right layer to do this hooking. The interaction
> with extensions, among other things, which you don't have to worry about on
> mobile.

What past discussions are you referring to? I had many, many meetings with
OWNERS of net/, particularly with pauljensen and mmenke, and this was the
approach we arrived at. Everyone knew that desktop platforms were on the table.

Note that I tried implementing this as a set of classes that wrapped
UrlRequestHttpJob etc. to add our functionality, but this required bigger
changes to the net stack interfaces, seemed error prone, and required far, far
more glue code.

[bengr, 2014-07-22 18:52:52]

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

https://codereview.chromium.org/382313003/diff/360001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:27: const
char kDefaultWarmupUrl[] = "http://www.gstatic.com/generate_204";
We put these here because ultimately we want to make the proxy available to any
app that wants to use it. We had these in platform-specific gyp before. We can
move them back there. Having the embedder pass them is possible too, but a
little clumsy, since we should require an embedder to know how to contact the
proxy.

On 2014/07/22 08:22:24, Ryan Sleevi wrote:
> On 2014/07/22 01:56:31, Ryan Sleevi wrote:
> > 95% certain that these do not belong in //components.
> > 
> > If you're going to have Google-specific endpoints, please use //google_apis
or
> > let the embedder supply them.
> > 
> > If that makes things difficult, it highlights that the API needs
reconsidering
> > if it is to be part of //components.
> 
> Further red-flags: The number of HTTP URLs here for Google services.
> 
> Minimally, this requires documentation.

[bengr, 2014-07-22 18:55:00]

https://codereview.chromium.org/382313003/diff/360001/net/proxy/proxy_service.h
File net/proxy/proxy_service.h (right):

https://codereview.chromium.org/382313003/diff/360001/net/proxy/proxy_service...
net/proxy/proxy_service.h:223: const GURL&
data_reduction_default_fallback_origin);
On 2014/07/22 01:56:31, Ryan Sleevi wrote:
> Sorry, this is a layering violation.

I agree. Would you be ok with introducing a ProxyService delegate?

[bengr, 2014-07-22 19:57:15]

https://codereview.chromium.org/382313003/diff/360001/components/data_reducti...
File components/data_reduction_proxy/common/data_reduction_proxy_headers.cc
(right):

https://codereview.chromium.org/382313003/diff/360001/components/data_reducti...
components/data_reduction_proxy/common/data_reduction_proxy_headers.cc:41:
std::string name = "chrome-proxy";
On 2014/07/22 08:22:24, Ryan Sleevi wrote:
> You're adding a new header into the exposed web platform. That it's already on
> mobile is an unfortunate thing, but we shouldn't be doing it in Chrome just
> because.

This isn't going to change, even for desktop. We've been through this one a lot
of times. The data reduction proxy is different from a standard proxy because it
can send additional instructions along with a response. For example, it can tell
the client to bypass it for 17 seconds. Eventually we may be able to standardize
this functionality, but not now.

https://codereview.chromium.org/382313003/diff/360001/net/http/http_log_util.cc
File net/http/http_log_util.cc (right):

https://codereview.chromium.org/382313003/diff/360001/net/http/http_log_util....
net/http/http_log_util.cc:37: void ElideChromeProxyDirective(const std::string&
header_value,
On 2014/07/22 08:22:25, Ryan Sleevi wrote:
> On 2014/07/22 01:56:31, Ryan Sleevi wrote:
> > Absolutely cannot support moving this out of the #ifdef.
> 
> This is one aspect we've talked about in the past. Eliding such proxy
directives
> does not belong in //net. The naming alone is a dead give-away.
> 
> Redacting such data - including from information like NetLog - really does not
> belong in //net.

mmenke@ was the reviewer on this CL (see:
https://codereview.chromium.org/361053002/) and we discussed offline the
possibility of wrapping the NetLog to include such logic, but his opinion (and
mine) was that it wasn't worth the effort.

https://codereview.chromium.org/382313003/diff/360001/net/http/http_log_util....
net/http/http_log_util.cc:58: if (!base::strcasecmp(header.c_str(),
"chrome-proxy")) {
On 2014/07/22 08:22:25, Ryan Sleevi wrote:
> On 2014/07/22 01:56:31, Ryan Sleevi wrote:
> > Nor this.
> 
> 1) //net knowing about chrome-proxy
> 2) Introducing chrome-proxy as a header
> 3) Whitewashing the NetLog from //net based on embedder needs.

We can file a bug to wrap NetLog, but this shouldn't block this CL.

https://codereview.chromium.org/382313003/diff/360001/net/proxy/proxy_service.cc
File net/proxy/proxy_service.cc (right):

https://codereview.chromium.org/382313003/diff/360001/net/proxy/proxy_service...
net/proxy/proxy_service.cc:1226: }
On 2014/07/22 08:22:25, Ryan Sleevi wrote:
> To be clear: A callback is (almost certainly) the wrong approach, because
you're
> still embedding higher level logic here.
> 
> A better question is whether composition/wrapping the ProxyService at the
> //components layer is suitable.

For another change I looked into what it would take to wrap ProxyService and it
wasn't trivial. I'm happy to discuss the right approach. I was thinking we'd
introduce a delegate.

https://codereview.chromium.org/382313003/diff/360001/net/proxy/proxy_service...
net/proxy/proxy_service.cc:1412:
net::HostPortPair::FromURL(data_reduction_default_fallback_origin);
On 2014/07/22 08:22:25, Ryan Sleevi wrote:
> This very concept - of two origins for data reduction proxy - don't belong in
> //net.

I completely agree.

[bengr, 2014-07-22 20:08:20]

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

https://codereview.chromium.org/382313003/diff/360001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:1109: // Proxy{Server|Service}.
On 2014/07/22 01:56:30, Ryan Sleevi wrote:
> Based on the team, I have trouble believing this.
> 
> At a minimum, file a bug, and note the bug, and set a Release-Block label with
> an M-Stone target. Refactoring/cleanup needs to be prioritized.

Ryan, factoring the data reduction proxy out of net is something we are
committed to. I also don't like this code being here. 

Rohit, when you file the bug, put M-38 on it. I don't think this is a release
blocker, but I do agree that we should schedule the work and get it done asap.

[Not at Google. Contact bengr, 2014-07-22 23:03:54]

https://codereview.chromium.org/382313003/diff/300001/chrome/browser/chrome_b...
File chrome/browser/chrome_browser_field_trials.cc (right):

https://codereview.chromium.org/382313003/diff/300001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_field_trials.cc:76: // Governs the rollout of the
_promo_ for the compression proxy
On 2014/07/22 13:48:23, Alexei Svitkine wrote:
> On 2014/07/21 21:45:09, kundaji wrote:
> > On 2014/07/21 20:25:35, Alexei Svitkine wrote:
> > > Do we have the promo UI on all platforms already?
> > 
> > No. Currently, this is not used on desktop and ChromeOS.
> 
> Then it seems incorrect to set up a trial with 10% active group for this on
all
> platforms here, given that it's not implemented everywhere.

The intention was that the trial would automatically be picked up when we
launched on more platforms. There isn't currently a good place to define the
common method DataCompressionProxyBaseFieldTrial(...) used to set up both
trials. While I could add a new data reduction proxy specific file, it seems we
will still have unused trials when we do this for desktop and not ChromeOS, so
didn't think it was worth it. Let me know if you prefer this, or if you have any
alternative suggestions.

https://codereview.chromium.org/382313003/diff/360001/chrome/browser/chrome_b...
File chrome/browser/chrome_browser_field_trials.cc (right):

https://codereview.chromium.org/382313003/diff/360001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_field_trials.cc:40: // a client-side configuration
is used.
On 2014/07/22 01:56:30, Ryan Sleevi wrote:
> NACK on this. Use Finch, control it by the server. We need a killbit for this
> level of experimentation.

Finch experiments only kick in on the second run. This is a limitation of Finch.
Without this code, a user who expects this feature won't see it on first run. 

After the first run we do use Finch.

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

https://codereview.chromium.org/382313003/diff/360001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:544: int drp_flags = 0;
On 2014/07/22 01:56:30, Ryan Sleevi wrote:
> drop the drp_. Don't abbreviate like that, but this isn't necessary.

Done.

https://codereview.chromium.org/382313003/diff/360001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:1109: // Proxy{Server|Service}.
On 2014/07/22 01:56:30, Ryan Sleevi wrote:
> Based on the team, I have trouble believing this.
> 
> At a minimum, file a bug, and note the bug, and set a Release-Block label with
> an M-Stone target. Refactoring/cleanup needs to be prioritized.

Done. Filed bug 396222.

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

https://codereview.chromium.org/382313003/diff/360001/chrome/browser/io_threa...
chrome/browser/io_thread.h:311: void
SetupDataReductionProxy(ChromeNetworkDelegate* network_delegate);
On 2014/07/22 01:56:30, Ryan Sleevi wrote:
> The Data Reduction Proxy is inherently privacy sensitive. I'm dismayed by the
> fact that Incognito is being routed through it. This absolutely should not be
> part of the IOThread Globals.

We do not proxy incognito. We are making a change to make
DataReductionProxySettings a browser context keyed service that will not be
available for incognito. This will go away then. I can hold off on this cl until
that is done if you prefer.

https://codereview.chromium.org/382313003/diff/360001/chrome/browser/profiles...
File chrome/browser/profiles/profile.h (right):

https://codereview.chromium.org/382313003/diff/360001/chrome/browser/profiles...
chrome/browser/profiles/profile.h:15: #include
"components/data_reduction_proxy/browser/data_reduction_proxy_settings.h"
On 2014/07/22 01:56:30, Ryan Sleevi wrote:
> You don't update anything else in this .h, so I cannot help but feel this is
> unnecessary.
> 
> Given the amount of forward declares in this file, it should have been a clear
> signal that more #includes are bad. 
> 
> http://www.chromium.org/developers/coding-style/cpp-dos-and-donts

Done.

https://codereview.chromium.org/382313003/diff/360001/chrome/browser/profiles...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/382313003/diff/360001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.cc:145: using
data_reduction_proxy::DataReductionProxyUsageStats;
On 2014/07/22 01:56:31, Ryan Sleevi wrote:
> General consensus is that such using statements are bad, and hinder
readability.
> 
> Please do not add these, and please consider removing the one that already
> exists.

Can you please clarify why using "using" for data_reduction_proxy::* is bad, but
not for content::*?

https://codereview.chromium.org/382313003/diff/360001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.cc:529:
DataReductionProxySettingsAndroid* proxySettingsAndroid =
On 2014/07/22 01:56:31, Ryan Sleevi wrote:
> STYLE: This variable name is wrong. Please fix to conform to Chrome style in a
> separate CL.
> 
> proxy_settings_android

Done.

https://codereview.chromium.org/382313003/diff/360001/chrome/browser/profiles...
File chrome/browser/profiles/profile_io_data.h (right):

https://codereview.chromium.org/382313003/diff/360001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.h:346: // |usage_stats| from
|IOThread::Globals| on |IOThread|.
On 2014/07/22 01:56:31, Ryan Sleevi wrote:
> 1) Only use || around variables. Not class names. (|IOThread::Globals| and
> |IOThread| are both incorrect)
> 2) Be careful about describing too much of the implementation. Does this
enable
> the DataReductionProxy? Configure it to be enabled? What shared state? Etc.
> 

Done.

https://codereview.chromium.org/382313003/diff/360001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.h:354:
data_reduction_proxy::DataReductionProxyUsageStats* usage_stats);
On 2014/07/22 01:56:31, Ryan Sleevi wrote:
> 1) Reformat this. Your style is non-compliant. "git cl format' can help
> 2) Don't use || around UIThread
> 
> Use "the UI thread" and "the IO thread" to refer to these concepts.

Done.

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

https://codereview.chromium.org/382313003/diff/360001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:27: const
char kDefaultWarmupUrl[] = "http://www.gstatic.com/generate_204";
On 2014/07/22 08:22:24, Ryan Sleevi wrote:
> On 2014/07/22 01:56:31, Ryan Sleevi wrote:
> > 95% certain that these do not belong in //components.
> > 
> > If you're going to have Google-specific endpoints, please use //google_apis
or
> > let the embedder supply them.
> > 
> > If that makes things difficult, it highlights that the API needs
reconsidering
> > if it is to be part of //components.
> 
> Further red-flags: The number of HTTP URLs here for Google services.
> 
> Minimally, this requires documentation.

Can you please clarify why the number of HTTP URLs is a red flag? This seems
like a product level decision.

https://codereview.chromium.org/382313003/diff/360001/components/data_reducti...
File components/data_reduction_proxy/common/data_reduction_proxy_headers.cc
(right):

https://codereview.chromium.org/382313003/diff/360001/components/data_reducti...
components/data_reduction_proxy/common/data_reduction_proxy_headers.cc:32: }  //
namespace anonymous
On 2014/07/22 08:22:24, Ryan Sleevi wrote:
> 1) line break after this
> 2) It's not // namespace anonymous. Just omit the comment.

Done.

https://codereview.chromium.org/382313003/diff/360001/components/data_reducti...
components/data_reduction_proxy/common/data_reduction_proxy_headers.cc:126:
net::ProxyService::DataReductionProxyBypassType
On 2014/07/22 08:22:24, Ryan Sleevi wrote:
> 1) linebreak between these constants.
> 2) Follow
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar...
> 3) function-level or unnamed namespace level. These are inviting ODR
violations
> because they're neither.

Moved constants to top in anonymous namespace.

https://codereview.chromium.org/382313003/diff/360001/components/data_reducti...
File components/data_reduction_proxy/common/data_reduction_proxy_headers.h
(right):

https://codereview.chromium.org/382313003/diff/360001/components/data_reducti...
components/data_reduction_proxy/common/data_reduction_proxy_headers.h:12:
#include "net/http/http_response_headers.h"
On 2014/07/22 08:22:25, Ryan Sleevi wrote:
> Forward declare

Done.

https://codereview.chromium.org/382313003/diff/360001/components/data_reducti...
components/data_reduction_proxy/common/data_reduction_proxy_headers.h:13:
#include "net/proxy/proxy_service.h"
On 2014/07/22 08:22:25, Ryan Sleevi wrote:
> Forward Declare

DataReductionProxyBypassType is an enum, so forward declaration is not possible.

https://codereview.chromium.org/382313003/diff/360001/components/data_reducti...
components/data_reduction_proxy/common/data_reduction_proxy_headers.h:15: using
net::ProxyService;
On 2014/07/22 08:22:25, Ryan Sleevi wrote:
> Do not use a Using directive in headers -
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Namesp...

Done.

https://codereview.chromium.org/382313003/diff/360001/net/proxy/proxy_list.cc
File net/proxy/proxy_list.cc (right):

https://codereview.chromium.org/382313003/diff/360001/net/proxy/proxy_list.cc...
net/proxy/proxy_list.cc:171: ProxyServer(), net_log);
On 2014/07/22 08:22:25, Ryan Sleevi wrote:
> This needs to be separated and dealt with first, independently.

I am isolating this to a separate cl. Will send it to you for review.

https://codereview.chromium.org/382313003/diff/360001/net/proxy/proxy_list.cc...
net/proxy/proxy_list.cc:207: DCHECK(retry_delay != base::TimeDelta());
On 2014/07/22 01:56:31, Ryan Sleevi wrote:
> No documentation/comment that I can tell why you're removing this.

We randomize the duration for data reduction proxy, and always use exactly 5
minutes for other proxies. Will awlays be passed into this method, so will no
longer be an issue.

[Ryan Sleevi, 2014-07-23 00:54:44]

https://codereview.chromium.org/382313003/diff/360001/chrome/browser/chrome_b...
File chrome/browser/chrome_browser_field_trials.cc (right):

https://codereview.chromium.org/382313003/diff/360001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_field_trials.cc:40: // a client-side configuration
is used.
On 2014/07/22 23:03:52, kundaji wrote:
> On 2014/07/22 01:56:30, Ryan Sleevi wrote:
> > NACK on this. Use Finch, control it by the server. We need a killbit for
this
> > level of experimentation.
> 
> Finch experiments only kick in on the second run. This is a limitation of
Finch.
> Without this code, a user who expects this feature won't see it on first run. 
> 
> After the first run we do use Finch.

Conversely, there's no way we can turn this off for users, because they may
always fall in to the first run bucket.

From the bug, mdw@ said there were no plans for field trials, so I find this a
bit confusing. I do think we should err on the side of caution here, but if
there's a concern, perhaps following up on the launch bug (ergo for the sake of
discussion with privacy and finch teams) might be a productive exercise.

https://codereview.chromium.org/382313003/diff/360001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_field_trials.cc:66: << ". Selected group id: " <<
v;
This is incredibly spammy, and isn't necessary in a Finch world of
server-controlled field trials (since you can determine experiment group via
chrome://version )

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

https://codereview.chromium.org/382313003/diff/360001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:547: if
(DataReductionProxyParams::IsIncludedInFieldTrial()) {
Procedurally, this has the effect of enabling the feature for all users, which
is not something we do prior to the approval of the bug (
http://crbug.com/375484 ). Even if you were going to do a finch experiment, you
wouldn't be landing this code until the necessary teams have signed off, which
doesn't seem to be the case.

See also the comments in DataCompressionProxyBaseFieldTrial, which is setup in
such a way that some users will be opted-in for this.

The solution to this is that this code needs to remain entirely disabled for all
users, and a separate CL dealing with field trials and enabling the feature -
even for a small population - need to be blocked on getting approvals and
reviews.

https://codereview.chromium.org/382313003/diff/360001/chrome/browser/profiles...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/382313003/diff/360001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.cc:145: using
data_reduction_proxy::DataReductionProxyUsageStats;
On 2014/07/22 23:03:53, kundaji wrote:
> On 2014/07/22 01:56:31, Ryan Sleevi wrote:
> > General consensus is that such using statements are bad, and hinder
> readability.
> > 
> > Please do not add these, and please consider removing the one that already
> > exists.
> 
> Can you please clarify why using "using" for data_reduction_proxy::* is bad,
but
> not for content::*?

This thread (
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/qSrl_UAqAkQ/mn5OV...
) has a much larger discussion, which Peter summarizes again in
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/qSrl_UAqAkQ/SGDZG...


From a naming perspective, naming your classes "DataReductionX" within the
"data_reduction_proxy" namespace is something generally discouraged. 

data_reduction_proxy::Params (or ProxyParams) would be far more consistent with
respect to some of the other areas of code, but that's more of an aside.

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

https://codereview.chromium.org/382313003/diff/360001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:27: const
char kDefaultWarmupUrl[] = "http://www.gstatic.com/generate_204";
On 2014/07/22 23:03:53, kundaji wrote:
> On 2014/07/22 08:22:24, Ryan Sleevi wrote:
> > On 2014/07/22 01:56:31, Ryan Sleevi wrote:
> > > 95% certain that these do not belong in //components.
> > > 
> > > If you're going to have Google-specific endpoints, please use
//google_apis
> or
> > > let the embedder supply them.
> > > 
> > > If that makes things difficult, it highlights that the API needs
> reconsidering
> > > if it is to be part of //components.
> > 
> > Further red-flags: The number of HTTP URLs here for Google services.
> > 
> > Minimally, this requires documentation.
> 
> Can you please clarify why the number of HTTP URLs is a red flag? This seems
> like a product level decision.

This is more about a security-team decision, but to some extent, product has
overridden.

We actually have presubmits to make sure you're not including HTTP URLs for
Google services; Google supports HTTPS, and HTTPS supports user's privacy and
authenticity.

To the extent that there's a product level decision to override these (which,
unfortunately, there is, but which I left a remark on the bug about the
importance of documenting and potentially re-evaluating in the context of
desktop), it's important to actually clarify here in the code that there is such
a decision; that HTTP is the exception, not the norm, and the exception here is
carefully crafted.

[Alexei Svitkine (slow), 2014-07-23 14:35:27]

https://codereview.chromium.org/382313003/diff/300001/chrome/browser/chrome_b...
File chrome/browser/chrome_browser_field_trials.cc (right):

https://codereview.chromium.org/382313003/diff/300001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_field_trials.cc:76: // Governs the rollout of the
_promo_ for the compression proxy
On 2014/07/22 23:03:52, kundaji wrote:
> On 2014/07/22 13:48:23, Alexei Svitkine wrote:
> > On 2014/07/21 21:45:09, kundaji wrote:
> > > On 2014/07/21 20:25:35, Alexei Svitkine wrote:
> > > > Do we have the promo UI on all platforms already?
> > > 
> > > No. Currently, this is not used on desktop and ChromeOS.
> > 
> > Then it seems incorrect to set up a trial with 10% active group for this on
> all
> > platforms here, given that it's not implemented everywhere.
> 
> The intention was that the trial would automatically be picked up when we
> launched on more platforms. There isn't currently a good place to define the
> common method DataCompressionProxyBaseFieldTrial(...) used to set up both
> trials. While I could add a new data reduction proxy specific file, it seems
we
> will still have unused trials when we do this for desktop and not ChromeOS, so
> didn't think it was worth it. Let me know if you prefer this, or if you have
any
> alternative suggestions.

Can you specify a default percentage of 0 on platforms that don't have the promo
code wired up yet? You can do it via a small ifdef in this code.

The other possibility is to keep the first-run config in the client code around
only for mobile, where presumably first run is more important. Do we really need
first run configs in the client code for other platforms? Is the promo on those
platforms only going to be shown on first run? (I would hope not, since that
would ignore the majority of existing users.)

[Not at Google. Contact bengr, 2014-08-26 17:31:09]

The issues pointed to here have been addressed in other cls. Please take a look
again.

https://codereview.chromium.org/382313003/diff/300001/chrome/browser/chrome_b...
File chrome/browser/chrome_browser_field_trials.cc (right):

https://codereview.chromium.org/382313003/diff/300001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_field_trials.cc:76: // Governs the rollout of the
_promo_ for the compression proxy
On 2014/07/23 14:35:26, Alexei Svitkine wrote:
> On 2014/07/22 23:03:52, kundaji wrote:
> > On 2014/07/22 13:48:23, Alexei Svitkine wrote:
> > > On 2014/07/21 21:45:09, kundaji wrote:
> > > > On 2014/07/21 20:25:35, Alexei Svitkine wrote:
> > > > > Do we have the promo UI on all platforms already?
> > > > 
> > > > No. Currently, this is not used on desktop and ChromeOS.
> > > 
> > > Then it seems incorrect to set up a trial with 10% active group for this
on
> > all
> > > platforms here, given that it's not implemented everywhere.
> > 
> > The intention was that the trial would automatically be picked up when we
> > launched on more platforms. There isn't currently a good place to define the
> > common method DataCompressionProxyBaseFieldTrial(...) used to set up both
> > trials. While I could add a new data reduction proxy specific file, it seems
> we
> > will still have unused trials when we do this for desktop and not ChromeOS,
so
> > didn't think it was worth it. Let me know if you prefer this, or if you have
> any
> > alternative suggestions.
> 
> Can you specify a default percentage of 0 on platforms that don't have the
promo
> code wired up yet? You can do it via a small ifdef in this code.
> 
> The other possibility is to keep the first-run config in the client code
around
> only for mobile, where presumably first run is more important. Do we really
need
> first run configs in the client code for other platforms? Is the promo on
those
> platforms only going to be shown on first run? (I would hope not, since that
> would ignore the majority of existing users.)

We decided to remove the trial for rollout, so that simplified things. Moved
this code back to mobile, and removed the rollout trial all together.

https://codereview.chromium.org/382313003/diff/360001/chrome/browser/chrome_b...
File chrome/browser/chrome_browser_field_trials.cc (right):

https://codereview.chromium.org/382313003/diff/360001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_field_trials.cc:40: // a client-side configuration
is used.
On 2014/07/23 00:54:44, Ryan Sleevi wrote:
> On 2014/07/22 23:03:52, kundaji wrote:
> > On 2014/07/22 01:56:30, Ryan Sleevi wrote:
> > > NACK on this. Use Finch, control it by the server. We need a killbit for
> this
> > > level of experimentation.
> > 
> > Finch experiments only kick in on the second run. This is a limitation of
> Finch.
> > Without this code, a user who expects this feature won't see it on first
run. 
> > 
> > After the first run we do use Finch.
> 
> Conversely, there's no way we can turn this off for users, because they may
> always fall in to the first run bucket.
> 
> From the bug, mdw@ said there were no plans for field trials, so I find this a
> bit confusing. I do think we should err on the side of caution here, but if
> there's a concern, perhaps following up on the launch bug (ergo for the sake
of
> discussion with privacy and finch teams) might be a productive exercise.

Removed field trial for launch. Moved promo field trial back to mobile.

https://codereview.chromium.org/382313003/diff/360001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_field_trials.cc:66: << ". Selected group id: " <<
v;
On 2014/07/23 00:54:43, Ryan Sleevi wrote:
> This is incredibly spammy, and isn't necessary in a Finch world of
> server-controlled field trials (since you can determine experiment group via
> chrome://version )

Done.

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

https://codereview.chromium.org/382313003/diff/360001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:547: if
(DataReductionProxyParams::IsIncludedInFieldTrial()) {
On 2014/07/23 00:54:44, Ryan Sleevi wrote:
> Procedurally, this has the effect of enabling the feature for all users, which
> is not something we do prior to the approval of the bug (
> http://crbug.com/375484 ). Even if you were going to do a finch experiment,
you
> wouldn't be landing this code until the necessary teams have signed off, which
> doesn't seem to be the case.
> 
> See also the comments in DataCompressionProxyBaseFieldTrial, which is setup in
> such a way that some users will be opted-in for this.
> 
> The solution to this is that this code needs to remain entirely disabled for
all
> users, and a separate CL dealing with field trials and enabling the feature -
> even for a small population - need to be blocked on getting approvals and
> reviews.

Removed trial code. Launching for all users.

https://codereview.chromium.org/382313003/diff/360001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:1109: // Proxy{Server|Service}.
On 2014/07/22 23:03:52, kundaji wrote:
> On 2014/07/22 01:56:30, Ryan Sleevi wrote:
> > Based on the team, I have trouble believing this.
> > 
> > At a minimum, file a bug, and note the bug, and set a Release-Block label
with
> > an M-Stone target. Refactoring/cleanup needs to be prioritized.
> 
> Done. Filed bug 396222.

Fixed https://code.google.com/p/chromium/issues/detail?id=396699

https://codereview.chromium.org/382313003/diff/360001/chrome/browser/profiles...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/382313003/diff/360001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.cc:145: using
data_reduction_proxy::DataReductionProxyUsageStats;
On 2014/07/23 00:54:44, Ryan Sleevi wrote:
> On 2014/07/22 23:03:53, kundaji wrote:
> > On 2014/07/22 01:56:31, Ryan Sleevi wrote:
> > > General consensus is that such using statements are bad, and hinder
> > readability.
> > > 
> > > Please do not add these, and please consider removing the one that already
> > > exists.
> > 
> > Can you please clarify why using "using" for data_reduction_proxy::* is bad,
> but
> > not for content::*?
> 
> This thread (
>
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/qSrl_UAqAkQ/mn5OV...
> ) has a much larger discussion, which Peter summarizes again in
>
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/qSrl_UAqAkQ/SGDZG...
> 
> 
> From a naming perspective, naming your classes "DataReductionX" within the
> "data_reduction_proxy" namespace is something generally discouraged. 
> 
> data_reduction_proxy::Params (or ProxyParams) would be far more consistent
with
> respect to some of the other areas of code, but that's more of an aside.

Done

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

https://codereview.chromium.org/382313003/diff/360001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:27: const
char kDefaultWarmupUrl[] = "http://www.gstatic.com/generate_204";
On 2014/07/23 00:54:44, Ryan Sleevi wrote:
> On 2014/07/22 23:03:53, kundaji wrote:
> > On 2014/07/22 08:22:24, Ryan Sleevi wrote:
> > > On 2014/07/22 01:56:31, Ryan Sleevi wrote:
> > > > 95% certain that these do not belong in //components.
> > > > 
> > > > If you're going to have Google-specific endpoints, please use
> //google_apis
> > or
> > > > let the embedder supply them.
> > > > 
> > > > If that makes things difficult, it highlights that the API needs
> > reconsidering
> > > > if it is to be part of //components.
> > > 
> > > Further red-flags: The number of HTTP URLs here for Google services.
> > > 
> > > Minimally, this requires documentation.
> > 
> > Can you please clarify why the number of HTTP URLs is a red flag? This seems
> > like a product level decision.
> 
> This is more about a security-team decision, but to some extent, product has
> overridden.
> 
> We actually have presubmits to make sure you're not including HTTP URLs for
> Google services; Google supports HTTPS, and HTTPS supports user's privacy and
> authenticity.
> 
> To the extent that there's a product level decision to override these (which,
> unfortunately, there is, but which I left a remark on the bug about the
> importance of documenting and potentially re-evaluating in the context of
> desktop), it's important to actually clarify here in the code that there is
such
> a decision; that HTTP is the exception, not the norm, and the exception here
is
> carefully crafted.

Discussed in https://code.google.com/p/chromium/issues/detail?id=396700.
Hardcoded values in this component seems like the way to go instead of using a
gyp.

As for http vs https URLs, this is an explicit product decision and is outside
of my control. I am not sure any comments here will be of any use to help
someone understand the code.

[Alexei Svitkine (slow), 2014-08-27 20:14:26]

histograms lgtm

[Alexei Svitkine (slow), 2014-08-27 20:18:47]

lgtm overall for field trial code changes too

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

https://codereview.chromium.org/382313003/diff/600001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl.cc:28: 
Nit: Remove empty line.

[Ryan Sleevi, 2014-08-28 17:21:01]

rsleevi@chromium.org changed reviewers:
+ mmenke@chromium.org

[Ryan Sleevi, 2014-08-28 17:21:01]

Still having trouble with some of the layering here, as below.

I'm also a little concerned because some of these TODOs about things to do right
seem to have been missed when this code is uncommenting.

I'd like to ask mmenke@ to take a look at the //chrome/browser side, paying
particular attention to any patterns he sees are dangerous/incomplete. When we
implemented this code for mobile, we made a lot of balanaced tradeoffs, with the
assumption that these short-term solutions were suitable for mobile. I want to
make sure we've paid down the right technical debt, or are not introducing new
debt (e.g. not handling Profile's correctly).

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

https://codereview.chromium.org/382313003/diff/600001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:545:
data_reduction_proxy::DataReductionProxyParams::kFallbackAllowed;
Reformat

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

https://codereview.chromium.org/382313003/diff/600001/chrome/browser/net/chro...
chrome/browser/net/chrome_network_delegate.cc:138: // the browser preference
will be taken.
1) This comment should be updated
2) You should "do the right thing" here w/r/t |profile|

https://codereview.chromium.org/382313003/diff/600001/net/http/http_log_util.cc
File net/http/http_log_util.cc (right):

https://codereview.chromium.org/382313003/diff/600001/net/http/http_log_util....
net/http/http_log_util.cc:51: }
You still haven't removed/resolved this, which is a core concern that I raised.

https://codereview.chromium.org/382313003/diff/600001/net/http/http_log_util....
net/http/http_log_util.cc:58: if (!base::strcasecmp(header.c_str(),
"chrome-proxy")) {
This is still a layering violation.

[Not at Google. Contact bengr, 2014-08-28 18:59:28]

Thanks for the review, Ryan.

Would be great for mmenke to take a look as well. 

I have been testing these changes on desktop as well as Android. While this cl
makes the data reduction proxy functionality available on all platforms, no new
functionality is actually exercised on desktop.

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

https://codereview.chromium.org/382313003/diff/600001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:545:
data_reduction_proxy::DataReductionProxyParams::kFallbackAllowed;
On 2014/08/28 17:21:01, Ryan Sleevi wrote:
> Reformat

Done.

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

https://codereview.chromium.org/382313003/diff/600001/chrome/browser/net/chro...
chrome/browser/net/chrome_network_delegate.cc:138: // the browser preference
will be taken.
On 2014/08/28 17:21:01, Ryan Sleevi wrote:
> 1) This comment should be updated
> 2) You should "do the right thing" here w/r/t |profile|

Good catch! Done.

Note that the "received content length" are stored in the local_state pref, so
the existing argument to UpdateContentLengthPrefs is still correct.

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

https://codereview.chromium.org/382313003/diff/600001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl.cc:28: 
On 2014/08/27 20:18:47, Alexei Svitkine - OOO wrote:
> Nit: Remove empty line.

Done.

https://codereview.chromium.org/382313003/diff/600001/net/http/http_log_util.cc
File net/http/http_log_util.cc (right):

https://codereview.chromium.org/382313003/diff/600001/net/http/http_log_util....
net/http/http_log_util.cc:51: }
On 2014/08/28 17:21:01, Ryan Sleevi wrote:
> You still haven't removed/resolved this, which is a core concern that I
raised.

This is the only remainging layer violation. I thought we agreed that while this
is very important, it wasn't worth blocking the rest of this cl over. I have bug
https://code.google.com/p/chromium/issues/detail?id=396807 filed against me for
this purpose, and will get to this next. Prefer getting this cl in and then
fixing this violation.

https://codereview.chromium.org/382313003/diff/600001/net/http/http_log_util....
net/http/http_log_util.cc:58: if (!base::strcasecmp(header.c_str(),
"chrome-proxy")) {
On 2014/08/28 17:21:01, Ryan Sleevi wrote:
> This is still a layering violation.

Same as above.

[Ryan Sleevi, 2014-09-02 19:30:10]

https://codereview.chromium.org/382313003/diff/600001/net/http/http_log_util.cc
File net/http/http_log_util.cc (right):

https://codereview.chromium.org/382313003/diff/600001/net/http/http_log_util....
net/http/http_log_util.cc:51: }
On 2014/08/28 18:59:28, kundaji wrote:
> On 2014/08/28 17:21:01, Ryan Sleevi wrote:
> > You still haven't removed/resolved this, which is a core concern that I
> raised.
> 
> This is the only remainging layer violation. I thought we agreed that while
this
> is very important, it wasn't worth blocking the rest of this cl over. I have
bug
> https://code.google.com/p/chromium/issues/detail?id=396807 filed against me
for
> this purpose, and will get to this next. Prefer getting this cl in and then
> fixing this violation.
> 

M-38 has shipped. This code is for M-39, that bug is targetted for M39, I would
want to see how you intend to solve this for M39, at the least, to be
comfortable accepting new technical debt into this.

Again, to reiterate, my very real concern is that once you launch this, with the
whole host of new issues that desktop (and ChromeOS, even if that's all this
targets) introduce, and that you will spend your time chasing down bugs or
regressions rather than getting to that.

Considering this has happened in the past several times, I feel it's important
that we do the right thing here, especially with several weeks left of M39.

[Matt Welsh, 2014-09-02 19:38:51]

On 2014/09/02 19:30:10, Ryan Sleevi wrote:
>
https://codereview.chromium.org/382313003/diff/600001/net/http/http_log_util.cc
> File net/http/http_log_util.cc (right):
> 
>
https://codereview.chromium.org/382313003/diff/600001/net/http/http_log_util....
> net/http/http_log_util.cc:51: }
> On 2014/08/28 18:59:28, kundaji wrote:
> > On 2014/08/28 17:21:01, Ryan Sleevi wrote:
> > > You still haven't removed/resolved this, which is a core concern that I
> > raised.
> > 
> > This is the only remainging layer violation. I thought we agreed that while
> this
> > is very important, it wasn't worth blocking the rest of this cl over. I have
> bug
> > https://code.google.com/p/chromium/issues/detail?id=396807 filed against me
> for
> > this purpose, and will get to this next. Prefer getting this cl in and then
> > fixing this violation.
> > 
> 
> M-38 has shipped. This code is for M-39, that bug is targetted for M39, I
would
> want to see how you intend to solve this for M39, at the least, to be
> comfortable accepting new technical debt into this.
> 
> Again, to reiterate, my very real concern is that once you launch this, with
the
> whole host of new issues that desktop (and ChromeOS, even if that's all this
> targets) introduce, and that you will spend your time chasing down bugs or
> regressions rather than getting to that.
> 
> Considering this has happened in the past several times, I feel it's important
> that we do the right thing here, especially with several weeks left of M39.

Ryan, I don't see Rohit saying he's not going to address this concern in the M39
timeframe. The question is whether it should be done *in this CL* vs. the next
CL.
I think we're all on the same page that the tech debt must be paid down before
ChromeOS or desktop support ships in any milestone.

[Ryan Sleevi, 2014-09-02 20:08:10]

On 2014/09/02 19:38:51, Matt Welsh wrote:
> On 2014/09/02 19:30:10, Ryan Sleevi wrote:
> >
>
https://codereview.chromium.org/382313003/diff/600001/net/http/http_log_util.cc
> > File net/http/http_log_util.cc (right):
> > 
> >
>
https://codereview.chromium.org/382313003/diff/600001/net/http/http_log_util....
> > net/http/http_log_util.cc:51: }
> > On 2014/08/28 18:59:28, kundaji wrote:
> > > On 2014/08/28 17:21:01, Ryan Sleevi wrote:
> > > > You still haven't removed/resolved this, which is a core concern that I
> > > raised.
> > > 
> > > This is the only remainging layer violation. I thought we agreed that
while
> > this
> > > is very important, it wasn't worth blocking the rest of this cl over. I
have
> > bug
> > > https://code.google.com/p/chromium/issues/detail?id=396807 filed against
me
> > for
> > > this purpose, and will get to this next. Prefer getting this cl in and
then
> > > fixing this violation.
> > > 
> > 
> > M-38 has shipped. This code is for M-39, that bug is targetted for M39, I
> would
> > want to see how you intend to solve this for M39, at the least, to be
> > comfortable accepting new technical debt into this.
> > 
> > Again, to reiterate, my very real concern is that once you launch this, with
> the
> > whole host of new issues that desktop (and ChromeOS, even if that's all this
> > targets) introduce, and that you will spend your time chasing down bugs or
> > regressions rather than getting to that.
> > 
> > Considering this has happened in the past several times, I feel it's
important
> > that we do the right thing here, especially with several weeks left of M39.
> 
> Ryan, I don't see Rohit saying he's not going to address this concern in the
M39
> timeframe. The question is whether it should be done *in this CL* vs. the next
> CL.
> I think we're all on the same page that the tech debt must be paid down before
> ChromeOS or desktop support ships in any milestone.

I wanted to make sure we're on the same page, and that this isn't being
targetted for M38.

I think the response is still the same - I'd rather see the blocking issue first
dealt with (outside of this CL) to be able to review this CL in the broader
context. I don't think I feel comfortable allowing this CL without that
dependent going through. This is fairly normal operation for when a review
identifies something - first fix the other thing (in a separate CL, as needed)
and then revisit the parent.

[Not at Google. Contact bengr, 2014-09-04 22:45:40]

https://codereview.chromium.org/382313003/diff/600001/net/http/http_log_util.cc
File net/http/http_log_util.cc (right):

https://codereview.chromium.org/382313003/diff/600001/net/http/http_log_util....
net/http/http_log_util.cc:51: }
On 2014/09/02 19:30:10, Ryan Sleevi wrote:
> On 2014/08/28 18:59:28, kundaji wrote:
> > On 2014/08/28 17:21:01, Ryan Sleevi wrote:
> > > You still haven't removed/resolved this, which is a core concern that I
> > raised.
> > 
> > This is the only remainging layer violation. I thought we agreed that while
> this
> > is very important, it wasn't worth blocking the rest of this cl over. I have
> bug
> > https://code.google.com/p/chromium/issues/detail?id=396807 filed against me
> for
> > this purpose, and will get to this next. Prefer getting this cl in and then
> > fixing this violation.
> > 
> 
> M-38 has shipped. This code is for M-39, that bug is targetted for M39, I
would
> want to see how you intend to solve this for M39, at the least, to be
> comfortable accepting new technical debt into this.
> 
> Again, to reiterate, my very real concern is that once you launch this, with
the
> whole host of new issues that desktop (and ChromeOS, even if that's all this
> targets) introduce, and that you will spend your time chasing down bugs or
> regressions rather than getting to that.
> 
> Considering this has happened in the past several times, I feel it's important
> that we do the right thing here, especially with several weeks left of M39.

I am in the process of finding an approach to this with help from net-dev.

Is this the only outstanding issue for this cl? If not, can you please review
the rest of it so that once we remove the layering violation in this file, this
cl is good to go.

This cl is the first of a few we need to land to have the Flywheel working on
desktop in M39, so this cl needs to land as soon as possible rather than towards
the end of M39.

[bengr, 2014-09-05 02:59:26]

mmenke: sleevi and I discussed the CL, and I suggested asking you to do the
review, since you are more familiar with the changes than sleevi. If you're ok
with doing this, please also take a high-level view over it and makes sure we
are comfortable with the tradeoffs made, as well as identify what things we need
to change now, and what in the future.

sleevi: as discussed, please also take a high-level view.

[Not at Google. Contact bengr, 2014-09-05 05:03:54]

I've removed the code in http_log_util.cc which removed the chrome-proxy header
from the logs. This code is no longer required since we recently made a change
which renders the credentials in the headers useless after atmost 24 hours.

So the layering violation we were concerned about has been removed. Please take
a look.

[mmenke, 2014-09-05 16:29:10]

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

https://codereview.chromium.org/382313003/diff/660001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:540: ChromeNetworkDelegate* network_delegate) {
Definition order should match declaration order.

https://codereview.chromium.org/382313003/diff/660001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:544: IsIncludedInAlternativeFieldTrial())
Use braces in multi-line if statements (x3).

https://codereview.chromium.org/382313003/diff/660001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:657: SetupDataReductionProxy(network_delegate);
Question:  The NetworkDelegate for system context doesn't have a profile, so
ChromeNetworkDelegate ends up not recording any stats.  Is this a problem?

https://codereview.chromium.org/382313003/diff/660001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:657: SetupDataReductionProxy(network_delegate);
Also, we never call InitDataReductionProxySettings on these settings.  Does that
mean the data reduction proxy is always or never enabled for the system context?

https://codereview.chromium.org/382313003/diff/660001/net/http/http_log_util_...
File net/http/http_log_util_unittest.cc (right):

https://codereview.chromium.org/382313003/diff/660001/net/http/http_log_util_...
net/http/http_log_util_unittest.cc:69: "Chrome-Proxy", "ps=123, c=foo, v=bar,
sid=456"));
Actually, you should just delete these 3 expects, since they test the code you
removed.

https://codereview.chromium.org/382313003/diff/660001/net/http/http_proxy_cli...
File net/http/http_proxy_client_socket_pool.cc (right):

https://codereview.chromium.org/382313003/diff/660001/net/http/http_proxy_cli...
net/http/http_proxy_client_socket_pool.cc:71: // Proxy connect timeout should be
independent of platform and be based on
nit:  This should be prefixed with "TODO(kundaji):".  Note that this is not a
promise that you'll be the one to do it, or that you ever have any intentions of
doing it, just a pointer about who has context on the TODO.

https://codereview.chromium.org/382313003/diff/660001/net/http/http_proxy_cli...
net/http/http_proxy_client_socket_pool.cc:73: #if (defined(OS_ANDROID) ||
defined(OS_IOS))
nit:  outer parentheses not needed.

[Not at Google. Contact bengr, 2014-09-05 21:24:36]

Matt, thank you very much for the quick review. PTAL.

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

https://codereview.chromium.org/382313003/diff/660001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:540: ChromeNetworkDelegate* network_delegate) {
On 2014/09/05 16:29:09, mmenke wrote:
> Definition order should match declaration order.

Done.

https://codereview.chromium.org/382313003/diff/660001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:544: IsIncludedInAlternativeFieldTrial())
On 2014/09/05 16:29:10, mmenke wrote:
> Use braces in multi-line if statements (x3).

Done.

https://codereview.chromium.org/382313003/diff/660001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:657: SetupDataReductionProxy(network_delegate);
On 2014/09/05 16:29:09, mmenke wrote:
> Question:  The NetworkDelegate for system context doesn't have a profile, so
> ChromeNetworkDelegate ends up not recording any stats.  Is this a problem?

Currently, data reduction proxy is turned on and off on a per profile basis.
However, stats are recorded globally in local state prefs. We plan to move the
stats to be per profile. Bug 
https://code.google.com/p/chromium/issues/detail?id=411373 tracks this.

Also, right now we never turn on data reduction for the system context. Our plan
is to turn it on for the system if it is turned on for the currently active
profile, but this cl doesn't get us there.

https://codereview.chromium.org/382313003/diff/660001/net/http/http_log_util_...
File net/http/http_log_util_unittest.cc (right):

https://codereview.chromium.org/382313003/diff/660001/net/http/http_log_util_...
net/http/http_log_util_unittest.cc:69: "Chrome-Proxy", "ps=123, c=foo, v=bar,
sid=456"));
On 2014/09/05 16:29:10, mmenke wrote:
> Actually, you should just delete these 3 expects, since they test the code you
> removed.

Done.

https://codereview.chromium.org/382313003/diff/660001/net/http/http_proxy_cli...
File net/http/http_proxy_client_socket_pool.cc (right):

https://codereview.chromium.org/382313003/diff/660001/net/http/http_proxy_cli...
net/http/http_proxy_client_socket_pool.cc:71: // Proxy connect timeout should be
independent of platform and be based on
On 2014/09/05 16:29:10, mmenke wrote:
> nit:  This should be prefixed with "TODO(kundaji):".  Note that this is not a
> promise that you'll be the one to do it, or that you ever have any intentions
of
> doing it, just a pointer about who has context on the TODO.

Done.

https://codereview.chromium.org/382313003/diff/660001/net/http/http_proxy_cli...
net/http/http_proxy_client_socket_pool.cc:73: #if (defined(OS_ANDROID) ||
defined(OS_IOS))
On 2014/09/05 16:29:10, mmenke wrote:
> nit:  outer parentheses not needed.

Done.

https://codereview.chromium.org/382313003/diff/740001/components/data_reducti...
File components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc
(right):

https://codereview.chromium.org/382313003/diff/740001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:77:
scoped_refptr<MessageLoopProxy> ui_thread_proxy)
FYI, I had to make this change to get it to build on desktop. Unless I change
MessageLoopProxy* to scoped_refptr<MessageLoopProxy>, this compiles only for
Android, and not desktop.

This was necessasitated by cl https://codereview.chromium.org/510323002

[Ryan Sleevi, 2014-09-05 21:28:31]

https://codereview.chromium.org/382313003/diff/740001/components/data_reducti...
File components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc
(right):

https://codereview.chromium.org/382313003/diff/740001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:77:
scoped_refptr<MessageLoopProxy> ui_thread_proxy)
On 2014/09/05 21:24:35, kundaji wrote:
> FYI, I had to make this change to get it to build on desktop. Unless I change
> MessageLoopProxy* to scoped_refptr<MessageLoopProxy>, this compiles only for
> Android, and not desktop.
> 
> This was necessasitated by cl https://codereview.chromium.org/510323002

This should be a const-ref. The only pass by value type is scoped_ptr - all else
get const-ref, including scoped_refptr

[Not at Google. Contact bengr, 2014-09-05 22:08:58]

https://codereview.chromium.org/382313003/diff/740001/components/data_reducti...
File components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc
(right):

https://codereview.chromium.org/382313003/diff/740001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:77:
scoped_refptr<MessageLoopProxy> ui_thread_proxy)
On 2014/09/05 21:28:31, Ryan Sleevi wrote:
> On 2014/09/05 21:24:35, kundaji wrote:
> > FYI, I had to make this change to get it to build on desktop. Unless I
change
> > MessageLoopProxy* to scoped_refptr<MessageLoopProxy>, this compiles only for
> > Android, and not desktop.
> > 
> > This was necessasitated by cl https://codereview.chromium.org/510323002
> 
> This should be a const-ref. The only pass by value type is scoped_ptr - all
else
> get const-ref, including scoped_refptr

Done. For my own edification, can you please explain the reasoning behind this
rule.

[Ryan Sleevi, 2014-09-05 22:15:55]

On 2014/09/05 22:08:58, kundaji wrote:
>
https://codereview.chromium.org/382313003/diff/740001/components/data_reducti...
> File
components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc
> (right):
> 
>
https://codereview.chromium.org/382313003/diff/740001/components/data_reducti...
>
components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:77:
> scoped_refptr<MessageLoopProxy> ui_thread_proxy)
> On 2014/09/05 21:28:31, Ryan Sleevi wrote:
> > On 2014/09/05 21:24:35, kundaji wrote:
> > > FYI, I had to make this change to get it to build on desktop. Unless I
> change
> > > MessageLoopProxy* to scoped_refptr<MessageLoopProxy>, this compiles only
for
> > > Android, and not desktop.
> > > 
> > > This was necessasitated by cl https://codereview.chromium.org/510323002
> > 
> > This should be a const-ref. The only pass by value type is scoped_ptr - all
> else
> > get const-ref, including scoped_refptr
> 
> Done. For my own edification, can you please explain the reasoning behind this
> rule.

See the Chromium/Google style guide for general notes about const-ref
Scoped_ptr is the one exception, due to our C++11 move emulation

As it specifically applies to scoped_refptr, it avoids an unnecessary refcount
churn.

[mmenke, 2014-09-08 18:11:14]

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

https://codereview.chromium.org/382313003/diff/800001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:1119: }
Should all this fieldtrial / flags stuff be moved into DataReductionProxyParams
(And get rid of the flag argument), or is there a reason it all needs to be
here?  No need to worry about it in this CL, just wondering if it needs to be
exposed to the embedder.

We currently have exactly duplicated code in
data_reduction_proxy_chrome_settings_factory.cc, and we should definitely unify
the code, at some point.

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

https://codereview.chromium.org/382313003/diff/800001/chrome/browser/io_threa...
chrome/browser/io_thread.h:202: data_reduction_proxy_auth_request_handler;
nit:  These 4 lines should all be indented by 2 more.

https://codereview.chromium.org/382313003/diff/800001/chrome/browser/profiles...
File chrome/browser/profiles/profile_io_data.h (right):

https://codereview.chromium.org/382313003/diff/800001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.h:253: bool
IsDataReductionProxyEnabled() const;
Is this used anywhere?  If so, I don't see any uses of it where the surrounding
OS_ANDROID has been removed in this CL.  If not, we should probably get rid of
it until/unless we need it.

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

https://codereview.chromium.org/382313003/diff/800001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:38: }  //
namespace anonymous
nit:  --anonymous

https://codereview.chromium.org/382313003/diff/800001/net/http/http_proxy_cli...
File net/http/http_proxy_client_socket_pool.cc (right):

https://codereview.chromium.org/382313003/diff/800001/net/http/http_proxy_cli...
net/http/http_proxy_client_socket_pool.cc:72: // based on proxy. Bug 407446.
nit:  Just for the lazy, should probably have a link here (i.e.
http://crbug.com/407446)

[Not at Google. Contact bengr, 2014-09-08 19:18:35]

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

https://codereview.chromium.org/382313003/diff/800001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:1119: }
On 2014/09/08 18:11:14, mmenke wrote:
> Should all this fieldtrial / flags stuff be moved into
DataReductionProxyParams
> (And get rid of the flag argument), or is there a reason it all needs to be
> here?  No need to worry about it in this CL, just wondering if it needs to be
> exposed to the embedder.
> 
> We currently have exactly duplicated code in
> data_reduction_proxy_chrome_settings_factory.cc, and we should definitely
unify
> the code, at some point.

Added a TODO.

I don't see any reason for it to be exposed to embedder and this should go in
DRPParams as you point out.

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

https://codereview.chromium.org/382313003/diff/800001/chrome/browser/io_threa...
chrome/browser/io_thread.h:202: data_reduction_proxy_auth_request_handler;
On 2014/09/08 18:11:14, mmenke wrote:
> nit:  These 4 lines should all be indented by 2 more.

Done.

https://codereview.chromium.org/382313003/diff/800001/chrome/browser/profiles...
File chrome/browser/profiles/profile_io_data.h (right):

https://codereview.chromium.org/382313003/diff/800001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.h:253: bool
IsDataReductionProxyEnabled() const;
On 2014/09/08 18:11:14, mmenke wrote:
> Is this used anywhere?  If so, I don't see any uses of it where the
surrounding
> OS_ANDROID has been removed in this CL.  If not, we should probably get rid of
> it until/unless we need it.

No current uses. Removed.

The intention was to switch all uses of data_reduction_proxy_enabled() with this
call(see comment on line 180). Discussed with Ben, and he said it was fine to
remove.

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

https://codereview.chromium.org/382313003/diff/800001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:38: }  //
namespace anonymous
On 2014/09/08 18:11:14, mmenke wrote:
> nit:  --anonymous

Done.

https://codereview.chromium.org/382313003/diff/800001/net/http/http_proxy_cli...
File net/http/http_proxy_client_socket_pool.cc (right):

https://codereview.chromium.org/382313003/diff/800001/net/http/http_proxy_cli...
net/http/http_proxy_client_socket_pool.cc:72: // based on proxy. Bug 407446.
On 2014/09/08 18:11:14, mmenke wrote:
> nit:  Just for the lazy, should probably have a link here (i.e.
> http://crbug.com/407446)

Done.

[Not at Google. Contact bengr, 2014-09-08 19:29:38]

https://codereview.chromium.org/382313003/diff/800001/chrome/browser/profiles...
File chrome/browser/profiles/profile_io_data.h (right):

https://codereview.chromium.org/382313003/diff/800001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.h:253: bool
IsDataReductionProxyEnabled() const;
On 2014/09/08 19:18:35, kundaji wrote:
> On 2014/09/08 18:11:14, mmenke wrote:
> > Is this used anywhere?  If so, I don't see any uses of it where the
> surrounding
> > OS_ANDROID has been removed in this CL.  If not, we should probably get rid
of
> > it until/unless we need it.
> 
> No current uses. Removed.
> 
> The intention was to switch all uses of data_reduction_proxy_enabled() with
this
> call(see comment on line 180). Discussed with Ben, and he said it was fine to
> remove.

Upon review, this is used in chrome_resource_dispatcher_host_delegate.cc. Did
not notice earlier since I built for desktop, but caught this when testing for
Android.
https://cs.corp.google.com/#clankium/src/chrome/browser/renderer_host/chrome_...

[Ryan Sleevi, 2014-09-08 20:58:55]

On 2014/09/08 19:29:38, kundaji wrote:
>
https://codereview.chromium.org/382313003/diff/800001/chrome/browser/profiles...
> File chrome/browser/profiles/profile_io_data.h (right):
> 
>
https://codereview.chromium.org/382313003/diff/800001/chrome/browser/profiles...
> chrome/browser/profiles/profile_io_data.h:253: bool
> IsDataReductionProxyEnabled() const;
> On 2014/09/08 19:18:35, kundaji wrote:
> > On 2014/09/08 18:11:14, mmenke wrote:
> > > Is this used anywhere?  If so, I don't see any uses of it where the
> > surrounding
> > > OS_ANDROID has been removed in this CL.  If not, we should probably get
rid
> of
> > > it until/unless we need it.
> > 
> > No current uses. Removed.
> > 
> > The intention was to switch all uses of data_reduction_proxy_enabled() with
> this
> > call(see comment on line 180). Discussed with Ben, and he said it was fine
to
> > remove.
> 
> Upon review, this is used in chrome_resource_dispatcher_host_delegate.cc. Did
> not notice earlier since I built for desktop, but caught this when testing for
> Android.
>
https://cs.corp.google.com/#clankium/src/chrome/browser/renderer_host/chrome_...

Don't post internal URLs.

You got lucky, this time -
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ren...
is in the public sources.

[chromium-reviews, 2014-09-08 21:05:55]

Oops. Thanks for the reminder.

On Mon, Sep 8, 2014 at 1:58 PM, <rsleevi@chromium.org> wrote:

> On 2014/09/08 19:29:38, kundaji wrote:
>
> https://codereview.chromium.org/382313003/diff/800001/
> chrome/browser/profiles/profile_io_data.h
>
>> File chrome/browser/profiles/profile_io_data.h (right):
>>
>
>
> https://codereview.chromium.org/382313003/diff/800001/
> chrome/browser/profiles/profile_io_data.h#newcode253
>
>> chrome/browser/profiles/profile_io_data.h:253: bool
>> IsDataReductionProxyEnabled() const;
>> On 2014/09/08 19:18:35, kundaji wrote:
>> > On 2014/09/08 18:11:14, mmenke wrote:
>> > > Is this used anywhere?  If so, I don't see any uses of it where the
>> > surrounding
>> > > OS_ANDROID has been removed in this CL.  If not, we should probably
>> get
>>
> rid
>
>> of
>> > > it until/unless we need it.
>> >
>> > No current uses. Removed.
>> >
>> > The intention was to switch all uses of data_reduction_proxy_enabled()
>> with
>> this
>> > call(see comment on line 180). Discussed with Ben, and he said it was
>> fine
>>
> to
>
>> > remove.
>>
>
>  Upon review, this is used in chrome_resource_dispatcher_host_delegate.cc.
>> Did
>> not notice earlier since I built for desktop, but caught this when
>> testing for
>> Android.
>>
>
> https://cs.corp.google.com/#clankium/src/chrome/browser/
> renderer_host/chrome_resource_dispatcher_host_delegate.cc&type=cs&q=
> IsDataReductionProxyEnabled%20package:%5Eclankium$%20file:
> (%5Esrc/chrome/browser/renderer_host/)&l=493
>
> Don't post internal URLs.
>
> You got lucky, this time -
> https://code.google.com/p/chromium/codesearch#chromium/
> src/chrome/browser/renderer_host/chrome_resource_
> dispatcher_host_delegate.cc&l=493
> is in the public sources.
>
>
> https://codereview.chromium.org/382313003/
>

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

[mmenke, 2014-09-10 15:16:48]

I think this looks reasonable, with the caveat (mentioned on other CLs as well)
that in a future cleanup, I'd really like chrome/ to only use one or two classes
(One on the IO thread, one on the UI thread) to interact with flywheel, rather
than knowing the details of how 6 classes interact with each other.

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

https://codereview.chromium.org/382313003/diff/860001/chrome/browser/io_threa...
chrome/browser/io_thread.h:198: scoped_ptr<chrome_browser_net::DnsProbeService>
dns_probe_service;
Can we get rid of the SPDY_PROXY_AUTH_ORIGIN define entirely now (As in remove
it from the build system)?

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

https://codereview.chromium.org/382313003/diff/860001/chrome/browser/net/chro...
chrome/browser/net/chrome_network_delegate.cc:139:
data_reduction_proxy::prefs::kDataReductionProxyEnabled);
Could we just move this into UpdateContentLengthPrefs?  My general feeling is
that, with the possible exception of actually configuring the pref, it makes the
most sense to either only read the pref in the component, or only read it in the
component, rather than to split the responsibility of checking it between the
two.

Does mean "UpdateContentLengthPrefs" has to distingush between local state
prefs, and per user prefs, which isn't lovely...but we have to do that, here,
anyways.

https://codereview.chromium.org/382313003/diff/860001/chrome/browser/net/chro...
chrome/browser/net/chrome_network_delegate.cc:144: request_type, prefs);
While you're here, mind fixing the indent here?

https://codereview.chromium.org/382313003/diff/860001/chrome/browser/profiles...
File chrome/browser/profiles/off_the_record_profile_io_data.cc (right):

https://codereview.chromium.org/382313003/diff/860001/chrome/browser/profiles...
chrome/browser/profiles/off_the_record_profile_io_data.cc:166:
io_data_->data_reduction_proxy_enabled()->Init(
You said the plan was to remove the ability to enable this for incognito?  Maybe
add a TODO about that?

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

https://codereview.chromium.org/382313003/diff/860001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:36: const
char kDefaultWarmupUrl[] = "http://www.gstatic.com/generate_204";
Should these URLs be in /components/google/core/browser/google_util?

We can either make sure they're empty for unofficial builds, or even keep them
in the internal repo (No so much for secrecy, but to make absolutely sure
they're only used by our official builds).

The latter option also at least makes it theoretically easier for third parties
to plug in their own, though this code isn't exactly intended to be general
purpose.

[Not at Google. Contact bengr, 2014-09-10 21:38:34]

Having chrome/ use one or two classes to interact with DRP is one of the clean
up tasks we have discussed internally.

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

https://codereview.chromium.org/382313003/diff/860001/chrome/browser/io_threa...
chrome/browser/io_thread.h:198: scoped_ptr<chrome_browser_net::DnsProbeService>
dns_probe_service;
On 2014/09/10 15:16:48, mmenke wrote:
> Can we get rid of the SPDY_PROXY_AUTH_ORIGIN define entirely now (As in remove
> it from the build system)?

Yes, that is the plan. I have already removed it from chromium/src code. It
Needs to be removed from Android and iOS code as well as all the gyp files.
Filed bug http://crbug/412890

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

https://codereview.chromium.org/382313003/diff/860001/chrome/browser/net/chro...
chrome/browser/net/chrome_network_delegate.cc:139:
data_reduction_proxy::prefs::kDataReductionProxyEnabled);
On 2014/09/10 15:16:48, mmenke wrote:
> Could we just move this into UpdateContentLengthPrefs?  My general feeling is
> that, with the possible exception of actually configuring the pref, it makes
the
> most sense to either only read the pref in the component, or only read it in
the
> component, rather than to split the responsibility of checking it between the
> two.
> 
> Does mean "UpdateContentLengthPrefs" has to distingush between local state
> prefs, and per user prefs, which isn't lovely...but we have to do that, here,
> anyways.

Done.

In the future we want to move off of local state completely and only use the
profile specific prefs.

https://codereview.chromium.org/382313003/diff/860001/chrome/browser/net/chro...
chrome/browser/net/chrome_network_delegate.cc:144: request_type, prefs);
On 2014/09/10 15:16:48, mmenke wrote:
> While you're here, mind fixing the indent here?

Done.

https://codereview.chromium.org/382313003/diff/860001/chrome/browser/profiles...
File chrome/browser/profiles/off_the_record_profile_io_data.cc (right):

https://codereview.chromium.org/382313003/diff/860001/chrome/browser/profiles...
chrome/browser/profiles/off_the_record_profile_io_data.cc:166:
io_data_->data_reduction_proxy_enabled()->Init(
On 2014/09/10 15:16:48, mmenke wrote:
> You said the plan was to remove the ability to enable this for incognito? 
Maybe
> add a TODO about that?

Done. Created bug http://crbug/412873.

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

https://codereview.chromium.org/382313003/diff/860001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:36: const
char kDefaultWarmupUrl[] = "http://www.gstatic.com/generate_204";
On 2014/09/10 15:16:48, mmenke wrote:
> Should these URLs be in /components/google/core/browser/google_util?
> 
> We can either make sure they're empty for unofficial builds, or even keep them
> in the internal repo (No so much for secrecy, but to make absolutely sure
> they're only used by our official builds).
> 
> The latter option also at least makes it theoretically easier for third
parties
> to plug in their own, though this code isn't exactly intended to be general
> purpose.

Not sure why you mention putting URLs in
/components/google/core/browser/google_util? The data reduction component seems
more self contained if the URLs are here.

Keeping the URLs in internal repo means that we need to have gyp conditionals
which does not seem ideal.

Note that the SPDY_PROXY_AUTH_VALUE will continue to live in gypi files and is
internal only. Data reduction proxy cannot be used without this value. I have a
bug assigned to me to ensure that data reduction functionality does not get
initialized when the auth value is missing.

Being able to reuse DRP component code with other similar non-google provided
service is not a current design goal.

[mmenke, 2014-09-10 21:46:34]

LGTM

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

https://codereview.chromium.org/382313003/diff/860001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:36: const
char kDefaultWarmupUrl[] = "http://www.gstatic.com/generate_204";
On 2014/09/10 21:38:34, kundaji wrote:
> On 2014/09/10 15:16:48, mmenke wrote:
> > Should these URLs be in /components/google/core/browser/google_util?
> > 
> > We can either make sure they're empty for unofficial builds, or even keep
them
> > in the internal repo (No so much for secrecy, but to make absolutely sure
> > they're only used by our official builds).
> > 
> > The latter option also at least makes it theoretically easier for third
> parties
> > to plug in their own, though this code isn't exactly intended to be general
> > purpose.
> 
> Not sure why you mention putting URLs in
> /components/google/core/browser/google_util? The data reduction component
seems
> more self contained if the URLs are here.
> 
> Keeping the URLs in internal repo means that we need to have gyp conditionals
> which does not seem ideal.
> 
> Note that the SPDY_PROXY_AUTH_VALUE will continue to live in gypi files and is
> internal only. Data reduction proxy cannot be used without this value. I have
a
> bug assigned to me to ensure that data reduction functionality does not get
> initialized when the auth value is missing.
> 
> Being able to reuse DRP component code with other similar non-google provided
> service is not a current design goal.

google_util is nominally where the google-specific URLs all hang out, though
there are certainly a huge number of violations of that.  It also ensures that
on Chromium/third-party builds, none of these URLs are even defined, so no
matter how badly third parties mangle the code, they won't accidentally end up
spamming requests to these URLs.  Anyhow, not something I'm going to insist on.

[Ryan Sleevi, 2014-09-10 23:11:24]

Matt's LGTM'd, so clearing my not.

Expect howler monkeys sent to screech and yell if some of these TODOs aren't
resolved soon, though. Don't ask where I'll get them. I know a guy.

[Not at Google. Contact bengr, 2014-09-10 23:13:24]

Thanks all!

[Not at Google. Contact bengr, 2014-09-10 23:13:46]

The CQ bit was checked by kundaji@chromium.org

[commit-bot: I haz the power, 2014-09-10 23:18:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/382313003/940001

[commit-bot: I haz the power, 2014-09-11 00:03:56]

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

[commit-bot: I haz the power, 2014-09-11 00:03:58]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
  android_clang_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)

[Not at Google. Contact bengr, 2014-09-12 18:41:11]

The CQ bit was checked by kundaji@chromium.org

[commit-bot: I haz the power, 2014-09-12 18:49:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/382313003/1000001

[commit-bot: I haz the power, 2014-09-12 20:34:33]

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

[commit-bot: I haz the power, 2014-09-12 20:34:35]

Try jobs failed on following builders:
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Not at Google. Contact bengr, 2014-09-13 00:32:05]

The CQ bit was checked by kundaji@chromium.org

[commit-bot: I haz the power, 2014-09-13 00:32:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/382313003/1020001

[commit-bot: I haz the power, 2014-09-13 03:14:27]

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

[commit-bot: I haz the power, 2014-09-13 03:14:28]

Try jobs failed on following builders:
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Not at Google. Contact bengr, 2014-09-22 23:26:09]

kundaji@chromium.org changed reviewers:
+ benm@chromium.org

[Not at Google. Contact bengr, 2014-09-22 23:26:31]

kundaji@chromium.org changed reviewers:
- mef@chromium.org

[Not at Google. Contact bengr, 2014-09-22 23:27:59]

benm@: android_webview/*

[boliu, 2014-09-23 00:08:47]

android_webview rs lgtm

[Not at Google. Contact bengr, 2014-09-23 00:09:25]

kundaji@chromium.org changed reviewers:
+ boliu@chromium.org
- benm@chromium.org

[Not at Google. Contact bengr, 2014-09-23 01:27:04]

The CQ bit was checked by kundaji@chromium.org

[commit-bot: I haz the power, 2014-09-23 01:28:13]

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

[commit-bot: I haz the power, 2014-09-23 01:35:32]

Committed patchset #54 (id:1060001) as 769f1352106188dae7ddcb4f765634606a63fe9a

[commit-bot: I haz the power, 2014-09-23 01:36:29]

Patchset 54 (id:??) landed as
https://crrev.com/88ca41196a5d2db1554d3848af0fe5038873a937
Cr-Commit-Position: refs/heads/master@{#296122}