Change the detection of preview request to via header.

chrome/renderer/page_load_histograms.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/renderer/page_load_histograms.h"
 
 #include <string>
 
 #include "base/command_line.h"
 #include "base/logging.h"
@@ skipping 155 matching lines @@
   return static_cast<URLPattern::SchemeMasks>(0);
 }
 
 // Returns true if the data reduction proxy was used. Note, this function will
 // produce a false positive if a page is fetched using SPDY and using a proxy,
 // and |kDatReductionProxyViaValue| is added to the Via header.
 // TODO(bengr): Plumb the hostname of the proxy from |HttpNetworkTransaction|
 // and check if it matches |SPDY_PROXY_AUTH_ORIGIN|.
 bool DataReductionProxyWasUsed(WebFrame* frame) {
 #if defined(SPDY_PROXY_AUTH_ORIGIN)
+  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";
-  const char kDatReductionProxyViaValue[] = "1.1 Chrome Compression Proxy";
 
   DocumentState* document_state =
       DocumentState::FromDataSource(frame->dataSource());
   if (!document_state->was_fetched_via_proxy())
     return false;
 
   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
 // a Google Web Search results page. This is a little non-deterministic
 // because desktop and mobile websearch differ and sometimes just provide
 // http://www.google.com/ as the referrer. In the case of /url we can be sure
 // that it came from websearch but we will be generous and allow for cases
 // where a non-Google URL was provided a bare Google URL as a referrer.
 // The domain validation matches the code used by the prerenderer for similar
 // purposes.
@@ skipping 23 matching lines @@
     return kNoExperiment;
 
   int experiment_id;
   if (!base::StringToInt(value, &experiment_id))
     return kNoExperiment;
   if (0 < experiment_id && experiment_id <= kMaxExperimentID)
     return experiment_id;
   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,
                      int* preview_experiment_id) {
   // 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()));
     }
   }
 }
 
 void DumpPerformanceTiming(const WebPerformance& performance,
                            DocumentState* document_state,
                            bool data_reduction_proxy_was_used,
                            bool came_from_websearch,
                            int websearch_chrome_joint_experiment_id,
                            GwsPreviewState preview_state,
@@ skipping 261 matching lines @@
       IsFromGoogleSearchResult(frame->document().url(),
                                GURL(frame->document().referrer()));
   int websearch_chrome_joint_experiment_id = kNoExperiment;
   if (came_from_websearch) {
     websearch_chrome_joint_experiment_id =
         GetQueryStringBasedExperiment(GURL(frame->document().referrer()));
   }
 
   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.
   // http://www.w3.org/TR/navigation-timing/
   // TODO(tonyg, jar): We are in the process of vetting these metrics against
   // the existing ones. Once we understand any differences, we will standardize
   // on a single set of metrics.
   DumpPerformanceTiming(frame->performance(), document_state,
                         data_reduction_proxy_was_used,
                         came_from_websearch,
@@ skipping 472 matching lines @@
 
   DCHECK(document_state);
   DCHECK(ds);
   GURL url(ds->request().url());
   Time start = document_state->start_load_time();
   Time finish = document_state->finish_load_time();
   // TODO(mbelshe): should we log more stats?
   VLOG(1) << "PLT: " << (finish - start).InMilliseconds() << "ms "
           << url.spec();
 }