Add internals.setIsAdSubframe and simplify web tests accordingly

Currently, many subresource_filter/ web_tests rely on the behavior that Frames are tagged as ads immediately when they are created if ad script is on the stack. This flow requires that first the script's URL is set as disallowed, then the script is loaded and then the frame can be created. This new call allows for web tests to more simply create ad frames by setting their ad status directly. We adapt existing web_tests to use this new functionality. We remove the ad script creation and onload setting boilerplate that is no longer necessary, also converting affected promise_tests to async_tests to further simplify them. These changes will additionally allow for an upcoming refactor that defers ad status calculation until the frame has navigated to access additional information. As this logic is in the subresource_filter/ component, it cannot be used in web-tests. Bug: 1119476 Change-Id: Id2fc2faa392f06c3b1845981f06f19a0650461d6 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2364956 Commit-Queue: Alex Turner <alexmt@chromium.org> Reviewed-by: Charlie Harrison <csharrison@chromium.org> Reviewed-by: Stefan Zager <szager@chromium.org> Cr-Commit-Position: refs/heads/master@{#807540}
qweri0p · Sep 16, 2020 · 22d5b10 · 22d5b10
1 parent 0d0a9ba
commit 22d5b10
Show file tree

Hide file tree

Showing 25 changed files with 466 additions and 622 deletions.
diff --git a/third_party/blink/renderer/core/testing/internals.cc b/third_party/blink/renderer/core/testing/internals.cc
@@ -3526,4 +3526,19 @@ void Internals::generateTestReport(const String& message) {
   ReportingContext::From(document_->domWindow())->QueueReport(report);
 }
 
+void Internals::setIsAdSubframe(HTMLIFrameElement* iframe,
+                                ExceptionState& exception_state) {
+  if (!iframe->ContentFrame() || !iframe->ContentFrame()->IsLocalFrame()) {
+    exception_state.ThrowDOMException(DOMExceptionCode::kNotSupportedError,
+                                      "Frame cannot be accessed.");
+    return;
+  }
+  LocalFrame* parent_frame = iframe->GetDocument().GetFrame();
+  LocalFrame* child_frame = To<LocalFrame>(iframe->ContentFrame());
+  bool parent_is_ad = parent_frame && parent_frame->IsAdSubframe();
+  child_frame->SetIsAdSubframe(parent_is_ad
+                                   ? blink::mojom::AdFrameType::kChildAd
+                                   : blink::mojom::AdFrameType::kRootAd);
+}
+
 }  // namespace blink
diff --git a/third_party/blink/renderer/core/testing/internals.h b/third_party/blink/renderer/core/testing/internals.h
@@ -611,6 +611,9 @@ class Internals final : public ScriptWrappable {
 
   void generateTestReport(const String& message);
 
+  void setIsAdSubframe(HTMLIFrameElement* iframe,
+                       ExceptionState& exception_state);
+
  private:
   Document* ContextDocument() const;
   Vector<String> IconURLs(Document*, int icon_types_mask) const;

diff --git a/third_party/blink/renderer/core/testing/internals.idl b/third_party/blink/renderer/core/testing/internals.idl
@@ -450,4 +450,6 @@
 
     // Request generation of a Reporting report.
     void generateTestReport(DOMString message);
+
+    [RaisesException] void setIsAdSubframe(HTMLIFrameElement iframe);
 };
diff --git a/third_party/blink/web_tests/http/tests/inspector-protocol/page/navigated-heavy-ad.js b/third_party/blink/web_tests/http/tests/inspector-protocol/page/navigated-heavy-ad.js
@@ -4,24 +4,21 @@
   await dp.Page.enable();
   session.evaluate(`
     if (window.testRunner) {
-      // Inject a subresource filter to mark 'ad-iframe-writer.js' as a would be disallowed resource.
-      testRunner.setDisallowedSubresourcePathSuffixes(["ad-iframe-writer.js"], false /* block_subresources */);
       testRunner.setHighlightAds();
     }
 
-    // Script must be loaded after disallowed paths are set to be marked as an ad.
-    let ad_script = document.createElement("script");
-    ad_script.async = false;
-    ad_script.src = "../resources/ad-iframe-writer.js";
-    ad_script.onload = function () {
-      ad_frame = createAdFrame();
-      ad_frame.width = 100;
-      ad_frame.height = 200;
-    };
-    document.body.appendChild(ad_script);
+    let ad_frame = document.createElement('iframe');
+    document.body.appendChild(ad_frame);
+    internals.setIsAdSubframe(ad_frame);
+    ad_frame.width = 100;
+    ad_frame.height = 200;
+    ad_frame.src = "about:blank";
   `);
