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

Allow embed to have services that its parent lacks #10122

Merged
merged 2 commits into from Jun 27, 2017
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
16 changes: 12 additions & 4 deletions examples/a4a.amp.html
Expand Up @@ -82,10 +82,18 @@ <h2>GMOSSP ad network</h2>
<div fallback></div>
</amp-ad>

<h2>Doubleclick</h2>
<amp-ad width="320" height="50"
type="doubleclick"
data-slot="/4119129/mobile_ad_banner">
<h2>Regular ad</h2>
<amp-ad data-slot="/30497360/a4a/non_a4a_native"
height="250"
type="doubleclick"
width="300">
</amp-ad>

<h2>A4A ad</h2>
<amp-ad data-slot="/30497360/a4a/a4a_native"
height="250"
type="doubleclick"
width="300">
</amp-ad>
</body>
</html>
18 changes: 12 additions & 6 deletions src/service.js
Expand Up @@ -241,6 +241,7 @@ export function getServicePromiseOrNull(win, id) {

/**
* Returns a service for the given id and ampdoc (a per-ampdoc singleton).
* Expects service `id` to be registered.
* @param {!Node|!./service/ampdoc-impl.AmpDoc} nodeOrDoc
* @param {string} id of the service.
* @return {T}
Expand Down Expand Up @@ -601,8 +602,8 @@ export function isEmbeddable(service) {
*/
export function adoptServiceForEmbed(embedWin, serviceId) {
const adopted = adoptServiceForEmbedIfEmbeddable(embedWin, serviceId);
dev().assert(adopted,
`${serviceId} required to implement EmbeddableService.`);
dev().assert(adopted, `Service ${serviceId} not found on parent ` +
'or doesn\'t implement EmbeddableService.');
}


Expand All @@ -617,12 +618,17 @@ export function adoptServiceForEmbedIfEmbeddable(embedWin, serviceId) {
const frameElement = /** @type {!Node} */ (dev().assert(
embedWin.frameElement,
'frameElement not found for embed'));
const ampdoc = getAmpdoc(frameElement);
const holder = getAmpdocServiceHolder(ampdoc);
if (!isServiceRegistered(holder, serviceId)) {
return false;
}
const service = getServiceForDoc(frameElement, serviceId);
if (isEmbeddable(service)) {
service.adoptEmbedWindow(embedWin);
return true;
if (!isEmbeddable(service)) {
return false;
}
return false;
service.adoptEmbedWindow(embedWin);
return true;
}


Expand Down
50 changes: 31 additions & 19 deletions test/functional/test-service.js
Expand Up @@ -568,30 +568,42 @@ describe('service', () => {
expect(isEmbeddable(nonEmbeddable)).to.be.false;
});

it('should adopt embeddable', () => {
adoptServiceForEmbed(embedWin, 'embeddable');
expect(embeddable.adoptEmbedWindow).to.be.calledOnce;
expect(embeddable.adoptEmbedWindow.args[0][0]).to.equal(embedWin);
});
describe('adoptServiceForEmbed()', () => {
it('should adopt embeddable', () => {
adoptServiceForEmbed(embedWin, 'embeddable');
expect(embeddable.adoptEmbedWindow).to.be.calledOnce;
expect(embeddable.adoptEmbedWindow.args[0][0]).to.equal(embedWin);
});

it('should refuse adopt of non-embeddable', () => {
expect(() => {
adoptServiceForEmbed(embedWin, 'nonEmbeddable');
}).to.throw(/implement EmbeddableService/);
});

it('should refuse adopt of non-embeddable', () => {
expect(() => {
adoptServiceForEmbed(embedWin, 'nonEmbeddable');
}).to.throw(/required to implement EmbeddableService/);
it('should refuse adopt of unknown service', () => {
expect(() => {
adoptServiceForEmbed(embedWin, 'unknown');
}).to.throw(/unknown/);
});
});

it('should adopt embeddable if embeddable', () => {
adoptServiceForEmbedIfEmbeddable(embedWin, 'embeddable');
expect(embeddable.adoptEmbedWindow).to.be.calledOnce;
expect(embeddable.adoptEmbedWindow.args[0][0]).to.equal(embedWin);
describe('adoptServiceForServiceIfEmbeddable()', () => {
it('should adopt embeddable if embeddable', () => {
adoptServiceForEmbedIfEmbeddable(embedWin, 'embeddable');
expect(embeddable.adoptEmbedWindow).to.be.calledOnce;
expect(embeddable.adoptEmbedWindow.args[0][0]).to.equal(embedWin);

adoptServiceForEmbedIfEmbeddable(embedWin, 'nonEmbeddable'); // No-op.
});
// Should not throw 'required to implement EmbeddableService' error.
adoptServiceForEmbedIfEmbeddable(embedWin, 'nonEmbeddable');
});

it('should refuse adopt of unknown service', () => {
expect(() => {
adoptServiceForEmbed(embedWin, 'unknown');
}).to.throw(/unknown/);
it('should soft fail adopt of unknown service', () => {
// If the embed has an extension service that the parent doesn't,
// e.g. AmpFormService if parent lacks amp-form extension.
const adopted = adoptServiceForEmbedIfEmbeddable(embedWin, 'unknown');
expect(adopted).to.be.false;
});
});
});
});
Expand Down