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

Lower the load priority of ad shaped iframes. #3863

Merged
merged 1 commit into from
Jul 2, 2016
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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 38 additions & 0 deletions extensions/amp-iframe/0.1/amp-iframe.js
Original file line number Diff line number Diff line change
Expand Up @@ -353,6 +353,14 @@ export class AmpIframe extends AMP.BaseElement {
}
}

/** @override */
getPriority() {
if (isAdLike(this.element)) {
return 2; // See AmpAd3PImpl.
}
return super.getPriority();
}

/**
* Makes the iframe visible.
* @private
Expand Down Expand Up @@ -436,6 +444,36 @@ function makeIOsScrollable(element) {
return element;
}

// Most common ad sizes
// Array of [width, height] pairs.
const adSizes = [[300, 250], [320, 50], [300, 50], [320, 100]];

/**
* Guess whether this element might be an ad.
* @param {!Element} element An amp-iframe element.
* @return {boolean}
* @visibleForTesting
*/
export function isAdLike(element) {
const height = parseInt(element.getAttribute('height'), 10);
const width = parseInt(element.getAttribute('width'), 10);
for (let i = 0; i < adSizes.length; i++) {
const refWidth = adSizes[i][0];
const refHeight = adSizes[i][1];
if (refHeight > height) {
continue;
}
if (refWidth > width) {
continue;
}
// Fuzzy matching to account for padding.
if (height - refHeight <= 20 && width - refWidth <= 20) {
return true;
}
}
return false;
}

/**
* @param {number} ms
*/
Expand Down
30 changes: 26 additions & 4 deletions extensions/amp-iframe/0.1/test/test-amp-iframe.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,11 @@
*/

import {Timer} from '../../../../src/timer';
import {AmpIframe, setTrackingIframeTimeoutForTesting} from '../amp-iframe';
import {
AmpIframe,
isAdLike,
setTrackingIframeTimeoutForTesting,
} from '../amp-iframe';
import {adopt} from '../../../../src/runtime';
import {
createIframePromise,
Expand Down Expand Up @@ -142,20 +146,23 @@ describe('amp-iframe', () => {
expect(amp.iframe.getAttribute('sandbox')).to.equal('');
expect(amp.iframe.parentNode).to.equal(amp.scrollWrapper);
expect(impl.looksLikeTrackingIframe_()).to.be.false;
expect(impl.getPriority()).to.equal(0);
return timer.promise(50).then(() => {
expect(ranJs).to.equal(0);
});
});
});

it('should allow JS and propagate scrolling', () => {
it('should allow JS and propagate scrolling and have lower priority', () => {
return getAmpIframe({
src: iframeSrc,
sandbox: 'allow-scripts',
width: 100,
height: 100,
width: 320,
height: 250,
scrolling: 'no',
}).then(amp => {
const impl = amp.container.implementation_;
expect(impl.getPriority()).to.equal(2);
expect(amp.iframe.getAttribute('sandbox')).to.equal('allow-scripts');
return waitForJsInIframe().then(() => {
expect(ranJs).to.equal(1);
Expand Down Expand Up @@ -519,4 +526,19 @@ describe('amp-iframe', () => {
expect(impl.looksLikeTrackingIframe_()).to.be.false;
});
});

it('should correctly classify ads', () => {
function e(width, height) {
const element = document.createElement('test');
element.setAttribute('width', width);
element.setAttribute('height', height);
return element;
}
expect(isAdLike(e(300, 250))).to.be.true;
expect(isAdLike(e(320, 270))).to.be.true;
expect(isAdLike(e(299, 249))).to.be.false;
expect(isAdLike(e(320, 100))).to.be.true;
expect(isAdLike(e(335, 100))).to.be.true;
expect(isAdLike(e(341, 100))).to.be.false;
});
});