Make inabox-viewport measure the top level window directly if same do…

…main (ampproject#20459) * Make inabox-viewport measure the top level window directly if same domain Instead of sending a post message, if the inabox iframe can directly measure the host window then have it get its position directly from there. * fix types * Fix presubmit * Test disconnect function * Fix lint in new test * Try enabling friendly, no host script inabox test on Safari * Trivial fix to rerun Travis * Try the no host script test with the example AMP ad Checking if the serve mode of the example AMP test is changed somehow * don't whitelist the iframe * More work * Fix lint * Fix lint in test * fix oops * Throttle the listeners * Use the existing inabox host observer instead of rewriting a new one This makes the code cleaner and changes in the future easier * fix type issue * Revert the changes to test-amp-inabox.js I believe that the while the integration tests do test the inabox host script, they're are not actually testing the local AMP inabox run time. This might have to be fixed in a separate PR. * Gate the behavior behind experiment, and move canInspectWindow to helper file * Integrate the changes in ampproject#20599, and make sure the position observer is not null. * Inline the function * Fix type * Disconnect observer properly, and add a TODO to investigate a quirk in the listener. Specifically, why is Resources needed.
bramanudom · Mar 22, 2019 · 4ec1b3a · 4ec1b3a
1 parent 9ee6b8c
commit 4ec1b3a
Show file tree

Hide file tree

Showing 6 changed files with 248 additions and 100 deletions.
diff --git a/ads/inabox/inabox-messaging-host.js b/ads/inabox/inabox-messaging-host.js
@@ -21,6 +21,7 @@ import {
   serializeMessage,
 } from '../../src/3p-frame-messaging';
 import {PositionObserver} from './position-observer';
+import {canInspectWindow} from '../../src/iframe-helper';
 import {dev, devAssert} from '../../src/log';
 import {dict} from '../../src/utils/object';
 import {getData} from '../../src/event-helper';
@@ -300,7 +301,7 @@ export class InaboxMessagingHost {
     // this loop breaks immediately.
     let topXDomainWin;
     for (let j = 0, tempWin = win;
-      j < 10 && tempWin != tempWin.top && !canInspectWindow_(tempWin);
+      j < 10 && tempWin != tempWin.top && !canInspectWindow(tempWin);
       j++, topXDomainWin = tempWin, tempWin = tempWin.parent) {}
     // If topXDomainWin exists, we know that the frame we want to measure
     // is a x-domain frame. Unfortunately, you can not access properties
@@ -347,25 +348,3 @@ export class InaboxMessagingHost {
     }
   }
 }
-
-/**
- * Returns true if win's properties can be accessed and win is defined.
- * This functioned is used to determine if a window is cross-domained
- * from the perspective of the current window.
- * @param {!Window} win
- * @return {boolean}
- * @private
- */
-function canInspectWindow_(win) {
-  // TODO: this is not reliable.  The compiler assumes that property reads are
-  // side-effect free.  The recommended fix is to use goog.reflect.sinkValue
-  // but since we're not using the closure library I'm not sure how to do this.
-  // See https://github.com/google/closure-compiler/issues/3156
-  try {
-    // win['test'] could be truthy but not true the compiler shouldn't be able
-    // to optimize this check away.
-    return !!win.location.href && (win['test'] || true);
-  } catch (unusedErr) { // eslint-disable-line no-unused-vars
-    return false;
-  }
-}
diff --git a/src/iframe-helper.js b/src/iframe-helper.js
@@ -546,3 +546,25 @@ export function disableScrollingOnIframe(iframe) {
 
   return iframe;
 }
+
+/**
+ * Returns true if win's properties can be accessed and win is defined.
+ * This functioned is used to determine if a window is cross-domained
+ * from the perspective of the current window.
+ * @param {!Window} win
+ * @return {boolean}
+ * @private
+ */
+export function canInspectWindow(win) {
+  // TODO: this is not reliable.  The compiler assumes that property reads are
+  // side-effect free.  The recommended fix is to use goog.reflect.sinkValue
+  // but since we're not using the closure library I'm not sure how to do this.
+  // See https://github.com/google/closure-compiler/issues/3156
+  try {
+    // win['test'] could be truthy but not true the compiler shouldn't be able
+    // to optimize this check away.
+    return !!win.location.href && (win['test'] || true);
+  } catch (unusedErr) { // eslint-disable-line no-unused-vars
+    return false;
+  }
+}
diff --git a/src/inabox/inabox-viewport.js b/src/inabox/inabox-viewport.js
@@ -16,9 +16,11 @@
 
 import {MessageType} from '../../src/3p-frame-messaging';
 import {Observable} from '../observable';
+import {PositionObserver} from '../../ads/inabox/position-observer';
 import {Services} from '../services';
 import {Viewport} from '../service/viewport/viewport-impl';
 import {ViewportBindingDef} from '../service/viewport/viewport-binding-def';
+import {canInspectWindow} from '../iframe-helper';
 import {dev, devAssert} from '../log';
 import {iframeMessagingClientFor} from './inabox-iframe-messaging-client';
 import {isExperimentOn} from '../experiments';
