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

content/public/common/content_features.h

Index: content/public/common/content_features.h
diff --git a/content/public/common/content_features.h b/content/public/common/content_features.h
index 04178037a82674bef80642d9afbc57903e7bbebe..6167855374f2c05f9e34b1226250d35297ca1504 100644
--- a/content/public/common/content_features.h
+++ b/content/public/common/content_features.h
@@ -72,7 +72,22 @@ CONTENT_EXPORT extern const base::Feature kSkipCompositingSmallScrollers;
 CONTENT_EXPORT extern const base::Feature kSlimmingPaintInvalidation;
 CONTENT_EXPORT extern const base::Feature kTimerThrottlingForHiddenFrames;
 CONTENT_EXPORT extern const base::Feature kTokenBinding;
+
 CONTENT_EXPORT extern const base::Feature kTopDocumentIsolation;
+
+CONTENT_EXPORT extern const char kTopDocumentIsolationModeParam[];
+#define FOR_EACH_TDI_MODE(V)                                                  \
+  V(Xsite, 1, "isolate all frames from sites other than the top-level frame") \

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

We don't really use the term Xsite anywhere in Chrome.  Would it be a problem to
say CrossSite (here and in CL description)?

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

Thanks for catching "Xsite".  I've changed this to "CrossSite".

I think I still want to keep the mode description above though.  I didn't want
to say that this mode isolates all cross-site frames, because this sounds very
much like --site-per-process and adding the "from top-level frame" qualifier
might not help much (i.e. if top-level is from a.com, then cross-site subframes
from b.com and c.com would both be put into the same TDI process).

WDYT?  Would you like to see a different description ("isolate all cross-site
frames from the top-level frame"?)?

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

Oh, I'm fine with the mode description here in the code.  I was referring to the
Xsite mentions in the CL description.  :)

[Łukasz Anforowicz, 2017/07/06 20:41:57]

Oh, right :-).  I've updated the CL description.

+  V(Ads, 2, "isolate ads detected by heuristics")
+
+enum class TopDocumentIsolationMode {
+  Default = 0,

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

Sorry, I'm getting confused about the difference between these names and values
and those in the CL description (which had 5 different choices).  How do we end
up with 5 values in about:flags, and how do we map between them?  Maybe that's
worth explaining in a comment?

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

I've added a comment to the code above.

chrome://flags gives 3 entries for free:
- Default (whether the feature is disabled or enabled - this depends on what we
say in base::Feature definition for kTopDocumentIsolation)
- Disabled
- Enabled (this corresponds to "Mode::Default" enum value above)

we explicitly declare 2 more entries in about_flags.cc - one for each of the
modes covered by FOR_EACH_TDI_MODE - they correspond to "Enabled (<name> -
<description>)" entries in chrome://flags.

+
+#define DEFINE_TDI_MODE_ENUM_VALUE(name, value, description) name = value,
+  FOR_EACH_TDI_MODE(DEFINE_TDI_MODE_ENUM_VALUE)
+#undef DEFINE_TDI_MODE_ENUM_VALUE
+};
+
 CONTENT_EXPORT extern const base::Feature kTouchpadAndWheelScrollLatching;
 CONTENT_EXPORT extern const base::Feature kUseFeaturePolicyForPermissions;
 CONTENT_EXPORT extern const base::Feature kUseMojoAudioOutputStreamFactory;