Fixed DataReductionProxyParams::AreProxiesBypassed logic

If the primary data reduction proxy is disabled, it won't appear in the
retry info map, even if all data reduction proxies are bypassed. Therefore,
to check if all data reduction proxies are bypassed,
consider two cases:

!fallback_allowed: if the primary is on the map, we consider all data reduction
proxies to be bypassed.

fallback_allowed: if (at least) the fallback is on the map, we consider all
data reduction proxies to be bypassed. 
We still see if the primary is in the map to check if it has the min_retry_delay.

BUG=401281
Committed: https://crrev.com/cb7f288df05ca48c13f311c7a0d89150a227da3e
Cr-Commit-Position: refs/heads/master@{#291799}

[megjablon, 2014-08-12 21:06:54]

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

https://codereview.chromium.org/464023002/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:373:
return true;
Ben, with the proxy logic does this work for min_retry_delay? If we bypass on
the non-alt proxies, then we will never check the alt proxies and see if the
delay is smaller. I know the previous method just looked at all proxies in the
retry map and found the minimum, so I was wondering if that was necessary to do
here.

[bengr, 2014-08-15 01:15:07]

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

https://codereview.chromium.org/464023002/diff/20001/components/data_reductio...
components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:406: //
If the request is https, we only care about the ssl proxy.
// ... consider only the ssl proxy

https://codereview.chromium.org/464023002/diff/20001/components/data_reductio...
components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:426:
return proxies_bypassed || alt_proxies_bypassed;
I would return before dealing with all proxies if proxies_bypassed is true.

https://codereview.chromium.org/464023002/diff/20001/components/data_reductio...
components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:434: //
We look for the primary proxy in the retry map. This must be done before
Look for...

https://codereview.chromium.org/464023002/diff/20001/components/data_reductio...
components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:441: //
If the primary proxy is found, we check it's delay. If the delay is less
Remove "we" everywhere.

https://codereview.chromium.org/464023002/diff/20001/components/data_reductio...
components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:455:
net::ProxyServer::SCHEME_HTTPS : net::ProxyServer::SCHEME_HTTP,
Indentation is a little weird. Maybe break after SchemeIs(

[megjablon, 2014-08-15 02:01:35]

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

https://codereview.chromium.org/464023002/diff/20001/components/data_reductio...
components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:406: //
If the request is https, we only care about the ssl proxy.
On 2014/08/15 01:15:07, bengr1 wrote:
> // ... consider only the ssl proxy

Done.

https://codereview.chromium.org/464023002/diff/20001/components/data_reductio...
components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:426:
return proxies_bypassed || alt_proxies_bypassed;
On 2014/08/15 01:15:07, bengr1 wrote:
> I would return before dealing with all proxies if proxies_bypassed is true.
Fixed as discussed offline.

https://codereview.chromium.org/464023002/diff/20001/components/data_reductio...
components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:434: //
We look for the primary proxy in the retry map. This must be done before
On 2014/08/15 01:15:07, bengr1 wrote:
> Look for...

Done.

https://codereview.chromium.org/464023002/diff/20001/components/data_reductio...
components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:441: //
If the primary proxy is found, we check it's delay. If the delay is less
On 2014/08/15 01:15:07, bengr1 wrote:
> Remove "we" everywhere.

Done.

https://codereview.chromium.org/464023002/diff/20001/components/data_reductio...
components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:455:
net::ProxyServer::SCHEME_HTTPS : net::ProxyServer::SCHEME_HTTP,
On 2014/08/15 01:15:07, bengr1 wrote:
> Indentation is a little weird. Maybe break after SchemeIs(
That looked very strange. I tried something different.

[bengr, 2014-08-15 06:16:06]

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

https://codereview.chromium.org/464023002/diff/80001/components/data_reductio...
components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:441: if
(found != retry_map.end()) {
You don't have to do this work if !fallback_allowed_, do you?

https://codereview.chromium.org/464023002/diff/80001/components/data_reductio...
components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:456:
delay = found->second.current_delay;
Does this work? If the primary proxy is not found then delay is 0 when we get
here, which is not greater than found->second.current_delay, so delay won't be
set.

[bengr, 2014-08-15 17:58:57]

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

https://codereview.chromium.org/464023002/diff/80001/components/data_reductio...
components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:441: if
(found != retry_map.end()) {
On 2014/08/15 06:16:06, bengr1 wrote:
> You don't have to do this work if !fallback_allowed_, do you?

I see. This is needed.

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

https://codereview.chromium.org/464023002/diff/100001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:404: if
(is_https) {
I see this form in Chrome more often:

if (is_https) {
  if (alt_allowed_) {
    return ArePrimary...();
  }
  NOT_REACHED();
  return false;
}

https://codereview.chromium.org/464023002/diff/100001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:441: if
(found != retry_map.end()) {
Ah, you don't have to do this work if fallback_allowed_ and min_retry_delay ==
NULL

https://codereview.chromium.org/464023002/diff/100001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:461:
*min_retry_delay = delay;
nit: you could write to min_retury_delay directly and get rid of delay.

[megjablon, 2014-08-15 19:04:24]

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

https://codereview.chromium.org/464023002/diff/80001/components/data_reductio...
components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:456:
delay = found->second.current_delay;
On 2014/08/15 06:16:05, bengr1 wrote:
> Does this work? If the primary proxy is not found then delay is 0 when we get
> here, which is not greater than found->second.current_delay, so delay won't be
> set.

Added in setting delay to max to fix.

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

https://codereview.chromium.org/464023002/diff/100001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:404: if
(is_https) {
On 2014/08/15 17:58:57, bengr1 wrote:
> I see this form in Chrome more often:
> 
> if (is_https) {
>   if (alt_allowed_) {
>     return ArePrimary...();
>   }
>   NOT_REACHED();
>   return false;
> }

Done.

https://codereview.chromium.org/464023002/diff/100001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:441: if
(found != retry_map.end()) {
On 2014/08/15 17:58:57, bengr1 wrote:
> Ah, you don't have to do this work if fallback_allowed_ and min_retry_delay ==
> NULL

Done.

https://codereview.chromium.org/464023002/diff/100001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:461:
*min_retry_delay = delay;
On 2014/08/15 17:58:57, bengr1 wrote:
> nit: you could write to min_retury_delay directly and get rid of delay.

Done.

[bengr, 2014-08-25 21:59:00]

lgtm

[megjablon, 2014-08-25 22:28:37]

The CQ bit was checked by megjablon@chromium.org

[commit-bot: I haz the power, 2014-08-25 22:30:00]

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

[commit-bot: I haz the power, 2014-08-26 00:22:16]

Committed patchset #5 (120001) as 8951c1c755dc8967ce33d52528642e48457c4dda

[commit-bot: I haz the power, 2014-09-10 02:38:56]

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