Remove BaseElement.viewportCallback from ads. (#30859)

* amp-ads / a4a: remove BaseElement.viewportCallback * private --> protected * fix doubleclick test * protected fix, this.isInViewport fix, delay observe * Update test-doubleclick-fluid.js * make code fine with a test that calls layooutCallback twice in a row
ampproject · Oct 30, 2020 · a7c8190 · a7c8190
1 parent d69a716
commit a7c8190
Show file tree

Hide file tree

Showing 5 changed files with 27 additions and 18 deletions.
diff --git a/extensions/amp-a4a/0.1/amp-a4a.js b/extensions/amp-a4a/0.1/amp-a4a.js
@@ -58,6 +58,10 @@ import {installUrlReplacementsForEmbed} from '../../../src/service/url-replaceme
 import {isAdPositionAllowed} from '../../../src/ad-helper';
 import {isArray, isEnumValue, isObject} from '../../../src/types';
 import {listenOnce} from '../../../src/event-helper';
+import {
+  observeWithSharedInOb,
+  unobserveWithSharedInOb,
+} from '../../../src/viewport-observer';
 import {padStart} from '../../../src/string';
 import {parseJson} from '../../../src/json';
 import {processHead} from './head-validation';
@@ -367,6 +371,9 @@ export class AmpA4A extends AMP.BaseElement {
      * @private {?function(!Element)}
      */
     this.transferDomBody_ = null;
+
+    /** @private {function(boolean)} */
+    this.boundViewportCallback_ = this.viewportCallbackTemp.bind(this);
   }
 
   /** @override */
@@ -1235,7 +1242,9 @@ export class AmpA4A extends AMP.BaseElement {
     if (this.isRefreshing) {
       this.destroyFrame(true);
     }
-    return this.attemptToRenderCreative();
+    return this.attemptToRenderCreative().then(() => {
+      observeWithSharedInOb(this.element, this.boundViewportCallback_);
+    });
   }
 
   /**
@@ -1326,6 +1335,7 @@ export class AmpA4A extends AMP.BaseElement {
 
   /** @override  */
   unlayoutCallback() {
+    unobserveWithSharedInOb(this.element);
     this.tearDownSlot();
     return true;
   }
@@ -1402,8 +1412,12 @@ export class AmpA4A extends AMP.BaseElement {
     }
   }
 
-  /** @override  */
-  viewportCallback(inViewport) {
+  // TODO: Rename to viewportCallback once BaseElement.viewportCallback has been removed.
+  /**
+   * @param {boolean}  inViewport
+   * @protected
+   */
+  viewportCallbackTemp(inViewport) {
     if (this.xOriginIframeHandler_) {
       this.xOriginIframeHandler_.viewportCallback(inViewport);
     }

diff --git a/extensions/amp-ad-network-doubleclick-impl/0.1/amp-ad-network-doubleclick-impl.js b/extensions/amp-ad-network-doubleclick-impl/0.1/amp-ad-network-doubleclick-impl.js
@@ -1150,8 +1150,8 @@ export class AmpAdNetworkDoubleclickImpl extends AmpA4A {
   }
 
   /** @override */
-  viewportCallback(inViewport) {
-    super.viewportCallback(inViewport);
+  viewportCallbackTemp(inViewport) {
+    super.viewportCallbackTemp(inViewport);
     if (this.reattemptToExpandFluidCreative_ && !inViewport) {
       // If the initial expansion attempt failed (e.g., the slot was within the
       // viewport), then we will re-attempt to expand it here whenever the slot

diff --git a/extensions/amp-ad-network-doubleclick-impl/0.1/test/test-doubleclick-fluid.js b/extensions/amp-ad-network-doubleclick-impl/0.1/test/test-doubleclick-fluid.js
@@ -374,9 +374,9 @@ describes.realWin('DoubleClick Fast Fetch Fluid', realWinConfig, (env) => {
     impl.isVerifiedAmpCreative_ = true;
     impl.reattemptToExpandFluidCreative_ = true;
     // Should do nothing
-    impl.viewportCallback(true);
+    impl.viewportCallbackTemp(true);
     expect(attemptChangeHeightStub).to.not.be.called;
-    impl.viewportCallback(false);
+    impl.viewportCallbackTemp(false);
     expect(attemptChangeHeightStub).to.be.calledOnce;
   });
 

diff --git a/extensions/amp-ad/0.1/amp-ad-xorigin-iframe-handler.js b/extensions/amp-ad/0.1/amp-ad-xorigin-iframe-handler.js
@@ -81,6 +81,9 @@ export class AmpAdXOriginIframeHandler {
     /** @private @const {!../../../src/service/viewport/viewport-interface.ViewportInterface} */
     this.viewport_ = Services.viewportForDoc(this.baseInstance_.getAmpDoc());
 
+    /** @private {boolean} */
+    this.inViewport_ = false;
+
     /** @private {boolean} */
     this.sendPositionPending_ = false;
   }
@@ -111,7 +114,7 @@ export class AmpAdXOriginIframeHandler {
       this.iframe,
       'send-embed-state',
       true,
-      () => this.sendEmbedInfo_(this.baseInstance_.isInViewport())
+      () => this.sendEmbedInfo_(this.inViewport_)
     );
 
     // Enable creative position observer if inabox experiment enabled OR
@@ -197,7 +200,7 @@ export class AmpAdXOriginIframeHandler {
 
     this.unlisteners_.push(
       this.baseInstance_.getAmpDoc().onVisibilityChanged(() => {
-        this.sendEmbedInfo_(this.baseInstance_.isInViewport());
+        this.sendEmbedInfo_(this.inViewport_);
       })
     );
 
@@ -593,6 +596,7 @@ export class AmpAdXOriginIframeHandler {
    * @param {boolean} inViewport
    */
   viewportCallback(inViewport) {
+    this.inViewport_ = inViewport;
     this.sendEmbedInfo_(inViewport);
   }
 

diff --git a/extensions/amp-fx-flying-carpet/0.1/amp-fx-flying-carpet.js b/extensions/amp-fx-flying-carpet/0.1/amp-fx-flying-carpet.js
@@ -101,15 +101,6 @@ export class AmpFlyingCarpet extends AMP.BaseElement {
     );
   }
 
-  /** @override */
-  viewportCallback(inViewport) {
-    Services.ownersForDoc(this.element).updateInViewport(
-      this.element,
-      this.children_,
-      inViewport
-    );
-  }
-
   /**
    * Asserts that the flying carpet does not appear in the first or last
    * viewport.