Add support for field trial group to relax missing via header bypass logic.

Add support for field trial group to relax missing via header bypass logic.

This change refactors the bypass logic into a class and adds support for
a new group in the DataReductionProxyRemoveMissingViaHeaderOtherBypass
field trial. This new group relaxes the non-4xx response missing via
header bypass logic to only apply to the first response through the
proxy after a network change.

BUG=413035, 409466
Committed: https://crrev.com/db1198c5327b5c69b93fafd814b9f516b88a15fd
Cr-Commit-Position: refs/heads/master@{#310698}

[sclittle, 2015-01-03 03:35:50]

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

[sclittle, 2015-01-03 03:35:50]

bengr: all

Thanks in advance!

[bengr, 2015-01-05 22:21:39]

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

https://codereview.chromium.org/836583002/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/core/browser/data_reduction_proxy_interceptor.h:56:
scoped_ptr<DataReductionProxyProtocol> protocol_;
Add a comment describing what this is.

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

https://codereview.chromium.org/836583002/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/core/browser/data_reduction_proxy_protocol.cc:126:
IsIncludedInRelaxMissingViaHeaderOtherBypassFieldTrial() &&
indent 4 more

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

https://codereview.chromium.org/836583002/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/core/browser/data_reduction_proxy_protocol.h:33:
class DataReductionProxyProtocol
Rename: DataReductionProxyBypassProtocol?

https://codereview.chromium.org/836583002/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/core/browser/data_reduction_proxy_protocol.h:36:
DataReductionProxyProtocol(DataReductionProxyParams* params,
Add a comment. Can these be null? Must they outlive this?

https://codereview.chromium.org/836583002/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/core/browser/data_reduction_proxy_protocol.h:38:
~DataReductionProxyProtocol() override;
Add a blank line above.

https://codereview.chromium.org/836583002/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/core/browser/data_reduction_proxy_protocol.h:41:
// put it on the proxy retry list. Returns true if the request should be
proxy retry list -> proxy retry map, which is maintained by the ProxyService of
the URLRequestContext.

https://codereview.chromium.org/836583002/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/core/browser/data_reduction_proxy_protocol.h:58:
// Must outlive |this|.
Add a blank line above.

https://codereview.chromium.org/836583002/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/core/browser/data_reduction_proxy_protocol.h:65:
std::set<net::HostPortPair> proxies_resilient_to_missing_via_headers_;
This variable name makes no sense to me. How about via_header_producing_proxies_
or proxies_with_via_headers_in_responses_?

https://codereview.chromium.org/836583002/diff/1/components/data_reduction_pr...
File components/data_reduction_proxy/core/common/data_reduction_proxy_params.h
(right):

https://codereview.chromium.org/836583002/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/core/common/data_reduction_proxy_params.h:92: //
response code is missing the data reduction proxy via header unless no
Very awkward, but technically correct. Consider rewriting.

[sclittle, 2015-01-06 00:18:21]

Patchset #2 (id:20001) has been deleted

[sclittle, 2015-01-06 00:18:37]

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

https://codereview.chromium.org/836583002/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/core/browser/data_reduction_proxy_interceptor.h:56:
scoped_ptr<DataReductionProxyProtocol> protocol_;
On 2015/01/05 22:21:38, bengr wrote:
> Add a comment describing what this is.

Done.

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

https://codereview.chromium.org/836583002/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/core/browser/data_reduction_proxy_protocol.cc:126:
IsIncludedInRelaxMissingViaHeaderOtherBypassFieldTrial() &&
On 2015/01/05 22:21:38, bengr wrote:
> indent 4 more

Done.

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

https://codereview.chromium.org/836583002/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/core/browser/data_reduction_proxy_protocol.h:33:
class DataReductionProxyProtocol
On 2015/01/05 22:21:39, bengr wrote:
> Rename: DataReductionProxyBypassProtocol?

OK. I've renamed the file as well.

https://codereview.chromium.org/836583002/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/core/browser/data_reduction_proxy_protocol.h:36:
DataReductionProxyProtocol(DataReductionProxyParams* params,
On 2015/01/05 22:21:39, bengr wrote:
> Add a comment. Can these be null? Must they outlive this?

Done.

https://codereview.chromium.org/836583002/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/core/browser/data_reduction_proxy_protocol.h:38:
~DataReductionProxyProtocol() override;
On 2015/01/05 22:21:38, bengr wrote:
> Add a blank line above.

Done.

https://codereview.chromium.org/836583002/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/core/browser/data_reduction_proxy_protocol.h:41:
// put it on the proxy retry list. Returns true if the request should be
On 2015/01/05 22:21:38, bengr wrote:
> proxy retry list -> proxy retry map, which is maintained by the ProxyService
of
> the URLRequestContext.

Done.

https://codereview.chromium.org/836583002/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/core/browser/data_reduction_proxy_protocol.h:58:
// Must outlive |this|.
On 2015/01/05 22:21:39, bengr wrote:
> Add a blank line above.

Done.

https://codereview.chromium.org/836583002/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/core/browser/data_reduction_proxy_protocol.h:65:
std::set<net::HostPortPair> proxies_resilient_to_missing_via_headers_;
On 2015/01/05 22:21:39, bengr wrote:
> This variable name makes no sense to me. How about
via_header_producing_proxies_
> or proxies_with_via_headers_in_responses_?

Done.

https://codereview.chromium.org/836583002/diff/1/components/data_reduction_pr...
File components/data_reduction_proxy/core/common/data_reduction_proxy_params.h
(right):

https://codereview.chromium.org/836583002/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/core/common/data_reduction_proxy_params.h:92: //
response code is missing the data reduction proxy via header unless no
On 2015/01/05 22:21:39, bengr wrote:
> Very awkward, but technically correct. Consider rewriting.

Reworded.

[sclittle, 2015-01-06 01:52:44]

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

[sclittle, 2015-01-06 01:52:44]

mmenke: chrome/browser/io_thread.cc,
chrome/browser/profiles/profile_impl_io_data.cc

I renamed a file, so the includes in these files were changed.

[mmenke, 2015-01-06 19:07:46]

On 2015/01/06 01:52:44, sclittle wrote:
> mmenke: chrome/browser/io_thread.cc,
> chrome/browser/profiles/profile_impl_io_data.cc
> 
> I renamed a file, so the includes in these files were changed.

chrome/browser trivially LGTM.

[bengr, 2015-01-07 00:55:03]

https://codereview.chromium.org/836583002/diff/40001/components/data_reductio...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_bypass_protocol.cc
(right):

https://codereview.chromium.org/836583002/diff/40001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_bypass_protocol.cc:221:
void OnResolveProxyHandler(const GURL& url,
This has nothing to do with bypass, so I'd suggest moving it to
DataReductionProxyNetworkDelegate.

https://codereview.chromium.org/836583002/diff/40001/components/data_reductio...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_bypass_protocol.h
(right):

https://codereview.chromium.org/836583002/diff/40001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_bypass_protocol.h:36:
// Constructs a DataReductionProxyBypassProtocol object. |params| and
Can the params be null?

https://codereview.chromium.org/836583002/diff/40001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_bypass_protocol.h:57:
// From NetworkChangeNotifier::IPAddressObserver.
// Override from NetworkChangeNotifier::IPAddressObserver:

https://codereview.chromium.org/836583002/diff/40001/components/data_reductio...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_interceptor.h
(right):

https://codereview.chromium.org/836583002/diff/40001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_interceptor.h:58:
scoped_ptr<DataReductionProxyBypassProtocol> protocol_;
Rename as bypass_protocol_

[sclittle, 2015-01-07 01:51:27]

https://codereview.chromium.org/836583002/diff/40001/components/data_reductio...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_bypass_protocol.cc
(right):

https://codereview.chromium.org/836583002/diff/40001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_bypass_protocol.cc:221:
void OnResolveProxyHandler(const GURL& url,
On 2015/01/07 00:55:02, bengr wrote:
> This has nothing to do with bypass, so I'd suggest moving it to
> DataReductionProxyNetworkDelegate.

OK. Why do we have to inject OnResolveProxyHandler into DRPNetworkDelegate as a
callback? Could we just call OnResolveProxyHandler directly in
DRPNetworkDelegate::OnResolveProxy instead?

https://codereview.chromium.org/836583002/diff/40001/components/data_reductio...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_bypass_protocol.h
(right):

https://codereview.chromium.org/836583002/diff/40001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_bypass_protocol.h:36:
// Constructs a DataReductionProxyBypassProtocol object. |params| and
On 2015/01/07 00:55:02, bengr wrote:
> Can the params be null?

can't be null. clarified comment.

https://codereview.chromium.org/836583002/diff/40001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_bypass_protocol.h:57:
// From NetworkChangeNotifier::IPAddressObserver.
On 2015/01/07 00:55:02, bengr wrote:
> // Override from NetworkChangeNotifier::IPAddressObserver:

Done.

https://codereview.chromium.org/836583002/diff/40001/components/data_reductio...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_interceptor.h
(right):

https://codereview.chromium.org/836583002/diff/40001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_interceptor.h:58:
scoped_ptr<DataReductionProxyBypassProtocol> protocol_;
On 2015/01/07 00:55:03, bengr wrote:
> Rename as bypass_protocol_

Done.

[bengr, 2015-01-08 21:12:32]

https://codereview.chromium.org/836583002/diff/60001/components/data_reductio...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_interceptor.cc
(right):

https://codereview.chromium.org/836583002/diff/60001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_interceptor.cc:27:
bypass_protocol_(
Does this even need to be dynamically allocated?

https://codereview.chromium.org/836583002/diff/60001/components/data_reductio...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc
(right):

https://codereview.chromium.org/836583002/diff/60001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:152:
on_resolve_proxy_handler_.Run(
This shouldn't be a callback anymore.

[sclittle, 2015-01-08 22:10:53]

https://codereview.chromium.org/836583002/diff/60001/components/data_reductio...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_interceptor.cc
(right):

https://codereview.chromium.org/836583002/diff/60001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_interceptor.cc:27:
bypass_protocol_(
On 2015/01/08 21:12:32, bengr wrote:
> Does this even need to be dynamically allocated?

Unfortunately it does, since
DataReductionProxyInterceptor::MaybeInterceptResponse is a const method, and
making this a scoped_ptr allows for calling
bypass_protocol_->MaybeBypassProxyAndPrepareToRetry, which is not a const
method.

https://codereview.chromium.org/836583002/diff/60001/components/data_reductio...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc
(right):

https://codereview.chromium.org/836583002/diff/60001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:152:
on_resolve_proxy_handler_.Run(
On 2015/01/08 21:12:32, bengr wrote:
> This shouldn't be a callback anymore.

Sure, but can I do it in a separate CL? I filed a bug (http://crbug.com/447346)
and added some TODOs. It actually didn't need to be a callback before either;
that fact is just more obvious now.

Making this not a callback would require removing members and code from
profile_impl and profile_impl_io_data, which seems outside the scope of this CL,
and like it would be better off as it's own CL.

[bengr, 2015-01-08 23:14:06]

lgtm

[sclittle, 2015-01-08 23:15:21]

The CQ bit was checked by sclittle@chromium.org

[commit-bot: I haz the power, 2015-01-08 23:17:14]

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

[commit-bot: I haz the power, 2015-01-08 23:29:11]

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

[commit-bot: I haz the power, 2015-01-08 23:29:12]

Try jobs failed on following builders:
  linux_chromium_gn_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sclittle, 2015-01-08 23:34:04]

The CQ bit was checked by sclittle@chromium.org

[commit-bot: I haz the power, 2015-01-08 23:35:45]

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

[commit-bot: I haz the power, 2015-01-09 00:06:04]

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

[commit-bot: I haz the power, 2015-01-09 00:06:05]

Try jobs failed on following builders:
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[sclittle, 2015-01-09 02:00:24]

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

[sclittle, 2015-01-09 02:00:25]

sgurun: removed unused include from aw_url_request_context_getter.cc

[sgurun-gerrit only, 2015-01-09 02:15:05]

On 2015/01/09 02:00:25, sclittle wrote:
> sgurun: removed unused include from aw_url_request_context_getter.cc

aw lgtm

[sclittle, 2015-01-09 02:16:45]

The CQ bit was checked by sclittle@chromium.org

[commit-bot: I haz the power, 2015-01-09 02:17:59]

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

[commit-bot: I haz the power, 2015-01-09 03:24:57]

Committed patchset #6 (id:120001)

[commit-bot: I haz the power, 2015-01-09 03:25:52]

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