Skip to content

Commit

Permalink
✨ Send extension versions from AMPHTML ads to FIE (#33190)
Browse files Browse the repository at this point in the history
* move to extensions

* fie tests

* more tests fix bugs

* fix test

* comment

* use latest mustache

* use array.some

* move helper funcs

* half written jsdoc

* use url from config

* dep check
  • Loading branch information
calebcordry authored Mar 13, 2021
1 parent df83808 commit 2fb0396
Show file tree
Hide file tree
Showing 11 changed files with 199 additions and 151 deletions.
2 changes: 2 additions & 0 deletions build-system/test-configs/dep-check-config.js
Original file line number Diff line number Diff line change
Expand Up @@ -329,6 +329,8 @@ exports.rules = [
// Parsing extension urls.
'extensions/amp-a4a/0.1/head-validation.js->' +
'src/service/extension-script.js',
'extensions/amp-a4a/0.1/amp-ad-utils.js->' +
'src/service/extension-script.js',
'extensions/amp-live-list/0.1/live-list-manager.js->' +
'src/service/extension-script.js',
'extensions/amp-video/0.1/amp-video.js->' +
Expand Down
34 changes: 15 additions & 19 deletions extensions/amp-a4a/0.1/amp-a4a.js
Original file line number Diff line number Diff line change
Expand Up @@ -57,12 +57,13 @@ import {
} from '../../../src/consent';
import {getContextMetadata} from '../../../src/iframe-attributes';
import {getExperimentBranch, isExperimentOn} from '../../../src/experiments';
import {getExtensionsFromMetadata} from './amp-ad-utils';
import {getMode} from '../../../src/mode';
import {insertAnalyticsElement} from '../../../src/extension-analytics';
import {
installFriendlyIframeEmbed,
isSrcdocSupported,
preloadFriendlyIframeEmbedExtensionIdsDeprecated,
preloadFriendlyIframeEmbedExtensions,
} from '../../../src/friendly-iframe-embed';
import {installRealTimeConfigServiceForDoc} from '../../../src/service/real-time-config/real-time-config-impl';
import {installUrlReplacementsForEmbed} from '../../../src/service/url-replacements-impl';
Expand Down Expand Up @@ -1122,12 +1123,8 @@ export class AmpA4A extends AMP.BaseElement {

// Load any extensions; do not wait on their promises as this
// is just to prefetch.
// TODO(#33020): switch to `preloadFriendlyIframeEmbedExtensions` with
// the format of `[{extensionId, extensionVersion}]`.
preloadFriendlyIframeEmbedExtensionIdsDeprecated(
this.win,
creativeMetaDataDef.customElementExtensions
);
const extensions = getExtensionsFromMetadata(creativeMetaDataDef);
preloadFriendlyIframeEmbedExtensions(this.win, extensions);

// Preload any fonts.
(creativeMetaDataDef.customStylesheets || []).forEach((font) =>
Expand Down Expand Up @@ -1825,13 +1822,9 @@ export class AmpA4A extends AMP.BaseElement {
body
);

// TODO(ccordry): FIE does not handle extension versioning, but we have
// it accessible here.
const extensionIds = extensions.map((extension) => extension.extensionId);

const fieInstallPromise = this.installFriendlyIframeEmbed_(
secureDoc,
extensionIds,
extensions,
fonts,
true // skipHtmlMerge
);
Expand All @@ -1845,6 +1838,7 @@ export class AmpA4A extends AMP.BaseElement {
}
);

const extensionIds = extensions.map((extension) => extension.extensionId);
return fieInstallPromise.then((friendlyIframeEmbed) => {
checkStillCurrent();
this.makeFieVisible_(
Expand Down Expand Up @@ -1906,10 +1900,11 @@ export class AmpA4A extends AMP.BaseElement {
});
}
const checkStillCurrent = this.verifyStillCurrent();
const {minifiedCreative, customElementExtensions} = creativeMetaData;
const {minifiedCreative} = creativeMetaData;
const extensions = getExtensionsFromMetadata(creativeMetaData);
return this.installFriendlyIframeEmbed_(
minifiedCreative,
customElementExtensions,
extensions,
fontsArray || [],
false // skipHtmlMerge
).then((friendlyIframeEmbed) =>
Expand All @@ -1924,12 +1919,12 @@ export class AmpA4A extends AMP.BaseElement {
/**
* Convert the iframe to FIE impl and append to DOM.
* @param {string} html
* @param {!Array<string>} extensionIds
* @param {!Array<{extensionId: string, extensionVersion: string}>} extensions
* @param {!Array<string>} fonts
* @param {boolean} skipHtmlMerge
* @return {!Promise<!../../../src/friendly-iframe-embed.FriendlyIframeEmbed>}
*/
installFriendlyIframeEmbed_(html, extensionIds, fonts, skipHtmlMerge) {
installFriendlyIframeEmbed_(html, extensions, fonts, skipHtmlMerge) {
return installFriendlyIframeEmbed(
devAssert(this.iframe),
this.element,
Expand All @@ -1938,9 +1933,7 @@ export class AmpA4A extends AMP.BaseElement {
// Need to guarantee that this is no longer null
url: devAssert(this.adUrl_),
html,
// TODO(#33020): provide the `extensions` property instead, in
// the format of `[{extensionId, extensionVersion}]`.
extensionIds,
extensions,
fonts,
skipHtmlMerge,
},
Expand Down Expand Up @@ -2263,6 +2256,9 @@ export class AmpA4A extends AMP.BaseElement {
} else {
metaData.customElementExtensions = [];
}
if (metaDataObj['extensions']) {
metaData.extensions = metaDataObj['extensions'];
}
if (metaDataObj['customStylesheets']) {
// Expect array of objects with at least one key being 'href' whose
// value is URL.
Expand Down
33 changes: 33 additions & 0 deletions extensions/amp-a4a/0.1/amp-ad-utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import {Services} from '../../../src/services';
import {dev} from '../../../src/log';
import {isArray, isObject} from '../../../src/types';
import {isSecureUrlDeprecated} from '../../../src/url';
import {parseExtensionUrl} from '../../../src/service/extension-script';
import {parseJson} from '../../../src/json';

const TAG = 'amp-ad-util';
Expand Down Expand Up @@ -160,3 +161,35 @@ export function getAmpAdMetadata(creative) {
return null;
}
}

/**
* Determine if parsed extensions metadata contains given element id.
* @param {!Array<{custom-element: string, src: string}>} extensions
* @param {string} id
* @return {boolean}
*/
export function extensionsHasElement(extensions, id) {
return extensions.some((entry) => entry['custom-element'] === id);
}

/**
* Parses extension urls from given metadata to retrieve name and version.
* @param {!./amp-ad-type-defs.CreativeMetaDataDef} creativeMetadata
* @return {!Array<?{extensionId: string, extensionVersion: string}>}
*/
export function getExtensionsFromMetadata(creativeMetadata) {
const parsedExtensions = [];
const {extensions} = creativeMetadata;
if (!extensions || !isArray(extensions)) {
return parsedExtensions;
}

for (let i = 0; i < extensions.length; i++) {
const extension = extensions[i];
const extensionData = parseExtensionUrl(extension.src);
if (extensionData) {
parsedExtensions.push(extensionData);
}
}
return parsedExtensions;
}
8 changes: 4 additions & 4 deletions extensions/amp-a4a/0.1/friendly-frame-util.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,13 @@
import {A4AVariableSource} from '../../amp-a4a/0.1/a4a-variable-source';
import {createElementWithAttributes} from '../../../src/dom';
import {dict} from '../../../src/utils/object';
import {getExtensionsFromMetadata} from './amp-ad-utils';
import {installFriendlyIframeEmbed} from '../../../src/friendly-iframe-embed';
import {installUrlReplacementsForEmbed} from '../../../src/service/url-replacements-impl';
import {setStyle} from '../../../src/style';

/**
* Renders a creative into a "NameFrame" iframe.
* Renders a creative into a friendly iframe.
*
* @param {string} adUrl The ad request URL.
* @param {!./amp-ad-type-defs.LayoutInfoDef} size The size and layout of the
Expand Down Expand Up @@ -66,16 +67,15 @@ export function renderCreativeIntoFriendlyFrame(
});
}

const extensions = getExtensionsFromMetadata(creativeMetadata);
return installFriendlyIframeEmbed(
iframe,
element,
{
host: element,
url: /** @type {string} */ (adUrl),
html: creativeMetadata.minifiedCreative,
// TODO(#33020): provide the `extensions` property instead, in
// the format of `[{extensionId, extensionVersion}]`.
extensionIds: creativeMetadata.customElementExtensions || [],
extensions,
fonts: fontsArray,
},
(embedWin, ampdoc) => {
Expand Down
41 changes: 25 additions & 16 deletions extensions/amp-a4a/0.1/template-validator.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,15 @@
*/

import {AdResponseType, Validator, ValidatorResult} from './amp-ad-type-defs';
import {getAmpAdMetadata} from './amp-ad-utils';
import {
extensionsHasElement,
getAmpAdMetadata,
getExtensionsFromMetadata,
} from './amp-ad-utils';
import {getAmpAdTemplateHelper} from './amp-ad-template-helper';
import {preloadFriendlyIframeEmbedExtensionIdsDeprecated} from '../../../src/friendly-iframe-embed';
import {pushIfNotExist} from '../../../src/utils/array';
import {preloadFriendlyIframeEmbedExtensions} from '../../../src/friendly-iframe-embed';
import {tryParseJson} from '../../../src/json';
import {urls} from '../../../src/config';
import {utf8Decode} from '../../../src/utils/bytes';

/** @const {string} */
Expand Down Expand Up @@ -65,21 +69,26 @@ export class TemplateValidator extends Validator {
.fetch(parsedResponseBody.templateUrl)
.then((template) => {
const creativeMetadata = getAmpAdMetadata(template);
const customElementExtensions =
creativeMetadata['customElementExtensions'];
if (parsedResponseBody.analytics) {
pushIfNotExist(customElementExtensions, 'amp-analytics');
creativeMetadata['extensions'] = creativeMetadata['extensions'] || [];
const extensions = creativeMetadata['extensions'];
if (
parsedResponseBody.analytics &&
!extensionsHasElement(extensions, 'amp-analytics')
) {
extensions.push({
'custom-element': 'amp-analytics',
src: `${urls.cdn}/v0/amp-analytics-0.1.js`,
});
}
if (!extensionsHasElement(extensions, 'amp-mustache')) {
extensions.push({
'custom-element': 'amp-mustache',
src: `${urls.cdn}/v0/amp-mustache-latest.js`,
});
}
pushIfNotExist(customElementExtensions, 'amp-mustache');

// Load any extensions; do not wait on their promises as this
// is just to prefetch.
// TODO(#33020): switch to `preloadFriendlyIframeEmbedExtensions` with
// the format of `[{extensionId, extensionVersion}]`.
preloadFriendlyIframeEmbedExtensionIdsDeprecated(
context.win,
customElementExtensions
);
const extensionsInfo = getExtensionsFromMetadata(creativeMetadata);
preloadFriendlyIframeEmbedExtensions(context.win, extensionsInfo);

// TODO(levitzky) Add preload logic for fonts / images.
return Promise.resolve(
Expand Down
22 changes: 22 additions & 0 deletions extensions/amp-a4a/0.1/test/test-amp-a4a.js
Original file line number Diff line number Diff line change
Expand Up @@ -2352,6 +2352,28 @@ describes.realWin('amp-a4a', {amp: true}, (env) => {
expect(actual).to.deep.equal(expected);
});

it('should copy over any extensions detail', () => {
metaData.extensions = [
{
'custom-element': 'amp-analytics',
'src': 'https://cdn.ampproject.org/v0/amp-analytics-0.1.js',
},
{
'custom-element': 'amp-mustache',
'src': 'https://cdn.ampproject.org/v0/amp-mustache-1.0.js',
},
];
const actual = a4a.getAmpAdMetadata(buildCreativeString(metaData));
expect(actual.extensions).to.deep.include({
'custom-element': 'amp-analytics',
'src': 'https://cdn.ampproject.org/v0/amp-analytics-0.1.js',
});
expect(actual.extensions).to.deep.include({
'custom-element': 'amp-mustache',
'src': 'https://cdn.ampproject.org/v0/amp-mustache-1.0.js',
});
});

// TODO(levitzky) remove the following two tests after metadata bug is
// fixed.
it('should parse metadata with wrong opening tag', () => {
Expand Down
65 changes: 64 additions & 1 deletion extensions/amp-a4a/0.1/test/test-amp-ad-utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,11 @@
*/

import {data} from './testdata/valid_css_at_rules_amp.reserialized';
import {getAmpAdMetadata} from '../amp-ad-utils';
import {
extensionsHasElement,
getAmpAdMetadata,
getExtensionsFromMetadata,
} from '../amp-ad-utils';

describe('getAmpAdMetadata', () => {
it('should parse metadata successfully', () => {
Expand Down Expand Up @@ -56,3 +60,62 @@ describe('getAmpAdMetadata', () => {
expect(creativeMetadata).to.be.null;
});
});

describe('extensionsHasElement', () => {
it('should return true if containing extension', () => {
const extensions = [
{
'custom-element': 'amp-cats',
src: 'https://cdn.ampproject.org/v0/amp-cats-0.1.js',
},
];
expect(extensionsHasElement(extensions, 'amp-cats')).to.be.true;
});

it('should return false if it does not contain extension', () => {
const extensions = [
{
'custom-element': 'amp-cats',
src: 'https://cdn.ampproject.org/v0/amp-cats-0.1.js',
},
];
expect(extensionsHasElement(extensions, 'amp-dogs')).to.be.false;
});

it('should return false if empty array', () => {
const extensions = [];
expect(extensionsHasElement(extensions, 'amp-dogs')).to.be.false;
});
});

describe('getExtensionsFromMetadata', () => {
it('should return extension name and version', () => {
const metadata = {
extensions: [
{
'custom-element': 'amp-analytics',
'src': 'https://cdn.ampproject.org/v0/amp-analytics-0.1.js',
},
{
'custom-element': 'amp-mustache',
'src': 'https://cdn.ampproject.org/v0/amp-mustache-1.0.js',
},
],
};
const extensions = getExtensionsFromMetadata(metadata);
expect(extensions).to.deep.include({
extensionId: 'amp-analytics',
extensionVersion: '0.1',
});
expect(extensions).to.deep.include({
extensionId: 'amp-mustache',
extensionVersion: '1.0',
});
});

it('should handle no `extensions` key in metadata', () => {
const metadata = {};
const extensions = getExtensionsFromMetadata(metadata);
expect(extensions).to.eql([]);
});
});
14 changes: 9 additions & 5 deletions extensions/amp-a4a/0.1/test/test-template-validator.js
Original file line number Diff line number Diff line change
Expand Up @@ -130,15 +130,19 @@ describes.realWin('TemplateValidator', realWinConfig, (env) => {
});
});

it('should have amp-analytics and mustache in customElementExtensions', () => {
it('should have amp-analytics and mustache in extensions', () => {
return validatorPromise.then((validatorOutput) => {
expect(validatorOutput).to.be.ok;
expect(validatorOutput.creativeData).to.be.ok;
const {creativeMetadata} = validatorOutput.creativeData;
expect(creativeMetadata.customElementExtensions).to.deep.equal([
'amp-analytics',
'amp-mustache',
]);
expect(creativeMetadata.extensions).to.deep.include({
'custom-element': 'amp-analytics',
'src': 'https://cdn.ampproject.org/v0/amp-analytics-0.1.js',
});
expect(creativeMetadata.extensions).to.deep.include({
'custom-element': 'amp-mustache',
'src': 'https://cdn.ampproject.org/v0/amp-mustache-latest.js',
});
});
});
});
Expand Down
1 change: 1 addition & 0 deletions extensions/amp-story/1.0/test/test-amp-story-page.js
Original file line number Diff line number Diff line change
Expand Up @@ -251,6 +251,7 @@ describes.realWin('amp-story-page', {amp: {extensions}}, (env) => {
const fiePromise = installFriendlyIframeEmbed(iframe, gridLayerEl, {
url: 'https://amp.dev',
html: '<video src="https://example.com/video.mp3"></video>',
extensions: [],
});
env.sandbox.stub(page, 'loadPromise').returns(Promise.resolve());

Expand Down
Loading

0 comments on commit 2fb0396

Please sign in to comment.