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

chrome/browser/chrome_content_browser_client.cc

 // Copyright (c) 2012 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/chrome_content_browser_client.h"
 
 #include <map>
 #include <set>
 #include <utility>
 #include <vector>
 
 #include "base/base_switches.h"
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/command_line.h"
 #include "base/files/scoped_file.h"
 #include "base/i18n/character_encoding.h"
 #include "base/json/json_reader.h"
 #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"
 #include "base/strings/string_split.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/threading/sequenced_worker_pool.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "build/build_config.h"
@@ skipping 18 matching lines @@
 #include "chrome/browser/media/router/presentation_service_delegate_impl.h"
 #include "chrome/browser/media/router/receiver_presentation_service_delegate_impl.h"
 #include "chrome/browser/media/webrtc/media_capture_devices_dispatcher.h"
 #include "chrome/browser/memory/chrome_memory_coordinator_delegate.h"
 #include "chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.h"
 #include "chrome/browser/nacl_host/nacl_browser_delegate_impl.h"
 #include "chrome/browser/net_benchmarking.h"
 #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"
 #include "chrome/browser/prefs/active_profile_pref_service.h"
 #include "chrome/browser/prerender/prerender_final_status.h"
 #include "chrome/browser/prerender/prerender_manager.h"
 #include "chrome/browser/prerender/prerender_manager_factory.h"
 #include "chrome/browser/prerender/prerender_message_filter.h"
 #include "chrome/browser/printing/printing_message_filter.h"
 #include "chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.h"
@@ skipping 1261 matching lines @@
     *is_renderer_initiated = false;
     *referrer = content::Referrer();
   }
 
 #if BUILDFLAG(ENABLE_EXTENSIONS)
   ChromeContentBrowserClientExtensionsPart::OverrideNavigationParams(
       site_instance, transition, is_renderer_initiated, referrer);
 #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
+  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;
-#endif
 }
 
 bool ChromeContentBrowserClient::IsSuitableHost(
     content::RenderProcessHost* process_host,
     const GURL& site_url) {
   Profile* profile =
       Profile::FromBrowserContext(process_host->GetBrowserContext());
   // This may be NULL during tests. In that case, just assume any site can
   // share any host.
   if (!profile)
@@ skipping 2105 matching lines @@
   // displayed URL when rewriting chrome://help to chrome://settings/help.
   return url->SchemeIs(content::kChromeUIScheme) &&
          url->host() == chrome::kChromeUISettingsHost;
 }
 
 // static
 void ChromeContentBrowserClient::SetDefaultQuotaSettingsForTesting(
     const storage::QuotaSettings* settings) {
   g_default_quota_settings = settings;
 }