+
+  // The first navigation will occur before the frame is set as an ad subframe.
+  // So, we wait for the second navigation before logging the adFrameType.
+  await dp.Page.onceFrameNavigated();
   const { params } = await dp.Page.onceFrameNavigated();
   testRunner.log({ adFrameType: params.frame.adFrameType });
-
   testRunner.completeTest();
 })
diff --git a/third_party/blink/web_tests/http/tests/inspector-protocol/resources/ad-iframe-writer.js b/third_party/blink/web_tests/http/tests/inspector-protocol/resources/ad-iframe-writer.js
diff --git a/...link/web_tests/http/tests/subresource_filter/ad-highlight-frame-larger-than-viewport.html b/...link/web_tests/http/tests/subresource_filter/ad-highlight-frame-larger-than-viewport.html
@@ -7,35 +7,27 @@
     margin: 0;
 }
 </style>
+<script src="resources/ad-iframe-writer.js"></script>
 </head>
 <body>
 <script type="text/javascript">
 if (window.testRunner) {
-  // Inject a subresource filter to mark 'ad-iframe-writer.js' as a would be disallowed resource.
-  testRunner.setDisallowedSubresourcePathSuffixes(["ad-iframe-writer.js"], false /* block_subresources */);
   testRunner.setHighlightAds();
 }
 
-// Script must be loaded after disallowed paths are set to be marked as an ad.
-let ad_script = document.createElement("script");
-ad_script.async = false;
-ad_script.src = "resources/ad-iframe-writer.js";
-ad_script.onload = function () {
-  ad_frame = createAdFrame();
-  ad_frame.style.borderWidth = 0;
-  ad_frame.width = 100;
-  ad_frame.height = 1000;
-  // Scroll down to verify that the overlay is drawn correctly for the portion of the
-  // frame that is larger than the viewport.
-  window.scrollTo(0, 400);
-  // Finish the test once the ad frame is loaded.
-  ad_frame.onload = function () {
-    if (window.testRunner)
-      testRunner.notifyDone();
-  }
-  ad_frame.src = "about:blank";
-};
-document.body.appendChild(ad_script);
+ad_frame = createAdFrame();
+ad_frame.style.borderWidth = 0;
+ad_frame.width = 100;
+ad_frame.height = 1000;
+// Scroll down to verify that the overlay is drawn correctly for the portion of the
+// frame that is larger than the viewport.
+window.scrollTo(0, 400);
+// Finish the test once the ad frame is loaded.
+ad_frame.onload = function () {
+  if (window.testRunner)
+    testRunner.notifyDone();
+}
+ad_frame.src = "about:blank";
 
 if (window.testRunner)
   testRunner.waitUntilDone();

diff --git a/third_party/blink/web_tests/http/tests/subresource_filter/ad-highlight-frame-resized.html b/third_party/blink/web_tests/http/tests/subresource_filter/ad-highlight-frame-resized.html
@@ -1,31 +1,23 @@
 <!DOCTYPE html>
 <html>
+<head><script src="resources/ad-iframe-writer.js"></script></head>
 <body>
 <script type="text/javascript">
 if (window.testRunner) {
-  // Inject a subresource filter to mark 'ad-iframe-writer.js' as a would be disallowed resource.
-  testRunner.setDisallowedSubresourcePathSuffixes(["ad-iframe-writer.js"], false /* block_subresources */);
   testRunner.setHighlightAds();
 }
 
-// Script must be loaded after disallowed paths are set to be marked as an ad.
-let ad_script = document.createElement("script");
-ad_script.async = false;
-ad_script.src = "resources/ad-iframe-writer.js";
-ad_script.onload = function () {
-  ad_frame = createAdFrame();
-  ad_frame.width = 100;
-  ad_frame.height = 200;
-  // Finish the test once the ad frame is loaded.
-  ad_frame.onload = function () {
-    // Resize the frame to verify that the highlight is also resized.
-    ad_frame.width = 300;
-    if (window.testRunner)
-      testRunner.notifyDone();
-  }
-  ad_frame.src = "about:blank";
-};
-document.body.appendChild(ad_script);
+ad_frame = createAdFrame();
+ad_frame.width = 100;
+ad_frame.height = 200;
+// Finish the test once the ad frame is loaded.
+ad_frame.onload = function () {
+  // Resize the frame to verify that the highlight is also resized.
+  ad_frame.width = 300;
+  if (window.testRunner)
+    testRunner.notifyDone();
+}
+ad_frame.src = "about:blank";
 
 if (window.testRunner)
   testRunner.waitUntilDone();

diff --git a/third_party/blink/web_tests/http/tests/subresource_filter/ad-highlight.html b/third_party/blink/web_tests/http/tests/subresource_filter/ad-highlight.html
@@ -1,29 +1,21 @@
 <!DOCTYPE html>
 <html>
