Summary

```--
With PlzNavigate, main frame navigations
don't have a valid (render_process_id, render_frame_id). This CL is a
preliminary refactoring to properly support PlzNavigate.

Details
```

--
To be able to do browser navigations to non yet created frames, we use
webcontents that will always be available. This is preparation work
to be able to use webcontents identification instead of frame ids easily.

At the higher level we are passing a WebContentsGetter and run it on a
UI thread, leading to new dedicated functions in the observer. We then
use a wrapper to initialize the NavigationID part of the request summary.
SummarizeResponse is not responsible for filling in NavigationID any more,
and the behavior has been updated in unit tests.

BUG=631966

Review-Url: https://codereview.chromium.org/2545943003
Cr-Commit-Position: refs/heads/master@{#437524}

chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc

@@ -510,7 +510,7 @@ void ChromeResourceDispatcherHostDelegate::RequestBeginning(
 
   if (io_data->resource_prefetch_predictor_observer()) {
     io_data->resource_prefetch_predictor_observer()->OnRequestStarted(
-        request, resource_type, info->GetChildID(), info->GetRenderFrameID());
+        request, resource_type, info->GetWebContentsGetterForRequest());
   }
 }
 
@@ -776,7 +776,8 @@ void ChromeResourceDispatcherHostDelegate::OnResponseStarted(
 #endif
 
   if (io_data->resource_prefetch_predictor_observer())
-    io_data->resource_prefetch_predictor_observer()->OnResponseStarted(request);
+    io_data->resource_prefetch_predictor_observer()->OnResponseStarted(
+        request, info->GetWebContentsGetterForRequest());
 
   mod_pagespeed::RecordMetrics(info->GetResourceType(), request->url(),
                                request->response_headers());
@@ -802,7 +803,7 @@ void ChromeResourceDispatcherHostDelegate::OnRequestRedirected(
 
   if (io_data->resource_prefetch_predictor_observer()) {
     io_data->resource_prefetch_predictor_observer()->OnRequestRedirected(
-        redirect_url, request);
+        request, redirect_url, info->GetWebContentsGetterForRequest());
   }
 
   if (io_data->policy_header_helper())

chrome/browser/net/resource_prefetch_predictor_observer.cc

@@ -4,17 +4,25 @@
 
 #include "chrome/browser/net/resource_prefetch_predictor_observer.h"
 
+#include <memory>
 #include <string>
+#include <utility>
 
+#include "base/memory/ptr_util.h"
 #include "base/metrics/histogram_macros.h"
 #include "content/public/browser/browser_thread.h"
-#include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/resource_request_info.h"
 #include "net/url_request/url_request.h"
 #include "url/gurl.h"
 
+namespace content {
+class WebContents;
+}
+
 using content::BrowserThread;
 using predictors::ResourcePrefetchPredictor;
+using URLRequestSummary =
+    predictors::ResourcePrefetchPredictor::URLRequestSummary;
 
 namespace {
 
@@ -51,6 +59,20 @@ void ReportMainFrameRequestStats(MainFrameRequestStats stat) {
                             MAIN_FRAME_REQUEST_STATS_MAX);
 }
 
+bool TryToFillNavigationID(
+    predictors::NavigationID* navigation_id,
+    const content::ResourceRequestInfo::WebContentsGetter& web_contents_getter,
+    const GURL& main_frame_url,
+    const base::TimeTicks& creation_time) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  content::WebContents* web_contents = web_contents_getter.Run();
+  if (!web_contents)
+    return false;
+  *navigation_id =
+      predictors::NavigationID(web_contents, main_frame_url, creation_time);
+  return true;
+}
+
 }  // namespace
 
 namespace chrome_browser_net {
@@ -69,8 +91,8 @@ ResourcePrefetchPredictorObserver::~ResourcePrefetchPredictorObserver() {
 void ResourcePrefetchPredictorObserver::OnRequestStarted(
     net::URLRequest* request,
     content::ResourceType resource_type,
-    int child_id,
-    int frame_id) {
+    const content::ResourceRequestInfo::WebContentsGetter&
+        web_contents_getter) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
   if (resource_type == content::RESOURCE_TYPE_MAIN_FRAME)
@@ -79,28 +101,26 @@ void ResourcePrefetchPredictorObserver::OnRequestStarted(
   if (!ResourcePrefetchPredictor::ShouldRecordRequest(request, resource_type))
     return;
 
-  ResourcePrefetchPredictor::URLRequestSummary summary;
-  summary.navigation_id.render_process_id = child_id;
-  summary.navigation_id.render_frame_id = frame_id;
-  summary.navigation_id.main_frame_url = request->first_party_for_cookies();
-  summary.navigation_id.creation_time = request->creation_time();
-  summary.resource_url = request->original_url();
-  summary.resource_type = resource_type;
+  auto summary = base::MakeUnique<URLRequestSummary>();
+  summary->resource_url = request->original_url();
+  summary->resource_type = resource_type;
 
   BrowserThread::PostTask(
-      BrowserThread::UI,
-      FROM_HERE,
-      base::Bind(&ResourcePrefetchPredictor::RecordURLRequest,
-                 predictor_,
-                 summary));
+      BrowserThread::UI, FROM_HERE,
+      base::Bind(&ResourcePrefetchPredictorObserver::OnRequestStartedOnUIThread,
+                 base::Unretained(this), base::Passed(std::move(summary)),
+                 web_contents_getter, request->first_party_for_cookies(),
+                 request->creation_time()));
 
   if (resource_type == content::RESOURCE_TYPE_MAIN_FRAME)
     ReportMainFrameRequestStats(MAIN_FRAME_REQUEST_STATS_PROCESSED_REQUESTS);
 }
 
 void ResourcePrefetchPredictorObserver::OnRequestRedirected(
+    net::URLRequest* request,
     const GURL& redirect_url,
-    net::URLRequest* request) {
+    const content::ResourceRequestInfo::WebContentsGetter&
+        web_contents_getter) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
   const content::ResourceRequestInfo* request_info =
@@ -113,20 +133,20 @@ void ResourcePrefetchPredictorObserver::OnRequestRedirected(
   if (!ResourcePrefetchPredictor::ShouldRecordRedirect(request))
     return;
 
-  ResourcePrefetchPredictor::URLRequestSummary summary;
+  auto summary = base::MakeUnique<URLRequestSummary>();
   if (!ResourcePrefetchPredictor::URLRequestSummary::SummarizeResponse(
-          *request, &summary)) {
+          *request, summary.get())) {
     return;
   }
-
-  summary.redirect_url = redirect_url;
+  summary->redirect_url = redirect_url;
 
   BrowserThread::PostTask(
-      BrowserThread::UI,
-      FROM_HERE,
-      base::Bind(&ResourcePrefetchPredictor::RecordURLRedirect,
-                 predictor_,
-                 summary));
+      BrowserThread::UI, FROM_HERE,
+      base::Bind(
+          &ResourcePrefetchPredictorObserver::OnRequestRedirectedOnUIThread,
+          base::Unretained(this), base::Passed(std::move(summary)),
+          web_contents_getter, request->first_party_for_cookies(),
+          request->creation_time()));
 
   if (request_info &&
       request_info->GetResourceType() == content::RESOURCE_TYPE_MAIN_FRAME) {
@@ -135,7 +155,9 @@ void ResourcePrefetchPredictorObserver::OnRequestRedirected(
 }
 
 void ResourcePrefetchPredictorObserver::OnResponseStarted(
-    net::URLRequest* request) {
+    net::URLRequest* request,
+    const content::ResourceRequestInfo::WebContentsGetter&
+        web_contents_getter) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
   ReportRequestStats(REQUEST_STATS_TOTAL_RESPONSES);
@@ -149,18 +171,19 @@ void ResourcePrefetchPredictorObserver::OnResponseStarted(
 
   if (!ResourcePrefetchPredictor::ShouldRecordResponse(request))
     return;
-  ResourcePrefetchPredictor::URLRequestSummary summary;
+  auto summary = base::MakeUnique<URLRequestSummary>();
   if (!ResourcePrefetchPredictor::URLRequestSummary::SummarizeResponse(
-          *request, &summary)) {
+          *request, summary.get())) {
     return;
   }
 
   BrowserThread::PostTask(
-      BrowserThread::UI,
-      FROM_HERE,
-      base::Bind(&ResourcePrefetchPredictor::RecordURLResponse,
-                 predictor_,
-                 summary));
+      BrowserThread::UI, FROM_HERE,
+      base::Bind(
+          &ResourcePrefetchPredictorObserver::OnResponseStartedOnUIThread,
+          base::Unretained(this), base::Passed(std::move(summary)),
+          web_contents_getter, request->first_party_for_cookies(),
+          request->creation_time()));
 
   ReportRequestStats(REQUEST_STATS_TOTAL_PROCESSED_RESPONSES);
   if (request_info &&
@@ -169,4 +192,43 @@ void ResourcePrefetchPredictorObserver::OnResponseStarted(
   }
 }
 
