Skip to content

Commit

Permalink
Removed iframeContextInName flag for experiment (#7107)
Browse files Browse the repository at this point in the history 
* Removed flag for context in name experiment

* Removed flag from prod and canary config
  • Loading branch information
@bradfrizzell @lannka
bradfrizzell authored and lannka committed Jan 19, 2017
1 parent d5ebb22 commit d2029fd
Show file tree
Hide file tree
Showing 7 changed files with 62 additions and 154 deletions.
23 changes: 9 additions & 14 deletions 3p/integration.js
View file
Expand Up @@ -154,21 +154,16 @@ const AMP_EMBED_ALLOWED = {
// Need to cache iframeName as it will be potentially overwritten by
// masterSelection, as per below.
const iframeName = window.name;

let data = parseFragment(location.hash);
if (data && data._context) {
let data = {};
try {
// TODO(bradfrizzell@): Change the data structure of the attributes
// to make it less terrible.
data = JSON.parse(iframeName).attributes;
window.context = data._context;
} else {
try {
// TODO(bradfrizzell@): Change the data structure of the attributes
// to make it less terrible.
data = JSON.parse(iframeName).attributes;
window.context = data._context;
} catch (err) {
window.context = {};
dev().info(
'INTEGRATION', 'Could not parse context from:', iframeName);
}
} catch (err) {
window.context = {};
dev().info(
'INTEGRATION', 'Could not parse context from:', iframeName);
}

// This should only be invoked after window.context is set
Expand Down
1 change: 0 additions & 1 deletion build-system/global-configs/canary-config.json
View file
Expand Up @@ -16,7 +16,6 @@
"amp-inabox": 1,
"ios-embed-wrapper": 1,
"amp-apester-media": 1,
"3p-frame-context-in-name": 1,
"amp-accordion-session-state-optout": 1,
"amp-playbuzz": 1,
"amp-selector": 1,
Expand Down
1 change: 0 additions & 1 deletion build-system/global-configs/prod-config.json
View file
Expand Up @@ -16,7 +16,6 @@
"amp-inabox": 1,
"ios-embed-wrapper": 1,
"amp-apester-media": 1,
"3p-frame-context-in-name": 1,
"amp-accordion-session-state-optout": 1,
"amp-playbuzz": 1,
"amp-selector": 1
Expand Down
56 changes: 14 additions & 42 deletions extensions/amp-ad/0.1/test/test-amp-ad-3p-impl.js
View file
Expand Up @@ -19,7 +19,6 @@ import {createIframePromise} from '../../../../testing/iframe';
import {stubService} from '../../../../testing/test-helper';
import {createElementWithAttributes} from '../../../../src/dom';
import * as adCid from '../../../../src/ad-cid';
import {isExperimentOn} from '../../../../src/experiments';
import '../../../amp-ad/0.1/amp-ad';
import '../../../amp-sticky-ad/0.1/amp-sticky-ad';
import * as lolex from 'lolex';
Expand All @@ -44,13 +43,6 @@ describe('amp-ad-3p-impl', () => {
let ad3p;
let win;

/**
* If true, then in experiment where the passing of context metadata
* has been moved from the iframe src hash to the iframe name attribute.
*/
const iframeContextInName = isExperimentOn(
window, '3p-frame-context-in-name');

beforeEach(() => {
sandbox = sinon.sandbox.create();
return createIframePromise(true).then(iframe => {
Expand Down Expand Up @@ -84,13 +76,8 @@ describe('amp-ad-3p-impl', () => {
expect(iframe.style.display).to.equal('');

let data;
if (iframeContextInName) {
expect(url).to.match(/frame(.max)?.html/);
data = JSON.parse(iframe.name).attributes;
} else {
expect(url).to.match(/frame(.max)?.html#{/);
data = JSON.parse(url.substr(url.indexOf('#') + 1));
}
expect(url).to.match(/frame(.max)?.html/);
data = JSON.parse(iframe.name).attributes;
expect(data).to.have.property('type', '_ping_');
expect(data).to.have.property('src', 'https://testsrc');
expect(data).to.have.property('width', 300);
Expand Down Expand Up @@ -118,15 +105,10 @@ describe('amp-ad-3p-impl', () => {
return ad3p.layoutCallback().then(() => {
const frame = ad3p.element.querySelector('iframe[src]');
expect(frame).to.be.ok;
if (iframeContextInName) {
const data = JSON.parse(frame.name).attributes;
expect(data).to.be.ok;
expect(data._context).to.be.ok;
expect(data._context.clientId).to.equal('sentinel123');
} else {
expect(frame.getAttribute('src')).to.contain(
'"clientId":"sentinel123"');
}
const data = JSON.parse(frame.name).attributes;
expect(data).to.be.ok;
expect(data._context).to.be.ok;
expect(data._context.clientId).to.equal('sentinel123');
});
});

Expand All @@ -137,15 +119,10 @@ describe('amp-ad-3p-impl', () => {
return ad3p.layoutCallback().then(() => {
const frame = ad3p.element.querySelector('iframe[src]');
expect(frame).to.be.ok;
if (iframeContextInName) {
const data = JSON.parse(frame.name).attributes;
expect(data).to.be.ok;
expect(data._context).to.be.ok;
expect(data._context.clientId).to.equal(null);
} else {
expect(frame.getAttribute('src')).to.contain(
'"clientId":null');
}
const data = JSON.parse(frame.name).attributes;
expect(data).to.be.ok;
expect(data._context).to.be.ok;
expect(data._context.clientId).to.equal(null);
});
});

Expand Down Expand Up @@ -193,15 +170,10 @@ describe('amp-ad-3p-impl', () => {
return ad3p.layoutCallback().then(() => {
const frame = ad3p.element.querySelector('iframe[src]');
expect(frame).to.be.ok;
if (iframeContextInName) {
const data = JSON.parse(frame.name).attributes;
expect(data).to.be.ok;
expect(data._context).to.be.ok;
expect(data._context.container).to.equal('AMP-STICKY-AD');
} else {
expect(frame.getAttribute('src')).to.contain(
'"container":"AMP-STICKY-AD"');
}
const data = JSON.parse(frame.name).attributes;
expect(data).to.be.ok;
expect(data._context).to.be.ok;
expect(data._context.container).to.equal('AMP-STICKY-AD');
});
});
});
Expand Down
40 changes: 13 additions & 27 deletions src/3p-frame.js
View file
Expand Up @@ -28,12 +28,6 @@ import {urls} from './config';
import {setStyle} from './style';
import {domFingerprint} from './utils/dom-fingerprint';

/**
* If true, then in experiment where the passing of context metadata
* has been moved from the iframe src hash to the iframe name attribute.
*/
const iframeContextInName = isExperimentOn(self, '3p-frame-context-in-name');

/** @type {!Object<string,number>} Number of 3p frames on the for that type. */
let count = {};

Expand Down Expand Up @@ -131,28 +125,20 @@ export function getIframe(parentWindow, parentElement, opt_type, opt_context) {

const baseUrl = getBootstrapBaseUrl(parentWindow);
const host = parseUrl(baseUrl).hostname;
let name;
if (iframeContextInName) {
// This name attribute may be overwritten if this frame is chosen to
// be the master frame. That is ok, as we will read the name off
// for our uses before that would occur.
// @see https://github.com/ampproject/amphtml/blob/master/3p/integration.js
name = JSON.stringify({
host,
type: attributes.type,
// https://github.com/ampproject/amphtml/pull/2955
count: count[attributes.type],
attributes,
});
// This name attribute may be overwritten if this frame is chosen to
// be the master frame. That is ok, as we will read the name off
// for our uses before that would occur.
// @see https://github.com/ampproject/amphtml/blob/master/3p/integration.js
const name = JSON.stringify({
host,
type: attributes.type,
// https://github.com/ampproject/amphtml/pull/2955
count: count[attributes.type],
attributes,
});

iframe.src = baseUrl;
iframe.ampLocation = parseUrl(baseUrl);
} else {
const src = baseUrl + '#' + JSON.stringify(attributes);
name = host + '_' + attributes.type + '_' + count[attributes.type];
iframe.src = src;
iframe.ampLocation = parseUrl(src);
}
iframe.src = baseUrl;
iframe.ampLocation = parseUrl(baseUrl);
iframe.name = name;
iframe.width = attributes.width;
iframe.height = attributes.height;
Expand Down
89 changes: 26 additions & 63 deletions test/functional/test-3p-frame.js
View file
Expand Up @@ -27,7 +27,7 @@ import {
} from '../../src/3p-frame';
import {documentInfoForDoc} from '../../src/document-info';
import {loadPromise} from '../../src/event-helper';
import {isExperimentOn, toggleExperiment} from '../../src/experiments';
import {toggleExperiment} from '../../src/experiments';
import {preconnectForElement} from '../../src/preconnect';
import {validateData} from '../../3p/3p';
import {viewerForDoc} from '../../src/viewer';
Expand All @@ -42,13 +42,6 @@ describe('3p-frame', () => {

const sentinelNames = ['sentinel', 'amp3pSentinel'];

/**
* If true, then in experiment where the passing of context metadata
* has been moved from the iframe src hash to the iframe name attribute.
*/
const iframeContextInName = isExperimentOn(
window, '3p-frame-context-in-name');

beforeEach(() => {
sandbox = sinon.sandbox.create();
clock = sandbox.useFakeTimers();
Expand Down Expand Up @@ -172,12 +165,8 @@ describe('3p-frame', () => {
const docInfo = documentInfoForDoc(window.document);
expect(docInfo.pageViewId).to.not.be.empty;
let sentinel;
if (iframeContextInName) {
const name = JSON.parse(decodeURIComponent(iframe.name));
sentinel = name.attributes._context[sentinelName];
} else {
sentinel = iframe.getAttribute('data-amp-3p-sentinel');
}
const name = JSON.parse(decodeURIComponent(iframe.name));
sentinel = name.attributes._context[sentinelName];
const fragment =
'{"testAttr":"value","ping":"pong","width":50,"height":100,' +
'"type":"_ping_",' +
Expand Down Expand Up @@ -206,35 +195,18 @@ describe('3p-frame', () => {
'{"width":100,"height":200},"intersectionRect":{' +
'"left":0,"top":0,"width":0,"height":0,"bottom":0,' +
'"right":0,"x":0,"y":0}}}}';
if (iframeContextInName) {
expect(src).to.equal(
'http://ads.localhost:9876/dist.3p/current/frame.max.html');
const parsedFragment = JSON.parse(fragment);
// Since DOM fingerprint changes between browsers and documents, to have
// stable tests, we can only verify its existence.
expect(name.attributes._context.domFingerprint).to.exist;
delete name.attributes._context.domFingerprint;
delete parsedFragment._context.domFingerprint;
expect(name.attributes).to.deep.equal(parsedFragment);

// Switch to same origin for inner tests.
iframe.src = '/dist.3p/current/frame.max.html';
} else {
const srcParts = src.split('#');
expect(srcParts[0]).to.equal(
'http://ads.localhost:9876/dist.3p/current/frame.max.html');
const expectedFragment = JSON.parse(srcParts[1]);
const parsedFragment = JSON.parse(fragment);
// Since DOM fingerprint changes between browsers and documents, to have
// stable tests, we can only verify its existence.
expect(expectedFragment._context.domFingerprint).to.exist;
delete expectedFragment._context.domFingerprint;
delete parsedFragment._context.domFingerprint;
expect(expectedFragment).to.deep.equal(parsedFragment);

// Switch to same origin for inner tests.
iframe.src = '/dist.3p/current/frame.max.html#' + fragment;
}
expect(src).to.equal(
'http://ads.localhost:9876/dist.3p/current/frame.max.html');
const parsedFragment = JSON.parse(fragment);
// Since DOM fingerprint changes between browsers and documents, to have
// stable tests, we can only verify its existence.
expect(name.attributes._context.domFingerprint).to.exist;
delete name.attributes._context.domFingerprint;
delete parsedFragment._context.domFingerprint;
expect(name.attributes).to.deep.equal(parsedFragment);

// Switch to same origin for inner tests.
iframe.src = '/dist.3p/current/frame.max.html';
document.body.appendChild(iframe);
return loadPromise(iframe).then(() => {
const win = iframe.contentWindow;
Expand Down Expand Up @@ -374,26 +346,17 @@ describe('3p-frame', () => {
};

container.appendChild(div);
if (iframeContextInName) {
const name = JSON.parse(getIframe(window, div).name);
resetBootstrapBaseUrlForTesting(window);
resetCountForTesting();
const newName = JSON.parse(getIframe(window, div).name);
expect(name.host).to.match(/d-\d+.ampproject.net/);
expect(name.type).to.match(/ping/);
expect(name.count).to.match(/1/);
expect(newName.host).to.match(/d-\d+.ampproject.net/);
expect(newName.type).to.match(/ping/);
expect(newName.count).to.match(/1/);
expect(newName).not.to.equal(name);
} else {
const name = getIframe(window, div).name;
resetBootstrapBaseUrlForTesting(window);
resetCountForTesting();
const newName = getIframe(window, div).name;
expect(name).to.match(/d-\d+.ampproject.net__ping__1/);
expect(newName).to.match(/d-\d+.ampproject.net__ping__1/);
}
const name = JSON.parse(getIframe(window, div).name);
resetBootstrapBaseUrlForTesting(window);
resetCountForTesting();
const newName = JSON.parse(getIframe(window, div).name);
expect(name.host).to.match(/d-\d+.ampproject.net/);
expect(name.type).to.match(/ping/);
expect(name.count).to.match(/1/);
expect(newName.host).to.match(/d-\d+.ampproject.net/);
expect(newName.type).to.match(/ping/);
expect(newName.count).to.match(/1/);
expect(newName).not.to.equal(name);
});

describe('serializeMessage', () => {
Expand Down
6 changes: 0 additions & 6 deletions tools/experiments/experiments.js
View file
Expand Up @@ -231,12 +231,6 @@ const EXPERIMENTS = [
name: 'AMP Accordion attribute to opt out of preserved state.',
Spec: 'https://github.com/ampproject/amphtml/issues/3813',
},
{
id: '3p-frame-context-in-name',
name: 'Move passing context metadata from url hash to name attribute',
cleanupIssue: 'https://github.com/ampproject/amphtml/issues/6760',
Spec: '',
},
{
id: 'sentinel-name-change',
name: 'Changed sentinel name from amp3pSentinel to sentinel',
Expand Down

0 comments on commit d2029fd

Please sign in to comment.