Fixed DataReductionProxyParams::AreProxiesBypassed logic

components/data_reduction_proxy/browser/data_reduction_proxy_params.cc

Index: components/data_reduction_proxy/browser/data_reduction_proxy_params.cc
diff --git a/components/data_reduction_proxy/browser/data_reduction_proxy_params.cc b/components/data_reduction_proxy/browser/data_reduction_proxy_params.cc
index 1074783a975a472fc2b87274aad69f540b7081be..d06a108f44ee7154cbe3f0a70bb54394cc6f9ff4 100644
--- a/components/data_reduction_proxy/browser/data_reduction_proxy_params.cc
+++ b/components/data_reduction_proxy/browser/data_reduction_proxy_params.cc
@@ -400,22 +400,30 @@ bool DataReductionProxyParams::AreProxiesBypassed(
   if (retry_map.size() == 0)
     return false;
 
+  if (min_retry_delay != NULL)
+    *min_retry_delay = base::TimeDelta::Max();
+
+  // If the request is https, we only care about the ssl proxy.

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

// ... consider only the ssl proxy

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

Done.

   if (is_https && alt_allowed_) {
     return ArePrimaryAndFallbackBypassed(
         retry_map, ssl_origin_, GURL(), min_retry_delay);
   }
 
-  if (allowed_ && ArePrimaryAndFallbackBypassed(
-      retry_map, origin_, fallback_origin_, min_retry_delay)) {
-    return true;
+  bool proxies_bypassed = false;
+  bool alt_proxies_bypassed = false;
+  // For http requests, we must look at all proxies on the retry map to get
+  // the |min_retry_delay|.
+  if (!is_https && allowed_) {
+    proxies_bypassed = ArePrimaryAndFallbackBypassed(
+        retry_map, origin_, fallback_origin_, min_retry_delay);
   }
 
-  if (alt_allowed_ && ArePrimaryAndFallbackBypassed(
-      retry_map, alt_origin_, alt_fallback_origin_, min_retry_delay)) {
-    return true;
+  if (!is_https && alt_allowed_) {
+    alt_proxies_bypassed = ArePrimaryAndFallbackBypassed(
+        retry_map, alt_origin_, alt_fallback_origin_, min_retry_delay);
   }
 
-  return false;
+  return proxies_bypassed || alt_proxies_bypassed;

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

I would return before dealing with all proxies if proxies_bypassed is true.

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

Fixed as discussed offline.

 }
 
 bool DataReductionProxyParams::ArePrimaryAndFallbackBypassed(
@@ -423,33 +431,40 @@ bool DataReductionProxyParams::ArePrimaryAndFallbackBypassed(
     const GURL& primary,
     const GURL& fallback,
     base::TimeDelta* min_retry_delay) const {
+  // We look for the primary proxy in the retry map. This must be done before

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

Look for...

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

Done.

+  // looking for the fallback in order to assign |min_retry_delay| if the
+  // primary proxy has a shorter delay.
   net::ProxyRetryInfoMap::const_iterator found = retry_map.find(
       net::ProxyServer(primary.SchemeIs(url::kHttpsScheme) ?
           net::ProxyServer::SCHEME_HTTPS : net::ProxyServer::SCHEME_HTTP,
           net::HostPortPair::FromURL(primary)).ToURI());
-
-  if (found == retry_map.end())
-    return false;
-
-  base::TimeDelta min_delay = found->second.current_delay;
-  if (!fallback_allowed_ || !fallback.is_valid()) {
-    if (min_retry_delay != NULL)
-      *min_retry_delay = min_delay;
-    return true;
+  // If the primary proxy is found, we check it's delay. If the delay is less

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

Remove "we" everywhere.

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

Done.

+  // than |min_retry_delay|, we reassign.
+  if (found != retry_map.end() &&
+      min_retry_delay != NULL &&
+      *min_retry_delay > found->second.current_delay) {
+    *min_retry_delay = found->second.current_delay;
   }
 
-  found = retry_map.find(
-      net::ProxyServer(fallback.SchemeIs(url::kHttpsScheme) ?
-          net::ProxyServer::SCHEME_HTTPS : net::ProxyServer::SCHEME_HTTP,
-          net::HostPortPair::FromURL(fallback)).ToURI());
+  if (fallback_allowed_ && fallback.is_valid()) {
+    // If fallback is allowed, we only need to know that the fallback proxy is
+    // on the retry map to know if there was a bypass. We can reset found and
+    // forget if the primary was on the retry map.
+    found = retry_map.find(
+        net::ProxyServer(fallback.SchemeIs(url::kHttpsScheme) ?
+            net::ProxyServer::SCHEME_HTTPS : net::ProxyServer::SCHEME_HTTP,

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

Indentation is a little weird. Maybe break after SchemeIs(

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

That looked very strange. I tried something different.

+            net::HostPortPair::FromURL(fallback)).ToURI());
+    // If the fallback proxy is found, we check it's delay. If the delay is less
+    // than |min_retry_delay|, we reassign.
+    if (found != retry_map.end() &&
+        min_retry_delay != NULL &&
+        *min_retry_delay > found->second.current_delay) {
+      *min_retry_delay = found->second.current_delay;
+    }
+  }
 
   if (found == retry_map.end())
     return false;
-
-  if (min_delay > found->second.current_delay)
-    min_delay = found->second.current_delay;
-  if (min_retry_delay != NULL)
-    *min_retry_delay = min_delay;
   return true;
 }