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

chrome/browser/page_load_metrics/observers/ads_detection.h

+// Copyright 2017 The Chromium Authors. All rights reserved.

[jkarlin, 2017/07/18 17:34:44]

I'm not sure that this utility file belongs in this directory. It may be best to
just leave it in the observer file and include that.

csharrison@ may have an alternative suggestion.

[Charlie Harrison, 2017/07/18 18:11:30]

I would slightly prefer it to be in chrome/browser/page_load_metrics or even
chrome/common/page_load_metrics if we think using it in the renderer ever makes
sense.

Honestly, page_load_metrics is a pretty weird place for it, so I was thinking it
could maybe live somewhere in //content, but I don't really mind.

[Łukasz Anforowicz, 2017/07/18 19:06:11]

sgtm - I've moved ads_detection.h/.cc to chrome/browser/page_load_metrics.
NavigationHandle is a browser-side-only concept.
I've talked with nasko@ and I think creis@ also thinks that we should keep ads
detection out of //content.  Two reasons:
1. Different //content embedders (e.g. Chrome, Opera, Brave, ...) should be free
to use different ad-detection heuristics.
2. Some //content embedders (e.g. Chromecast) might not be interested in
ad-detection at all.

[Charlie Harrison, 2017/07/18 19:24:41]

Yeah, it would take some maneuvering to put it in //common (i.e. make it not
depend on NavigationHandle/RFH), so I don't think we should do it yet. However,
I could totally see places in the renderer wanting to know this kind of
information, so it's worth keeping in mind.
Makes sense to me.

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROME_BROWSER_PAGE_LOAD_METRICS_OBSERVERS_ADS_DETECTION_H_
+#define CHROME_BROWSER_PAGE_LOAD_METRICS_OBSERVERS_ADS_DETECTION_H_
+
+#include <bitset>
+#include <string>
+
+#include "base/strings/string_piece_forward.h"
+
+namespace content {
+class NavigationHandle;
+}  // namespace content
+
+// The types of ads that one can detect or filter on.
+enum AdType {
+  AD_TYPE_GOOGLE = 0,
+  AD_TYPE_SUBRESOURCE_FILTER = 1,
+  AD_TYPE_ALL = 2,
+  AD_TYPE_MAX = AD_TYPE_ALL
+};
+using AdTypes = std::bitset<AD_TYPE_MAX>;
+
+// Uses various heuristics to determine if a given navigation tries to load an
+// advertisment.  For some ad types the results depend on
+// SetNavigationAdHeuristics, but some ad types can be detected based on the
+// basic navigation properties (like URL and/or name of the frame being
+// navigated).

[jkarlin, 2017/07/18 17:34:44]

Discussion about the SetNavigationAdHeuristics stuff seems like implementation
detail that doesn't belong in a public comment.

How about:
Uses heuristics to determine if |navigation_handle| is a navigation for an ad.
Must be called no earlier than NavigationHandleImpl::ReadyToCommitNavigation.

[Łukasz Anforowicz, 2017/07/18 19:06:11]

Done.
Also done.  Sadly, I don't see a way to DCHECK that this is properly used / that
this is not used before ReadyToCommitNavigation.

+AdTypes GetAdDetectionHeuristics(content::NavigationHandle* navigation_handle);

[jkarlin, 2017/07/18 17:34:44]

GetAdDetectionHeuristics sounds like it's returning heuristics, and not types. I
strongly prefer GetDetectedAdTypes.

[Łukasz Anforowicz, 2017/07/18 19:06:11]

Done.

+
+// Marks the |navigation_handle| as loading an advertisment of a given |type|.
+void SetAdDetectionHeuristics(content::NavigationHandle* navigation_handle,

[jkarlin, 2017/07/18 17:34:44]

Likewise, I much prefer SetDetectedAdType.

[Łukasz Anforowicz, 2017/07/18 19:06:11]

Done.

+                              AdType type);
+
+#endif  // CHROME_BROWSER_PAGE_LOAD_METRICS_OBSERVERS_ADS_DETECTION_H_