[PageLoadMetrics] Keep track of Ad Sizes on Pages

content/public/browser/resource_request_info.h

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef CONTENT_PUBLIC_BROWSER_RESOURCE_REQUEST_INFO_H_
 #define CONTENT_PUBLIC_BROWSER_RESOURCE_REQUEST_INFO_H_
 
 #include "base/callback_forward.h"
 #include "content/common/content_export.h"
 #include "content/public/browser/global_request_id.h"
 #include "content/public/common/previews_state.h"
 #include "content/public/common/resource_type.h"
 #include "third_party/WebKit/public/platform/WebPageVisibilityState.h"
 #include "third_party/WebKit/public/platform/WebReferrerPolicy.h"
 #include "ui/base/page_transition_types.h"
 
 namespace net {
 class URLRequest;
 }
 
 namespace content {
 class NavigationUIData;
+class RenderFrameHost;
 class ResourceContext;
 class WebContents;
 
 // Each URLRequest allocated by the ResourceDispatcherHost has a
 // ResourceRequestInfo instance associated with it.
 class ResourceRequestInfo {
  public:
   // Returns the ResourceRequestInfo associated with the given URLRequest.
   CONTENT_EXPORT static const ResourceRequestInfo* ForRequest(
       const net::URLRequest* request);
@@ skipping 33 matching lines @@
       const net::URLRequest* request);
 
   // A callback that returns a pointer to a WebContents. 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 WebContents,
   // nullptr will be returned instead.
   // The callback should only run on the UI thread and it should always be
   // 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
   // thread.
   // Note: Not all resource requests will be owned by a WebContents. For
   // 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;
 
   // The child process unique ID of the requestor.
   // To get a WebContents, use GetWebContentsGetterForRequest instead.
   virtual int GetChildID() const = 0;
 
   // The IPC route identifier for this request (this identifies the RenderView
   // or like-thing in the renderer that the request gets routed to).
   // To get a WebContents, use GetWebContentsGetterForRequest instead.
   // Don't use this method for new code, as RenderViews are going away.
   virtual int GetRouteID() const = 0;
 
   // The globally unique identifier for this request.
   virtual GlobalRequestID GetGlobalRequestID() const = 0;
 
   // The pid of the originating process, if the request is sent on behalf of a
   // another process.  Otherwise it is 0.
   virtual int GetOriginPID() const = 0;
 
   // 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.
   // To get a WebContents, use GetWebContentsGetterForRequest instead.
   // TODO(jam): once all navigation and resource requests are sent between
   // frames and RenderView/RenderViewHost aren't involved we can remove this and
   // just use GetRouteID above.
   virtual int GetRenderFrameID() const = 0;
 
   // True if GetRenderFrameID() represents a main frame in the RenderView.
@@ skipping 51 matching lines @@
   // UI thread at the beginning of navigation.
   virtual NavigationUIData* GetNavigationUIData() const = 0;
 
  protected:
   virtual ~ResourceRequestInfo() {}
 };
 
 }  // namespace content
 
 #endif  // CONTENT_PUBLIC_BROWSER_RESOURCE_REQUEST_INFO_H_