Make Data Saver proxy bypass logic apply to redirects.

Make Data Saver proxy bypass logic apply to redirects.

This change makes the DataReductionProxyInterceptor intercept redirects
as well as responses. This will cause clients on captive portal networks
that cause redirect loops when trying to use Data Saver bypass
immediately instead of waiting for 21 redirects.

BUG=460346
Committed: https://crrev.com/9e621293154d1419c1e270c923237958ef70a091
Cr-Commit-Position: refs/heads/master@{#319120}

[sclittle, 2015-02-26 23:58:56]

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

[bengr, 2015-02-27 23:55:12]

https://codereview.chromium.org/958163004/diff/1/components/data_reduction_pr...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_interceptor_unittest.cc
(right):

https://codereview.chromium.org/958163004/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/core/browser/data_reduction_proxy_interceptor_unittest.cc:299:
// 3 proxies should be available for use: primary, fallback, and direct.
Three

https://codereview.chromium.org/958163004/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/core/browser/data_reduction_proxy_interceptor_unittest.cc:305:
scoped_ptr<net::URLRequest> CreateAndExecuteRequest(
Add comments to these methods.

https://codereview.chromium.org/958163004/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/core/browser/data_reduction_proxy_interceptor_unittest.cc:306:
const GURL& url) {
Move to previous line if it fits.

https://codereview.chromium.org/958163004/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/core/browser/data_reduction_proxy_interceptor_unittest.cc:314:
net::TestDelegate* delegate() { return &delegate_; }
Move return to new line.

https://codereview.chromium.org/958163004/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/core/browser/data_reduction_proxy_interceptor_unittest.cc:314:
net::TestDelegate* delegate() { return &delegate_; }
const method?

https://codereview.chromium.org/958163004/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/core/browser/data_reduction_proxy_interceptor_unittest.cc:316:
net::MockClientSocketFactory* mock_socket_factory() {
const method?

https://codereview.chromium.org/958163004/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/core/browser/data_reduction_proxy_interceptor_unittest.cc:335:
// Response comes through proxy. Should not be retried.
Use complete sentences.

https://codereview.chromium.org/958163004/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/core/browser/data_reduction_proxy_interceptor_unittest.cc:459:
scoped_ptr<net::URLRequest> request =
Creating the request and checking expectations seems to be a common task in
these tests. Perhaps you should factor that out?

https://codereview.chromium.org/958163004/diff/1/components/data_reduction_pr...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_test_utils.h
(right):

https://codereview.chromium.org/958163004/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/core/browser/data_reduction_proxy_test_utils.h:160:
// Enable the Data Reduction Proxy, simulating a successful probe. This can
jeremyim just renamed probe to SecureProxyCheck.

[sclittle, 2015-03-02 19:51:45]

https://codereview.chromium.org/958163004/diff/1/components/data_reduction_pr...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_interceptor_unittest.cc
(right):

https://codereview.chromium.org/958163004/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/core/browser/data_reduction_proxy_interceptor_unittest.cc:299:
// 3 proxies should be available for use: primary, fallback, and direct.
On 2015/02/27 23:55:11, bengr wrote:
> Three

Done.

https://codereview.chromium.org/958163004/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/core/browser/data_reduction_proxy_interceptor_unittest.cc:305:
scoped_ptr<net::URLRequest> CreateAndExecuteRequest(
On 2015/02/27 23:55:11, bengr wrote:
> Add comments to these methods.

Done.

https://codereview.chromium.org/958163004/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/core/browser/data_reduction_proxy_interceptor_unittest.cc:306:
const GURL& url) {
On 2015/02/27 23:55:11, bengr wrote:
> Move to previous line if it fits.

Done.

https://codereview.chromium.org/958163004/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/core/browser/data_reduction_proxy_interceptor_unittest.cc:314:
net::TestDelegate* delegate() { return &delegate_; }
On 2015/02/27 23:55:11, bengr wrote:
> Move return to new line.

Both done.

https://codereview.chromium.org/958163004/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/core/browser/data_reduction_proxy_interceptor_unittest.cc:316:
net::MockClientSocketFactory* mock_socket_factory() {
On 2015/02/27 23:55:11, bengr wrote:
> const method?

Can't be const, I'm adding SocketDataProviders to the mock socket factory.

https://codereview.chromium.org/958163004/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/core/browser/data_reduction_proxy_interceptor_unittest.cc:335:
// Response comes through proxy. Should not be retried.
On 2015/02/27 23:55:11, bengr wrote:
> Use complete sentences.

Done.

https://codereview.chromium.org/958163004/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/core/browser/data_reduction_proxy_interceptor_unittest.cc:459:
scoped_ptr<net::URLRequest> request =
On 2015/02/27 23:55:11, bengr wrote:
> Creating the request and checking expectations seems to be a common task in
> these tests. Perhaps you should factor that out?

I don't want to do that, because it wouldn't be as clear what the test does.

I want to specify all the MockReads in the TEST_F itself. This way, it's easy to
see that there's a bypass then a retry or a redirect then a successful response
or whatever. Then, only the test knows that there's supposed to be a
200/Success/kBody response, so these EXPECTs should be in the test.

For creating the URLRequest, this is already largely factored out, but the
TEST_F needs to own it so that the URLRequest is still in scope when running the
EXPECTs.

https://codereview.chromium.org/958163004/diff/1/components/data_reduction_pr...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_test_utils.h
(right):

https://codereview.chromium.org/958163004/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/core/browser/data_reduction_proxy_test_utils.h:160:
// Enable the Data Reduction Proxy, simulating a successful probe. This can
On 2015/02/27 23:55:11, bengr wrote:
> jeremyim just renamed probe to SecureProxyCheck.

Done.

[bengr, 2015-03-03 18:17:57]

lgtm with nits

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

https://codereview.chromium.org/958163004/diff/20001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_interceptor_unittest.cc:318:
const net::TestDelegate* delegate() const {
Any reason this returns a const* instead of a const&?

https://codereview.chromium.org/958163004/diff/20001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_interceptor_unittest.cc:341:
// Response comes through proxy and should not be retried.
The response comes through the proxy and ...

[sclittle, 2015-03-03 18:41:29]

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

https://codereview.chromium.org/958163004/diff/20001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_interceptor_unittest.cc:318:
const net::TestDelegate* delegate() const {
On 2015/03/03 18:17:57, bengr wrote:
> Any reason this returns a const* instead of a const&?

Good point, changed to a const&.

https://codereview.chromium.org/958163004/diff/20001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_interceptor_unittest.cc:341:
// Response comes through proxy and should not be retried.
On 2015/03/03 18:17:57, bengr wrote:
> The response comes through the proxy and ...

Done.

[sclittle, 2015-03-03 18:42:50]

The CQ bit was checked by sclittle@chromium.org

[sclittle, 2015-03-03 18:42:50]

The patchset sent to the CQ was uploaded after l-g-t-m from bengr@chromium.org
Link to the patchset: https://codereview.chromium.org/958163004/#ps40001 (title:
"Addressed nits")

[commit-bot: I haz the power, 2015-03-03 18:45:28]

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

[commit-bot: I haz the power, 2015-03-03 20:05:27]

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

[commit-bot: I haz the power, 2015-03-03 20:05:29]

Try jobs failed on following builders:
  android_compile_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_compile...)

[sclittle, 2015-03-03 21:16:13]

The CQ bit was checked by sclittle@chromium.org

[sclittle, 2015-03-03 21:16:14]

The patchset sent to the CQ was uploaded after l-g-t-m from bengr@chromium.org
Link to the patchset: https://codereview.chromium.org/958163004/#ps60001 (title:
"Use ScopedVector in test instead of a vector of scoped_ptr")

[commit-bot: I haz the power, 2015-03-03 21:17:23]

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

[commit-bot: I haz the power, 2015-03-03 23:56:14]

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

[commit-bot: I haz the power, 2015-03-03 23:56:15]

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

[sclittle, 2015-03-04 00:00:17]

The CQ bit was checked by sclittle@chromium.org

[sclittle, 2015-03-04 00:00:18]

The patchset sent to the CQ was uploaded after l-g-t-m from bengr@chromium.org
Link to the patchset: https://codereview.chromium.org/958163004/#ps80001 (title:
"Rebased on master")

[sclittle, 2015-03-04 00:00:20]

The CQ bit was unchecked by sclittle@chromium.org

[sclittle, 2015-03-04 19:51:30]

The CQ bit was checked by sclittle@chromium.org

[sclittle, 2015-03-04 19:51:31]

The patchset sent to the CQ was uploaded after l-g-t-m from bengr@chromium.org
Link to the patchset: https://codereview.chromium.org/958163004/#ps100001
(title: "Suppress size_t to int warnings in gyp/gn files (crbug 167187)")

[commit-bot: I haz the power, 2015-03-04 19:53:06]

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

[commit-bot: I haz the power, 2015-03-04 20:39:33]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-03-04 20:40:41]

Patchset 6 (id:??) landed as
https://crrev.com/9e621293154d1419c1e270c923237958ef70a091
Cr-Commit-Position: refs/heads/master@{#319120}