Start experiment to Add Fast Fetch support to canonical AMP pages for DoubleClick by bradfrizzell · Pull Request #11063 · ampproject/amphtml

bradfrizzell · 2017-08-23T20:29:06Z

This change will start an experiment to add Fast Fetch support for Canonical AMP pages with DRX

keithwrightbos · 2017-08-23T20:30:45Z

ads/google/a4a/utils.js

-  return supportsNativeCrypto &&
-      (googleCdnProxyRegex.test(win.location.origin) || getMode(win).localDev ||
-       getMode(win).test);
+  return win.crypto && (win.crypto.subtle || win.crypto.webkitSubtle);


We can't change this otherwise AdSense would lose is AMP cache enforcement. You need to move the cdn check to a separate function so that you can check separately.

keithwrightbos · 2017-08-23T20:35:35Z

extensions/amp-ad-network-doubleclick-impl/0.1/doubleclick-a4a-config.js

      isTrafficEligible: () => true,
      branches: [DOUBLECLICK_EXPERIMENT_FEATURE.HOLDBACK_INTERNAL_CONTROL,
-        DOUBLECLICK_EXPERIMENT_FEATURE.HOLDBACK_INTERNAL],
+                 DOUBLECLICK_EXPERIMENT_FEATURE.HOLDBACK_INTERNAL,


This should be a separate experiment name with separate call to randomlySelectUnsetExperiments. You can use its results explicitly to determine FF eligibility (no need to include with other experiment selection) as those are isolated to AMP cache pages.

keithwrightbos · 2017-08-24T18:43:19Z

extensions/amp-ad-network-doubleclick-impl/0.1/doubleclick-a4a-config.js

