Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Redefine ampdoc-fie experiment as a branched experiment #25174

Merged
merged 4 commits into from
Oct 24, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions extensions/amp-a4a/0.1/test/test-amp-a4a.js
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ import {
import {layoutRectLtwh} from '../../../../src/layout-rect';
import {resetScheduledElementForTesting} from '../../../../src/service/custom-element-registry';
import {data as testFragments} from './testdata/test_fragments';
import {toggleExperiment} from '../../../../src/experiments';
import {toggleAmpdocFieForTesting} from '../../../../src/ampdoc-fie';
import {data as validCSSAmp} from './testdata/valid_css_at_rules_amp.reserialized';

describe('amp-a4a', () => {
Expand Down Expand Up @@ -2081,7 +2081,7 @@ describe('amp-a4a', () => {
it('should render correctly in ampdoc-fie mode', async () => {
const parentWin = a4aElement.ownerDocument.defaultView;
const ampdocService = Services.ampdocServiceFor(parentWin);
toggleExperiment(parentWin, 'ampdoc-fie', true);
toggleAmpdocFieForTesting(parentWin, true);
updateFieModeForTesting(ampdocService, true);
await a4a.renderAmpCreative_(metaData);
// Verify iframe presence.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
// Most other ad networks will want to put their A4A code entirely in the
// extensions/amp-ad-network-${NETWORK_NAME}-impl directory.

import {EXPERIMENT_INFO_MAP as AMPDOC_FIE_EXPERIMENT_INFO_MAP} from '../../../src/ampdoc-fie';
import {AdsenseSharedState} from './adsense-shared-state';
import {AmpA4A} from '../../amp-a4a/0.1/amp-a4a';
import {CONSENT_POLICY_STATE} from '../../../src/consent-state';
Expand Down Expand Up @@ -213,22 +214,25 @@ export class AmpAdNetworkAdsenseImpl extends AmpA4A {
*/
divertExperiments() {
const experimentInfoMap = /** @type {!Object<string,
!../../../src/experiments.ExperimentInfo>} */ ({
[FORMAT_EXP]: {
isTrafficEligible: () =>
!this.responsiveState_ &&
Number(this.element.getAttribute('width')) > 0 &&
Number(this.element.getAttribute('height')) > 0,
branches: ['21062003', '21062004'],
!../../../src/experiments.ExperimentInfo>} */ (Object.assign(
{
[FORMAT_EXP]: {
isTrafficEligible: () =>
!this.responsiveState_ &&
Number(this.element.getAttribute('width')) > 0 &&
Number(this.element.getAttribute('height')) > 0,
branches: ['21062003', '21062004'],
},
[[FIE_CSS_CLEANUP_EXP.branch]]: {
isTrafficEligible: () => true,
branches: [
[FIE_CSS_CLEANUP_EXP.control],
[FIE_CSS_CLEANUP_EXP.experiment],
],
},
},
[[FIE_CSS_CLEANUP_EXP.branch]]: {
isTrafficEligible: () => true,
branches: [
[FIE_CSS_CLEANUP_EXP.control],
[FIE_CSS_CLEANUP_EXP.experiment],
],
},
});
AMPDOC_FIE_EXPERIMENT_INFO_MAP
));
const setExps = randomlySelectUnsetExperiments(this.win, experimentInfoMap);
Object.keys(setExps).forEach(expName =>
addExperimentIdToElement(setExps[expName], this.element)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
// extensions/amp-ad-network-${NETWORK_NAME}-impl directory.

import '../../amp-a4a/0.1/real-time-config-manager';
import {EXPERIMENT_INFO_MAP as AMPDOC_FIE_EXPERIMENT_INFO_MAP} from '../../../src/ampdoc-fie';
import {
AmpA4A,
DEFAULT_SAFEFRAME_VERSION,
Expand Down Expand Up @@ -379,33 +380,36 @@ export class AmpAdNetworkDoubleclickImpl extends AmpA4A {
}
}
const experimentInfoMap = /** @type {!Object<string,
!../../../src/experiments.ExperimentInfo>} */ ({
// Only select into SRA experiments if SRA not already explicitly
// enabled and refresh is not being used by any slot.
[DOUBLECLICK_SRA_EXP]: {
isTrafficEligible: () =>
!forcedExperimentId &&
!this.win.document./*OK*/ querySelector(
'meta[name=amp-ad-enable-refresh], ' +
'amp-ad[type=doubleclick][data-enable-refresh], ' +
'meta[name=amp-ad-doubleclick-sra]'
!../../../src/experiments.ExperimentInfo>} */ (Object.assign(
{
// Only select into SRA experiments if SRA not already explicitly
// enabled and refresh is not being used by any slot.
[DOUBLECLICK_SRA_EXP]: {
isTrafficEligible: () =>
!forcedExperimentId &&
!this.win.document./*OK*/ querySelector(
'meta[name=amp-ad-enable-refresh], ' +
'amp-ad[type=doubleclick][data-enable-refresh], ' +
'meta[name=amp-ad-doubleclick-sra]'
),
branches: Object.keys(DOUBLECLICK_SRA_EXP_BRANCHES).map(
key => DOUBLECLICK_SRA_EXP_BRANCHES[key]
),
branches: Object.keys(DOUBLECLICK_SRA_EXP_BRANCHES).map(
key => DOUBLECLICK_SRA_EXP_BRANCHES[key]
),
},
[FLEXIBLE_AD_SLOTS_EXP]: {
isTrafficEligible: () => true,
branches: Object.values(FLEXIBLE_AD_SLOTS_BRANCHES),
},
[FLEXIBLE_AD_SLOTS_EXP]: {
isTrafficEligible: () => true,
branches: Object.values(FLEXIBLE_AD_SLOTS_BRANCHES),
},
[[FIE_CSS_CLEANUP_EXP.branch]]: {
isTrafficEligible: () => true,
branches: [
[FIE_CSS_CLEANUP_EXP.control],
[FIE_CSS_CLEANUP_EXP.experiment],
],
},
},
[[FIE_CSS_CLEANUP_EXP.branch]]: {
isTrafficEligible: () => true,
branches: [
[FIE_CSS_CLEANUP_EXP.control],
[FIE_CSS_CLEANUP_EXP.experiment],
],
},
});
AMPDOC_FIE_EXPERIMENT_INFO_MAP
));
const setExps = this.randomlySelectUnsetExperiments_(experimentInfoMap);
Object.keys(setExps).forEach(
expName => setExps[expName] && this.experimentIds.push(setExps[expName])
Expand Down
66 changes: 66 additions & 0 deletions src/ampdoc-fie.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
/**
* Copyright 2019 The AMP HTML Authors. All Rights Reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS-IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

import {
forceExperimentBranch,
getExperimentBranch,
isExperimentOn,
randomlySelectUnsetExperiments,
} from './experiments';

// TODO(#22733): Remove this file once "ampdoc-fie" is cleaned up.

const EXPERIMENT_ID = 'ampdoc-fie';

/**
* @const {{experiment: string, control: string, branch: string}}
*/
const EXPERIMENT = {
branch: EXPERIMENT_ID,
control: '21065001',
experiment: '21065002',
};

/**
* @const {!Object<string, !./experiments.ExperimentInfo>}
*/
export const EXPERIMENT_INFO_MAP = {
[EXPERIMENT_ID]: {
isTrafficEligible: () => true,
branches: [[EXPERIMENT.control], [EXPERIMENT.experiment]],
},
};

/**
* @param {!Window} win
* @param {boolean} on
* @visibleForTesting
*/
export function toggleAmpdocFieForTesting(win, on) {
forceExperimentBranch(win, EXPERIMENT_ID, on ? EXPERIMENT.experiment : null);
}

/**
* @param {!Window} win
* @return {boolean}
*/
export function isInAmpdocFieExperiment(win) {
if (!isExperimentOn(win, 'ampdoc-fie')) {
return false;
}
randomlySelectUnsetExperiments(win, EXPERIMENT_INFO_MAP);
return getExperimentBranch(win, EXPERIMENT_ID) === EXPERIMENT.experiment;
}
5 changes: 3 additions & 2 deletions src/friendly-iframe-embed.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,14 +36,15 @@ import {
setParentWindow,
} from './service';
import {escapeHtml} from './dom';
import {getExperimentBranch, isExperimentOn} from './experiments';
import {getExperimentBranch} from './experiments';
import {installAmpdocServices} from './service/core-services';
import {install as installCustomElements} from './polyfills/custom-elements';
import {install as installDOMTokenList} from './polyfills/domtokenlist';
import {install as installDocContains} from './polyfills/document-contains';
import {installStylesForDoc, installStylesLegacy} from './style-installer';
import {installTimerInEmbedWindow} from './service/timer-impl';
import {isDocumentReady} from './document-ready';
import {isInAmpdocFieExperiment} from './ampdoc-fie';
import {layoutRectLtwh, moveLayoutRect} from './layout-rect';
import {loadPromise} from './event-helper';
import {
Expand Down Expand Up @@ -142,7 +143,7 @@ export function installFriendlyIframeEmbed(
const win = getTopWindow(toWin(iframe.ownerDocument.defaultView));
/** @const {!./service/extensions-impl.Extensions} */
const extensions = Services.extensionsFor(win);
const ampdocFieExperimentOn = isExperimentOn(win, 'ampdoc-fie');
const ampdocFieExperimentOn = isInAmpdocFieExperiment(win);
/** @const {?./service/ampdoc-impl.AmpDocService} */
const ampdocService = ampdocFieExperimentOn
? Services.ampdocServiceFor(win)
Expand Down
6 changes: 3 additions & 3 deletions src/service.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@

import {Deferred} from './utils/promise';
import {dev, devAssert} from './log';
import {isExperimentOn} from './experiments';
import {isInAmpdocFieExperiment} from './ampdoc-fie';
import {toWin} from './types';

/**
Expand Down Expand Up @@ -85,7 +85,7 @@ export function getExistingServiceForDocInEmbedScope(element, id) {
const topWin = getTopWindow(win);
// First, try to resolve via local embed window (if applicable).
const isEmbed = win != topWin;
const ampdocFieExperimentOn = isExperimentOn(topWin, 'ampdoc-fie');
const ampdocFieExperimentOn = isInAmpdocFieExperiment(topWin);
if (isEmbed && !ampdocFieExperimentOn) {
if (isServiceRegistered(win, id)) {
return getServiceInternal(win, id);
Expand Down Expand Up @@ -116,7 +116,7 @@ export function installServiceInEmbedScope(embedWin, id, service) {
'Service override has already been installed: %s',
id
);
const ampdocFieExperimentOn = isExperimentOn(topWin, 'ampdoc-fie');
const ampdocFieExperimentOn = isInAmpdocFieExperiment(topWin);
if (ampdocFieExperimentOn) {
const ampdoc = getAmpdoc(embedWin.document);
registerServiceInternal(
Expand Down
4 changes: 2 additions & 2 deletions src/service/ampdoc-impl.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ import {dev, devAssert} from '../log';
import {getParentWindowFrameElement, registerServiceBuilder} from '../service';
import {getShadowRootNode} from '../shadow-embed';
import {isDocumentReady, whenDocumentReady} from '../document-ready';
import {isExperimentOn} from '../experiments';
import {isInAmpdocFieExperiment} from '../ampdoc-fie';
import {map} from '../utils/object';
import {parseQueryString} from '../url';
import {rootNodeFor, waitForBodyOpenPromise} from '../dom';
Expand Down Expand Up @@ -88,7 +88,7 @@ export class AmpDocService {
}

/** @private {boolean} */
this.ampdocFieExperimentOn_ = isExperimentOn(win, 'ampdoc-fie');
this.ampdocFieExperimentOn_ = isInAmpdocFieExperiment(win);

/** @private {boolean} */
this.mightHaveShadowRoots_ = !isSingleDoc;
Expand Down
6 changes: 3 additions & 3 deletions test/unit/test-ampdoc.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ import {
import {Signals} from '../../src/utils/signals';
import {createShadowRoot} from '../../src/shadow-embed';
import {setParentWindow} from '../../src/service';
import {toggleExperiment} from '../../src/experiments';
import {toggleAmpdocFieForTesting} from '../../src/ampdoc-fie';

describes.sandboxed('AmpDocService', {}, () => {
afterEach(() => {
Expand Down Expand Up @@ -320,7 +320,7 @@ describes.sandboxed('AmpDocService', {}, () => {
let host, shadowRoot, content;

beforeEach(() => {
toggleExperiment(window, 'ampdoc-fie', true);
toggleAmpdocFieForTesting(window, true);
service = new AmpDocService(window, /* isSingleDoc */ true);
content = document.createElement('amp-img');
host = document.createElement('div');
Expand All @@ -337,7 +337,7 @@ describes.sandboxed('AmpDocService', {}, () => {
});

afterEach(() => {
toggleExperiment(window, 'ampdoc-fie', false);
toggleAmpdocFieForTesting(window, false);
if (host.parentNode) {
host.parentNode.removeChild(host);
}
Expand Down
14 changes: 7 additions & 7 deletions test/unit/test-friendly-iframe-embed.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ import {isAnimationNone} from '../../testing/test-helper';
import {layoutRectLtwh} from '../../src/layout-rect';
import {loadPromise} from '../../src/event-helper';
import {resetScheduledElementForTesting} from '../../src/service/custom-element-registry';
import {toggleExperiment} from '../../src/experiments';
import {toggleAmpdocFieForTesting} from '../../src/ampdoc-fie';
import {updateFieModeForTesting} from '../../src/service/ampdoc-impl';

describes.realWin('friendly-iframe-embed', {amp: true}, env => {
Expand Down Expand Up @@ -83,7 +83,7 @@ describes.realWin('friendly-iframe-embed', {amp: true}, env => {
resourcesMock.verify();
ampdocServiceMock.verify();
setSrcdocSupportedForTesting(undefined);
toggleExperiment(window, 'ampdoc-fie', false);
toggleAmpdocFieForTesting(window, false);
sandbox.restore();
});

Expand Down Expand Up @@ -170,7 +170,7 @@ describes.realWin('friendly-iframe-embed', {amp: true}, env => {
});

it('should create ampdoc and install extensions', () => {
toggleExperiment(window, 'ampdoc-fie', true);
toggleAmpdocFieForTesting(window, true);

// AmpDoc is created.
const ampdocSignals = new Signals();
Expand Down Expand Up @@ -235,7 +235,7 @@ describes.realWin('friendly-iframe-embed', {amp: true}, env => {
});

it('should create ampdoc and install extensions with host', () => {
toggleExperiment(window, 'ampdoc-fie', true);
toggleAmpdocFieForTesting(window, true);

// host.
const hostSignals = new Signals();
Expand Down Expand Up @@ -373,7 +373,7 @@ describes.realWin('friendly-iframe-embed', {amp: true}, env => {
});

it('should dispose ampdoc', () => {
toggleExperiment(window, 'ampdoc-fie', true);
toggleAmpdocFieForTesting(window, true);

// AmpDoc is created.
const ampdocSignals = new Signals();
Expand Down Expand Up @@ -1326,7 +1326,7 @@ describes.realWin('installExtensionsInFie', {amp: true}, env => {

beforeEach(() => {
parentWin = env.win;
toggleExperiment(parentWin, 'ampdoc-fie', true);
toggleAmpdocFieForTesting(parentWin, true);
resetScheduledElementForTesting(parentWin, 'amp-test');
installExtensionsService(parentWin);
extensions = Services.extensionsFor(parentWin);
Expand Down Expand Up @@ -1368,7 +1368,7 @@ describes.realWin('installExtensionsInFie', {amp: true}, env => {
});

afterEach(() => {
toggleExperiment(parentWin, 'ampdoc-fie', false);
toggleAmpdocFieForTesting(parentWin, false);
if (iframe.parentElement) {
iframe.parentElement.removeChild(iframe);
}
Expand Down