Record UMA when googlezip proxies are removed from the proxy config.

chrome/browser/net/pref_proxy_config_tracker_impl.cc

Index: chrome/browser/net/pref_proxy_config_tracker_impl.cc
diff --git a/chrome/browser/net/pref_proxy_config_tracker_impl.cc b/chrome/browser/net/pref_proxy_config_tracker_impl.cc
index 49161ba7564b6119dba89c9f8a7dffb6913092f4..159b69f5f91903153b280e16927c2dd549bdad8f 100644
--- a/chrome/browser/net/pref_proxy_config_tracker_impl.cc
+++ b/chrome/browser/net/pref_proxy_config_tracker_impl.cc
@@ -5,6 +5,7 @@
 #include "chrome/browser/net/pref_proxy_config_tracker_impl.h"
 
 #include "base/bind.h"
+#include "base/metrics/histogram_macros.h"
 #include "base/prefs/pref_registry_simple.h"
 #include "base/prefs/pref_service.h"
 #include "base/strings/string_util.h"
@@ -30,10 +31,8 @@ bool IsGooglezipDataReductionProxy(const net::ProxyServer& proxy) {
 }
 
 // Removes any Data Reduction Proxies like *.googlezip.net from |proxy_list|.
-void RemoveGooglezipDataReductionProxiesFromList(net::ProxyList* proxy_list) {
-  if (proxy_list->IsEmpty())
-    return;
-
+// Returns true if any proxies were removed from |proxy_list|, false otherwise.
+bool RemoveGooglezipDataReductionProxiesFromList(net::ProxyList* proxy_list) {
   bool found_googlezip_proxy = false;
   for (const net::ProxyServer& proxy : proxy_list->GetAll()) {
     if (IsGooglezipDataReductionProxy(proxy)) {
@@ -42,7 +41,7 @@ void RemoveGooglezipDataReductionProxiesFromList(net::ProxyList* proxy_list) {
     }
   }
   if (!found_googlezip_proxy)
-    return;
+    return false;
 
   net::ProxyList replacement_list;
   for (const net::ProxyServer& proxy : proxy_list->GetAll()) {
@@ -53,6 +52,14 @@ void RemoveGooglezipDataReductionProxiesFromList(net::ProxyList* proxy_list) {
   if (replacement_list.IsEmpty())
     replacement_list.AddProxyServer(net::ProxyServer::Direct());
   *proxy_list = replacement_list;
+  return true;
+}
+
+void RecordGooglezipProxyRemovalResultHistogram(
+    PrefProxyConfigTrackerImpl::GooglezipProxyRemovalResult result) {
+  UMA_HISTOGRAM_ENUMERATION(
+      "Net.PrefProxyConfig.GooglezipProxyRemovalResult", result,
+      PrefProxyConfigTrackerImpl::GOOGLEZIP_PROXY_REMOVAL_RESULT_MAX);
 }
 
 // Remove any Data Reduction Proxies like *.googlezip.net from |proxy_rules|.
@@ -61,17 +68,30 @@ void RemoveGooglezipDataReductionProxiesFromList(net::ProxyList* proxy_list) {
 // the Data Reduction Proxy without adding any of the necessary authentication
 // headers or applying the Data Reduction Proxy bypass logic. See
 // http://crbug.com/476610.
-// TODO(sclittle): Add UMA to record how often this method is called, and how
-// often it actually removes a *.googlezip.net proxy. This method should be
-// removed once it stops actually finding and removing *.googlezip.net proxies
-// from the proxy rules.
+// TODO(sclittle): This method should be removed once the UMA indicates that
+// *.googlezip.net proxies are no longer present in the |proxy_rules|.

[eroman, 2015/05/14 22:20:15]

Is there a separate strategy to actually remove them from the prefs? (Since this
code here just works around bad prefs, but does not actually modify the prefs)

[sclittle, 2015/05/15 02:29:42]

The strategy to remove the bad pref actually already exists:
https://codereview.chromium.org/1104413003

The issue was that the problem was still occuring for clients with that fix. At
the moment, we suspect that this is caused by more than one bug in the code -
the pref migration issue that was addressed in
https://codereview.chromium.org/1104413003 - and something else that we haven't
uncovered yet.

This hack is just to make absolutely sure that the pref isn't the problem, just
in case there's a race with prefs or something like that which is triggering the
DRP to be used when it shouldn't be. We suspect that
https://codereview.chromium.org/1135153002 will treat the symptoms of that
second bug.

Ideally, the UMA added in this CL will indicate that this hack is helping a
negligible number of clients, and then we can remove this hack.

 void RemoveGooglezipDataReductionProxies(
     net::ProxyConfig::ProxyRules* proxy_rules) {
-  RemoveGooglezipDataReductionProxiesFromList(&proxy_rules->fallback_proxies);
-  RemoveGooglezipDataReductionProxiesFromList(&proxy_rules->proxies_for_ftp);
-  RemoveGooglezipDataReductionProxiesFromList(&proxy_rules->proxies_for_http);
-  RemoveGooglezipDataReductionProxiesFromList(&proxy_rules->proxies_for_https);
-  RemoveGooglezipDataReductionProxiesFromList(&proxy_rules->single_proxies);
+  // Remove any *.googlezip.net proxies from any of the proxy lists in
+  // |proxy_rules|. Bitwise ORs are used instead of logical ORs in order to
+  // avoid short-circuit evaluation.
+  if (RemoveGooglezipDataReductionProxiesFromList(

[eroman, 2015/05/14 22:20:15]

I don't like this. How about

bool removed = false;
removed |= RemoveGoogleZip...
removed |= RemovedGoogleZip..

Or alternately having RemoveGoogleZip return an integer count of how many were
removed. Then you could do:

size_t num_removed = RemoveGoogleZip...(...) + RemoveGoogleZip(...)

if (num_removed > 0) {
   RecordGoogleZip...
}

[sclittle, 2015/05/15 02:29:42]

Done. Changed to report total number of proxies removed.

+          &proxy_rules->fallback_proxies) |
+      RemoveGooglezipDataReductionProxiesFromList(
+          &proxy_rules->proxies_for_ftp) |
+      RemoveGooglezipDataReductionProxiesFromList(
+          &proxy_rules->proxies_for_http) |
+      RemoveGooglezipDataReductionProxiesFromList(
+          &proxy_rules->proxies_for_https) |
+      RemoveGooglezipDataReductionProxiesFromList(
+          &proxy_rules->single_proxies)) {
+    RecordGooglezipProxyRemovalResultHistogram(
+        PrefProxyConfigTrackerImpl::
+            GOOGLEZIP_PROXY_REMOVAL_RESULT_FOUND_AND_REMOVED);
+  } else {
+    RecordGooglezipProxyRemovalResultHistogram(

[eroman, 2015/05/14 22:20:15]

style: I don't like having two calls to RecordGooglezip...()
Maybe just set the ProxyRemovalResult, and then call RecordHistogram once.  And
in fact at that point may as well just inline it.

[sclittle, 2015/05/15 02:29:42]

Now that it's reporting total number of proxies removed, there's only one
RecordHistogram call.

+        PrefProxyConfigTrackerImpl::GOOGLEZIP_PROXY_REMOVAL_RESULT_NOT_FOUND);
+  }
 }
 
 }  // namespace