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.

Sign up for GitHub

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

Jump to bottom

Amp Ad Fast Fetch allow amp-image prefetch #11391

Merged
merged 4 commits into from Sep 25, 2017
Merged

Amp Ad Fast Fetch allow amp-image prefetch #11391

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
769f71b
Amp Ad Fast Fetch allow amp-image prefetch
keithwrightbos Sep 22, 2017
4da8234
type check fix
keithwrightbos Sep 22, 2017
ee793fe
Merge branch 'master' of github.com:ampproject/amphtml into a4a_image…
keithwrightbos Sep 25, 2017
d6f7916
feedback from @dvoytenko
keithwrightbos Sep 25, 2017
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
14 changes: 11 additions & 3 deletions extensions/amp-a4a/0.1/amp-a4a.js
View file
Open in desktop
Expand Up @@ -40,7 +40,7 @@ import {isArray, isObject, isEnumValue} from '../../../src/types';
import {utf8Decode} from '../../../src/utils/bytes';
import {getBinaryType, isExperimentOn} from '../../../src/experiments';
import {setStyle} from '../../../src/style';
import {assertHttpsUrl} from '../../../src/url';
import {assertHttpsUrl, isSecureUrl} from '../../../src/url';
import {parseJson} from '../../../src/json';
import {handleClick} from '../../../ads/alp/handler';
import {
Expand Down Expand Up @@ -106,7 +106,8 @@ export let SizeInfoDef;
/** @typedef {{
minifiedCreative: string,
customElementExtensions: !Array<string>,
customStylesheets: !Array<{href: string}>
customStylesheets: !Array<{href: string}>,
images: (Array<string>|undefined),
}} */
let CreativeMetaDataDef;

Expand Down Expand Up @@ -733,6 +734,9 @@ export class AmpA4A extends AMP.BaseElement {
// Preload any fonts.
(creativeMetaDataDef.customStylesheets || []).forEach(font =>
this.preconnect.preload(font.href));
// Preload any AMP images.
(creativeMetaDataDef.images || []).forEach(image =>
isSecureUrl(image) && this.preconnect.preload(image));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Who controls the origin of these images? How do we know we're not preconneting to a tracking service?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should pass in an as value. Otherwise it'll be downloaded 2x in Safari.

return creativeMetaDataDef;
})
.catch(error => {
Expand Down Expand Up @@ -1472,11 +1476,15 @@ export class AmpA4A extends AMP.BaseElement {
metaData.customStylesheets.forEach(stylesheet => {
if (!isObject(stylesheet) || !stylesheet['href'] ||
typeof stylesheet['href'] !== 'string' ||
!/^https:\/\//i.test(stylesheet['href'])) {
!isSecureUrl(stylesheet['href'])) {
throw new Error(errorMsg);
}
});
}
if (isArray(metaDataObj['images'])) {
// Load maximum of 5 images.
metaData.images = metaDataObj['images'].splice(0, 5);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not match the comment! #slice is not #splice.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, this will work since #splice returns the removed elements. But still, not the canonical use, #slice would be better here.

}
// TODO(keithwrightbos): OK to assume ampRuntimeUtf16CharOffsets is before
// metadata as its in the head?
metaData.minifiedCreative =
Expand Down
136 changes: 82 additions & 54 deletions extensions/amp-a4a/0.1/test/test-amp-a4a.js
View file
Open in desktop
Expand Up @@ -152,7 +152,7 @@ describe('amp-a4a', () => {

function buildCreativeString(opt_additionalInfo) {
const baseTestDoc = testFragments.minimalDocOneStyle;
const offsets = opt_additionalInfo || {};
const offsets = Object.assign({}, opt_additionalInfo || {});
offsets.ampRuntimeUtf16CharOffsets = [
baseTestDoc.indexOf('<style amp4ads-boilerplate'),
baseTestDoc.lastIndexOf('</script>') + '</script>'.length,
Expand Down Expand Up @@ -941,6 +941,35 @@ describe('amp-a4a', () => {
});
});
});
// TODO (keithwrightbos) - move into above e2e once signed creative with
// image within creative can be regenerated.
it('should prefetch amp images', () => {
return createIframePromise().then(fixture => {
setupForAdTesting(fixture);
fetchMock.getOnce(
TEST_URL + '&__amp_source_origin=about%3Asrcdoc', () => adResponse,
{name: 'ad'});
const doc = fixture.doc;
const a4aElement = createA4aElement(doc);
const a4a = new MockA4AImpl(a4aElement);
sandbox.stub(a4a, 'getAmpAdMetadata_', creative => {
const metaData = AmpA4A.prototype.getAmpAdMetadata_(creative);
metaData.images = ['https://prefetch.me.com?a=b', 'http://do.not.prefetch.me.com?c=d',
'https://prefetch.metoo.com?e=f'];
return metaData;
});
a4a.buildCallback();
a4a.onLayoutMeasure();
return a4a.layoutCallback().then(() => {
expect(doc.querySelector('link[rel=preload]' +
'[href="https://prefetch.me.com?a=b"]')).to.be.ok;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

String continuation onto next line should be indented 4 spaces, correct? (I suspect this is a case of the indentation being wrong but the file is omitted by the linter)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It passes gulp lint and I am fairly positive it is enforced on test files.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, nvm then

expect(doc.querySelector('link[rel=preload]' +
'[href="https://prefetch.metoo.com?e=f"]')).to.be.ok;
expect(doc.querySelector('link[rel=preload]' +
'[href="http://do.not.prefetch.me.com?c=d"]')).to.not.be.ok;
});
});
});
it('must not be position:fixed', () => {
return createIframePromise().then(fixture => {
setupForAdTesting(fixture);
Expand Down Expand Up @@ -1417,61 +1446,44 @@ describe('amp-a4a', () => {

describe('#getAmpAdMetadata_', () => {
let a4a;
let metaData;
beforeEach(() => {
metaData = {
customElementExtensions: ['amp-vine', 'amp-vine', 'amp-vine'],
customStylesheets: [
{href: 'https://fonts.googleapis.com/css?foobar'},
{href: 'https://fonts.com/css?helloworld'},
],
images: ['https://some.image.com/a=b', 'https://other.image.com'],
};
return createIframePromise().then(fixture => {
setupForAdTesting(fixture);
a4a = new MockA4AImpl(createA4aElement(fixture.doc));
return fixture;
});
});
it('should parse metadata', () => {
const actual = a4a.getAmpAdMetadata_(buildCreativeString({
customElementExtensions: ['amp-vine', 'amp-vine', 'amp-vine'],
customStylesheets: [
{href: 'https://fonts.googleapis.com/css?foobar'},
{href: 'https://fonts.com/css?helloworld'},
],
}));
const expected = {
const actual = a4a.getAmpAdMetadata_(buildCreativeString(metaData));
const expected = Object.assign(metaData, {
minifiedCreative: testFragments.minimalDocOneStyleSrcDoc,
customElementExtensions: ['amp-vine', 'amp-vine', 'amp-vine'],
customStylesheets: [
{href: 'https://fonts.googleapis.com/css?foobar'},
{href: 'https://fonts.com/css?helloworld'},
],
};
});
expect(actual).to.deep.equal(expected);
});
// TODO(levitzky) remove the following two tests after metadata bug is
// fixed.
it('should parse metadata with wrong opening tag', () => {
const creative = buildCreativeString({
customElementExtensions: ['amp-vine', 'amp-vine', 'amp-vine'],
customStylesheets: [
{href: 'https://fonts.googleapis.com/css?foobar'},
{href: 'https://fonts.com/css?helloworld'},
],
}).replace('<script type="application/json" amp-ad-metadata>',
const creative = buildCreativeString(metaData).replace(
'<script type="application/json" amp-ad-metadata>',
'<script type=application/json amp-ad-metadata>');
const actual = a4a.getAmpAdMetadata_(creative);
const expected = {
const expected = Object.assign({
minifiedCreative: testFragments.minimalDocOneStyleSrcDoc,
customElementExtensions: ['amp-vine', 'amp-vine', 'amp-vine'],
customStylesheets: [
{href: 'https://fonts.googleapis.com/css?foobar'},
{href: 'https://fonts.com/css?helloworld'},
],
};
}, metaData);
expect(actual).to.deep.equal(expected);
});
it('should return null if metadata opening tag is (truly) wrong', () => {
const creative = buildCreativeString({
customElementExtensions: ['amp-vine', 'amp-vine', 'amp-vine'],
customStylesheets: [
{href: 'https://fonts.googleapis.com/css?foobar'},
{href: 'https://fonts.com/css?helloworld'},
],
}).replace('<script type="application/json" amp-ad-metadata>',
const creative = buildCreativeString(metaData).replace(
'<script type="application/json" amp-ad-metadata>',
'<script type=application/json" amp-ad-metadata>');
expect(a4a.getAmpAdMetadata_(creative)).to.be.null;
});
Expand All @@ -1485,28 +1497,44 @@ describe('amp-a4a', () => {
baseTestDoc.slice(splicePoint))).to.be.null;
});
it('should return null if invalid extensions', () => {
expect(a4a.getAmpAdMetadata_(buildCreativeString({
customElementExtensions: 'amp-vine',
customStylesheets: [
{href: 'https://fonts.googleapis.com/css?foobar'},
{href: 'https://fonts.com/css?helloworld'},
],
}))).to.be.null;
metaData.customElementExtensions = 'amp-vine';
expect(a4a.getAmpAdMetadata_(buildCreativeString(metaData))).to.be.null;
});
it('should return null if non-array stylesheets', () => {
expect(a4a.getAmpAdMetadata_(buildCreativeString({
customElementExtensions: ['amp-vine', 'amp-vine', 'amp-vine'],
customStylesheets: 'https://fonts.googleapis.com/css?foobar',
}))).to.be.null;
metaData.customStylesheets = 'https://fonts.googleapis.com/css?foobar';
expect(a4a.getAmpAdMetadata_(buildCreativeString(metaData))).to.be.null;
});
it('should return null if invalid stylesheet object', () => {
expect(a4a.getAmpAdMetadata_(buildCreativeString({
customElementExtensions: ['amp-vine', 'amp-vine', 'amp-vine'],
customStylesheets: [
{href: 'https://fonts.googleapis.com/css?foobar'},
{foo: 'https://fonts.com/css?helloworld'},
],
}))).to.be.null;
metaData.customStylesheets = [
{href: 'https://fonts.googleapis.com/css?foobar'},
{foo: 'https://fonts.com/css?helloworld'},
];
expect(a4a.getAmpAdMetadata_(buildCreativeString(metaData))).to.be.null;
});
it('should not include amp images if not an array', () => {
metaData.images = 'https://foo.com';
const actual = a4a.getAmpAdMetadata_(buildCreativeString(metaData));
const expected = Object.assign({
minifiedCreative: testFragments.minimalDocOneStyleSrcDoc,
}, metaData);
delete expected.images;
expect(actual).to.deep.equal(expected);
});
it('should tolerate missing images', () => {
delete metaData.images;
const actual = a4a.getAmpAdMetadata_(buildCreativeString(metaData));
const expected = Object.assign({
minifiedCreative: testFragments.minimalDocOneStyleSrcDoc,
}, metaData);
delete expected.images;
expect(actual).to.deep.equal(expected);
});
it('should limit to 5 images', () => {
while (metaData.images.length < 10) {
metaData.images.push('https://another.image.com?abc=def');
}
expect(a4a.getAmpAdMetadata_(buildCreativeString(metaData)).images.length)
.to.equal(5);
});
// FAILURE cases here
});
Expand Down