Switch variations http headers to only be reported over https.

components/variations/net/variations_http_headers.cc

Index: components/variations/net/variations_http_headers.cc
diff --git a/components/variations/net/variations_http_headers.cc b/components/variations/net/variations_http_headers.cc
index 48e84eb190c96f0f8d1ea955ddda215676d9d206..885d616ecc40ac528ad90fc6678771ae01792a27 100644
--- a/components/variations/net/variations_http_headers.cc
+++ b/components/variations/net/variations_http_headers.cc
@@ -7,6 +7,7 @@
 #include <stddef.h>
 
 #include "base/macros.h"
+#include "base/metrics/histogram_macros.h"
 #include "base/strings/string_util.h"
 #include "components/google/core/browser/google_util.h"
 #include "components/variations/variations_http_header_provider.h"
@@ -39,6 +40,45 @@ const char* kHostsToSetHeadersFor[] = {
 const char kChromeUMAEnabled[] = "X-Chrome-UMA-Enabled";
 const char kClientData[] = "X-Client-Data";
 
+// The result of checking if a URL should have variations headers appended.
+// This enum is used to record UMA histogram values, and should not be
+// reordered.
+enum ShouldAppendHeadersResult {
+  NOT_GOOGLE_DOMAIN,
+  GOOGLE_NOT_HTTPS,
+  SHOULD_APPEND,
+  SHOULD_APPEND_HEADERS_RESULT_SIZE,
+};
+
+// Checks whether headers should be appended to the |url|, based on the domain
+// of |url|, and if it's an http/https spec.

[rkaplow, 2016/12/12 18:25:05]

this method doesn't care about the ssl-ness 

[jwd, 2016/12/12 19:23:08]

Changed code.

+bool ShouldAppendForDomain(const GURL& url) {
+  // IsGoogleDomainUrl checks if the url is valid and is http/https.

[rkaplow, 2016/12/12 18:25:05]

I don't quite get the comment - this doesn't fully check if it's valid since
it's not checking if its HTTPS. Isn't this jut checking if it is a google
domain? maybe rephase?

[jwd, 2016/12/12 19:23:08]

Changed code.

+  if (google_util::IsGoogleDomainUrl(url, google_util::ALLOW_SUBDOMAIN,
+                                     google_util::ALLOW_NON_STANDARD_PORTS)) {
+    return true;
+  }
+
+  if (!url.is_valid() || !url.SchemeIsHTTPOrHTTPS())

[rkaplow, 2016/12/12 18:25:05]

i realize this is just refactored, but it seems odd to me that this occurs after
isgoogeldomainURL. However I checked and isGoogleDomain checks this already,
with:
https://codesearch.chromium.org/chromium/src/components/google/core/browser/g...

(not sure if it's worth changing to that method, up to you).

Another nit on this - seems more intuitive to check before the Google check, but
doesn't make a difference

[jwd, 2016/12/12 19:23:08]

Pulled check out to calling function.

+    return false;
+
+  // Some domains don't have international TLD extensions, so testing for them
+  // is very straight forward.
+  const std::string host = url.host();
+  for (size_t i = 0; i < arraysize(kSuffixesToSetHeadersFor); ++i) {
+    if (base::EndsWith(host, kSuffixesToSetHeadersFor[i],
+                       base::CompareCase::INSENSITIVE_ASCII))
+      return true;
+  }
+  for (size_t i = 0; i < arraysize(kHostsToSetHeadersFor); ++i) {
+    if (base::LowerCaseEqualsASCII(host, kHostsToSetHeadersFor[i]))
+      return true;
+  }
+
+  return google_util::IsYoutubeDomainUrl(url, google_util::ALLOW_SUBDOMAIN,

[rkaplow, 2016/12/12 18:25:05]

this is a minor nit, but seems strange that this is logically away from the
isGoogleDomainURL. I'd prefer this check live up there, and then just a return
false here

[jwd, 2016/12/12 19:23:08]

Done.

+                                         google_util::ALLOW_NON_STANDARD_PORTS);
+}
+
 }  // namespace
 
 void AppendVariationHeaders(const GURL& url,
@@ -80,29 +120,19 @@ namespace internal {
 
 // static
 bool ShouldAppendVariationHeaders(const GURL& url) {
-  if (google_util::IsGoogleDomainUrl(url, google_util::ALLOW_SUBDOMAIN,
-                                     google_util::ALLOW_NON_STANDARD_PORTS)) {
-    return true;
-  }
-
-  if (!url.is_valid() || !url.SchemeIsHTTPOrHTTPS())
+  const char kShouldAppendResultHistogram[] =
+      "Variations.Headers.ShouldAppendResult";

[rkaplow, 2016/12/12 18:25:05]

I don't love the name of the histogram - it seems a bit indirect. I'd prefer
something that says this is checking the URL. Not sure...
V.H.URLValidationResult? 

[jwd, 2016/12/12 19:23:08]

Done.

+  if (!ShouldAppendForDomain(url)) {
+    UMA_HISTOGRAM_ENUMERATION(kShouldAppendResultHistogram, NOT_GOOGLE_DOMAIN,
+                              SHOULD_APPEND_HEADERS_RESULT_SIZE);
     return false;
-
-  // Some domains don't have international TLD extensions, so testing for them
-  // is very straight forward.
-  const std::string host = url.host();
-  for (size_t i = 0; i < arraysize(kSuffixesToSetHeadersFor); ++i) {
-    if (base::EndsWith(host, kSuffixesToSetHeadersFor[i],
-                       base::CompareCase::INSENSITIVE_ASCII))
-      return true;
-  }
-  for (size_t i = 0; i < arraysize(kHostsToSetHeadersFor); ++i) {
-    if (base::LowerCaseEqualsASCII(host, kHostsToSetHeadersFor[i]))
-      return true;
   }
 
-  return google_util::IsYoutubeDomainUrl(url, google_util::ALLOW_SUBDOMAIN,
-                                         google_util::ALLOW_NON_STANDARD_PORTS);
+  bool isHttps = url.SchemeIs("https");
+  UMA_HISTOGRAM_ENUMERATION(kShouldAppendResultHistogram,
+                            isHttps ? SHOULD_APPEND : GOOGLE_NOT_HTTPS,
+                            SHOULD_APPEND_HEADERS_RESULT_SIZE);
+  return isHttps;
 }
 
 }  // namespace internal