Merge pull request #819 from cramforce/prefetch

Tune heuristic when to load ads.

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_,
           '<amp-ad> is not allowed to be placed in elements with ' +
           'position:fixed: %s', this.element);

src/viewport.js

@@ -86,6 +86,9 @@ export class Viewport {
     /** @private {boolean} */
     this.scrollTracking_ = false;
 
+    /** @private {number} */
+    this.scrollCount_ = 0;
+
     /** @private @const {!Observable<!ViewportChangedEvent>} */
     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();

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;
+      });
+    });
+  });
 });

testing/iframe.js

@@ -145,14 +145,15 @@ 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,
  *   iframe: !Element,
  *   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;