Fixing DataReductionProxy.BypassedBytes.LocalBypassRules

Fixing bug where DataReductionProxy.BypassedBytes.LocalBypassRules
reports bypasses for requests that don't pass the data reduction
proxy bypass rules or didn't go through the proxy because of
managed PAC.

Now DataReducitonProxy.BypassedBytes.ManagedPac reports whenever
there is a managed config regardless of whether the data reduction 
proxy was used.

LocalBypassRules reports for proxies when the data reduction proxy
was the effective configuration and the request does not pass
the data reduction proxy rules.

Also changed {Short|Medium}TriggeringRequest, {Short|Medium}All,
and ShortAudioVideo so that they are all mutually exclusive.

BUG=401305
Committed: https://crrev.com/25ff71d351379440fb6309a8bec1784896b3848e
Cr-Commit-Position: refs/heads/master@{#293419}

[megjablon, 2014-08-29 01:07:16]

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

[bengr, 2014-08-29 19:29:48]

https://codereview.chromium.org/510353004/diff/20001/chrome/browser/net/chrom...
File chrome/browser/net/chrome_network_delegate.cc (right):

https://codereview.chromium.org/510353004/diff/20001/chrome/browser/net/chrom...
chrome/browser/net/chrome_network_delegate.cc:586: if
(data_reduction_proxy_enabled_ && data_reduction_proxy_usage_stats_ &&
nit: It might be a bit cleaner to put each condition on its own line

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

https://codereview.chromium.org/510353004/diff/20001/components/data_reductio...
components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:370: bool
DataReductionProxyParams::PassesDataReductionProxyLocalBypassRules(
Maybe rename as IsBypassedByDataReductionProxyLocalRules()

https://codereview.chromium.org/510353004/diff/20001/components/data_reductio...
components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:372:
const net::ProxyConfig& data_reduction_proxy_config) {
It looks like this function can be const.

https://codereview.chromium.org/510353004/diff/20001/components/data_reductio...
File components/data_reduction_proxy/browser/data_reduction_proxy_params.h
(right):

https://codereview.chromium.org/510353004/diff/20001/components/data_reductio...
components/data_reduction_proxy/browser/data_reduction_proxy_params.h:135: //
based on applying the |data_reduction_proxy_config| param rules to the URL.
to the request URL.

https://codereview.chromium.org/510353004/diff/20001/components/data_reductio...
File components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc
(right):

https://codereview.chromium.org/510353004/diff/20001/components/data_reductio...
components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:169:
request.context()->proxy_service()->config())) {
indent 4 more

https://codereview.chromium.org/510353004/diff/20001/components/data_reductio...
File components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.h
(right):

https://codereview.chromium.org/510353004/diff/20001/components/data_reductio...
components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.h:65:
const net::ProxyConfig& data_reduction_proxy_config);
Update the comment to describe this parameter.

https://codereview.chromium.org/510353004/diff/20001/components/data_reductio...
components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.h:75:
MANAGED_PAC,              /* Bypass due to managed config. */
This isn't quite right. I'd call this MANAGED_PROXY_CONFIG, because it doesn't
necessary have to be a PAC.

[megjablon, 2014-08-29 20:26:31]

megjablon@chromium.org changed reviewers:
+ asvitkine@chromium.org, mmenke@chromium.org

[megjablon, 2014-08-29 20:26:32]

asvitkine: histograms.xml
mmenke: chrome_network_delegate.cc

https://codereview.chromium.org/510353004/diff/20001/chrome/browser/net/chrom...
File chrome/browser/net/chrome_network_delegate.cc (right):

https://codereview.chromium.org/510353004/diff/20001/chrome/browser/net/chrom...
chrome/browser/net/chrome_network_delegate.cc:586: if
(data_reduction_proxy_enabled_ && data_reduction_proxy_usage_stats_ &&
On 2014/08/29 19:29:47, bengr1 wrote:
> nit: It might be a bit cleaner to put each condition on its own line

Done.

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

https://codereview.chromium.org/510353004/diff/20001/components/data_reductio...
components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:370: bool
DataReductionProxyParams::PassesDataReductionProxyLocalBypassRules(
On 2014/08/29 19:29:48, bengr1 wrote:
> Maybe rename as IsBypassedByDataReductionProxyLocalRules()

Ya, I like that better

https://codereview.chromium.org/510353004/diff/20001/components/data_reductio...
components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:372:
const net::ProxyConfig& data_reduction_proxy_config) {
On 2014/08/29 19:29:48, bengr1 wrote:
> It looks like this function can be const.

Done.

https://codereview.chromium.org/510353004/diff/20001/components/data_reductio...
File components/data_reduction_proxy/browser/data_reduction_proxy_params.h
(right):

https://codereview.chromium.org/510353004/diff/20001/components/data_reductio...
components/data_reduction_proxy/browser/data_reduction_proxy_params.h:135: //
based on applying the |data_reduction_proxy_config| param rules to the URL.
On 2014/08/29 19:29:48, bengr1 wrote:
> to the request URL.

Done.

https://codereview.chromium.org/510353004/diff/20001/components/data_reductio...
File components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc
(right):

https://codereview.chromium.org/510353004/diff/20001/components/data_reductio...
components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:169:
request.context()->proxy_service()->config())) {
On 2014/08/29 19:29:48, bengr1 wrote:
> indent 4 more

Done.

https://codereview.chromium.org/510353004/diff/20001/components/data_reductio...
File components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.h
(right):

https://codereview.chromium.org/510353004/diff/20001/components/data_reductio...
components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.h:65:
const net::ProxyConfig& data_reduction_proxy_config);
On 2014/08/29 19:29:48, bengr1 wrote:
> Update the comment to describe this parameter.

Done.

https://codereview.chromium.org/510353004/diff/20001/components/data_reductio...
components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.h:75:
MANAGED_PAC,              /* Bypass due to managed config. */
On 2014/08/29 19:29:48, bengr1 wrote:
> This isn't quite right. I'd call this MANAGED_PROXY_CONFIG, because it doesn't
> necessary have to be a PAC.

Done.

[Alexei Svitkine (slow), 2014-08-29 22:19:08]

lgtm

[bengr, 2014-09-02 05:11:38]

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

https://codereview.chromium.org/510353004/diff/40001/components/data_reductio...
components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:168:
if (!data_reduction_proxy_config.Equals(
Does this "Equals" do the right thing? E.g., what if the config IDs are
different? They will be even when both configs are for the data reduction proxy.

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

https://codereview.chromium.org/510353004/diff/40001/components/data_reductio...
components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.h:75:
MANAGED_PROXY_CONFIG,              /* Bypass due to managed config. */
indent of comment.

[mmenke, 2014-09-02 14:30:52]

https://codereview.chromium.org/510353004/diff/40001/chrome/browser/net/chrom...
File chrome/browser/net/chrome_network_delegate.cc (right):

https://codereview.chromium.org/510353004/diff/40001/chrome/browser/net/chrom...
chrome/browser/net/chrome_network_delegate.cc:592: proxy_config_getter_.Run());
Can't RecordBypassedBytesHistograms just use request->proxy_server()?  Looking
at the config rather than what proxy was actually used seems weird to me.

[megjablon, 2014-09-02 21:00:16]

https://codereview.chromium.org/510353004/diff/40001/chrome/browser/net/chrom...
File chrome/browser/net/chrome_network_delegate.cc (right):

https://codereview.chromium.org/510353004/diff/40001/chrome/browser/net/chrom...
chrome/browser/net/chrome_network_delegate.cc:592: proxy_config_getter_.Run());
On 2014/09/02 14:30:52, mmenke wrote:
> Can't RecordBypassedBytesHistograms just use request->proxy_server()?  Looking
> at the config rather than what proxy was actually used seems weird to me.
To know if the proxy was bypassed because of the data reduction proxy bypass
rules, we need to have the data reduction proxy config to have a handle to those
rules. We also want to know if the effective config is the data reduction proxy
config. This tells us if a managed proxy config is being used and overriding the
use of the data reduction proxy config.

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

https://codereview.chromium.org/510353004/diff/40001/components/data_reductio...
components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:168:
if (!data_reduction_proxy_config.Equals(
On 2014/09/02 05:11:38, bengr1 wrote:
> Does this "Equals" do the right thing? E.g., what if the config IDs are
> different? They will be even when both configs are for the data reduction
proxy.

Yes. This equals compares ignoring the differences in id and source.

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

https://codereview.chromium.org/510353004/diff/40001/components/data_reductio...
components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.h:75:
MANAGED_PROXY_CONFIG,              /* Bypass due to managed config. */
On 2014/09/02 05:11:38, bengr1 wrote:
> indent of comment.

Done.

[mmenke, 2014-09-03 15:30:38]

chrome_network_delegate.cc LGTM.

[bengr, 2014-09-03 17:06:55]

https://codereview.chromium.org/510353004/diff/60001/components/data_reductio...
File components/data_reduction_proxy/browser/data_reduction_proxy_params.h
(right):

https://codereview.chromium.org/510353004/diff/60001/components/data_reductio...
components/data_reduction_proxy/browser/data_reduction_proxy_params.h:134: //
Returns true if this request could be sent through the data request proxy
The return description and the function name seem to contradict. Please change
one or the other. I suggest changing the return so that true means the request
would not go to Flywheel because it would be bypassed by local rules.

Actually, I think that's what your code is doing, but he description is wrong.

[megjablon, 2014-09-03 17:20:12]

https://codereview.chromium.org/510353004/diff/60001/components/data_reductio...
File components/data_reduction_proxy/browser/data_reduction_proxy_params.h
(right):

https://codereview.chromium.org/510353004/diff/60001/components/data_reductio...
components/data_reduction_proxy/browser/data_reduction_proxy_params.h:134: //
Returns true if this request could be sent through the data request proxy
On 2014/09/03 17:06:55, bengr1 wrote:
> The return description and the function name seem to contradict. Please change
> one or the other. I suggest changing the return so that true means the request
> would not go to Flywheel because it would be bypassed by local rules.
> 
> Actually, I think that's what your code is doing, but he description is wrong.

Ya, I forgot to change the description when I flip flopped the return.

[bengr, 2014-09-03 19:38:54]

lgtm

[megjablon, 2014-09-03 21:54:33]

The CQ bit was checked by megjablon@chromium.org

[megjablon, 2014-09-03 22:17:22]

The CQ bit was unchecked by megjablon@chromium.org

[megjablon, 2014-09-04 22:47:11]

The CQ bit was checked by megjablon@chromium.org

[commit-bot: I haz the power, 2014-09-04 22:49:00]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/megjablon@chromium.org/510353004/100001

[commit-bot: I haz the power, 2014-09-05 03:25:45]

Committed patchset #6 (id:100001) as 7355011638dee0c3b687ca7adb261b8337909a37

[megjablon, 2014-09-06 00:30:40]

Patchset #7 (id:120001) has been deleted

[commit-bot: I haz the power, 2014-09-10 03:36:12]

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