[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..f4b08ca5f9d72a8a3abfac5d0471e911cae0d855 100644
--- a/chrome/browser/prerender/prerender_resource_throttle.cc
+++ b/chrome/browser/prerender/prerender_resource_throttle.cc
@@ -26,6 +26,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(net::URLRequest* request) {

[pasko, 2016/09/02 16:34:48]

would it be possible to pass the argument by const reference, as recommended in
the style guide?

[droger, 2016/09/05 12:38:47]

Done.

+  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 +68,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 +81,12 @@ 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");
-  }
-
   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_), request_->url_chain().size() - 1,

[pasko, 2016/09/02 16:34:48]

Even though size of the chain is unlikely to be zero here, I want to be on the
safe side with this unsigned to signed conversion

how about:
    base::saturated_cast<int32_t>(request_->url_chain().size()),

and having int32_t everywhere after that just as base::HistogramBase::Sample?

[droger, 2016/09/05 12:38:47]

Done.

+                 info->GetChildID(), info->GetRenderFrameID()));
 }
 
 const char* PrerenderResourceThrottle::GetNameForLogging() const {
@@ -144,6 +145,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 +153,10 @@ void PrerenderResourceThrottle::WillRedirectRequestOnUI(
   PrerenderContents* prerender_contents =
       PrerenderContentsFromRenderFrame(render_process_id, render_frame_id);
   if (prerender_contents) {
+    prerender_contents->prerender_manager()->RecordResourcePrefetch(
+        prerender_contents->origin(),
+        content::IsResourceTypeFrame(resource_type), true /*is_redirect*/,

[pasko, 2016/09/02 16:34:48]

most code I see surrounds the parameter name with spaces, e.g.:

/* is_redirect */

Also aesthetically this looks nicer to me.

[droger, 2016/09/05 12:38:47]

Done.

+        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 +189,7 @@ void PrerenderResourceThrottle::WillRedirectRequestOnUI(
 // static
 void PrerenderResourceThrottle::WillProcessResponseOnUI(bool is_main_resource,
                                                         bool is_no_store,
+                                                        size_t redirect_count,
                                                         int render_process_id,
                                                         int render_frame_id) {
   PrerenderContents* prerender_contents =
@@ -194,7 +201,10 @@ void PrerenderResourceThrottle::WillProcessResponseOnUI(bool is_main_resource,
     return;
 
   prerender_contents->prerender_manager()->RecordResourcePrefetch(
-      prerender_contents->origin(), is_main_resource, is_no_store);
+      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