[PageLoadMetrics] Reenable AdsMetrics and handle case where navigation aborts

chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc

Index: chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc
diff --git a/chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc b/chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc
index f6a08a182e620ff14f156528597e3cf507d7550c..01fb0749d0d0c48b212c79301ee6fc9158160bcf 100644
--- a/chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc
+++ b/chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc
@@ -19,25 +19,18 @@
 
 namespace {
 
-const base::Feature kAdsFeature{"AdsMetrics",
-                                base::FEATURE_DISABLED_BY_DEFAULT};
-
-bool FrameIsAd(content::NavigationHandle* navigation_handle) {
-  content::RenderFrameHost* current_frame_host =
-      navigation_handle->GetRenderFrameHost();
-  DCHECK(current_frame_host);
-  const std::string& name = current_frame_host->GetFrameName();
-  const GURL& url = navigation_handle->GetURL();
+const base::Feature kAdsFeature{"AdsMetrics", base::FEATURE_ENABLED_BY_DEFAULT};

[Charlie Harrison, 2017/05/04 20:21:14]

Please mention in the description that this CL re-enables the feature.

[jkarlin, 2017/05/05 19:56:28]

Done.

 
+bool FrameIsAd(const GURL& url, const std::string& frame_name) {
   // Because sub-resource filtering isn't always enabled, and doesn't work
   // well in monitoring mode (no CSS enforcement), it's difficult to identify
   // ads. Google ads are prevalent and easy to track, so we'll start by
   // tracking those. Note that the frame name can be very large, so be careful
   // to avoid full string searches if possible.
   // TODO(jkarlin): Track other ad networks that are easy to identify.
-  return base::StartsWith(name, "google_ads_iframe",
+  return base::StartsWith(frame_name, "google_ads_iframe",
                           base::CompareCase::SENSITIVE) ||
-         base::StartsWith(name, "google_ads_frame",
+         base::StartsWith(frame_name, "google_ads_frame",
                           base::CompareCase::SENSITIVE) ||
          (url.host_piece() == "tpc.googlesyndication.com" &&
           base::StartsWith(url.path_piece(), "/safeframe",
@@ -79,17 +72,29 @@ AdsPageLoadMetricsObserver::OnDidFinishSubFrameNavigation(
     content::NavigationHandle* navigation_handle) {
   FrameTreeNodeId frame_tree_node_id = navigation_handle->GetFrameTreeNodeId();
 
-  if (!navigation_handle->HasCommitted() ||
-      navigation_handle->IsSameDocument() || navigation_handle->IsErrorPage()) {
+  if ((!navigation_handle->HasCommitted() ||
+       navigation_handle->IsSameDocument() ||
+       navigation_handle->IsErrorPage()) &&
+      navigation_handle->GetNetErrorCode() != net::ERR_ABORTED) {

[Charlie Harrison, 2017/05/04 20:21:14]

#include net errors

[jkarlin, 2017/05/05 19:56:28]

Done.

     // We're not interested in tracking this navigation. In case we've seen a

[Charlie Harrison, 2017/05/04 20:21:14]

IMO pulling this whole comment above the if statement is clearer and makes the
if statement easier to parse.

[jkarlin, 2017/05/05 19:56:28]

Done.

     // resource for the navigation before this message, clear it from
     // ongoing_navigation_resources_.
+    //
+    // Note that we *do* keep track of net::ERR_ABORTED navigations because
+    // the frame can continue to load resources (either from the previous
+    // navigation or from a document written by doc.write which aborted the
+    // navigation).

[Charlie Harrison, 2017/05/04 20:21:14]

I imagine this is not the only way the navigation can be aborted, is it?

[jkarlin, 2017/05/05 19:56:28]

Done.

     ongoing_navigation_resources_.erase(frame_tree_node_id);
     return CONTINUE_OBSERVING;
   }
 
-  content::RenderFrameHost* parent_frame_host =
-      navigation_handle->GetRenderFrameHost()->GetParent();
+  // Get the RFH from the frame tree node id because
+  // NavigationHandle->GetRenderFrameHost doesn't like being called for

[Charlie Harrison, 2017/05/04 20:21:14]

Slightly awkward wording. It's helpful to differentiate between the FTN the
navigation is happening in, and the RFH. The canonical RFH associated with a
navigation is the one it commits in. "Doesn't like" peppers over this
distinction as arbitrary.

[jkarlin, 2017/05/05 19:56:28]

Done.

+  // uncomitted navigations (e.g., due to an abort).
+  content::RenderFrameHost* current_frame_host =
+      navigation_handle->GetWebContents()->FindFrameByFrameTreeNodeId(
+          frame_tree_node_id);
+  content::RenderFrameHost* parent_frame_host = current_frame_host->GetParent();
   DCHECK(parent_frame_host);
 
   bool top_level_subframe = !parent_frame_host->GetParent();
@@ -105,7 +110,8 @@ AdsPageLoadMetricsObserver::OnDidFinishSubFrameNavigation(
         // This is the top-most frame in the ad.
         UMA_HISTOGRAM_BOOLEAN(
             "PageLoad.Clients.Ads.Google.Navigations.AdFrameRenavigatedToAd",
-            FrameIsAd(navigation_handle));
+            FrameIsAd(navigation_handle->GetURL(),
+                      current_frame_host->GetFrameName()));
       }
 
       return CONTINUE_OBSERVING;
@@ -114,7 +120,8 @@ AdsPageLoadMetricsObserver::OnDidFinishSubFrameNavigation(
     // any child frames that were ads, those will still be recorded.
     UMA_HISTOGRAM_BOOLEAN(
         "PageLoad.Clients.Ads.Google.Navigations.NonAdFrameRenavigatedToAd",
-        FrameIsAd(navigation_handle));
+        FrameIsAd(navigation_handle->GetURL(),
+                  current_frame_host->GetFrameName()));
   } else if (top_level_subframe) {
     top_level_subframe_count_ += 1;
   }
@@ -125,7 +132,8 @@ AdsPageLoadMetricsObserver::OnDidFinishSubFrameNavigation(
   DCHECK(parent_id_and_data != ad_frames_data_.end());
   AdFrameData* ad_data = parent_id_and_data->second;
 
-  if (!ad_data && FrameIsAd(navigation_handle)) {
+  if (!ad_data && FrameIsAd(navigation_handle->GetURL(),
+                            current_frame_host->GetFrameName())) {
     // This frame is not nested within an ad frame but is itself an ad.
     ad_frames_data_storage_.emplace_back(frame_tree_node_id);
     ad_data = &ad_frames_data_storage_.back();