-    return false;
+let singleton;
+
+export class DoubleclickA4aEligibility {


Make this visibleForTesting

keithwrightbos · 2017-08-24T18:43:36Z

extensions/amp-ad-network-doubleclick-impl/0.1/doubleclick-a4a-config.js

-      element.hasAttribute('useSameDomainRenderingUntilDeprecated') ||
-      !isGoogleAdsA4AValidEnvironment(win)) {
-    return false;
+let singleton;


Any reason you can't assign the singleton immediately and make const?

keithwrightbos · 2017-08-24T18:43:50Z

extensions/amp-ad-network-doubleclick-impl/0.1/doubleclick-a4a-config.js

-    dev().info(TAG, `beta forced a4a selection ${element}`);
-    return true;
+
+  supportsCrypto(win) {


Doc here and throughout

keithwrightbos · 2017-08-24T18:44:12Z

extensions/amp-ad-network-doubleclick-impl/0.1/doubleclick-a4a-config.js

+  }
+
+  isA4aEnabled(win, element) {
+    element = element;


wha? you're assigning it to itself?

guaranteeing that it is who it says it is. Can't trust variables these days.

jk jk that was a messed up find/replace. Removed!

keithwrightbos · 2017-08-24T18:45:54Z

extensions/amp-ad-network-doubleclick-impl/0.1/doubleclick-a4a-config.js

+   * @param {!Element} element
+   * @param {?string} experimentId
+   * @param {!Array} validBranches
+   * @param {!string} experimentName}


Needs return doc

keithwrightbos · 2017-08-24T18:48:01Z

extensions/amp-ad-network-doubleclick-impl/0.1/doubleclick-a4a-config.js

+      experimentId = URL_EXPERIMENT_MAPPING[urlExperimentId];
+      dev().info(
+          TAG, `url experiment selection ${urlExperimentId}: ${experimentId}.`);
+      return this.addAndForceExperiment(win, element, experimentId, [


this function forces the experimentId given (if present) and then returns if the experimentId is in the array? Wouldn't that cause holdback external control to use FF?

Holdback external control should be using FF. That's the correct behavior.

keithwrightbos · 2017-08-24T18:48:17Z

extensions/amp-ad-network-doubleclick-impl/0.1/doubleclick-a4a-config.js

+ */
+export function doubleclickIsA4AEnabled(win, element) {
+  const eligibilityChecker = new DoubleclickA4aEligibility();
+  return eligibilityChecker.isA4aEnabled(win, element);


return singleton.isA4AEnabled(win, element);

keithwrightbos · 2017-08-24T19:50:12Z

extensions/amp-ad-network-doubleclick-impl/0.1/doubleclick-a4a-config.js

+/** @visibleForTesting */
+export class DoubleclickA4aEligibility {
+  constructor() {
+    return singleton;


keithwrightbos · 2017-08-24T19:53:44Z

extensions/amp-ad-network-doubleclick-impl/0.1/doubleclick-a4a-config.js

+        DOUBLECLICK_EXPERIMENT_FEATURE.CANONICAL_CONTROL,
+        DOUBLECLICK_EXPERIMENT_FEATURE.CANONICAL_EXPERIMENT,
+      ], DFP_CANONICAL_FF_EXPERIMENT_NAME);
+      if (experimentId) {


You could just have an else:

let experimentName = DFP_CANONICAL_FF_EXPERIMENT_NAME; if (!this.isCdnProxy(win)) { experimentId = <select into canonical exp> } else { experimentName = DOUBLECLICK_A4A_EXPERIMENT_NAME; if (element.hasAttribute... } if (experimentId) { addExperimentIdToElement(experimentId, element); forceExperimentBranch(win, experimentName, experimentId) } return ![DOUBLECLICK_EXPERIMENT_FEATURE.CANONICAL_CONTROL, DOUBLECLICK_EXPERIMENT_FEATURE.HOLDBACK_EXTERNAL, DOUBLECLICK_EXPERIMENT_FEATURE.HOLDBACK_INTERNAL, DOUBLECLICK_EXPERIMENT_FEATURE.SFG_CONTROL_ID, DOUBLECLICK_EXPERIMENT_FEATURE.SFG_EXP_ID].includes(experimentId);

keithwrightbos · 2017-08-24T19:54:19Z

extensions/amp-ad-network-doubleclick-impl/0.1/doubleclick-a4a-config.js

-    DOUBLECLICK_EXPERIMENT_FEATURE.SFG_EXP_ID].includes(experimentId);
+}
+
+const singleton = new DoubleclickA4aEligibility();


keithwrightbos · 2017-08-24T19:55:45Z

extensions/amp-ad-network-doubleclick-impl/0.1/test/test-doubleclick-a4a-config.js

-      mockWin.location = parseUrl(
-          'https://cdn.ampproject.org/some/path/to/content.html');
+    it('should enable a4a when native crypto is supported', () => {
+      forceExperimentBranch(mockWin, DFP_CANONICAL_FF_EXPERIMENT_NAME,


Ideally we wouldn't force anymore. Instead we would setup stub for experiment selection function and get experiment functions returning the values we want combined withArgs

Why is it better to stub than to force the experiment branch? Seems like it would be a more useful test for catching changes if we call the actual experiment selection

Agreed, see my previous comment that we do not currently test experiment selection at all.

keithwrightbos · 2017-08-24T19:57:02Z

extensions/amp-ad-network-doubleclick-impl/0.1/test/test-doubleclick-a4a-config.js

-    it('should enable a4a when requested and in local dev', () => {
+    it('should not enable a4a when native crypto is not supported', () => {
+      const instance = new DoubleclickA4aEligibility();
+      sandbox.stub(instance, 'supportsCrypto', () => false);


I'm surprised this works give you never pass instance to anything. I would have expected:

sandbox.stub(DoubleclickA4aEligibility.prototype, 'supportsCrypto').returns(false);

It works because it's a singleton, but I can do it the other way if this way is unclear

keithwrightbos · 2017-08-25T14:53:37Z

ads/google/a4a/utils.js

-  const supportsNativeCrypto = win.crypto &&
-      (win.crypto.subtle || win.crypto.webkitSubtle);
  const googleCdnProxyRegex =
      /^https:\/\/([a-zA-Z0-9_-]+\.)?cdn\.ampproject\.org/;


Are we consisted about the domain cdn.amproject.org.foo.com? Seems like we should guard against that.

^https:\/\/([a-zA-Z0-9_-]+\.)?cdn\.ampproject\.org((\/.*)|($))+ would work, please check it though

keithwrightbos · 2017-08-25T14:54:21Z

extensions/amp-ad-network-doubleclick-impl/0.1/doubleclick-a4a-config.js

 /** @const {string} */
 export const BETA_EXPERIMENT_ID = '2077831';

+/** @visibleForTesting */


Combine in the same comment as description

keithwrightbos · 2017-08-25T14:54:29Z

extensions/amp-ad-network-doubleclick-impl/0.1/doubleclick-a4a-config.js

-      !isGoogleAdsA4AValidEnvironment(win)) {
-    return false;
+export class DoubleclickA4aEligibility {
+  constructor() {}


are you sure you need an empty constructor?

keithwrightbos · 2017-08-25T14:54:54Z

extensions/amp-ad-network-doubleclick-impl/0.1/doubleclick-a4a-config.js

+   */
+  isCdnProxy(win) {
+    const googleCdnProxyRegex =
+      /^https:\/\/([a-zA-Z0-9_-]+\.)?cdn\.ampproject\.org/;


make sure if you tighten you do so here as well.

keithwrightbos · 2017-08-25T14:55:06Z

extensions/amp-ad-network-doubleclick-impl/0.1/doubleclick-a4a-config.js

+  isCdnProxy(win) {
+    const googleCdnProxyRegex =
+      /^https:\/\/([a-zA-Z0-9_-]+\.)?cdn\.ampproject\.org/;
+    return !!googleCdnProxyRegex.test(win.location.origin);


Are you sure you need the !!?

see #10141 (comment)

that being said I'll remove it

keithwrightbos · 2017-08-25T14:56:49Z

extensions/amp-ad-network-doubleclick-impl/0.1/doubleclick-a4a-config.js

+        DOUBLECLICK_EXPERIMENT_FEATURE.CANONICAL_EXPERIMENT,
+      ], DFP_CANONICAL_FF_EXPERIMENT_NAME);
+    } else {
+      experimentName = DOUBLECLICK_A4A_EXPERIMENT_NAME;


could make this after beta check (as its not needed if it returns true)

keithwrightbos · 2017-08-25T14:57:30Z

extensions/amp-ad-network-doubleclick-impl/0.1/doubleclick-a4a-config.js

+  /**
+   * @param {!Window} win
+   * @param {!Element} element
+   * @param {!Array} selectionBranches


confused, that's what I already have here...

Somehow my comment didn't appear correctly... I meant to say that you can sub type this as an array of strings

keithwrightbos · 2017-08-25T14:57:55Z

extensions/amp-ad-network-doubleclick-impl/0.1/doubleclick-a4a-config.js

+   * @param {!Element} element
+   * @param {!Array} selectionBranches
+   * @param {!string} experimentName}
+   * @return {?string} Experiment branch ID.


brand id or null if not selected

keithwrightbos · 2017-08-25T14:59:15Z

extensions/amp-ad-network-doubleclick-impl/0.1/test/test-doubleclick-a4a-config.js

@@ -56,33 +58,32 @@ describe('doubleclick-a4a-config', () => {
  });

  describe('#doubleclickIsA4AEnabled', () => {


I am generally displeased that this class does not actually test any of the random selection code. The tests just force a selection and verify it returns the correct value. Given you're restructuring the class to ease testing, how would you feel about addressing this deficiency?

keithwrightbos · 2017-08-25T14:59:45Z

extensions/amp-ad-network-doubleclick-impl/0.1/test/test-doubleclick-a4a-config.js

-      mockWin.location = parseUrl(
-          'https://cdn.ampproject.org/some/path/to/content.html');
+    it('should enable a4a when native crypto is supported', () => {
+      forceExperimentBranch(mockWin, DFP_CANONICAL_FF_EXPERIMENT_NAME,


Agreed, see my previous comment that we do not currently test experiment selection at all.

keithwrightbos

Please file an issue to add more testing and make sure to test on local gulp against real sites to ensure its behaving properly.

keithwrightbos · 2017-08-25T18:22:12Z

extensions/amp-ad-network-doubleclick-impl/0.1/doubleclick-a4a-config.js

+  /**
+   * @param {!Window} win
+   * @param {!Element} element
+   * @param {!Array} selectionBranches


Somehow my comment didn't appear correctly... I meant to say that you can sub type this as an array of strings

bradfrizzell · 2017-08-29T18:13:36Z

Manually tested that we properly select in on CDN pages with and without experiment param, and tested that non-CDN pages now select in.

Initial changes

8659f2e

bradfrizzell assigned keithwrightbos Aug 23, 2017

googlebot added the cla: yes label Aug 23, 2017

keithwrightbos requested changes Aug 23, 2017

View reviewed changes

bradfrizzell added 2 commits August 24, 2017 13:32

Refactored doubleclick-a4a-config

4e06345

Fixed lint

61c1b93

keithwrightbos requested changes Aug 24, 2017

View reviewed changes

responded to feedback

583dbe2

keithwrightbos assigned bradfrizzell and unassigned keithwrightbos Aug 24, 2017

keithwrightbos requested changes Aug 24, 2017

View reviewed changes

Responded to feedback

f541a6c

keithwrightbos requested changes Aug 25, 2017

View reviewed changes

Responded to feedback

9720e18

keithwrightbos approved these changes Aug 25, 2017

View reviewed changes

Fix lint

721412a

keithwrightbos merged commit 4cbe1be into ampproject:master Aug 30, 2017

keithwrightbos changed the title ~~Add Fast Fetch support to canonical AMP pages for DoubleClick~~ Start experiment to Add Fast Fetch support to canonical AMP pages for DoubleClick Aug 31, 2017

keithwrightbos mentioned this pull request Aug 31, 2017

Doubleclick FF: fix issue where forcing via url for development no longer applying on non-cache #11156

Merged

bradfrizzell mentioned this pull request Sep 11, 2017

Start 1% experiment for canonical FF support for doubleclick #11256

Merged

bradfrizzell mentioned this pull request Oct 27, 2017

[Master Feature] Intent to Implement: Deprecation of Delayed Fetch for DoubleClick #11834

Closed

Elisabeth1983 mentioned this pull request Dec 19, 2023

[Snyk] Security upgrade puppeteer from 5.2.1 to 21.3.7 Elisabeth1983/amphtml#52

Open

t10-13rocket mentioned this pull request Dec 19, 2023

[Snyk] Security upgrade puppeteer from 5.2.1 to 21.3.7 t10-13rocket/amphtml#36

Open

muratkalenderoglu mentioned this pull request Dec 19, 2023

[Snyk] Security upgrade puppeteer from 5.2.1 to 21.3.7 muratkalenderoglu/amphtml#46

Open

Masterci84 mentioned this pull request Dec 20, 2023

[Snyk] Security upgrade puppeteer from 5.2.1 to 21.3.7 Masterci84/amphtml#60

Open

		@@ -56,33 +58,32 @@ describe('doubleclick-a4a-config', () => {
		});

		describe('#doubleclickIsA4AEnabled', () => {

Conversation

bradfrizzell commented Aug 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keithwrightbos Aug 24, 2017 • edited by bradfrizzell Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

keithwrightbos Aug 24, 2017 •

edited by bradfrizzell

Loading