[PageLoadMetrics] Keep track of Ad Sizes on Pages

chrome/browser/page_load_metrics/metrics_web_contents_observer.cc

Index: chrome/browser/page_load_metrics/metrics_web_contents_observer.cc
diff --git a/chrome/browser/page_load_metrics/metrics_web_contents_observer.cc b/chrome/browser/page_load_metrics/metrics_web_contents_observer.cc
index e3f51bf4946a1b2cc9437a0e973f4b77a78e5325..7f85c903103f5ab4d9344941ffcfc9aa7f52c632 100644
--- a/chrome/browser/page_load_metrics/metrics_web_contents_observer.cc
+++ b/chrome/browser/page_load_metrics/metrics_web_contents_observer.cc
@@ -245,6 +245,8 @@ PageLoadTracker* MetricsWebContentsObserver::GetTrackerOrNullForRequest(
 }
 
 void MetricsWebContentsObserver::OnRequestComplete(
+    const GURL& url,
+    int frame_tree_node_id,
     const content::GlobalRequestID& request_id,
     content::ResourceType resource_type,
     bool was_cached,
@@ -256,8 +258,8 @@ void MetricsWebContentsObserver::OnRequestComplete(
       GetTrackerOrNullForRequest(request_id, resource_type, creation_time);
   if (tracker) {
     ExtraRequestInfo extra_request_info(
-        was_cached, raw_body_bytes, used_data_reduction_proxy,
-        was_cached ? 0 : original_content_length);
+        url, frame_tree_node_id, was_cached, raw_body_bytes,
+        used_data_reduction_proxy, was_cached ? 0 : original_content_length);
     tracker->OnLoadedResource(extra_request_info);
   }
 }
@@ -270,8 +272,12 @@ MetricsWebContentsObserver::GetPageLoadExtraInfoForCommittedLoad() {
 
 void MetricsWebContentsObserver::DidFinishNavigation(
     content::NavigationHandle* navigation_handle) {
-  if (!navigation_handle->IsInMainFrame())
+  if (!navigation_handle->IsInMainFrame()) {
+    if (committed_load_ && navigation_handle->HasCommitted() &&
+        !navigation_handle->IsSameDocument())
+      committed_load_->DidCommitSubFrame(navigation_handle);

[Charlie Harrison, 2017/04/13 19:09:57]

Do we really want to intentially send this for error pages?

[Bryan McQuade, 2017/04/13 19:21:27]

Good question. I do think we want the downstream client to be able to know that
the page they thought was committed in a frame is no longer committed. Telling
about every commit, including error pages, seems like one way to accomplish
that, at the cost that clients also have to think about error pages.

Maybe the right thing for now is to keep it simple and inform about all commits,
but add comments. If more observers start using this API we may get a better
sense for what their needs/goals are and can refine & refactor to address those
needs well. How does that sound?

[Charlie Harrison, 2017/04/13 19:26:36]

I dunno. I feel like making observers do all the legwork is an easy way for bugs
to leak in. OTOH specifying a policy leads to churn later on. I'm OK with either
choice.

[jkarlin, 2017/04/24 17:27:58]

I've changed it to not send for error pages as I'm guessing (of course that's
going to be wrong) that most observers (including ads) won't want them and we
can change it later if they do. 

     return;
+  }
 
   std::unique_ptr<PageLoadTracker> finished_nav(
       std::move(provisional_loads_[navigation_handle]));