From 2d522e9d9a8f8f92229e8cccf3b30a439c8d1cbf Mon Sep 17 00:00:00 2001 From: Malte Ubl Date: Sat, 31 Oct 2015 14:34:52 -0700 Subject: [PATCH] Tune heuristic when to load ads. Instead of only loading them when they are in viewport, we also load them if they are outside the viewport: - if the user has ever scrolled (So we don't interfere with e.g. swiping through articles). - we haven't started loading another ad in the last second. In practice this should load a large percentage of ads before they enter the viewport while interference with scrolling is kept to a similar value as with the old heuristic. --- builtins/amp-ad.js | 30 +++++++++- src/viewport.js | 12 ++++ test/functional/test-amp-ad.js | 105 +++++++++++++++++++++++++++------ testing/iframe.js | 6 +- 4 files changed, 132 insertions(+), 21 deletions(-) diff --git a/builtins/amp-ad.js b/builtins/amp-ad.js index 06c5a899ebde..9ffb24b273d7 100644 --- a/builtins/amp-ad.js +++ b/builtins/amp-ad.js @@ -22,6 +22,7 @@ import {loadPromise} from '../src/event-helper'; import {registerElement} from '../src/custom-element'; import {getIframe, listen, prefetchBootstrap} from '../src/3p-frame'; import {adPrefetch, adPreconnect} from '../ads/_prefetch'; +import {timer} from '../src/timer'; /** @@ -98,11 +99,31 @@ export function upgradeImages_(images) { * @return {undefined} */ export function installAd(win) { + + /** + * @type {boolean} Heuristic boolean as for whether another ad is currently + * loading. + */ + let loadingAdsCount = 0; + class AmpAd extends BaseElement { /** @override */ renderOutsideViewport() { - return false; + // Before the user has scrolled we only render ads in view. This prevents + // excessive jank in situations like swiping through a lot of articles. + if (!this.getViewport().hasScrolled()) { + return false; + }; + + // If another ad is currently loading we only load ads that are currently + // in viewport. + if (loadingAdsCount > 0) { + return false; + } + + // Otherwise the ad is good to go. + return true; } /** @override */ @@ -205,6 +226,13 @@ export function installAd(win) { /** @override */ layoutCallback() { + loadingAdsCount++; + timer.delay(() => { + // Unfortunately we don't really have a good way to measure how long it + // takes to load an ad, so we'll just pretend it takes 1 second for + // now. + loadingAdsCount--; + }, 1000); assert(!this.isInFixedContainer_, ' is not allowed to be placed in elements with ' + 'position:fixed: %s', this.element); diff --git a/src/viewport.js b/src/viewport.js index c40aab71782c..c13bda948cf5 100644 --- a/src/viewport.js +++ b/src/viewport.js @@ -86,6 +86,9 @@ export class Viewport { /** @private {boolean} */ this.scrollTracking_ = false; + /** @private {number} */ + this.scrollCount_ = 0; + /** @private @const {!Observable} */ this.changeObservable_ = new Observable(); @@ -299,6 +302,14 @@ export class Viewport { return false; } + /** + * Returns whether the user has scrolled yet. + * @return {boolean} + */ + hasScrolled() { + return this.scrollCount_ > 0; + } + /** * Updates touch zoom meta data. Returns `true` if any actual * changes have been done. @@ -357,6 +368,7 @@ export class Viewport { /** @private */ scroll_() { + this.scrollCount_++; this.scrollObservable_.fire(); this.scrollLeft_ = this.binding_.getScrollLeft(); diff --git a/test/functional/test-amp-ad.js b/test/functional/test-amp-ad.js index fd813c469f70..6782f50ad144 100644 --- a/test/functional/test-amp-ad.js +++ b/test/functional/test-amp-ad.js @@ -15,29 +15,34 @@ */ import {createIframePromise} from '../../testing/iframe'; -import {installAd, scoreDimensions_, upgradeImages_} from +import {installAd, scoreDimensions_, upgradeImages_, AD_LOAD_TIME_MS} from '../../builtins/amp-ad'; +import {viewportFor} from + '../../src/viewport'; +import * as sinon from 'sinon'; describe('amp-ad', () => { - function getAd(attributes, canonical, opt_handleElement) { - return createIframePromise().then(iframe => { - installAd(iframe.win); - if (canonical) { - var link = iframe.doc.createElement('link'); - link.setAttribute('rel', 'canonical'); - link.setAttribute('href', canonical); - iframe.doc.head.appendChild(link); - } - var a = iframe.doc.createElement('amp-ad'); - for (var key in attributes) { - a.setAttribute(key, attributes[key]); - } - if (opt_handleElement) { - a = opt_handleElement(a); - } - return iframe.addElement(a); - }); + function getAd(attributes, canonical, opt_handleElement, + opt_beforeLayoutCallback) { + return createIframePromise(undefined, opt_beforeLayoutCallback) + .then(iframe => { + installAd(iframe.win); + if (canonical) { + var link = iframe.doc.createElement('link'); + link.setAttribute('rel', 'canonical'); + link.setAttribute('href', canonical); + iframe.doc.head.appendChild(link); + } + var a = iframe.doc.createElement('amp-ad'); + for (var key in attributes) { + a.setAttribute(key, attributes[key]); + } + if (opt_handleElement) { + a = opt_handleElement(a); + } + return iframe.addElement(a); + }); } it('render an ad', () => { @@ -189,4 +194,66 @@ describe('amp-ad', () => { expect(images['320x50'][0]).to.equal('backfill-6@2x.png'); }); }); + + describe('renderOutsideViewport', () => { + + let clock; + let sandbox; + + afterEach(() => { + if (clock) { + clock.restore(); + } + if (sandbox) { + sandbox.restore(); + } + }); + + function getGoodAd(cb, layoutCb) { + return getAd({ + width: 300, + height: 250, + type: 'a9', + src: 'https://testsrc', + 'data-aax_size': '300x250', + 'data-aax_pubname': 'test123', + 'data-aax_src': '302', + // Test precedence + 'data-width': '6666' + }, 'https://schema.org', element => { + cb(element.implementation_); + return element; + }, layoutCb); + } + + it('should return false before scrolling', () => { + return getGoodAd(ad => { + expect(ad.renderOutsideViewport()).to.be.false; + }); + }); + + it('should return true after scrolling', () => { + return getGoodAd(ad => { + viewportFor(ad.element.ownerDocument.defaultView).scrollCount_++; + expect(ad.renderOutsideViewport()).to.be.true; + }); + }); + + it('should return true after scrolling and then false for 1s', () => { + return getGoodAd(ad => { + viewportFor(ad.element.ownerDocument.defaultView).scrollCount_++; + expect(ad.renderOutsideViewport()).to.be.true; + }, () => { + sandbox = sinon.sandbox.create(); + clock = sandbox.useFakeTimers(); + }).then(ad => { + // False because we just rendered one. + expect(ad.renderOutsideViewport()).to.be.false; + clock.tick(900); + expect(ad.renderOutsideViewport()).to.be.false; + clock.tick(100); + expect(ad.renderOutsideViewport()).to.be.true; + }); + }); + }); }); diff --git a/testing/iframe.js b/testing/iframe.js index 6389cb6490c0..99195f18b0e4 100644 --- a/testing/iframe.js +++ b/testing/iframe.js @@ -145,6 +145,7 @@ export function createFixtureIframe(fixture, initialIframeHeight, done) { * that element. When the promise is resolved we will have called the entire * lifecycle including layoutCallback. * @param {boolean=} opt_runtimeOff Whether runtime should be turned off. + * @param {function()=} opt_beforeLayoutCallback * @return {!Promise<{ * win: !Window, * doc: !Document, @@ -152,7 +153,7 @@ export function createFixtureIframe(fixture, initialIframeHeight, done) { * addElement: function(!Element):!Promise * }>} */ -export function createIframePromise(opt_runtimeOff) { +export function createIframePromise(opt_runtimeOff, opt_beforeLayoutCallback) { return new Promise(function(resolve, reject) { var iframe = document.createElement('iframe'); iframe.name = 'test_' + iframeCount++; @@ -179,6 +180,9 @@ export function createIframePromise(opt_runtimeOff) { element.style.display = 'block'; element.build(true); if (element.layoutCount_ == 0) { + if (opt_beforeLayoutCallback) { + opt_beforeLayoutCallback(); + } element.layoutCallback(); } return element;