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"
 #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"
 #include "base/time/time.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/renderer/chrome_content_renderer_client.h"
 #include "content/public/common/content_constants.h"
 #include "content/public/renderer/document_state.h"
 #include "content/public/renderer/render_thread.h"
 #include "content/public/renderer/render_view.h"
@@ skipping 142 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.

[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";
-  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
+  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;
 }
 
 // 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
@@ skipping 24 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, "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()));
     }
   }
 }
 
 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();
 }