AMP Ad Fast Fetch: FIE do not remove creative in unlayoutCallback #9396
Conversation
| @@ -446,6 +446,12 @@ export class AmpA4A extends AMP.BaseElement { | |||
|
|
|||
| /** @override */ | |||
| resumeCallback() { | |||
| // FIE that was not destroyed on unlayoutCallback does not require a new | |||
| // ad request. | |||
| if (!isExperimentOn(this.win, 'a4a-fie-unlayout-enabled') && | |||
There was a problem hiding this comment.
Is there another PR to add this experiment?
There was a problem hiding this comment.
No need unless we want/need to disable this new feature. This was added so that we could easily disable if needed.
| @@ -446,6 +446,12 @@ export class AmpA4A extends AMP.BaseElement { | |||
|
|
|||
| /** @override */ | |||
| resumeCallback() { | |||
| // FIE that was not destroyed on unlayoutCallback does not require a new | |||
| // ad request. | |||
| if (!isExperimentOn(this.win, 'a4a-fie-unlayout-enabled') && | |||
| @@ -446,6 +446,12 @@ export class AmpA4A extends AMP.BaseElement { | |||
|
|
|||
| /** @override */ | |||
| resumeCallback() { | |||
| // FIE that was not destroyed on unlayoutCallback does not require a new | |||
| // ad request. | |||
| if (!isExperimentOn(this.win, 'a4a-fie-unlayout-enabled') && | |||
There was a problem hiding this comment.
This should match #unlayoutCallback's check, which I think is incorrect.
| @@ -1000,6 +1006,10 @@ export class AmpA4A extends AMP.BaseElement { | |||
|
|
|||
| /** @override */ | |||
| unlayoutCallback() { | |||
| if (!isExperimentOn(this.win, 'a4a-fie-unlayout-enabled') && | |||
There was a problem hiding this comment.
I believe this is incorrect, the ! should be removed.
There was a problem hiding this comment.
Correct me if I'm wrong, Keith, but based off Keith's reply to my question above, I think the approach here is that this new behavior will run when the experiment flag doesn't exist, and we'll only add the flag if we need to shut it off.
There was a problem hiding this comment.
Right this experiment exists as an easy means to disable the new behavior as opposed to enable it. If this is a concern, I can invert and set to 100% enabled in prod/canary configs
There was a problem hiding this comment.
This should be "disabled" then.
|
Actually I disagree as setting this experiment arguably "re-enables"
unlayoutCallback
…On Wed, May 17, 2017, 5:58 PM Justin Ridgewell ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In extensions/amp-a4a/0.1/amp-a4a.js
<#9396 (comment)>:
> @@ -1000,6 +1006,10 @@ export class AmpA4A extends AMP.BaseElement {
/** @OverRide */
unlayoutCallback() {
+ if (!isExperimentOn(this.win, 'a4a-fie-unlayout-enabled') &&
This should be "disabled" then.
—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
<#9396 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABr0L4c4VSgi8H9gKO4_e5E80PuzLe1xks5r622ZgaJpZM4NeD9U>
.
|
AMP creatives rendered via friendly iframe embed are not removed during unlayoutCallback allowing them to be immediately visible when the user swipes back to the publisher page within the viewer. ResumeCallback does not call onLayoutMeasure in this case as its unnecessary. Wrapped in a4a-fie-unlayout-enabled experiment allowing this feature to be disabled (default is enabled).
Closes #9153