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
Add getHtml support in AmpContext API. #12423
Conversation
test/integration/test-amp-ad-3p.js
Outdated
@@ -147,6 +163,18 @@ function createIframeWithApis(fixture) { | |||
return poll('wait for new IO entry', () => { | |||
return lastIO != null; | |||
}); | |||
}).then(() => { | |||
let resolver; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be simplified a lot:
// ...
}).then(() => new Promise((resolve, reject) => {
iframe.contentWindow.context.getHtml('a', ['href'], content => {
if (content == '<a href="http://test.com/test">Test link</a>') {
resolve();
} else {
reject(new Error('Invalid getHtml result: ' + content));
}
});
}));
I also made changes for a couple of nits here:
- No need for strict equality when comparing against a non-empty string.
- message should say "invalid" rather than "incorrect".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point.
Done.
test/integration/test-amp-ad-3p.js
Outdated
@@ -147,6 +163,16 @@ function createIframeWithApis(fixture) { | |||
return poll('wait for new IO entry', () => { | |||
return lastIO != null; | |||
}); | |||
}).then(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how about inlining the return as part of the fat arrow function? :)
.then(() => new Promise(resolve, reject) => {
// ...
}));
Nit: Invalid getHtml result
would be a better message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
3p/ampcontext.js
Outdated
@@ -177,6 +180,31 @@ export class AbstractAmpContext { | |||
return unlisten; | |||
}; | |||
|
|||
/** | |||
* Send message to runtime requesting HTML snippet of the parent window. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, missed this before.
A few doc nits:
- Description leaks implementation details. How about
Requests HTML snippet from the parent window.
? {!Array<string>}
- How about replacing
an array of tag attributes to be left in the stringified HTML
withWhitelist of attributes to be kept in the returned HTML string.
Callback to be invoked with the HTML string as the first parameter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. How about now?
* Add getHtml support in AmpContext API. * lint * revert fakead3p examples * revert fakead3p examples * revert fakead3p examples * More linter * Address comments * Address comments * Address comments * Address comments
Closes #12333