[PageLoadMetrics] Keep track of Ad Sizes on Pages

content/public/browser/resource_request_info.h

Index: content/public/browser/resource_request_info.h
diff --git a/content/public/browser/resource_request_info.h b/content/public/browser/resource_request_info.h
index 7f0d825d7f0060f4d70993db026c32f665f8a792..9d718a3c32f3834909b6c99395e17e70f21ca955 100644
--- a/content/public/browser/resource_request_info.h
+++ b/content/public/browser/resource_request_info.h
@@ -20,6 +20,7 @@ class URLRequest;
 
 namespace content {
 class NavigationUIData;
+class RenderFrameHost;
 class ResourceContext;
 class WebContents;
 
@@ -73,6 +74,14 @@ class ResourceRequestInfo {
   // non-null.
   using WebContentsGetter = base::Callback<WebContents*(void)>;
 
+  // A callback that returns a pointer to a RenderFrameHost. The callback can
+  // always be used, but it may return nullptr: if the info used to
+  // instantiate the callback can no longer be used to return a
+  // RenderFrameHost, nullptr will be returned instead.
+  // The callback should only run on the UI thread and it should always be
+  // non-null.
+  using RenderFrameHostGetter = base::Callback<RenderFrameHost*(void)>;
+
   // Returns a callback that returns a pointer to the WebContents this request
   // is associated with, or nullptr if it no longer exists or the request is
   // not associated with a WebContents. The callback should only run on the UI
@@ -81,6 +90,14 @@ class ResourceRequestInfo {
   // example, requests made by a ServiceWorker.
   virtual WebContentsGetter GetWebContentsGetterForRequest() const = 0;
 
+  // Returns a callback that returns a pointer to the RenderFrameHost this
+  // request is associated with, or nullptry if it no longer exists or the
+  // request is not associated with a RenderFrameHost. This callback should
+  // only be run on the UI thread.
+  // Note: Not all resource requests will be owned by a RenderFrameHost. For
+  // example, requests made by a ServiceWorker.

[clamy, 2017/04/14 13:56:09]

Looking at the implementation, this comment is simply not true for navigation
requests with PlzNavigate enabled. The request is simply not associated with the
current RenderFrameHost. It may be a browser-initiated cross-process navigation,
in which case the current RFH will _never_ hear about it.

Thus, I am opposed to this RenderFrameHostGetter returning anything but nullptr
for navigation requests when PlzNavigate is enabled. Navigation requests _are
not_ associated with a RFH, and we should not pretend otherwise.

We can discuss other ways to address the issues you're facing, but this
direction seems wrong.

[Charlie Harrison, 2017/04/14 14:02:25]

I think returning nullptr here for navigations is reasonable, provided that we
can properly get the frame tree node ids unconditionally. That is the only thing
we are pulling out of the RFH currently.

Correct me if I'm wrong jkarlin

[jkarlin, 2017/04/14 17:50:20]

I spoke with clamy@ and csharrison@. We agreed on a frame tree node id getter
instead of a render frame host getter. Done.

+  virtual RenderFrameHostGetter GetRenderFrameHostGetterForRequest() const = 0;
+
   // Returns the associated ResourceContext.
   virtual ResourceContext* GetContext() const = 0;
 
@@ -103,6 +120,8 @@ class ResourceRequestInfo {
 
   // Returns the FrameTreeNode ID for this frame. This ID is browser-global and
   // uniquely identifies a frame that hosts content.
+  // Note: Returns -1 for all requests except PlzNavigate requests.
+  // TODO(jkarlin): Set the id for all requests from frames.
   virtual int GetFrameTreeNodeId() const = 0;
 
   // The IPC route identifier of the RenderFrame.