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..11e71cb4e3547292e8df4fbe74e6305be81ef165 100644
--- a/content/public/common/content_features.h
+++ b/content/public/common/content_features.h
@@ -72,7 +72,20 @@ 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[];
+enum class TopDocumentIsolationMode {
+  Default = 0,

[Łukasz Anforowicz, 2017/06/30 15:28:37]

Hmmm... maybe this should be spelled kDefault... 
https://google.github.io/styleguide/cppguide.html#Enumerator_Names

+
+  // Isolate all frames that are x-site from the top-level frame.
+  Xsite = 1,
+
+  // Isolate frames that our heursitics recognize as ads.
+  Ads = 2,
+};

[Łukasz Anforowicz, 2017/06/30 15:28:37]

I don't really know what is the best place for declaring/defining 1) the
kTopDocumentIsolationModeParam string/const char* and 2) the
TopDocumentIsolationMode enum.  I think having them in content_features.h is
okay (although maybe I should move them to the end of this file).  One
alternative would be to introduce
content/public/browser/top_document_isolation_feature.h and move 1)
kTopDocumentIsolation base::Feature, 2) kTopDocumentIsolationModeParam, 3)
TopDocumentIsolationMode enum, 4) EnableTopDocumentIsolationForTesting (and
maybe even 5. IsTopDocumentIsolationEnabled) into this new compilation unit -
this is done in a few other places in Chromium like:

- chrome/browser/prerender/prerender_field_trial.h
- chrome/browser/experiments/memory_ablation_experiment.h (the only file in this
directory :-P)
- components/omnibox/browser/omnibox_field_trial.h

[Charlie Reis, 2017/07/06 20:40:35]

I'm not sure either.  I guess it feels a bit out of place in this file (which
only has base::Features), so your top_document_isolation_feature.h file could
make sense.  Worth checking with people who know Features a bit more.

[Łukasz Anforowicz, 2017/07/06 21:02:29]

asvitkine@ - WDYT / what would you do here?  Keep the changes as-is (slightly
updated in the most recent patchset) VS move them into a separate/new
compilation unit (top_document_isolation_trial.h in the same directory?).

Hmmm... I guess the move can happen in a separate CL, to keep the current one
self-contained / clean.

+
 CONTENT_EXPORT extern const base::Feature kTouchpadAndWheelScrollLatching;
 CONTENT_EXPORT extern const base::Feature kUseFeaturePolicyForPermissions;
 CONTENT_EXPORT extern const base::Feature kUseMojoAudioOutputStreamFactory;