+<head><script src="resources/ad-iframe-writer.js"></script></head>
 <body>
 <script type="text/javascript">
 if (window.testRunner) {
-  // Inject a subresource filter to mark 'ad-iframe-writer.js' as a would be disallowed resource.
-  testRunner.setDisallowedSubresourcePathSuffixes(["ad-iframe-writer.js"], false /* block_subresources */);
   testRunner.setHighlightAds();
 }
 
-// Script must be loaded after disallowed paths are set to be marked as an ad.
-let ad_script = document.createElement("script");
-ad_script.async = false;
-ad_script.src = "resources/ad-iframe-writer.js";
-ad_script.onload = function () {
-  ad_frame = createAdFrame();
-  ad_frame.width = 100;
-  ad_frame.height = 200;
-  // Finish the test once the ad frame is loaded.
-  ad_frame.onload = function () {
-    if (window.testRunner)
-      testRunner.notifyDone();
-  }
-  ad_frame.src = "about:blank";
-};
-document.body.appendChild(ad_script);
+ad_frame = createAdFrame();
+ad_frame.width = 100;
+ad_frame.height = 200;
+// Finish the test once the ad frame is loaded.
+ad_frame.onload = function () {
+  if (window.testRunner)
+    testRunner.notifyDone();
+}
+ad_frame.src = "about:blank";
 
 if (window.testRunner)
   testRunner.waitUntilDone();

diff --git a/.../blink/web_tests/http/tests/subresource_filter/large-sticky-ad-bottom-fixed-position.html b/.../blink/web_tests/http/tests/subresource_filter/large-sticky-ad-bottom-fixed-position.html
@@ -23,65 +23,51 @@
 </style>
 <script src="../resources/testharness.js"></script>
 <script src="../resources/testharnessreport.js"></script>
+<script src="resources/large-sticky-ad-testharness.js"></script>
 </head>
 <body marginwidth="0" marginheight="0">
 <!-- To trigger the first contentful paint at the very start -->
 <p>some content</p>
 <!-- To be positioned further down in the main page to make the page scrollable -->
 <div class="bottom"></div>
 <script>
