Changes to postMessage code in AMP for nested iframe communication #3327
Conversation
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed, please reply here (e.g.
|
| for (let win = parentWindow; win != window.top; win = win.parent) { | ||
| windowDepth++; | ||
| } | ||
| return '' + windowDepth + '-' + Math.round(Math.random() * 1000000); |
There was a problem hiding this comment.
You could just share this code:
https://github.com/ampproject/amphtml/blob/master/src/3p-frame.js#L216
|
@nekodo Very nice! |
…ion for sentinel and switched from === to == for window comparison.
|
Yup, I'll be taking a look at it today. |
|
LGTM from me. We need to make sure to do good manual testing in canary. |
| */ | ||
| export function generateSentinel(parentWindow) { | ||
| let windowDepth = 0; | ||
| for (let win = parentWindow; win != window.top; win = win.parent) { |
There was a problem hiding this comment.
Don't we usually use window !== window.parent for our checks?
There was a problem hiding this comment.
Malte mentioned that window comparisons should use == rather than ===, I assumed the same for != vs !==.
There was a problem hiding this comment.
The window.parent vs the current window.top was what I meant, sorry for the confusion.
There was a problem hiding this comment.
Right, I switched to a consistent pattern of navigating the window hierarchy using win!=win.parent to identify the top of the hierarchy.
| // messages for it ( | ||
| let listener = function(data, source, origin) { | ||
| // Exclude nested frames if necessary. | ||
| if (!opt_includingNestedWindows && source != iframe.contentWindow) { |
There was a problem hiding this comment.
It might be worth adding a comment here that at this point, source is either iframe.contentWindow, or one of its child frames, and cannot be an unrelated window.
|
LGTM |
…rchy and use it uniformly everywhere in the CL.
|
LGTM as well. |
First stage of implementing #3019. Switches the 3P messaging code to use a per-iframe sentinel as opposed to a per-release sentinel and adds an option to listen for messages not only from a given frame, but also from any nested frames within it (as long as they carry the correct sentinel).
As a first application, IntersectionObserver is modified to allow nested frames to request updates on the visibility of the top frame.
Note that this change will likely worsen the performance slightly:
Fortunately we should be able to regain the performance by adding a WeakMap-based cache keyed by the source window so that we only need to do all these lookup on the first message from a window. To reduce complexity I left that for a follow up change.