+void ResourcePrefetchPredictorObserver::OnRequestStartedOnUIThread(
+    std::unique_ptr<URLRequestSummary> summary,
+    const content::ResourceRequestInfo::WebContentsGetter& web_contents_getter,
+    const GURL& main_frame_url,
+    const base::TimeTicks& creation_time) const {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  if (!TryToFillNavigationID(&summary->navigation_id, web_contents_getter,
+                             main_frame_url, creation_time)) {
+    return;
+  }
+  predictor_->RecordURLRequest(*summary);
+}
+
+void ResourcePrefetchPredictorObserver::OnRequestRedirectedOnUIThread(
+    std::unique_ptr<URLRequestSummary> summary,
+    const content::ResourceRequestInfo::WebContentsGetter& web_contents_getter,
+    const GURL& main_frame_url,
+    const base::TimeTicks& creation_time) const {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  if (!TryToFillNavigationID(&summary->navigation_id, web_contents_getter,
+                             main_frame_url, creation_time)) {
+    return;
+  }
+  predictor_->RecordURLRedirect(*summary);
+}
+
+void ResourcePrefetchPredictorObserver::OnResponseStartedOnUIThread(
+    std::unique_ptr<URLRequestSummary> summary,
+    const content::ResourceRequestInfo::WebContentsGetter& web_contents_getter,
+    const GURL& main_frame_url,
+    const base::TimeTicks& creation_time) const {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  if (!TryToFillNavigationID(&summary->navigation_id, web_contents_getter,
+                             main_frame_url, creation_time)) {
+    return;
+  }
+  predictor_->RecordURLResponse(*summary);
+}
+
 }  // namespace chrome_browser_net

