Change the detection of preview request to via header.

chrome/renderer/page_load_histograms.cc

Index: chrome/renderer/page_load_histograms.cc
diff --git a/chrome/renderer/page_load_histograms.cc b/chrome/renderer/page_load_histograms.cc
index 901b9324c1d2eec9208909436908b5e5c5a677ad..12b0d6797a5590abdfc7e2286e1d09706bb429ec 100644
--- a/chrome/renderer/page_load_histograms.cc
+++ b/chrome/renderer/page_load_histograms.cc
@@ -173,8 +173,15 @@ URLPattern::SchemeMasks GetSupportedSchemeType(const GURL& url) {
 // and check if it matches |SPDY_PROXY_AUTH_ORIGIN|.
 bool DataReductionProxyWasUsed(WebFrame* frame) {
 #if defined(SPDY_PROXY_AUTH_ORIGIN)
-  const char kViaHeaderName[] = "Via";
   const char kDatReductionProxyViaValue[] = "1.1 Chrome Compression Proxy";
+  return ViaHeaderContains(frame, kDatReductionProxyViaValue);
+#endif
+  return false;
+}
+
+// Helper function to check for string in 'via' header.
+bool ViaHeaderContains(WebFrame* frame, const std::string& viaValue) {

[jochen (gone - plz use gerrit), 2014/01/07 10:22:23]

via_value

[ksimbili, 2014/01/08 01:11:38]

Done.

+  const char kViaHeaderName[] = "Via";
 
   DocumentState* document_state =
       DocumentState::FromDataSource(frame->dataSource());
@@ -183,9 +190,7 @@ bool DataReductionProxyWasUsed(WebFrame* frame) {
 
   std::string via_header(base::UTF16ToUTF8(
       frame->dataSource()->response().httpHeaderField(kViaHeaderName)));
-  return via_header.find(kDatReductionProxyViaValue) != std::string::npos;
-#endif
-  return false;
+  return via_header.find(viaValue) != std::string::npos;

[bengr, 2014/01/07 17:16:09]

This is overly inclusive. E.g., if some other proxy adds "foo bar
promise-preview" to the via header, ViaHeaderContains("promise-preview") will
return true. Please do something like the following instead and fix for the data
reduction proxy:

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/net...

[ksimbili, 2014/01/08 01:11:38]

Done.

 }
 
 // Returns true if the provided URL is a referrer string that came from
@@ -229,40 +234,8 @@ int GetQueryStringBasedExperiment(const GURL& referrer) {
   return kNoExperiment;
 }
 
-// Appends "cerivrj_*" and "gcjeid" query parameters from preview_url to url.
-// Returns true if "cerivrj_*" query is found.
-// This will be used only for search results URLs.
-bool AppendPreviewQueryFromURL(const GURL& preview_url, GURL* url) {
-  bool preview_query_found = false;
-  for (net::QueryIterator it(preview_url); !it.IsAtEnd(); it.Advance()) {
-    const std::string param_name = it.GetKey();
-    bool is_preview = StartsWithASCII(param_name, "cerivrj_", true);
-    if (url && (is_preview || param_name == "gcjeid"))
-      net::AppendQueryParameter(*url, param_name, it.GetValue());
-    preview_query_found = preview_query_found || is_preview;
-  }
-  return preview_query_found;
-}
-
-// Returns true if the provided referrer URL is the preview URL of the current
-// URL. Preview URL differs from original only with "cerivrj_*", "gcjeid" query
-// parameters.
-bool IsReferrerPreviewOfURL(const GURL& url,
-                            const base::string16& referrer_str) {
-  GURL referrer(referrer_str);
-  if (referrer.is_valid()) {
-    GURL generated_preview_url(url);
-    // Now try to copy "cerivrj_*" and "gcjeid" paramters to url and check if
-    // they exactly match.
-    if (AppendPreviewQueryFromURL(referrer, &generated_preview_url))
-      return generated_preview_url == referrer;
-  }
-  return false;
-}
-
 // Returns preview state by looking at url and referer url.
-void GetPreviewState(const GURL& url,
-                     const base::string16& referrer,
+void GetPreviewState(WebFrame* frame,
                      bool came_from_websearch,
                      bool data_reduction_proxy_was_used,
                      GwsPreviewState* preview_state,
@@ -270,15 +243,17 @@ void GetPreviewState(const GURL& url,
   // Conditions for GWS preview are,
   // 1. Data reduction proxy was used.
   // 2. URL is loaded from web search.
-  // If the URL contains "cerivrj_*' query parameter record under
-  // "Preview".
+  // Based on via header PREVIEW/PREVIEW_WAS_SHOWN/PREVIEW_NOT_USED is
+  // determined.
   if (data_reduction_proxy_was_used) {
     if (came_from_websearch) {
       *preview_state =
-          AppendPreviewQueryFromURL(url, NULL) ? PREVIEW : PREVIEW_NOT_USED;
-    } else if (IsReferrerPreviewOfURL(url, referrer)) {
+          ViaHeaderContains(frame, "promise-preview") ? PREVIEW :

[bengr, 2014/01/07 17:16:09]

I would use annotations that are less likely to appear in natural browsing.
E.g., "Google promise-preview" or "Chrome promise-preview". Same for
"promise-original"

Also, this is a bit of an abuse of the Via header, which should say simply that
the request went through the promise proxy. Perhaps you can differentiate
preview from original some other way? 

[ksimbili, 2014/01/08 01:11:38]

Via header was the way we(flywheel and promise team) agreed upon instead of
query based checking.

[bengr, 2014/01/09 17:32:46]

I know. I just don't think you should use a header value that could appear in
natural traffic. Adding the "Google" helps. Also, please don't refer to team
names, code names, etc. that are internal to Google. Chromium is an open source
project.

+          PREVIEW_NOT_USED;
+    } else if (ViaHeaderContains(frame, "promise-original")) {
       *preview_state = PREVIEW_WAS_SHOWN;
-      *preview_experiment_id = GetQueryStringBasedExperiment(GURL(referrer));
+      *preview_experiment_id = GetQueryStringBasedExperiment(
+          GURL(frame->document().referrer()));
     }
   }
 }
@@ -560,8 +535,7 @@ void PageLoadHistograms::Dump(WebFrame* frame) {
 
   GwsPreviewState preview_state = PREVIEW_NONE;
   int preview_experiment_id = websearch_chrome_joint_experiment_id;
-  GetPreviewState(frame->document().url(), frame->document().referrer(),
-                  came_from_websearch, data_reduction_proxy_was_used,
+  GetPreviewState(frame, came_from_websearch, data_reduction_proxy_was_used,
                   &preview_state, &preview_experiment_id);
 
   // Times based on the Web Timing metrics.