[NoStatePrefetch] Track redirects in UMA

chrome/browser/prerender/prerender_resource_throttle.cc

Index: chrome/browser/prerender/prerender_resource_throttle.cc
diff --git a/chrome/browser/prerender/prerender_resource_throttle.cc b/chrome/browser/prerender/prerender_resource_throttle.cc
index a8b9796acdbb0e0f567d3e3cde37cd0acccf8309..9aef66c2b59a30034e43454f7c10f96df55a4a75 100644
--- a/chrome/browser/prerender/prerender_resource_throttle.cc
+++ b/chrome/browser/prerender/prerender_resource_throttle.cc
@@ -4,6 +4,8 @@
 
 #include "chrome/browser/prerender/prerender_resource_throttle.h"
 
+#include "base/logging.h"
+#include "base/numerics/safe_conversions.h"
 #include "build/build_config.h"
 #include "chrome/browser/prerender/prerender_final_status.h"
 #include "chrome/browser/prerender/prerender_manager.h"
@@ -26,6 +28,14 @@ static const char kFollowOnlyWhenPrerenderShown[] =
     "follow-only-when-prerender-shown";
 
 PrerenderContents* g_prerender_contents_for_testing;
+
+// Returns true if the response has a "no-store" cache control header.
+bool IsNoStoreResponse(const net::URLRequest& request) {
+  const net::HttpResponseInfo& response_info = request.response_info();
+  return response_info.headers.get() &&
+         response_info.headers->HasHeaderValue("cache-control", "no-store");
+}
+
 }  // namespace
 
 void PrerenderResourceThrottle::OverridePrerenderContentsForTesting(
@@ -60,12 +70,11 @@ void PrerenderResourceThrottle::WillRedirectRequest(
   request_->GetResponseHeaderByName(kFollowOnlyWhenPrerenderShown, &header);
 
   content::BrowserThread::PostTask(
-      content::BrowserThread::UI,
-      FROM_HERE,
+      content::BrowserThread::UI, FROM_HERE,
       base::Bind(&PrerenderResourceThrottle::WillRedirectRequestOnUI,
                  AsWeakPtr(), header, info->GetResourceType(), info->IsAsync(),
-                 info->GetChildID(), info->GetRenderFrameID(),
-                 redirect_info.new_url));
+                 IsNoStoreResponse(*request_), info->GetChildID(),
+                 info->GetRenderFrameID(), redirect_info.new_url));
 }
 
 void PrerenderResourceThrottle::WillProcessResponse(bool* defer) {
@@ -74,18 +83,16 @@ void PrerenderResourceThrottle::WillProcessResponse(bool* defer) {
   if (!info)
     return;
 
-  bool is_no_store = false;
-  const net::HttpResponseInfo& response_info = request_->response_info();
-  if (response_info.headers.get()) {
-    is_no_store =
-        response_info.headers->HasHeaderValue("cache-control", "no-store");
-  }
+  DCHECK_GT(request_->url_chain().size(), 0u);
+  int redirect_count =
+      base::saturated_cast<int>(request_->url_chain().size() - 1);

[pasko, 2016/09/05 14:39:48]

size() returns an unsigned int. Suppose it returns zero, then by subtracting 1
we would have an UINT_MAX, which after saturated_cast turns into INT_MAX, I
believe (modulo some weird platforms).

I wanted it to become a -1 and let the histogram code deal with it (I think they
have underflow and overflow buckets, and underflow would be easier to read on
the dashboard). This would be achievable with:

int redirect_count = base::saturated_cast<int>(request_->url_chain().size()) -
1;

WDYT?

[droger, 2016/09/05 15:03:44]

Good catch.
Of course. That's what I wanted to do, but misplaced the bracket.

 
   content::BrowserThread::PostTask(
       content::BrowserThread::UI, FROM_HERE,
       base::Bind(&PrerenderResourceThrottle::WillProcessResponseOnUI,
                  content::IsResourceTypeFrame(info->GetResourceType()),
-                 is_no_store, info->GetChildID(), info->GetRenderFrameID()));
+                 IsNoStoreResponse(*request_), redirect_count,
+                 info->GetChildID(), info->GetRenderFrameID()));
 }
 
 const char* PrerenderResourceThrottle::GetNameForLogging() const {
@@ -144,6 +151,7 @@ void PrerenderResourceThrottle::WillRedirectRequestOnUI(
     const std::string& follow_only_when_prerender_shown_header,
     ResourceType resource_type,
     bool async,
+    bool is_no_store,
     int render_process_id,
     int render_frame_id,
     const GURL& new_url) {
@@ -151,6 +159,10 @@ void PrerenderResourceThrottle::WillRedirectRequestOnUI(
   PrerenderContents* prerender_contents =
       PrerenderContentsFromRenderFrame(render_process_id, render_frame_id);
   if (prerender_contents) {
+    prerender_contents->prerender_manager()->RecordPrefetchResponseReceived(
+        prerender_contents->origin(),
+        content::IsResourceTypeFrame(resource_type), true /* is_redirect */,
+        is_no_store);
     // Abort any prerenders with requests which redirect to invalid schemes.
     if (!PrerenderManager::DoesURLHaveValidScheme(new_url)) {
       prerender_contents->Destroy(FINAL_STATUS_UNSUPPORTED_SCHEME);
@@ -183,6 +195,7 @@ void PrerenderResourceThrottle::WillRedirectRequestOnUI(
 // static
 void PrerenderResourceThrottle::WillProcessResponseOnUI(bool is_main_resource,
                                                         bool is_no_store,
+                                                        int redirect_count,
                                                         int render_process_id,
                                                         int render_frame_id) {
   PrerenderContents* prerender_contents =
@@ -193,8 +206,11 @@ void PrerenderResourceThrottle::WillProcessResponseOnUI(bool is_main_resource,
   if (prerender_contents->prerender_mode() != PREFETCH_ONLY)
     return;
 
-  prerender_contents->prerender_manager()->RecordResourcePrefetch(
-      prerender_contents->origin(), is_main_resource, is_no_store);
+  prerender_contents->prerender_manager()->RecordPrefetchResponseReceived(
+      prerender_contents->origin(), is_main_resource, false /* is_redirect */,
+      is_no_store);
+  prerender_contents->prerender_manager()->RecordPrefetchRedirectCount(
+      prerender_contents->origin(), is_main_resource, redirect_count);
 }
 
 // static