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

chrome/browser/chrome_content_browser_client.cc

Index: chrome/browser/chrome_content_browser_client.cc
diff --git a/chrome/browser/chrome_content_browser_client.cc b/chrome/browser/chrome_content_browser_client.cc
index 6461d76826efa245ffa8394dd215028ea1f5938e..d7221ad034a948d4f8667087c231bc284d236d51 100644
--- a/chrome/browser/chrome_content_browser_client.cc
+++ b/chrome/browser/chrome_content_browser_client.cc
@@ -19,6 +19,7 @@
 #include "base/lazy_instance.h"
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
+#include "base/metrics/field_trial_params.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/path_service.h"
 #include "base/strings/string_number_conversions.h"
@@ -57,6 +58,7 @@
 #include "chrome/browser/notifications/platform_notification_service_impl.h"
 #include "chrome/browser/page_load_metrics/experiments/delay_navigation_throttle.h"
 #include "chrome/browser/page_load_metrics/metrics_navigation_throttle.h"
+#include "chrome/browser/page_load_metrics/observers/ads_detection.h"
 #include "chrome/browser/password_manager/chrome_password_manager_client.h"
 #include "chrome/browser/permissions/permission_context_base.h"
 #include "chrome/browser/platform_util.h"
@@ -1338,20 +1340,36 @@ void ChromeContentBrowserClient::OverrideNavigationParams(
 #endif
 }
 
-bool ChromeContentBrowserClient::
-    ShouldFrameShareParentSiteInstanceDespiteTopDocumentIsolation(
-        const GURL& url,
-        content::SiteInstance* parent_site_instance) {
-  if (IsNTPSiteInstance(parent_site_instance))
-    return true;
+bool ChromeContentBrowserClient::ShouldIsolateFrameFromMainContent(
+    content::RenderFrameHost* frame,
+    const GURL& dest_url,
+    content::SiteInstance* main_frame_site_instance) {
+  if (IsNTPSiteInstance(main_frame_site_instance))
+    return false;
 
 #if BUILDFLAG(ENABLE_EXTENSIONS)
-  return ChromeContentBrowserClientExtensionsPart::
-      ShouldFrameShareParentSiteInstanceDespiteTopDocumentIsolation(
-          url, parent_site_instance);
-#else
-  return false;
+  if (ChromeContentBrowserClientExtensionsPart::
+          IsMainFrameSiteInstanceExcludedFromTopDocumentIsolation(
+              main_frame_site_instance))
+    return false;
 #endif
+
+  features::TopDocumentIsolationMode tdiMode =
+      static_cast<features::TopDocumentIsolationMode>(
+          base::GetFieldTrialParamByFeatureAsInt(
+              ::features::kTopDocumentIsolation,
+              ::features::kTopDocumentIsolationModeParam,
+              static_cast<int>(features::TopDocumentIsolationMode::Default)));
+  switch (tdiMode) {
+    case features::TopDocumentIsolationMode::Default:

[Charlie Reis, 2017/06/30 23:28:45]

I thought "Default" meant disabled?  Shouldn't we return false here?

[Łukasz Anforowicz, 2017/07/01 00:10:53]

TDI state has 2 parts:
part1: is the feature enabled / disabled / in the default enabled-or-disabled
state
part2: what mode do we use for selecting frames that go into the tdi process:
xsite / ads / default (= ads).

In the CL description I've listed 5 states that TDI exposes in chrome://flags
after this CL.  Let me repeat these 5 state below, annotating them with part1
and part2 values.

1. "Default" (part1 = disabled, part2 = n/a)
2. "Enabled" (part1 = enabled, part2 = default)
3. "Enabled (Xsite - ...)" (part1 = enabled, part2 = xsite)
4. "Enabled (Ads - ...)" (part1 = enabled, part2 = ads)
5. Disabled (part1 = disabled, part2 = n/a)

The "Default" in the code above comes from TopDocumentIsolation*Mode* enum and
corresponds to part2 - it is the "default mode" (only consulted when TDI is
enabled), but not the "default disabled-vs-enabled state".


Does the explanation above help?  Is there something I should do in the code to
make it easier to understand?

[Charlie Reis, 2017/07/06 20:02:29]

Thanks, I understand now.  Yes, we should try to make it easier for others to
understand at first glance.  See my later suggestion about renaming one of the
"Default" labels, and if that doesn't work out, maybe we can include some of
your reply here in the declaration of the enum.

+    case features::TopDocumentIsolationMode::Ads:
+      return IsAdFrame(frame->GetFrameName(), dest_url);
+
+    case features::TopDocumentIsolationMode::Xsite:
+      return true;
+  }
+  NOTREACHED();
+  return false;
 }
 
 bool ChromeContentBrowserClient::IsSuitableHost(