Use FrameIsAd to decide whether to isolate a frame in TopDocumentIsolation mode.

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 710ade67ea35456e90a0d098570fc76535991d96..34b32e220905d50cde82605a8d50994dffb61e0f 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
@@ -11,6 +11,7 @@
 #include "base/logging.h"
 #include "base/memory/ptr_util.h"
 #include "base/strings/string_util.h"
+#include "chrome/browser/page_load_metrics/ads_detection.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"
@@ -23,56 +24,21 @@ const base::Feature kAdsFeature{"AdsMetrics", base::FEATURE_ENABLED_BY_DEFAULT};
 
 #define ADS_HISTOGRAM(suffix, hist_macro, ad_type, value)                  \
   switch (ad_type) {                                                       \
-    case AdsPageLoadMetricsObserver::AD_TYPE_GOOGLE:                       \
+    case AD_TYPE_GOOGLE:                                                   \
       hist_macro("PageLoad.Clients.Ads.Google." suffix, value);            \
       break;                                                               \
-    case AdsPageLoadMetricsObserver::AD_TYPE_SUBRESOURCE_FILTER:           \
+    case AD_TYPE_SUBRESOURCE_FILTER:                                       \
       hist_macro("PageLoad.Clients.Ads.SubresourceFilter." suffix, value); \
       break;                                                               \
-    case AdsPageLoadMetricsObserver::AD_TYPE_ALL:                          \
+    case AD_TYPE_ALL:                                                      \
       hist_macro("PageLoad.Clients.Ads.All." suffix, value);               \
       break;                                                               \
   }
 
-bool DetectGoogleAd(content::NavigationHandle* navigation_handle) {
-  // 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.
-
-  // In case the navigation aborted, look up the RFH by the Frame Tree Node
-  // ID. It returns the committed frame host or the initial frame host for the
-  // frame if no committed host exists. Using a previous host is fine because
-  // once a frame has an ad we always consider it to have an ad.
-  // We use the unsafe method of FindFrameByFrameTreeNodeId because we're not
-  // concerned with which process the frame lives on (we're just measuring
-  // bytes and not granting security priveleges).
-  content::RenderFrameHost* current_frame_host =
-      navigation_handle->GetWebContents()->UnsafeFindFrameByFrameTreeNodeId(
-          navigation_handle->GetFrameTreeNodeId());
-  if (current_frame_host) {
-    const std::string& frame_name = current_frame_host->GetFrameName();
-    if (base::StartsWith(frame_name, "google_ads_iframe",
-                         base::CompareCase::SENSITIVE) ||
-        base::StartsWith(frame_name, "google_ads_frame",
-                         base::CompareCase::SENSITIVE)) {
-      return true;
-    }
-  }
-
-  const GURL& url = navigation_handle->GetURL();
-  return url.host_piece() == "tpc.googlesyndication.com" &&
-         base::StartsWith(url.path_piece(), "/safeframe",
-                          base::CompareCase::SENSITIVE);
-}
-
-void RecordParentExistsForSubFrame(
-    bool parent_exists,
-    const AdsPageLoadMetricsObserver::AdTypes& ad_types) {
-  ADS_HISTOGRAM("ParentExistsForSubFrame", UMA_HISTOGRAM_BOOLEAN,
-                AdsPageLoadMetricsObserver::AD_TYPE_ALL, parent_exists);
+void RecordParentExistsForSubFrame(bool parent_exists,
+                                   const AdTypes& ad_types) {
+  ADS_HISTOGRAM("ParentExistsForSubFrame", UMA_HISTOGRAM_BOOLEAN, AD_TYPE_ALL,
+                parent_exists);
 }
 
 }  // namespace
@@ -137,7 +103,7 @@ void AdsPageLoadMetricsObserver::OnDidFinishSubFrameNavigation(
   content::RenderFrameHost* parent_frame_host =
       navigation_handle->GetParentFrame();
 
-  AdTypes ad_types = DetectAds(navigation_handle);
+  AdTypes ad_types = GetDetectedAdTypes(navigation_handle);
 
   const auto& id_and_data = ad_frames_data_.find(frame_tree_node_id);
   if (id_and_data != ad_frames_data_.end()) {
@@ -213,8 +179,7 @@ void AdsPageLoadMetricsObserver::OnSubframeNavigationEvaluated(
   // and therefore would provide bad histogram data. Note that WOULD_DISALLOW
   // is only seen in dry runs.
   if (load_policy == subresource_filter::LoadPolicy::WOULD_DISALLOW) {
-    unfinished_subresource_ad_frames_.insert(
-        navigation_handle->GetFrameTreeNodeId());
+    SetDetectedAdTypes(navigation_handle, AD_TYPE_SUBRESOURCE_FILTER);

[jkarlin, 2017/07/19 13:32:36]

Yay for this change. This is much nicer than unfinished_soubresource_ad_frames_.

   }
 }
 
@@ -222,24 +187,6 @@ void AdsPageLoadMetricsObserver::OnSubresourceFilterGoingAway() {
   subresource_observer_.RemoveAll();
 }
 
-bool AdsPageLoadMetricsObserver::DetectSubresourceFilterAd(
-    FrameTreeNodeId frame_tree_node_id) {
-  return unfinished_subresource_ad_frames_.erase(frame_tree_node_id);
-}
-
-AdsPageLoadMetricsObserver::AdTypes AdsPageLoadMetricsObserver::DetectAds(
-    content::NavigationHandle* navigation_handle) {
-  AdTypes ad_types;
-
-  if (DetectGoogleAd(navigation_handle))
-    ad_types.set(AD_TYPE_GOOGLE);
-
-  if (DetectSubresourceFilterAd(navigation_handle->GetFrameTreeNodeId()))
-    ad_types.set(AD_TYPE_SUBRESOURCE_FILTER);
-
-  return ad_types;
-}
-
 void AdsPageLoadMetricsObserver::ProcessLoadedResource(
     const page_load_metrics::ExtraRequestCompleteInfo& extra_request_info) {
   const auto& id_and_data =