Skip to content

Commit

Permalink
Separate render-start and loaded phases for Ads (ampproject#7391)
Browse files Browse the repository at this point in the history 
* Separate render-start and loaded phases for Ads

* lints

* fixes

* Process bootstrap-start event

* fixes

* fixes

* remove visibility expectation from load promise

* lints

* review fixes

* lints

* sticky-ad-0.1
  • Loading branch information
Dima Voytenko authored Feb 9, 2017
1 parent 9e4c54a commit 8fc6792
Show file tree
Hide file tree
Showing 12 changed files with 406 additions and 133 deletions.
119 changes: 73 additions & 46 deletions extensions/amp-ad/0.1/amp-ad-xorigin-iframe-handler.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,14 +25,15 @@ import {
IntersectionObserver,
} from '../../../src/intersection-observer';
import {viewerForDoc} from '../../../src/viewer';
import {dev, user} from '../../../src/log';
import {dev} from '../../../src/log';
import {timerFor} from '../../../src/timer';
import {setStyle} from '../../../src/style';
import {loadPromise} from '../../../src/event-helper';
import {AdDisplayState} from './amp-ad-ui';
import {getHtml} from '../../../src/get-html';

const TIMEOUT_VALUE = 10000;
const VISIBILITY_TIMEOUT = 10000;


export class AmpAdXOriginIframeHandler {

Expand Down Expand Up @@ -64,9 +65,6 @@ export class AmpAdXOriginIframeHandler {

/** @private @const {!../../../src/service/viewer-impl.Viewer} */
this.viewer_ = viewerForDoc(this.baseInstance_.getAmpDoc());

/** @private {?Promise} */
this.adResponsePromise_ = null;
}

/**
Expand All @@ -81,6 +79,7 @@ export class AmpAdXOriginIframeHandler {
this.iframe = iframe;
this.iframe.setAttribute('scrolling', 'no');
this.baseInstance_.applyFillContent(this.iframe);
const timer = timerFor(this.baseInstance_.win);

// Init IntersectionObserver service.
this.intersectionObserver_ = new IntersectionObserver(
Expand Down Expand Up @@ -125,34 +124,58 @@ export class AmpAdXOriginIframeHandler {
this.sendEmbedInfo_(this.baseInstance_.isInViewport());
}));

// Iframe.onload normally called by the Ad after full load.
const iframeLoadPromise = loadPromise(this.iframe).then(() => {
// Wait just a little to allow `no-content` message to arrive.
return timer.promise(10);
});
if (this.baseInstance_.emitLifecycleEvent) {
// Only set up a load listener if we know that we can send lifecycle
// messages.
loadPromise(this.iframe).then(() => {
iframeLoadPromise.then(() => {
this.baseInstance_.emitLifecycleEvent('xDomIframeLoaded');
});
}

// Install API that listens to ad response
if (this.baseInstance_.config
&& this.baseInstance_.config.renderStartImplemented) {
// If support render-start, create a race between render-start no-content
this.adResponsePromise_ = listenForOncePromise(this.iframe,
['render-start', 'no-content'], true).then(info => {
const data = info.data;
if (data.type == 'render-start') {
this.renderStart_(info);
} else {
this.noContent_();
}
});
// Calculate render-start and no-content signals.
let renderStartResolve;
const renderStartPromise = new Promise(resolve => {
renderStartResolve = resolve;
});
let noContentResolve;
const noContentPromise = new Promise(resolve => {
noContentResolve = resolve;
});
if (this.baseInstance_.config &&
this.baseInstance_.config.renderStartImplemented) {
// When `render-start` is supported, these signals are mutually
// exclusive. Whichever arrives first wins.
listenForOncePromise(this.iframe,
['render-start', 'no-content'], true).then(info => {
const data = info.data;
if (data.type == 'render-start') {
this.renderStart_(info);
renderStartResolve();
} else {
this.noContent_();
noContentResolve();
}
});
} else {
// If NOT support render-start, listen to bootstrap-loaded no-content
// respectively
this.adResponsePromise_ = listenForOncePromise(this.iframe,
'bootstrap-loaded', true);
listenForOncePromise(this.iframe, 'no-content', true)
.then(() => this.noContent_());
// If `render-start` is not supported, listen to `bootstrap-loaded`.
// This will avoid keeping the Ad empty until it's fully loaded, which
// could be a long time.
listenForOncePromise(this.iframe, 'bootstrap-loaded', true).then(() => {
this.renderStart_();
renderStartResolve();
});
// Likewise, no-content is observed here. However, it's impossible to
// assure exclusivity between `no-content` and `bootstrap-loaded` b/c
// `bootstrap-loaded` always arrives first.
listenForOncePromise(this.iframe, 'no-content', true).then(() => {
this.noContent_();
noContentResolve();
});
}

if (opt_isA4A) {
Expand All @@ -161,37 +184,41 @@ export class AmpAdXOriginIframeHandler {
this.element_.appendChild(this.iframe);
return Promise.resolve();
}
// Set iframe initially hidden which will be removed on load event +
// post message.

// Set iframe initially hidden which will be removed on render-start or
// load, whichever is earlier.
setStyle(this.iframe, 'visibility', 'hidden');
this.element_.appendChild(this.iframe);
Promise.race([
renderStartPromise,
iframeLoadPromise,
timer.promise(VISIBILITY_TIMEOUT),
]).then(() => {
if (this.iframe) {
setStyle(this.iframe, 'visibility', '');
if (this.baseInstance_.emitLifecycleEvent) {
this.baseInstance_.emitLifecycleEvent('adSlotUnhidden');
}
}
});

return timerFor(this.baseInstance_.win).timeoutPromise(TIMEOUT_VALUE,
this.adResponsePromise_,
'timeout waiting for ad response').catch(e => {
this.noContent_();
user().warn('AMP-AD', e);
}).then(() => {
if (this.iframe) {
setStyle(this.iframe, 'visibility', '');
if (this.baseInstance_.emitLifecycleEvent) {
this.baseInstance_.emitLifecycleEvent('adSlotUnhidden');
}
}
});
// The actual ad load is eariliest of iframe.onload event and no-content.
return Promise.race([iframeLoadPromise, noContentPromise]);
}

/**
* callback functon on receiving render-start
* @param {!Object} info
* @param {!Object=} opt_info
* @private
*/
renderStart_(info) {
const data = info.data;
renderStart_(opt_info) {
this.uiHandler_.setDisplayState(AdDisplayState.LOADED_RENDER_START);
this.updateSize_(data.height, data.width,
info.source, info.origin);
this.baseInstance_.signals().signal('render-start');
this.baseInstance_.renderStarted();
if (!opt_info) {
return;
}
const data = opt_info.data;
this.updateSize_(data.height, data.width, opt_info.source, opt_info.origin);
if (this.baseInstance_.emitLifecycleEvent) {
this.baseInstance_.emitLifecycleEvent('renderCrossDomainStart');
}
Expand Down
80 changes: 48 additions & 32 deletions extensions/amp-ad/0.1/test/test-amp-ad-xorigin-iframe-handler.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ describe('amp-ad-xorigin-iframe-handler', () => {
let sandbox;
let adImpl;
let signals;
let renderStartedSpy;
let iframeHandler;
let iframe;
let testIndex = 0;
Expand All @@ -45,6 +46,11 @@ describe('amp-ad-xorigin-iframe-handler', () => {
};
signals = new Signals();
adElement.signals = () => signals;
renderStartedSpy = sandbox.spy();
adElement.renderStarted = () => {
renderStartedSpy();
signals.signal('render-start');
};
adImpl = new BaseElement(adElement);
adImpl.getFallback = () => {
return null;
Expand Down Expand Up @@ -84,24 +90,34 @@ describe('amp-ad-xorigin-iframe-handler', () => {
initPromise = iframeHandler.init(iframe);
});

it('should resolve on iframe.onload', () => {
expect(iframe.style.visibility).to.equal('hidden');
return initPromise.then(() => {
expect(iframe.style.visibility).to.equal('');
expect(renderStartedSpy).to.not.be.called;
expect(signals.get('render-start')).to.be.null;
});
});

it('should resolve on message "render-start"', () => {
expect(iframe.style.visibility).to.equal('hidden');
iframe.postMessageToParent({
sentinel: 'amp3ptest' + testIndex,
type: 'render-start',
});
return initPromise.then(() => {
const renderStartPromise = signals.whenSignal('render-start');
return Promise.all([renderStartPromise, initPromise]).then(() => {
expect(iframe.style.visibility).to.equal('');
expect(signals.get('render-start')).to.be.ok;
expect(renderStartedSpy).to.be.calledOnce;
}).then(() => {
iframe.postMessageToParent({
sentinel: 'amp3ptest' + testIndex,
type: 'no-content',
});
return expectPostMessage(iframe.contentWindow, window, {
const message = {
sentinel: 'amp3ptest' + testIndex,
type: 'no-content',
}).then(() => {
};
const promise = expectPostMessage(
iframe.contentWindow, window, message);
iframe.postMessageToParent(message);
return promise.then(() => {
expect(noContentSpy).to.not.been.called;
});
});
Expand All @@ -115,14 +131,19 @@ describe('amp-ad-xorigin-iframe-handler', () => {
type: 'render-start',
sentinel: 'amp3ptest' + testIndex,
});
return initPromise.then(() => {
const expectResponsePromise = iframe.expectMessageFromParent(
'amp-' + JSON.stringify({
requestedWidth: 114,
requestedHeight: 217,
type: 'embed-size-changed',
sentinel: 'amp3ptest' + testIndex,
}));
const renderStartPromise = signals.whenSignal('render-start');
return Promise.all([renderStartPromise, initPromise]).then(() => {
return initPromise;
}).then(() => {
expect(iframe.style.visibility).to.equal('');
return iframe.expectMessageFromParent('amp-' + JSON.stringify({
requestedWidth: 114,
requestedHeight: 217,
type: 'embed-size-changed',
sentinel: 'amp3ptest' + testIndex,
}));
return expectResponsePromise;
});
});

Expand All @@ -147,7 +168,6 @@ describe('amp-ad-xorigin-iframe-handler', () => {
type: 'no-content',
});
return initPromise.then(() => {
expect(iframe.style.visibility).to.equal('');
expect(noContentSpy).to.be.calledOnce;
expect(noContentSpy).to.be.calledWith(true);
});
Expand Down Expand Up @@ -175,9 +195,8 @@ describe('amp-ad-xorigin-iframe-handler', () => {
});
});

it('should resolve on message "bootstrap-loaded" if render-start is'
+ 'NOT implemented', done => {

it('should trigger render-start on message "bootstrap-loaded" if' +
' render-start is NOT implemented', done => {
initPromise = iframeHandler.init(iframe);
iframe.onload = () => {
expect(iframe.style.visibility).to.equal('hidden');
Expand All @@ -187,27 +206,24 @@ describe('amp-ad-xorigin-iframe-handler', () => {
});
initPromise.then(() => {
expect(iframe.style.visibility).to.equal('');
expect(renderStartedSpy).to.be.calledOnce;
done();
});
};
});

it('should resolve on timeout', done => {
const noContentSpy =
sandbox.spy/*OK*/(iframeHandler, 'freeXOriginIframe');
it('should trigger visibility on timeout', done => {
const clock = sandbox.useFakeTimers();

iframe.name = 'test_master';
initPromise = iframeHandler.init(iframe);
clock.tick(9999);
expect(noContentSpy).to.not.be.called;
clock.tick(1);
initPromise.then(() => {
expect(iframe.style.visibility).to.equal('');
expect(noContentSpy).to.be.calledOnce;
expect(noContentSpy).to.be.calledWith(true);
done();
});
iframe.onload = () => {
clock.tick(10000);
initPromise.then(() => {
expect(iframe.style.visibility).to.equal('');
expect(renderStartedSpy).to.not.be.called;
done();
});
};
});

it('should resolve directly if it is A4A', () => {
Expand Down
26 changes: 15 additions & 11 deletions extensions/amp-sticky-ad/0.1/amp-sticky-ad.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ import {Layout} from '../../../src/layout';
import {dev,user} from '../../../src/log';
import {removeElement} from '../../../src/dom';
import {toggle} from '../../../src/style';
import {listenOnce} from '../../../src/event-helper';

class AmpStickyAd extends AMP.BaseElement {
/** @param {!AmpElement} element */
Expand Down Expand Up @@ -141,18 +140,15 @@ class AmpStickyAd extends AMP.BaseElement {
}

/**
* Function that check if ad has been built
* If not, wait for the amp:built event
* otherwise schedule layout for ad.
* Function that check if ad has been built. If not, wait for the 'built'
* signal. Otherwise schedule layout for ad.
* @private
*/
scheduleLayoutForAd_() {
if (this.ad_.isBuilt()) {
this.layoutAd_();
} else {
listenOnce(dev().assertElement(this.ad_), 'amp:built', () => {
this.layoutAd_();
});
this.ad_.whenBuilt().then(this.layoutAd_.bind(this));
}
}

Expand All @@ -161,10 +157,18 @@ class AmpStickyAd extends AMP.BaseElement {
* @private
*/
layoutAd_() {
this.updateInViewport(dev().assertElement(this.ad_), true);
this.scheduleLayout(dev().assertElement(this.ad_));
listenOnce(dev().assertElement(this.ad_), 'amp:load:end', () => {
this.vsync_.mutate(() => {
const ad = dev().assertElement(this.ad_);
this.updateInViewport(ad, true);
this.scheduleLayout(ad);
// Wait for the earliest: `render-start` or `load-end` signals.
// `render-start` is expected to arrive first, but it's not emitted by
// all types of ads.
const signals = ad.signals();
return Promise.race([
signals.whenSignal('render-start'),
signals.whenSignal('load-end'),
]).then(() => {
return this.vsync_.mutatePromise(() => {
// Set sticky-ad to visible and change container style
this.element.setAttribute('visible', '');
this.element.classList.add('amp-sticky-ad-loaded');
Expand Down
Loading

0 comments on commit 8fc6792

Please sign in to comment.