Skip to content

Commit

Permalink
A4a 3p remove load from init (ampproject#5936)
Browse files Browse the repository at this point in the history 
* Update amp-ad-network-doubleclick-impl.md

* Reverting doc changes

The previous change was made in error (wrong branch).

* Remove load listener from AmpAdXOriginIframeHandler.init

* Correct conflict

* PR feedback
  • Loading branch information
@keithwrightbos
keithwrightbos authored and Vanessa Pasque committed Dec 22, 2016
1 parent 87c9bbd commit c4aaf9d
Show file tree
Hide file tree
Showing 4 changed files with 45 additions and 67 deletions.
10 changes: 1 addition & 9 deletions extensions/amp-a4a/0.1/amp-a4a.js
View file
Expand Up @@ -508,7 +508,6 @@ export class AmpA4A extends AMP.BaseElement {
// creatives which rendered via the buildCallback promise chain. Ensure
// slot counts towards 3p loading count until we know that the creative is
// valid AMP.
this.emitLifecycleEvent('preAdThrottle');
return this.adPromise_.then(rendered => {
if (rendered instanceof Error || !rendered) {
this.emitLifecycleEvent('preAdThrottle');
Expand Down Expand Up @@ -795,14 +794,7 @@ export class AmpA4A extends AMP.BaseElement {
// TODO(keithwrightbos): noContentCallback?
this.xOriginIframeHandler_ = new AMP.AmpAdXOriginIframeHandler(this);
this.rendered_ = true;
// Set opt_defaultVisible to true as 3p draw code never executed causing
// render-start event never to fire which will remove visiblity hidden.
const handlerPromise = this.xOriginIframeHandler_.init(
iframe, /* opt_isA4A */ true);
if (getMode().localDev || getMode().test) {
this.onCrossDomainIframeCreated(iframe);
}
return handlerPromise;
return this.xOriginIframeHandler_.init(iframe, /* opt_isA4A */ true);
}

/**
Expand Down
26 changes: 9 additions & 17 deletions extensions/amp-a4a/0.1/test/test-amp-a4a.js
View file
Expand Up @@ -128,18 +128,10 @@ describe('amp-a4a', () => {
return element;
}

function verifyNonAMPRender(a4a, win) {
function verifyNonAMPRender(a4a) {
a4a.onAmpCreativeRender = () => {
assert.fail('AMP creative should never have rendered!');
};
a4a.onCrossDomainIframeCreated = iframe => {
// Iframe should be hidden at time of creation and only made visible
// after load.
expect(isStyleVisible(win, iframe)).to.be.false;
iframe.onload = () => {
expect(isStyleVisible(win, iframe)).to.be.true;
};
};
}

/**
Expand Down Expand Up @@ -175,7 +167,7 @@ describe('amp-a4a', () => {
});

it('for SafeFrame rendering case', () => {
verifyNonAMPRender(a4a, fixture.win);
verifyNonAMPRender(a4a);
// Make sure there's no signature, so that we go down the 3p iframe path.
mockResponse.headers.delete('X-Google-header');
// If rendering type is safeframe, we SHOULD attach a SafeFrame.
Expand All @@ -193,7 +185,7 @@ describe('amp-a4a', () => {
});

it('for cached content iframe rendering case', () => {
verifyNonAMPRender(a4a, fixture.win);
verifyNonAMPRender(a4a);
// Make sure there's no signature, so that we go down the 3p iframe path.
mockResponse.headers.delete('X-Google-header');
fixture.doc.body.appendChild(a4aElement);
Expand Down Expand Up @@ -280,7 +272,7 @@ describe('amp-a4a', () => {
a4aElement.setAttribute('height', 50);
a4aElement.setAttribute('type', 'adsense');
const a4a = new MockA4AImpl(a4aElement);
verifyNonAMPRender(a4a, fixture.win);
verifyNonAMPRender(a4a);
doc.body.appendChild(a4aElement);
a4a.onLayoutMeasure();
return a4a.layoutCallback().then(() => {
Expand Down Expand Up @@ -315,7 +307,7 @@ describe('amp-a4a', () => {
a4aElement.setAttribute('height', 50);
a4aElement.setAttribute('type', 'adsense');
const a4a = new MockA4AImpl(a4aElement);
verifyNonAMPRender(a4a, fixture.win);
verifyNonAMPRender(a4a);
doc.body.appendChild(a4aElement);
a4a.onLayoutMeasure();
return a4a.layoutCallback().then(() => {
Expand Down Expand Up @@ -464,7 +456,7 @@ describe('amp-a4a', () => {
a4aElement.setAttribute('height', 50);
a4aElement.setAttribute('type', 'adsense');
const a4a = new MockA4AImpl(a4aElement);
verifyNonAMPRender(a4a, fixture.win);
verifyNonAMPRender(a4a);
const getAdUrlSpy = sandbox.spy(a4a, 'getAdUrl');
sandbox.stub(a4a, 'extractCreativeAndSignature').returns(
Promise.resolve({creative: mockResponse.arrayBuffer()}));
Expand Down Expand Up @@ -570,7 +562,7 @@ describe('amp-a4a', () => {
a4aElement.setAttribute('type', 'adsense');
const a4a = new MockA4AImpl(a4aElement);
const getAdUrlSpy = sandbox.spy(a4a, 'getAdUrl');
verifyNonAMPRender(a4a, fixture.win);
verifyNonAMPRender(a4a);
a4a.onLayoutMeasure();
expect(a4a.adPromise_).to.be.instanceof(Promise);
return a4a.layoutCallback().then(() => {
Expand All @@ -592,7 +584,7 @@ describe('amp-a4a', () => {
const doc = fixture.doc;
const a4aElement = createA4aElement(doc);
const a4a = new MockA4AImpl(a4aElement);
verifyNonAMPRender(a4a, fixture.win);
verifyNonAMPRender(a4a);
a4a.onLayoutMeasure();
return a4a.adPromise_.then(() => a4a.layoutCallback().then(() => {
a4a.vsync_.runScheduledTasks_();
Expand All @@ -614,7 +606,7 @@ describe('amp-a4a', () => {
const doc = fixture.doc;
const a4aElement = createA4aElement(doc);
const a4a = new MockA4AImpl(a4aElement);
verifyNonAMPRender(a4a, fixture.win);
verifyNonAMPRender(a4a);
a4a.onLayoutMeasure();
const layoutCallbackPromise = a4a.layoutCallback();
rejectXhr(new Error('XHR Error'));
Expand Down
46 changes: 24 additions & 22 deletions extensions/amp-ad/0.1/amp-ad-xorigin-iframe-handler.js
View file
Expand Up @@ -15,7 +15,6 @@
*/

import {removeElement} from '../../../src/dom';
import {loadPromise} from '../../../src/event-helper';
import {
SubscriptionApi,
listenFor,
Expand Down Expand Up @@ -69,7 +68,7 @@ export class AmpAdXOriginIframeHandler {
* Sets up listeners and iframe state for iframe containing ad creative.
* @param {!Element} iframe
* @param {boolean=} opt_isA4A when true do not listen to ad response
* @return {!Promise} awaiting load event for ad frame
* @return {!Promise} awaiting render complete promise
* @suppress {checkTypes} // TODO(tdrl): Temporary, for lifecycleReporter.
*/
init(iframe, opt_isA4A) {
Expand All @@ -96,10 +95,19 @@ export class AmpAdXOriginIframeHandler {
this.updateSize_(data.height, data.width, source, origin);
}, true, true));

// Install API that listen to ad response
this.unlisteners_.push(this.viewer_.onVisibilityChanged(() => {
this.sendEmbedInfo_(this.baseInstance_.isInViewport());
}));

if (opt_isA4A) {
this.adResponsePromise_ = Promise.resolve();
} else if (this.baseInstance_.config
// A4A writes creative frame directly to page therefore does not expect
// post message to unset visibility hidden
this.element_.appendChild(this.iframe);
return Promise.resolve();
}

// Install API that listen 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,
Expand All @@ -121,26 +129,20 @@ export class AmpAdXOriginIframeHandler {
}

// Set iframe initially hidden which will be removed on load event +
// post message depending on if A4A flow.
// post message.
this.iframe.style.visibility = 'hidden';

this.unlisteners_.push(this.viewer_.onVisibilityChanged(() => {
this.sendEmbedInfo_(this.baseInstance_.isInViewport());
}));

this.element_.appendChild(this.iframe);
return loadPromise(this.iframe).then(() => {
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) {
this.iframe.style.visibility = '';
}
});
});
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) {
this.iframe.style.visibility = '';
}
});
}

