Accessing navigation information via webcontents

chrome/browser/net/resource_prefetch_predictor_observer.cc

Index: chrome/browser/net/resource_prefetch_predictor_observer.cc
diff --git a/chrome/browser/net/resource_prefetch_predictor_observer.cc b/chrome/browser/net/resource_prefetch_predictor_observer.cc
index 9a7db58f1f42590bfdd7cf78436b3e54969188f8..c849d4d9b47c25fe147731affa2b0be6b441bff4 100644
--- a/chrome/browser/net/resource_prefetch_predictor_observer.cc
+++ b/chrome/browser/net/resource_prefetch_predictor_observer.cc
@@ -4,12 +4,16 @@
 
 #include "chrome/browser/net/resource_prefetch_predictor_observer.h"
 
+#include <memory>

[ahemery, 2016/12/05 09:38:44]

for unique_ptr

 #include <string>
 
+#include "base/memory/ptr_util.h"

[ahemery, 2016/12/05 09:38:45]

For MakeUnique and base::Passed

 #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/render_process_host.h"
 #include "content/public/browser/resource_request_info.h"
+#include "content/public/browser/web_contents.h"
 #include "net/url_request/url_request.h"
 #include "url/gurl.h"
 
@@ -69,8 +73,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 +83,36 @@ 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>();

[ahemery, 2016/12/05 09:38:44]

Now using a pointer as we are passing it to a different thread

+  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;
 
   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));
 
   if (resource_type == content::RESOURCE_TYPE_MAIN_FRAME)
     ReportMainFrameRequestStats(MAIN_FRAME_REQUEST_STATS_PROCESSED_REQUESTS);
 }

[ahemery, 2016/12/06 14:31:11]

Private methods moved at the end of the file

[alexilin, 2016/12/06 15:51:54]

Thanks!
Could you reply directly under initial comment pressing "reply" button next
time? Then it would be clearer that comment was answered.

 
+void ResourcePrefetchPredictorObserver::OnRequestStartedOnUIThread(

[alexilin, 2016/12/05 13:20:00]

nit: Could you put all definitions of private functions to the end of file?
Function declaration order should match function definition order:
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

+    std::unique_ptr<URLRequestSummary> summary,
+    const content::ResourceRequestInfo::WebContentsGetter&
+        web_contents_getter) const {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  RetrieveNavigationID(summary->navigation_id, web_contents_getter);
+  predictor_->RecordURLRequest(*summary);
+}
+
 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 +125,21 @@ void ResourcePrefetchPredictorObserver::OnRequestRedirected(
   if (!ResourcePrefetchPredictor::ShouldRecordRedirect(request))
     return;
 
-  ResourcePrefetchPredictor::URLRequestSummary summary;
+  auto summary = base::MakeUnique<URLRequestSummary>();
+  summary->navigation_id.main_frame_url = request->first_party_for_cookies();

[ahemery, 2016/12/05 09:38:45]

main_frame_url and creation_time is not done in SummarizeResponse anymore.

+  summary->navigation_id.creation_time = request->creation_time();
+  summary->redirect_url = redirect_url;
   if (!ResourcePrefetchPredictor::URLRequestSummary::SummarizeResponse(
-          *request, &summary)) {
+          *request, summary.get())) {
     return;
   }
 
-  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));
 
   if (request_info &&
       request_info->GetResourceType() == content::RESOURCE_TYPE_MAIN_FRAME) {
@@ -134,8 +147,19 @@ void ResourcePrefetchPredictorObserver::OnRequestRedirected(
   }
 }
 
+void ResourcePrefetchPredictorObserver::OnRequestRedirectedOnUIThread(
+    std::unique_ptr<URLRequestSummary> summary,
+    const content::ResourceRequestInfo::WebContentsGetter& web_contents_getter)
+    const {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  RetrieveNavigationID(summary->navigation_id, web_contents_getter);
+  predictor_->RecordURLRedirect(*summary);
+}
+
 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 +173,20 @@ void ResourcePrefetchPredictorObserver::OnResponseStarted(
 
   if (!ResourcePrefetchPredictor::ShouldRecordResponse(request))
     return;
-  ResourcePrefetchPredictor::URLRequestSummary summary;
+  auto summary = base::MakeUnique<URLRequestSummary>();
+  summary->navigation_id.main_frame_url = request->first_party_for_cookies();
+  summary->navigation_id.creation_time = request->creation_time();
   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));
 
   ReportRequestStats(REQUEST_STATS_TOTAL_PROCESSED_RESPONSES);
   if (request_info &&
@@ -169,4 +195,26 @@ void ResourcePrefetchPredictorObserver::OnResponseStarted(
   }
 }
 
+void ResourcePrefetchPredictorObserver::OnResponseStartedOnUIThread(
+    std::unique_ptr<URLRequestSummary> summary,
+    const content::ResourceRequestInfo::WebContentsGetter&
+        web_contents_getter) const {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  RetrieveNavigationID(summary->navigation_id, web_contents_getter);
+  predictor_->RecordURLResponse(*summary);
+}
+
+void ResourcePrefetchPredictorObserver::RetrieveNavigationID(

[Benoit L, 2016/12/05 13:25:56]

Actually, considering the remark below, this function should return a boolean,
and be named perhaps "MaybeSetNavigationIDFromWebContents()" or something along
these lines.

+    predictors::NavigationID& navigation_id,

[alexilin, 2016/12/05 13:20:00]

Pass an object by pointer if you want to change its state. All parameters passed
by reference must be labeled const:
https://google.github.io/styleguide/cppguide.html#Reference_Arguments

[Benoit L, 2016/12/05 13:25:56]

We don't use non-const references in Chrome.
See https://google.github.io/styleguide/cppguide.html#Reference_Arguments

The alternative is to use a pointer.

+    const content::ResourceRequestInfo::WebContentsGetter&
+        web_contents_getter) const {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  content::WebContents* web_contents = web_contents_getter.Run();
+  DCHECK(web_contents);

[alexilin, 2016/12/05 13:20:00]

Based on comment to GetWebContentsGetterForRequest() function the callback
WebContentsGetter can return nullptr so it shouldn't be DCHECK.
I suggest to make return type bool for this function and return false if
web_contents_getter returned false. Outside callers also should check return
value of this function.

[Benoit L, 2016/12/05 13:25:56]

A DCHECK() is not correct here, as the returned WebContents can be nullptr.
Early return with a comment is better here.


There are several reasons for which the returned WebContents can be nullptr:
- the WebContents has been destroyed in the meantime
- the request was never associated with a WebContents (see the comment in
ResourceRequestInfoImpl::GetWebContentsGetterForRequest())

+  navigation_id.render_process_id =

[alexilin, 2016/12/05 13:20:00]

nit:
I'm wondering if we can eliminate the mess with NavigationID constructors.
For example, we could use 
  explicit NavigationID(content::WebContents* web_contents);
here and delete 
  NavigationID(int render_process_id,
               int render_frame_id,
               const GURL& main_frame_url);
at all.

[Benoit L, 2016/12/05 13:25:56]

The partial initialization of NavigationID here is a bit too weird. Can you
fully initialize it in one place?

+      web_contents->GetRenderProcessHost()->GetID();
+  navigation_id.render_frame_id =
+      web_contents->GetMainFrame()->GetRoutingID();
+}
+
 }  // namespace chrome_browser_net