+async_test(async function(t) {
+  // Create the large-sticky-ad.
+  appendAdFrameTo(document.body);
 
-if (window.testRunner) {
-  // Inject a subresource filter to mark 'large-sticky-ad-testharness.js' as a would be disallowed resource.
-  testRunner.setDisallowedSubresourcePathSuffixes(["large-sticky-ad-testharness.js"], false /* block_subresources */);
-}
-
-promise_test(() => {
-  return new Promise((resolve, reject) => {
-
-    let ad_script = document.createElement("script");
-    ad_script.async = false;
-    ad_script.src = "resources/large-sticky-ad-testharness.js";
-    ad_script.onload = async() => {
-      // Create the large-sticky-ad.
-      appendAdFrameTo(document.body);
+  // After 1500ms, force a layout update so that the sticky ad detector is
+  // aware of the sticky ad candidate.
+  await timeout(1500);
+  await forceLayoutUpdate();
 
-      // After 1500ms, force a layout update so that the sticky ad detector is
-      // aware of the sticky ad candidate.
-      await timeout(1500);
-      await forceLayoutUpdate();
-      if (internals.isUseCounted(document, kLargeStickyAd)) {
-        reject();
-      }
-
-      // Scroll down to 1px.
-      window.scrollTo(0, 1);
+  t.step(function () {
+    assert_false(internals.isUseCounted(document, kLargeStickyAd));
+  });
 
-      // After 1500ms, force a layout update. At this point the scrolling
-      // position hasn't changed much since the detector first saw the
-      // candidate, so we expect no use counter for kLargeStickyAd.
-      await timeout(1500);
-      await forceLayoutUpdate();
-      if (internals.isUseCounted(document, kLargeStickyAd)) {
-        reject();
-      }
+  // Scroll down to 1px.
+  window.scrollTo(0, 1);
 
-      // Scroll down to 5000px.
-      window.scrollTo(0, 5000);
+  // After 1500ms, force a layout update. At this point the scrolling
+  // position hasn't changed much since the detector first saw the
+  // candidate, so we expect no use counter for kLargeStickyAd.
+  await timeout(1500);
+  await forceLayoutUpdate();
+  t.step(function () {
+    assert_false(internals.isUseCounted(document, kLargeStickyAd));
+  });
 
-      // After 1500ms, force a layout update. At this point the scrolling
-      // position has changed by a distance greater than the candidate's height,
-      // so the use counter kLargeStickyAd should be recorded.
-      await timeout(1500);
-      await forceLayoutUpdate();
-      if (!internals.isUseCounted(document, kLargeStickyAd)) {
-        reject();
-      }
+  // Scroll down to 5000px.
+  window.scrollTo(0, 5000);
 
-      resolve();
-    };
-    document.body.appendChild(ad_script);
+  // After 1500ms, force a layout update. At this point the scrolling
+  // position has changed by a distance greater than the candidate's height,
+  // so the use counter kLargeStickyAd should be recorded.
+  await timeout(1500);
+  await forceLayoutUpdate();
+  t.step(function () {
+    assert_true(internals.isUseCounted(document, kLargeStickyAd));
   });
+  t.done();
 }, "Test UseCounter for large-sticky-ad at the bottom when the frame itself has a fixed position.");
 </script>
 </body>

diff --git a/...k/web_tests/http/tests/subresource_filter/large-sticky-ad-bottom-frequent-reposition.html b/...k/web_tests/http/tests/subresource_filter/large-sticky-ad-bottom-frequent-reposition.html
@@ -23,58 +23,44 @@
 </style>
 <script src="../resources/testharness.js"></script>
 <script src="../resources/testharnessreport.js"></script>
+<script src="resources/large-sticky-ad-testharness.js"></script>
 </head>
 <body marginwidth="0" marginheight="0">
 <!-- To trigger the first contentful paint at the very start -->
 <p>some content</p>
 <!-- To be positioned further down in the main page to make the page scrollable -->
 <div class="bottom"></div>
 <script>
+async_test(async function(t) {
+  // Create the large-sticky-ad.
+  appendAdFrameTo(document.body);
 
-if (window.testRunner) {
-  // Inject a subresource filter to mark 'large-sticky-ad-testharness.js' as a would be disallowed resource.
-  testRunner.setDisallowedSubresourcePathSuffixes(["large-sticky-ad-testharness.js"], false /* block_subresources */);
-}
-
-promise_test(() => {
-  return new Promise((resolve, reject) => {
-
-    let ad_script = document.createElement("script");
-    ad_script.async = false;
-    ad_script.src = "resources/large-sticky-ad-testharness.js";
-    ad_script.onload = async() => {
-      // Create the large-sticky-ad.
-      appendAdFrameTo(document.body);
-
-      let ad_candidate = document.getElementsByTagName("iframe")[0];
-      let initial_margin_top = parseInt(window.getComputedStyle(ad_candidate).marginTop, 10);
+  let ad_candidate = document.getElementsByTagName("iframe")[0];
+  let initial_margin_top = parseInt(window.getComputedStyle(ad_candidate).marginTop, 10);
 
-      // After 1500ms, force a layout update so that the sticky ad detector is
-      // aware of the sticky ad candidate.
-      await timeout(1500);
-      await forceLayoutUpdate();
-      if (internals.isUseCounted(document, kLargeStickyAd)) {
-        reject();
-      }
-
-      // Scroll down to 5000px. Reset the frame's position w.r.t. the viewport
-      // to its initial position.
-      window.scrollTo(0, 5000);
-      ad_candidate.style.marginTop = initial_margin_top + window.scrollY + 'px';
+  // After 1500ms, force a layout update so that the sticky ad detector is
+  // aware of the sticky ad candidate.
+  await timeout(1500);
+  await forceLayoutUpdate();
+  t.step(function () {
+    assert_false(internals.isUseCounted(document, kLargeStickyAd));
+  });
 
-      // After 1500ms, force a layout update. At this point the scrolling
-      // position has changed by a distance greater than the candidate's height,
-      // so the use counter kLargeStickyAd should be recorded.
-      await timeout(1500);
-      await forceLayoutUpdate();
-      if (!internals.isUseCounted(document, kLargeStickyAd)) {
-        reject();
-      }
+  // Scroll down to 5000px. Reset the frame's position w.r.t. the viewport
+  // to its initial position.
+  window.scrollTo(0, 5000);
+  ad_candidate.style.marginTop = initial_margin_top + window.scrollY + 'px';
 
-      resolve();
-    };
-    document.body.appendChild(ad_script);
+  // After 1500ms, force a layout update. At this point the scrolling
+  // position has changed by a distance greater than the candidate's height,
+  // so the use counter kLargeStickyAd should be recorded.
+  await timeout(1500);
+  await forceLayoutUpdate();
+  t.step(function() {
+    assert_true(internals.isUseCounted(document, kLargeStickyAd));
   });
+
+  t.done();
 }, "Test UseCounter for large-sticky-ad at the bottom when the frame itself has an absolute position, and along with scrolling the ad's position w.r.t. the browser viewport gets frequently reset to the initial postion.");
 </script>
 </body>