AMP Fast Fetch: Attempt to revert resize changes within unlayoutCallback #9066
Conversation
| @@ -279,6 +279,9 @@ export class AmpA4A extends AMP.BaseElement { | |||
| height: this.element.getAttribute('height'), | |||
| }; | |||
|
|
|||
| /** @private {?../../../src/layout-rect.LayoutRectDef} */ | |||
| this.originalSlotSize_ = null; | |||
There was a problem hiding this comment.
Why not initialize with the correct value here, once and for good?
There was a problem hiding this comment.
Seemed sensical to wait for onLayoutMeasure to ensure the values derived were correct (and its not required before then).
extensions/amp-a4a/0.1/amp-a4a.js
Outdated
| // Store original size of slot in order to allow re-expansion on | ||
| // unlayoutCallback so that it is reverted to original size in case | ||
| // of resumeCallback. | ||
| this.originalSlotSize_ = this.getIntersectionElementLayoutBox(); |
There was a problem hiding this comment.
#getLayoutBox. #getIntersectionElementLayoutBox is meant to be overridable for getting the iframe's layout box.
extensions/amp-a4a/0.1/amp-a4a.js
Outdated
| // Store original size of slot in order to allow re-expansion on | ||
| // unlayoutCallback so that it is reverted to original size in case | ||
| // of resumeCallback. | ||
| this.originalSlotSize_ = this.getIntersectionElementLayoutBox(); |
There was a problem hiding this comment.
Also, can the ad never request another resize? Shouldn't this be only set once?
There was a problem hiding this comment.
Done, check for this.originalSlotSize_ already been set.
extensions/amp-a4a/0.1/amp-a4a.js
Outdated
| @@ -958,6 +984,9 @@ export class AmpA4A extends AMP.BaseElement { | |||
| */ | |||
| forceCollapse() { | |||
| dev().assert(this.uiHandler); | |||
| // Store original size to allow for reverting on unlayoutCallback so that | |||
| // subsequent pageview allows for ad request. | |||
| this.originalSlotSize_ = this.getIntersectionElementLayoutBox(); | |||
There was a problem hiding this comment.
Ditto only setting once.
extensions/amp-a4a/0.1/amp-a4a.js
Outdated
| if (this.originalSlotSize_) { | ||
| // Attempt to revert any size change, this could fail but is unlikely as | ||
| // we can assume the document is no longer visible. | ||
| protectFunctionWrapper( |
There was a problem hiding this comment.
This crazy thing is useless. #attemptChangeSize will never throw an error, but it might return a rejected promise which you're not handling.
|
@jridgewell PTAL Thanks |
| super.attemptChangeSize( | ||
| this.originalSlotSize_.height, this.originalSlotSize_.width) | ||
| .then(() => { | ||
| this.originalSlotSize_ = null; |
There was a problem hiding this comment.
Left so that if resize fails, it could reattempt on next unlayoutCallback attempt (user swipes back and forth). Perhaps that won't happen in practice but can't hurt :)
There was a problem hiding this comment.
But it would attempt to change back to an already changed size, not the original.
There was a problem hiding this comment.
Not sure I follow. Given we don't override the value if set, it would
attempt to go back to the first size, no?
Sorry, missed that this was in the onResolve block.
| /** @override */ | ||
| unlayoutCallback() { | ||
| // Increment promiseId to cause any pending promise to cancel. | ||
| this.promiseId_++; | ||
| this.protectedEmitLifecycleEvent_('adSlotCleared'); | ||
| this.uiHandler.setDisplayState(AdDisplayState.NOT_LAID_OUT); | ||
| if (this.originalSlotSize_) { | ||
| super.attemptChangeSize( |
There was a problem hiding this comment.
@dvoytenko: Since this is in #unlayoutCallback, would a call to #changeSize be ok?
There was a problem hiding this comment.
That'd be dangerous since unlayout is not necessary due to whole document being backgrounded. I think it'd be better to use "attempt", but ensure that in Resources "attempt" is equivalent to "certain change" when viewer.isVisible() == false. I think it should be the case now, but always good to check.
There was a problem hiding this comment.
Yah, attempt will always succeed if the page isn't visible. Thanks for the clarification.
|
Not sure I follow. Given we don't override the value if set, it would
attempt to go back to the first size, no?
…On Tue, May 2, 2017, 2:16 PM Justin Ridgewell ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In extensions/amp-a4a/0.1/amp-a4a.js
<#9066 (comment)>:
> /** @OverRide */
unlayoutCallback() {
// Increment promiseId to cause any pending promise to cancel.
this.promiseId_++;
this.protectedEmitLifecycleEvent_('adSlotCleared');
this.uiHandler.setDisplayState(AdDisplayState.NOT_LAID_OUT);
+ if (this.originalSlotSize_) {
+ super.attemptChangeSize(
+ this.originalSlotSize_.height, this.originalSlotSize_.width)
+ .then(() => {
+ this.originalSlotSize_ = null;
But it would attempt to change back to an already changed size, not the
original.
—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
<#9066 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABr0L0N13koW6C3cXYLuuI2PYtsY84yJks5r13MNgaJpZM4NNPwm>
.
|
…ack (ampproject#9066) * Attempt to revert resize changes within unlayoutcallback * review feedback * fix lint error * fix type error * fix test failures
When users swipe between pages within the viewer, unlayoutCallback is executed which resets state. This reset should include attempting to revert the slot size back to its original as it could have been modified via resizes or collapse.
cc/ @ampproject/a4a