chrome/browser/net/resource_prefetch_predictor_observer.h

@@ -5,9 +5,12 @@
 #ifndef CHROME_BROWSER_NET_RESOURCE_PREFETCH_PREDICTOR_OBSERVER_H_
 #define CHROME_BROWSER_NET_RESOURCE_PREFETCH_PREDICTOR_OBSERVER_H_
 
+#include <memory>
+
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
 #include "chrome/browser/predictors/resource_prefetch_predictor.h"
+#include "content/public/browser/resource_request_info.h"
 #include "content/public/common/resource_type.h"
 
 namespace net {
@@ -22,8 +25,9 @@ namespace chrome_browser_net {
 // the ResourcePrefetchPredictor about the ones it is interested in.
 //  - Has an instance per profile, and is owned by the corresponding
 //    ProfileIOData.
-//  - Needs to be constructed on UI thread. Rest of the functions can only be
-//    called on the IO thread. Can be destroyed on UI or IO thread.
+//  - Needs to be constructed on UI thread. Can be destroyed on UI or IO thread.
+//    As for member functions, public members are meant to be called on the IO
+//    thread and private members from the UI thread.
 class ResourcePrefetchPredictorObserver {
  public:
   explicit ResourcePrefetchPredictorObserver(
@@ -33,12 +37,40 @@ class ResourcePrefetchPredictorObserver {
   // Parts of the ResourceDispatcherHostDelegate that we want to observe.
   void OnRequestStarted(net::URLRequest* request,
                         content::ResourceType resource_type,
-                        int child_id,
-                        int frame_id);
-  void OnRequestRedirected(const GURL& redirect_url, net::URLRequest* request);
-  void OnResponseStarted(net::URLRequest* request);
+                        const content::ResourceRequestInfo::WebContentsGetter&
+                            web_contents_getter);
+  void OnRequestRedirected(
+      net::URLRequest* request,
+      const GURL& redirect_url,
+      const content::ResourceRequestInfo::WebContentsGetter&
+          web_contents_getter);
+  void OnResponseStarted(net::URLRequest* request,
+                         const content::ResourceRequestInfo::WebContentsGetter&
+                             web_contents_getter);
 
  private:
+  void OnRequestStartedOnUIThread(
+      std::unique_ptr<predictors::ResourcePrefetchPredictor::URLRequestSummary>
+          summary,
+      const content::ResourceRequestInfo::WebContentsGetter&
+          web_contents_getter,
+      const GURL& main_frame_url,
+      const base::TimeTicks& creation_time) const;
+  void OnRequestRedirectedOnUIThread(
+      std::unique_ptr<predictors::ResourcePrefetchPredictor::URLRequestSummary>
+          summary,
+      const content::ResourceRequestInfo::WebContentsGetter&
+          web_contents_getter,
+      const GURL& main_frame_url,
+      const base::TimeTicks& creation_time) const;
+  void OnResponseStartedOnUIThread(
+      std::unique_ptr<predictors::ResourcePrefetchPredictor::URLRequestSummary>
+          summary,
+      const content::ResourceRequestInfo::WebContentsGetter&
+          web_contents_getter,
+      const GURL& main_frame_url,
+      const base::TimeTicks& creation_time) const;
+
   // Owned by profile.
   base::WeakPtr<predictors::ResourcePrefetchPredictor> predictor_;
 

chrome/browser/predictors/resource_prefetch_common.cc

@@ -81,13 +81,6 @@ NavigationID::NavigationID()
       render_frame_id(-1) {
 }
 
-NavigationID::NavigationID(int render_process_id,
-                           int render_frame_id,
-                           const GURL& main_frame_url)
-    : render_process_id(render_process_id),
-      render_frame_id(render_frame_id),
-      main_frame_url(main_frame_url) {}
-
 NavigationID::NavigationID(const NavigationID& other)
     : render_process_id(other.render_process_id),
       render_frame_id(other.render_frame_id),
@@ -101,6 +94,14 @@ NavigationID::NavigationID(content::WebContents* web_contents)
       main_frame_url(web_contents->GetURL()) {
 }
 
+NavigationID::NavigationID(content::WebContents* web_contents,
+                           const GURL& main_frame_url,
+                           const base::TimeTicks& creation_time)
+    : render_process_id(web_contents->GetRenderProcessHost()->GetID()),
+      render_frame_id(web_contents->GetMainFrame()->GetRoutingID()),
+      main_frame_url(main_frame_url),
+      creation_time(creation_time) {}
+
 bool NavigationID::is_valid() const {
   return render_process_id != -1 && render_frame_id != -1 &&
       !main_frame_url.is_empty();

chrome/browser/predictors/resource_prefetch_common.h

@@ -38,11 +38,11 @@ enum class PrefetchOrigin { NAVIGATION, EXTERNAL };
 // Represents a single navigation for a render frame.
 struct NavigationID {
   NavigationID();
-  NavigationID(int render_process_id,
-               int render_frame_id,
-               const GURL& main_frame_url);
-  NavigationID(const NavigationID& other);
   explicit NavigationID(content::WebContents* web_contents);
+  NavigationID(content::WebContents* web_contents,
+               const GURL& main_frame_url,
+               const base::TimeTicks& creation_time);
+  NavigationID(const NavigationID& other);
   bool operator<(const NavigationID& rhs) const;
   bool operator==(const NavigationID& rhs) const;
 

chrome/browser/predictors/resource_prefetch_predictor.cc

@@ -325,20 +325,14 @@ ResourcePrefetchPredictor::URLRequestSummary::~URLRequestSummary() {
 bool ResourcePrefetchPredictor::URLRequestSummary::SummarizeResponse(
     const net::URLRequest& request,
     URLRequestSummary* summary) {
-  const content::ResourceRequestInfo* info =
+  const content::ResourceRequestInfo* request_info =
       content::ResourceRequestInfo::ForRequest(&request);
-  if (!info)
-    return false;
-
-  int render_process_id, render_frame_id;
-  if (!info->GetAssociatedRenderFrame(&render_process_id, &render_frame_id))
+  if (!request_info)
     return false;
 
-  summary->navigation_id = NavigationID(render_process_id, render_frame_id,
-                                        request.first_party_for_cookies());
-  summary->navigation_id.creation_time = request.creation_time();
   summary->resource_url = request.original_url();
-  content::ResourceType resource_type_from_request = info->GetResourceType();
+  content::ResourceType resource_type_from_request =
+      request_info->GetResourceType();
   summary->priority = request.priority();
   request.GetMimeType(&summary->mime_type);
   summary->was_cached = request.was_cached();

chrome/browser/predictors/resource_prefetch_predictor.h

@@ -96,7 +96,8 @@ class ResourcePrefetchPredictor
     bool always_revalidate;
 
     // Initializes a |URLRequestSummary| from a |URLRequest| response.
-    // Returns true for success.
+    // Returns true for success. Note: NavigationID is NOT initialized
+    // by this function.
     static bool SummarizeResponse(const net::URLRequest& request,
                                   URLRequestSummary* summary);
   };

chrome/browser/predictors/resource_prefetch_predictor_test_util.cc

@@ -63,7 +63,10 @@ RedirectData CreateRedirectData(const std::string& primary_key,
 NavigationID CreateNavigationID(int process_id,
                                 int render_frame_id,
                                 const std::string& main_frame_url) {
-  NavigationID navigation_id(process_id, render_frame_id, GURL(main_frame_url));
+  NavigationID navigation_id;
+  navigation_id.render_process_id = process_id;
+  navigation_id.render_frame_id = render_frame_id;
+  navigation_id.main_frame_url = GURL(main_frame_url);
   navigation_id.creation_time = base::TimeTicks::Now();
   return navigation_id;
 }

chrome/browser/predictors/resource_prefetch_predictor_unittest.cc

@@ -1324,14 +1324,16 @@ TEST_F(ResourcePrefetchPredictorTest, SummarizeResponse) {
       url, net::MEDIUM, content::RESOURCE_TYPE_IMAGE, 1, 1, true);
   URLRequestSummary summary;
   EXPECT_TRUE(URLRequestSummary::SummarizeResponse(*request, &summary));
-  EXPECT_EQ(1, summary.navigation_id.render_process_id);
-  EXPECT_EQ(1, summary.navigation_id.render_frame_id);
-  EXPECT_EQ(url, summary.navigation_id.main_frame_url);
   EXPECT_EQ(url, summary.resource_url);
   EXPECT_EQ(content::RESOURCE_TYPE_IMAGE, summary.resource_type);
   EXPECT_TRUE(summary.was_cached);
   EXPECT_FALSE(summary.has_validators);
   EXPECT_FALSE(summary.always_revalidate);
+
+  // Navigation_id elements should be unset by default.
+  EXPECT_EQ(-1, summary.navigation_id.render_process_id);
+  EXPECT_EQ(-1, summary.navigation_id.render_frame_id);
+  EXPECT_EQ(GURL(), summary.navigation_id.main_frame_url);
 }
 
 TEST_F(ResourcePrefetchPredictorTest, SummarizeResponseContentType) {

chrome/browser/predictors/resource_prefetcher_unittest.cc

@@ -7,11 +7,13 @@
 #include <stddef.h>
 
 #include <memory>
+#include <string>
 #include <utility>
 
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
 #include "base/message_loop/message_loop.h"
+#include "chrome/browser/predictors/resource_prefetch_predictor_test_util.h"
 #include "chrome/browser/predictors/resource_prefetcher_manager.h"
 #include "chrome/test/base/testing_profile.h"
 #include "content/public/test/test_browser_thread.h"
@@ -164,7 +166,7 @@ TEST_F(ResourcePrefetcherTest, TestPrefetcherFinishes) {
                             GURL("http://yahoo.com/resource4.png"),
                             GURL("http://yahoo.com/resource5.png")};
 
-  NavigationID navigation_id(1, 2, main_frame_url);
+  NavigationID navigation_id = CreateNavigationID(1, 2, main_frame_url.spec());
 
   prefetcher_.reset(new TestResourcePrefetcher(&prefetcher_delegate_, config_,
                                                main_frame_url, urls));
@@ -236,7 +238,7 @@ TEST_F(ResourcePrefetcherTest, TestPrefetcherStopped) {
                             GURL("http://yahoo.com/resource3.png"),
                             GURL("http://m.google.com/resource1.jpg")};
 
-  NavigationID navigation_id(1, 2, main_frame_url);
+  NavigationID navigation_id = CreateNavigationID(1, 2, main_frame_url.spec());
 
   prefetcher_.reset(new TestResourcePrefetcher(&prefetcher_delegate_, config_,
                                                main_frame_url, urls));