@@ -149,35 +151,74 @@ export class ViewportBindingInabox {
     /** @private @const {boolean} */
     this.useLayers_ = isExperimentOn(this.win, 'layers');
 
+    /** @private {?../../ads/inabox/position-observer.PositionObserver} */
+    this.topWindowPositionObserver_ = null;
+
+    /** @private {?UnlistenDef} */
+    this.unobserveFunction_ = null;
+
     dev().fine(TAG, 'initialized inabox viewport');
   }
 
   /** @override */
   connect() {
-    this.listenForPosition_();
+    if (isExperimentOn(this.win, 'inabox-viewport-friendly') &&
+        canInspectWindow(this.win.top)) {
+      this.listenForPositionSameDomain();
+    } else {
+      this.listenForPosition_();
+    }
   }
 
   /** @private */
   listenForPosition_() {
-
     this.iframeClient_.makeRequest(
         MessageType.SEND_POSITIONS, MessageType.POSITION,
         data => {
           dev().fine(TAG, 'Position changed: ', data);
-          const oldViewportRect = this.viewportRect_;
-          this.viewportRect_ = data['viewportRect'];
-
-          this.updateBoxRect_(data['targetRect']);
+          this.updateLayoutRects_(data['viewportRect'], data['targetRect']);
+        });
+  }
 
-          if (isResized(this.viewportRect_, oldViewportRect)) {
-            this.resizeObservable_.fire();
-          }
-          if (isMoved(this.viewportRect_, oldViewportRect)) {
-            this.fireScrollThrottle_();
-          }
+  /** @visibleForTesting */
+  listenForPositionSameDomain() {
+    // Set up listener but only after the resources service is properly
+    // registered (since it's registered after the inabox services so it won't
+    // be available immediately).
+    // TODO(lannka): Investigate why this is the case.
+    if (this.topWindowPositionObserver_) {
+      return Promise.resolve();
+    }
+    return Services.resourcesPromiseForDoc(this.win.document.documentElement)
+        .then(() => {
+          this.topWindowPositionObserver_ = new PositionObserver(this.win.top);
+          this.unobserveFunction_ = this.topWindowPositionObserver_.observe(
+              /** @type {!HTMLIFrameElement} */(this.win.frameElement),
+              data => {
+                this.updateLayoutRects_(
+                    data['viewportRect'],
+                    data['targetRect']);
+              });
         });
   }
 
+  /**
+   * @private
+   * @param {!../layout-rect.LayoutRectDef} viewportRect
+   * @param {!../layout-rect.LayoutRectDef} targetRect
+   */
+  updateLayoutRects_(viewportRect, targetRect) {
+    const oldViewportRect = this.viewportRect_;
+    this.viewportRect_ = viewportRect;
+    this.updateBoxRect_(targetRect);
+    if (isResized(this.viewportRect_, oldViewportRect)) {
+      this.resizeObservable_.fire();
+    }
+    if (isMoved(this.viewportRect_, oldViewportRect)) {
+      this.fireScrollThrottle_();
+    }
+  }
+
   /** @override */
   getLayoutRect(el) {
     const b = el./*OK*/getBoundingClientRect();
@@ -283,6 +324,13 @@ export class ViewportBindingInabox {
 
   /** @override */
   getRootClientRectAsync() {
+    if (isExperimentOn(this.win, 'inabox-viewport-friendly') &&
+        canInspectWindow(this.win.top)) {
+      // Set up the listener if we haven't already.
+      return this.listenForPositionSameDomain().then(() =>
+        this.topWindowPositionObserver_.getTargetRect(
+            /** @type {!HTMLIFrameElement} */(this.win.frameElement)));
+    }
     if (!this.requestPositionPromise_) {
       this.requestPositionPromise_ = new Promise(resolve => {
         this.iframeClient_.requestOnce(
@@ -378,7 +426,13 @@ export class ViewportBindingInabox {
     return dev().assertElement(this.win.document.body);
   }
 
-  /** @override */ disconnect() {/* no-op */}
+  /** @override */
+  disconnect() {
+    if (this.unobserveFunction_) {
+      this.unobserveFunction_();
+    }
+  }
+
   /** @override */ updatePaddingTop() {/* no-op */}
   /** @override */ hideViewerHeader() {/* no-op */}
   /** @override */ showViewerHeader() {/* no-op */}

diff --git a/src/services.js b/src/services.js
@@ -326,6 +326,15 @@ export class Services {
       getServiceForDoc(elementOrAmpDoc, 'resources'));
   }
 
+  /**
+   * @param {!Element|!./service/ampdoc-impl.AmpDoc} elementOrAmpDoc
+   * @return {!Promise<!./service/resources-impl.Resources>}
+   */
+  static resourcesPromiseForDoc(elementOrAmpDoc) {
+    return /** @type {!Promise<!./service/resources-impl.Resources>} */ (
+      getServicePromiseForDoc(elementOrAmpDoc, 'resources'));
+  }
+
   /**
    * @param {!Window} win
    * @return {?Promise<?{incomingFragment: string, outgoingFragment: string}>}