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.

Sign up for GitHub

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

Jump to bottom

Make sentinel optional for iframe-helper #7307

Closed
aghassemi opened this issue Feb 2, 2017 · 7 comments
Closed

Make sentinel optional for iframe-helper #7307

aghassemi opened this issue Feb 2, 2017 · 7 comments
Assignees
@jridgewell
Labels
P3: When Possible Type: Feature Request WG: runtime
Milestone
Backlog Bugs

Comments

@aghassemi
Copy link
Contributor

aghassemi commented Feb 2, 2017 •
edited by ampwafflebot
Loading

Currently listenFor in iframe-helper is too tied to a sentinel which makes it unusable for listening to postMessages that are not AMP-specific ( e.g. messages from YouTube iframes ).

It would be nice if sentinel becomes optional so code can be reused for listening to arbitrary messages. It may make sense to use origin instead in cases where sentinel does not make sense.
@aghassemi aghassemi added this to the New FRs milestone Feb 2, 2017
@jridgewell
Copy link
Contributor

That entire file got a little out of hand. 😳

@aghassemi
Copy link
Contributor Author

Another place that could use this amp-instagram: #7705

@jridgewell jridgewell modified the milestones: Fixit Week EOQ1, New FRs Mar 28, 2017
@jridgewell
Copy link
Contributor

Would have caught this bug: https://github.com/ampproject/amphtml/blob/master/src/iframe-helper.js#L249-L250

@aghassemi
Copy link
Contributor Author

Must be a rare race-condition (well, at least of ads as I assume they normally don't cache) otherwise #2942 would have been noticed again.

@ampprojectbot
Copy link
Member

This issue hasn't been updated in awhile. @jridgewell Do you have any updates?

1 similar comment
@ampprojectbot
Copy link
Member

This issue hasn't been updated in awhile. @jridgewell Do you have any updates?

@aghassemi
Copy link
Contributor Author

eh, not really needed. listenFor already helps. Also #15527 refactor multiple places into one anyway. Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@jridgewell jridgewell

Labels
P3: When Possible Type: Feature Request WG: runtime
Projects
None yet
Milestone
Backlog Bugs
Development

No branches or pull requests
4 participants
@jridgewell @aghassemi @ampprojectbot @ampwafflebot