Remove ad during unlayout #2697
Conversation
| } | ||
|
|
||
| this.iframe_ = null; | ||
| this.intersectionObserver_ = null; |
There was a problem hiding this comment.
To double-check - intersection observer is likely to listen on viewport or such. Is there some sort of unlisten for it?
There was a problem hiding this comment.
The unlistener doesn't get returned during construction, so no way to call it. Apparently it only listens to the iframe though, so it should be cleaned up by virtue of the the iframe's contentWindow being destroyed.
There was a problem hiding this comment.
It does also listen on viewport. But it looks like all listeners are freed via onViewportCallback(false) which is called from the element's onViewportCallback. Could you confirm if you always call onViewportCallback(false) on the element before unlayout?
There was a problem hiding this comment.
Cool. Then no worries. But I'd still add a comment - "The listeners have already been freed via setInViewport(false) before unlayout" - or something like this.
Should improve the swiping performance.
jridgewell
force-pushed
the
remove-ad-on-pause
branch
from
db41bc0
to
03850ee
Compare
|
LGTM |
Should improve the swiping performance. Right now, I've set
amp-adto unlayout on pause, meaning we will remove the ad at the beginning of the swipe. If this hurts performance, we can remove that and have it only remove on unlayout (when the swipe completes).Fixes #2563.