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..010d834a7a138ea83bf8ac71310efb595e5dd4b3 100644
--- a/chrome/renderer/page_load_histograms.cc
+++ b/chrome/renderer/page_load_histograms.cc
@@ -11,6 +11,7 @@
 #include "base/metrics/field_trial.h"
 #include "base/metrics/histogram.h"
 #include "base/strings/string_number_conversions.h"
+#include "base/strings/string_split.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
@@ -173,18 +174,29 @@ 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.

[Alexei Svitkine (slow), 2014/01/09 16:39:29]

Expand comment to mention that this is only done for pages fetched via the
proxy.

[ksimbili, 2014/01/09 18:54:50]

Done.

+bool ViaHeaderContains(WebFrame* frame, const std::string& via_value) {
+  const char kViaHeaderName[] = "Via";
 
   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
+  std::vector<std::string> via_header_values;
+  base::SplitString(base::UTF16ToUTF8(

[nikhilmadan, 2014/01/08 22:25:29]

Add a comment explaining why we do this

[ksimbili, 2014/01/09 18:54:50]

Done.

+      frame->dataSource()->response().httpHeaderField(kViaHeaderName)),
+      ',', &via_header_values);

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

This will work most of the time, but please follow a pattern from the network
stack implementation (e.g., See HttpResponseHeaders, or the code I linked).
There are lots of subtle ways to get header parsing wrong, so you shouldn't be
rolling your own.

[ksimbili, 2014/01/09 18:54:50]

Like I said, we don't have HttpResponseHeaders here.

+  for (unsigned int i = 0; i < via_header_values.size(); ++i) {
+    if (via_header_values[i].find(via_value) != std::string::npos)

[Alexei Svitkine (slow), 2014/01/09 16:39:29]

Will this find any part separate by commas that contains the string "Via"? If
so, please document in the comment. Though, it also seems the perhaps this check
should be stricter than that.

Also, if you're just doing it this way, why split the string at all - wouldn't
just doing a find() on the pre-split value be equivalent?

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

Via values shouldn't have commas in them. Why not use Enumerate header the way I
did in the code I linked? 

[ksimbili, 2014/01/09 18:54:50]

We don't have network stack HTTPResponseHeaders here. All we have a parsed
header values into WebResponse.

[ksimbili, 2014/01/09 18:54:50]

we are not using find() directly to make sure that we don't match others proxy
which also has this string in it.

[Alexei Svitkine (slow), 2014/01/09 19:01:42]

If |via_value| never contains a comma (which is the case in this CL for all
callers of this function), in what case would this logic be different than the
following?

const string16 header_value =
    frame->dataSource()->response().httpHeaderField(kViaHeaderName));
return header_value.find(via_value) != std::string::npos;

Separately, if you're trying to do a more strict check, should it actually be
more strict (e.g. instead of using .find(), use ==)?

[ksimbili, 2014/01/09 19:13:49]

No. Sorry for the confusion. 
There can be multiple Via headers present, and all these via_values are
coalesced into one value using ',' as the seperator.
Modified the comment to mean it.
eg; Via: 1.0 Compression proxy, 1.1 Google promise preview

[Alexei Svitkine (slow), 2014/01/09 19:34:38]

Ok, thanks for explaining the format of that header. I would actually recommend
putting the above example in the comment itself to make things clear.

However, I still don't think this answers my question: in what case would
iterating over them produce a different result than just doing a .find() on the
unsplit value? It seems that it would still be equivalent - so then why split?

[ksimbili, 2014/01/09 20:16:21]

Sorry, corrected the code. I should have checked for equality.
We were trying not to match for 'foo' in 
Via: bar, foobar, goo

+      return true;
+  }
   return false;
 }
 
@@ -229,40 +241,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 +250,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, "Google Promise Preview") ? PREVIEW :

[Alexei Svitkine (slow), 2014/01/09 16:39:29]

Nit: Incorrect indent.

[ksimbili, 2014/01/09 18:54:50]

Done.

+          PREVIEW_NOT_USED;
+    } else if (ViaHeaderContains(frame, "Google Promise Original")) {
       *preview_state = PREVIEW_WAS_SHOWN;
-      *preview_experiment_id = GetQueryStringBasedExperiment(GURL(referrer));
+      *preview_experiment_id = GetQueryStringBasedExperiment(
+          GURL(frame->document().referrer()));
     }
   }
 }
@@ -560,8 +542,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.