Creative API: resize - attempt 2 #4236
Conversation
…-size messages. Added a test using a nested frame to verify that these messages are received correctly.
| }); | ||
| updateSize_(height, width, source, origin) { | ||
| this.baseInstance_.attemptChangeSize(height, width, | ||
| () => this.sendEmbedSizeResponse_( |
There was a problem hiding this comment.
#attemptChangeSize doesn't take a success and failure callback, it returns a Promise. You'll have to put these in a #then:
attemptChangeSize(height, width).then(success, failure);
There was a problem hiding this comment.
You seem to be correct, but that is not what the code at head is doing: https://github.com/ampproject/amphtml/blob/master/extensions/amp-ad/0.1/amp-ad-api-handler.js#L159.
Is the code at head actually broken?
There was a problem hiding this comment.
Is the code at head actually broken?
😭. @zhouyx
There was a problem hiding this comment.
😢 My bad. messed up on rebase, #4258
There was a problem hiding this comment.
This should be fixed now. Please rebase.
|
Re: #4023 |
| success, requestedWidth, requestedHeight, source, origin) { | ||
| postMessageToWindows( | ||
| this.iframe_, | ||
| [{win: source, origin}], |
There was a problem hiding this comment.
origin & source should already be present in the event object right? why do we have to pass them around?
There was a problem hiding this comment.
We need to tell postMessageToWindows which windows to send the message to. With this change we are no longer messaging only the contentWindow of the iframe, but potentially also any of the child windows within that iframe. That's the point of the change.
There was a problem hiding this comment.
forgive my ignorance. Why do other windows care about resize response? only the requestor should benefit from it right?
There was a problem hiding this comment.
I think this is for a nested child iframe to send message to its ancestor.
There was a problem hiding this comment.
So, I'm joining this a bit late and I see (most likely) the original GitHub issue in #3019. Could you please catch me up:
- Is Intent to implement: postMessage API for 3rd party iframes #3019 indeed the origin issue?
- Why did we decide to allow
amp-adto communicate with child frames directly vs proxying these messages with 3p iframe (most immediate child ofamp-ad)? I'm not saying that this is wrong, I'd just like to get into right context on this.
There was a problem hiding this comment.
Indeed, #3019 is the issue that this PR pertains to.
Direct communication achieves a few different things:
- It offers better performance, especially for things like IntersectionObserver emulation, as it avoids adding forwarding. It also makes it easier to handle double-nesting without adding extra complexity.
- It supports A4A use case where the returned ad is a non-AMP creative. In that case the ad is rendered in a 3P iframe, but it does not have access to the normal window.context. With the current implementation of messaging there will be no extra work required to support IntersectionObserver or resizing in that case.
Note that the nested iframes can only communicate if they know a per-3P-frame unique sentinel value. This sentinel is available within the frame's contextWindow and has to be explicitly passed to any child iframes (i.e. it functions as an authorization token).
FYI, the underlying changes to the messaging code happened some time ago in #3327 along with the changes for IntersectionObserver and for page visibility in #4021.
There was a problem hiding this comment.
@nekodo Thanks. This all sounds good.
@sriramkrish85 I'm ok with these changes.
|
LGTM. Defer to @sriramkrish85 for the final approval. |
|
This'll need to be rebased after #4258. |
|
Ping @lannka. |
|
LGTM. |
|
@nekodo let me know if you want me to merge this. |
|
@sriramkrish85 Yes please:) |
* Changed <amp-ad> to allow nested frames to request resize using embed-size messages. Added a test using a nested frame to verify that these messages are received correctly. * Fixed function name in a test. * Made linter happy. * Refactored a bit around sendEmbedContext. * OK lint, you win again. * Begone lint errors! * Switched to use the returned Promise from attemptChangeSize.
* Changed <amp-ad> to allow nested frames to request resize using embed-size messages. Added a test using a nested frame to verify that these messages are received correctly. * Fixed function name in a test. * Made linter happy. * Refactored a bit around sendEmbedContext. * OK lint, you win again. * Begone lint errors! * Switched to use the returned Promise from attemptChangeSize.
Adds support for requesting resize using embed-size messages from nested frames. This time should dodge the issues my previous patch had by no longer having the weird "fail the pending request" logic since attemptChangeSize is now more reasonable and takes a failure callback.