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

Remove listeners from <amp-facebook*> components when they aren't visible #14930

Merged
merged 1 commit into from
Apr 30, 2018

Conversation

nainar
Copy link
Contributor

@nainar nainar commented Apr 27, 2018

Part of #14761

Copy link
Contributor

@jridgewell jridgewell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You also have to remove its assignments.

@nainar
Copy link
Contributor Author

nainar commented Apr 27, 2018

Removed the later assignments as well.

@cvializ
Copy link
Contributor

cvializ commented Apr 27, 2018

Shouldn't we call this.unlistenMessage_ in unlayoutCallback, like amp-3q-player does, instead of deleting it?

unlayoutCallback() {
if (this.iframe_) {
removeElement(this.iframe_);
this.iframe_ = null;
}
if (this.unlistenMessage_) {
this.unlistenMessage_();
}

Copy link
Contributor

@cvializ cvializ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(question above)

@nainar
Copy link
Contributor Author

nainar commented Apr 27, 2018

Hmm yeah, all other components follow the same pattern, what value does calling it add though? We are already listening for messages.

@cvializ
Copy link
Contributor

cvializ commented Apr 30, 2018

unlayoutCallback is a hook so components can free up any expensive browser resources when they aren't visible. Listeners are one kind of resource that isn't needed when the component isn't visible. The iframe message callbacks here don't need to run if e.g. the user swipes to another document in the viewer, and calling this.unlistenMessage_ will cleanup that listener.

amphtml/src/base-element.js

Lines 503 to 511 in 08c581f

/**
* Requests the element to unload any expensive resources when the element
* goes into non-visible state. The scope is up to the actual component.
* The component must return `true` if it'd like to later receive
* {@link layoutCallback} in case document becomes active again.
*
* @return {boolean}
*/
unlayoutCallback() {

I think it is a case-by-case decision which resources to clean up though. If you decide that cleaning up this particular listener is unnecessary when the document is not visible, then feel free to disregard.

@nainar nainar changed the title Remove unused variables from <amp-facebook*> components Remove listeners from <amp-facebook*> components when they aren't visible Apr 30, 2018
@nainar
Copy link
Contributor Author

nainar commented Apr 30, 2018

@cvializ,
Your suggestion definitely makes sense. Thanks for the explainer :) Made the changes and edited the title of the PR as well. PTAL?

@cvializ cvializ merged commit 4785619 into ampproject:master Apr 30, 2018
noranazmy pushed a commit to noranazmy/amphtml that referenced this pull request May 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants