[PageLoadMetrics] Reenable AdsMetrics and handle case where navigation aborts

chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc

 // Copyright 2017 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.
 
 #include "chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.h"
 
 #include <string>
 #include <utility>
 
 #include "base/feature_list.h"
 #include "base/logging.h"
 #include "base/memory/ptr_util.h"
 #include "base/strings/string_util.h"
 #include "chrome/browser/page_load_metrics/page_load_metrics_util.h"
 #include "content/public/browser/navigation_handle.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/web_contents.h"
 #include "url/gurl.h"
 
 namespace {
 
-const base::Feature kAdsFeature{"AdsMetrics",
-                                base::FEATURE_DISABLED_BY_DEFAULT};
+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(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();
-
+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",
                            base::CompareCase::SENSITIVE));
 }
 
 }  // namespace
 
 AdsPageLoadMetricsObserver::AdFrameData::AdFrameData(
     FrameTreeNodeId frame_tree_node_id)
@@ skipping 21 matching lines @@
   ad_frames_data_[navigation_handle->GetFrameTreeNodeId()] = nullptr;
   ProcessOngoingNavigationResource(navigation_handle->GetFrameTreeNodeId());
   return CONTINUE_OBSERVING;
 }
 
 page_load_metrics::PageLoadMetricsObserver::ObservePolicy
 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();
 
   const auto& id_and_data = ad_frames_data_.find(frame_tree_node_id);
   if (id_and_data != ad_frames_data_.end()) {
     // An existing subframe is navigating again.
     if (id_and_data->second) {
       // The subframe was an ad to begin with, keep tracking it as an ad.
       ProcessOngoingNavigationResource(frame_tree_node_id);
 
       if (frame_tree_node_id == id_and_data->second->frame_tree_node_id) {
         // 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;
     }
     // This frame was previously not an ad, process it as usual. If it had
     // 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;
   }
 
   // Determine who the parent frame's ad ancestor is.
   const auto& parent_id_and_data =
       ad_frames_data_.find(parent_frame_host->GetFrameTreeNodeId());
   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();
   }
 
   ad_frames_data_[frame_tree_node_id] = ad_data;
 
   if (top_level_subframe && ad_data)
     top_level_ad_frame_count_ += 1;
 
@@ skipping 150 matching lines @@
 void AdsPageLoadMetricsObserver::ProcessOngoingNavigationResource(
     FrameTreeNodeId frame_tree_node_id) {
   const auto& frame_id_and_request =
       ongoing_navigation_resources_.find(frame_tree_node_id);
   if (frame_id_and_request == ongoing_navigation_resources_.end())
     return;
 
   ProcessLoadedResource(frame_id_and_request->second);
   ongoing_navigation_resources_.erase(frame_id_and_request);
 }