Add probe to warm data reduction proxy

Add probe to warm data reduction proxy

On startup and on network changes, a request is made to the data reduction proxy to warm the connection. This reduces the page load time for first loads.

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

[bengr, 2014-06-16 05:34:32]

mef: chrome_network_delegate.cc
marq: components/data_reduction_proxy/*

[mef, 2014-06-16 15:04:25]

The CL could use better description.
chrome_network_delegate.cc lgtm.

[marq (ping after 24h), 2014-06-16 17:23:35]

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

https://codereview.chromium.org/338723002/diff/40001/components/data_reductio...
components/data_reduction_proxy/browser/data_reduction_proxy_settings.cc:312: 
DCHECK(source == fetcher_.get()), so that adding another fetcher and not
handling it here is a programming error.

https://codereview.chromium.org/338723002/diff/40001/components/data_reductio...
components/data_reduction_proxy/browser/data_reduction_proxy_settings.cc:611:
net::URLFetcher* DataReductionProxySettings::GetURLFetcher() {
I think you can tighten things up a bit and roll the specific fetcher getters
into ProbeWhetherDataReductionProxyIsAvailable() and WarmProxyConnection(), and
just use GetBaseURLFetcher as the injection point for mocks in the tests.
Because the URL gets passed into GetBaseURLFetcher(), the mocking code can
expect calls with specific inputs and return the right response for each URL.

https://codereview.chromium.org/338723002/diff/40001/components/data_reductio...
components/data_reduction_proxy/browser/data_reduction_proxy_settings.cc:627:
net::LOAD_DISABLE_CACHE);
This will fit on one line.

https://codereview.chromium.org/338723002/diff/40001/components/data_reductio...
File components/data_reduction_proxy/browser/data_reduction_proxy_settings.h
(right):

https://codereview.chromium.org/338723002/diff/40001/components/data_reductio...
components/data_reduction_proxy/browser/data_reduction_proxy_settings.h:176: //
Returns a fetcher for the probe to check if OK to use SPDY.
This probe checks for carrier blocking, right? Not SPDY.

Should this be GetURLFetcherForAvailabiliyCheck() or something?

Add a note that the difference between this fetcher and the proxy-warmup fetcher
is that this one is flagged to bypass proxy settings.

Add a note that these methods are virtualized for testing.

https://codereview.chromium.org/338723002/diff/40001/components/data_reductio...
components/data_reduction_proxy/browser/data_reduction_proxy_settings.h:296:
scoped_ptr<net::URLFetcher> fetcher_;
Rename to (e.g.) availabilty_check_fetcher_

https://codereview.chromium.org/338723002/diff/40001/components/data_reductio...
File
components/data_reduction_proxy/browser/data_reduction_proxy_settings_test_utils.h
(right):

https://codereview.chromium.org/338723002/diff/40001/components/data_reductio...
components/data_reduction_proxy/browser/data_reduction_proxy_settings_test_utils.h:111:
const std::string& warming_test_url,
Be consistent in your wording: warmup vs. warming, here and elsewhere.

[bengr, 2014-06-17 03:42:10]

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

https://codereview.chromium.org/338723002/diff/40001/components/data_reductio...
components/data_reduction_proxy/browser/data_reduction_proxy_settings.cc:312: 
On 2014/06/16 17:23:35, marq_ wrote:
> DCHECK(source == fetcher_.get()), so that adding another fetcher and not
> handling it here is a programming error.

Done.

https://codereview.chromium.org/338723002/diff/40001/components/data_reductio...
components/data_reduction_proxy/browser/data_reduction_proxy_settings.cc:611:
net::URLFetcher* DataReductionProxySettings::GetURLFetcher() {
On 2014/06/16 17:23:34, marq_ wrote:
> I think you can tighten things up a bit and roll the specific fetcher getters
> into ProbeWhetherDataReductionProxyIsAvailable() and WarmProxyConnection(),
and
> just use GetBaseURLFetcher as the injection point for mocks in the tests.
> Because the URL gets passed into GetBaseURLFetcher(), the mocking code can
> expect calls with specific inputs and return the right response for each URL.

This is the way it is to make testing easier.

https://codereview.chromium.org/338723002/diff/40001/components/data_reductio...
components/data_reduction_proxy/browser/data_reduction_proxy_settings.cc:627:
net::LOAD_DISABLE_CACHE);
On 2014/06/16 17:23:35, marq_ wrote:
> This will fit on one line.

Done.

https://codereview.chromium.org/338723002/diff/40001/components/data_reductio...
File components/data_reduction_proxy/browser/data_reduction_proxy_settings.h
(right):

https://codereview.chromium.org/338723002/diff/40001/components/data_reductio...
components/data_reduction_proxy/browser/data_reduction_proxy_settings.h:176: //
Returns a fetcher for the probe to check if OK to use SPDY.
On 2014/06/16 17:23:35, marq_ wrote:
> This probe checks for carrier blocking, right? Not SPDY.
> 
> Should this be GetURLFetcherForAvailabiliyCheck() or something?
> 
> Add a note that the difference between this fetcher and the proxy-warmup
fetcher
> is that this one is flagged to bypass proxy settings.
> 
> Add a note that these methods are virtualized for testing.

We've considered reasons other than carrier blocking to disallow SPDY, so I left
the comment more or less as is, but added the note about it being virtual for
testing. I changed the name of the function.

https://codereview.chromium.org/338723002/diff/40001/components/data_reductio...
File
components/data_reduction_proxy/browser/data_reduction_proxy_settings_test_utils.h
(right):

https://codereview.chromium.org/338723002/diff/40001/components/data_reductio...
components/data_reduction_proxy/browser/data_reduction_proxy_settings_test_utils.h:111:
const std::string& warming_test_url,
On 2014/06/16 17:23:35, marq_ wrote:
> Be consistent in your wording: warmup vs. warming, here and elsewhere.

Done.

[marq (ping after 24h), 2014-06-18 00:03:18]

lgtm

[bengr, 2014-06-18 16:44:26]

The CQ bit was checked by bengr@chromium.org

[commit-bot: I haz the power, 2014-06-18 16:45:05]

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

[commit-bot: I haz the power, 2014-06-19 01:03:23]

Change committed as 278210