/**
Expand Down
30 changes: 11 additions & 19 deletions extensions/amp-ad/0.1/test/test-amp-ad-xorigin-iframe-handler.js
View file
Expand Up @@ -61,7 +61,7 @@ describe('amp-ad-xorigin-iframe-handler', () => {

let noContentSpy;

beforeEach(done => {
beforeEach(() => {
adImpl.config = {renderStartImplemented: true};
sandbox.stub(adImpl, 'attemptChangeSize', (height, width) => {
expect(height).to.equal(217);
Expand All @@ -72,9 +72,6 @@ describe('amp-ad-xorigin-iframe-handler', () => {
sandbox.spy/*OK*/(iframeHandler, 'freeXOriginIframe');

initPromise = iframeHandler.init(iframe);
iframe.onload = () => {
done();
};
});

it('should resolve on message "render-start"', () => {
Expand Down Expand Up @@ -191,17 +188,15 @@ describe('amp-ad-xorigin-iframe-handler', () => {

iframe.name = 'test_master';
initPromise = iframeHandler.init(iframe);
iframe.onload = () => {
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();
});
};
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();
});
});

it('should resolve directly if it is A4A', () => {
Expand All @@ -213,11 +208,8 @@ describe('amp-ad-xorigin-iframe-handler', () => {

describe('Initialized iframe', () => {

beforeEach(done => {
beforeEach(() => {
iframeHandler.init(iframe);
iframe.onload = () => {
done();
};
});

it('should be able to use embed-state API', () => {
Expand Down

0 comments on commit c4aaf9d

Please sign in to comment.