From dfb4d5c06474b86d53db37914d778d420ad3cba4 Mon Sep 17 00:00:00 2001
From: Enrique Marroquin <artezan@google.com>
Date: Tue, 11 Feb 2020 19:29:17 -0500
Subject: [PATCH 1/8] navigation logic

---
 src/amp-story-player.js | 261 +++++++++++++++++++++++++++++++++++++---
 1 file changed, 241 insertions(+), 20 deletions(-)

diff --git a/src/amp-story-player.js b/src/amp-story-player.js
index 7e2e8bc3037c..2af8ea0f9c12 100644
--- a/src/amp-story-player.js
+++ b/src/amp-story-player.js
@@ -22,10 +22,10 @@ import {
   parseUrlWithA,
   removeFragment,
 } from './url';
-import {dict} from './utils/object';
+import {dict, map} from './utils/object';
 // Source for this constant is css/amp-story-player-iframe.css
+import {Deferred} from './utils/promise';
 import {cssText} from '../build/amp-story-player-iframe.css';
-import {findIndex} from './utils/array';
 import {setStyle} from './style';
 import {toArray} from './types';
 
@@ -36,6 +36,12 @@ const LoadStateClass = {
   ERROR: 'i-amphtml-story-player-error',
 };
 
+/** @const {number} */
+const MAX_IFRAMES = 3;
+
+/** @const {string} */
+const IFRAME_IDX = '__AMP_IFRAME_IDX__';
+
 /**
  * Note that this is a vanilla JavaScript class and should not depend on AMP
  * services, as v0.js is not expected to be loaded in this context.
@@ -78,6 +84,18 @@ export class AmpStoryPlayer {
 
     /** @private {boolean} */
     this.isLaidOut_ = false;
+
+    /** @private {!IframePool} */
+    this.iframePool_ = new IframePool();
+
+    /** @private @const {!Object<string, !Promise>} */
+    this.connectedPromises_ = map();
+
+    /** @private @const {!Object<string, !Function>} */
+    this.connectedResolvers_ = map();
+
+    /** @private {number} */
+    this.currentIdx_ = 0;
   }
 
   /**
@@ -93,9 +111,25 @@ export class AmpStoryPlayer {
     this.stories_ = toArray(this.element_.querySelectorAll('a'));
 
     this.initializeShadowRoot_();
+    this.initializeIframes_();
+  }
 
-    // TODO(#26308): Build all child iframes.
-    this.buildIframe_(this.stories_[0]);
+  /** @private */
+  initializeIframes_() {
+    this.stories_.forEach(story => {
+      story[IFRAME_IDX] = null;
+    });
+
+    for (let idx = 0; idx < MAX_IFRAMES && idx < this.stories_.length; idx++) {
+      const story = this.stories_[idx];
+      this.buildIframe_(story);
+
+      story[IFRAME_IDX] = idx;
+      this.setUpMessagingForIframe_(story, this.iframes_[idx]);
+
+      this.iframePool_.addIframe(idx);
+      this.iframePool_.addStory(idx);
+    }
   }
 
   /** @private */
@@ -128,20 +162,27 @@ export class AmpStoryPlayer {
 
     this.initializeLoadingListeners_(iframeEl);
     this.rootEl_.appendChild(iframeEl);
+  }
+
+  /**
+   * Sets up messaging for a story inside an iframe.
+   * @param {!Element} story
+   * @param {!Element} iframeEl
+   * @private
+   */
+  setUpMessagingForIframe_(story, iframeEl) {
+    const iframeIdx = story[IFRAME_IDX];
+
+    const deferred = new Deferred();
+    this.connectedPromises_[iframeIdx] = deferred.promise;
+    this.connectedResolvers_[iframeIdx] = deferred.resolve;
 
     this.initializeHandshake_(story, iframeEl).then(
       messaging => {
-        const iframeIdx = findIndex(
-          this.iframes_,
-          iframe => iframe === iframeEl
-        );
-
-        messaging.setDefaultHandler(() => {});
+        messaging.setDefaultHandler(() => Promise.resolve());
 
         this.messagingFor_[iframeIdx] = messaging;
-
-        // TODO(#26308): Appropiately set visibility to stories.
-        this.displayStory_(iframeIdx);
+        this.connectedResolvers_[iframeIdx](iframeIdx);
       },
       err => {
         console /*OK*/
@@ -193,12 +234,100 @@ export class AmpStoryPlayer {
       return;
     }
 
-    // TODO(#26308): Layout all child iframes.
-    this.layoutIframe_(this.stories_[0], this.iframes_[0]);
+    // Display first story.
+    const firstStory = this.stories_[0];
+    const iframeIdx = firstStory[IFRAME_IDX];
+    const storyIframe = this.iframes_[iframeIdx];
+    this.layoutIframe_(firstStory, storyIframe);
+    this.displayStory_(iframeIdx);
+
+    // Pre-render next stories.
+    for (let idx = 1; idx < this.stories_.length && idx < MAX_IFRAMES; idx++) {
+      const story = this.stories_[idx];
+      const iframeIdx = story[IFRAME_IDX];
+      const iframe = this.iframes_[iframeIdx];
+      this.layoutIframe_(story, iframe);
+      this.preRenderStory_(iframeIdx);
+    }
 
     this.isLaidOut_ = true;
   }
 
+  /**
+   * Navigates to the next story in the player.
+   * @private
+   */
+  next_() {
+    if (this.currentIdx_ + 1 >= this.stories_.length) {
+      return;
+    }
+
+    this.currentIdx_++;
+
+    const previousStory = this.stories_[this.currentIdx_ - 1];
+    this.pauseStory_(previousStory[IFRAME_IDX]);
+
+    const currentStory = this.stories_[this.currentIdx_];
+    this.displayStory_(currentStory[IFRAME_IDX]);
+
+    const nextStoryIdx = this.currentIdx_ + 1;
+    if (
+      nextStoryIdx < this.stories_.length &&
+      this.stories_[nextStoryIdx][IFRAME_IDX] === null
+    ) {
+      this.swapIframes_(nextStoryIdx);
+      this.preRenderStory_(this.stories_[nextStoryIdx][IFRAME_IDX]);
+    }
+  }
+
+  /**
+   * Navigates to the previous story in the player.
+   * @private
+   */
+  previous_() {
+    if (this.currentIdx_ - 1 < 0) {
+      return;
+    }
+
+    this.currentIdx_--;
+
+    const previousStory = this.stories_[this.currentIdx_ + 1];
+    this.pauseStory_(previousStory[IFRAME_IDX]);
+
+    const currentStory = this.stories_[this.currentIdx_];
+    this.displayStory_(currentStory[IFRAME_IDX]);
+
+    const nextStoryIdx = this.currentIdx_ - 1;
+    if (nextStoryIdx >= 0 && this.stories_[nextStoryIdx][IFRAME_IDX] === null) {
+      this.swapIframes_(nextStoryIdx, true /** backwards */);
+      this.preRenderStory_(this.stories_[nextStoryIdx][IFRAME_IDX]);
+    }
+  }
+
+  /**
+   * Detaches iframe from a story and gives it to the next story. It detaches
+   * the iframe from the story furthest away; depending where the user is
+   * navigating and allocates it to a story that the user is close to seeing.
+   * @param {number} nextStoryIdx
+   * @param {boolean} backwards
+   * @private
+   */
+  swapIframes_(nextStoryIdx, backwards = false) {
+    const detachedStoryIdx = backwards
+      ? this.iframePool_.shiftBackwards(nextStoryIdx)
+      : this.iframePool_.shiftForwards(nextStoryIdx);
+
+    const detachedStory = this.stories_[detachedStoryIdx];
+    const nextStory = this.stories_[nextStoryIdx];
+
+    nextStory[IFRAME_IDX] = detachedStory[IFRAME_IDX];
+    detachedStory[IFRAME_IDX] = null;
+
+    const nextIframe = this.iframes_[nextStory[IFRAME_IDX]];
+    this.layoutIframe_(nextStory, nextIframe);
+    this.setUpMessagingForIframe_(nextStory, nextIframe);
+  }
+
   /**
    * @param {!Element} story
    * @param {!Element} iframe
@@ -248,11 +377,43 @@ export class AmpStoryPlayer {
    * @param {number} iframeIdx
    */
   displayStory_(iframeIdx) {
-    this.messagingFor_[iframeIdx].sendRequest(
-      'visibilitychange',
-      {state: 'visible'},
-      true
-    );
+    this.connectedPromises_[iframeIdx].then(() => {
+      this.messagingFor_[iframeIdx].sendRequest(
+        'visibilitychange',
+        {state: 'visible'},
+        true
+      );
+    });
+  }
+
+  /**
+   * Sends a message to the story document to pre-render it.
+   * @private
+   * @param {number} iframeIdx
+   */
+  preRenderStory_(iframeIdx) {
+    this.connectedPromises_[iframeIdx].then(() => {
+      this.messagingFor_[iframeIdx].sendRequest(
+        'visibilitychange',
+        {state: 'prerender'},
+        true
+      );
+    });
+  }
+
+  /**
+   * Sends a message to the story document to pause it.
+   * @private
+   * @param {number} iframeIdx
+   */
+  pauseStory_(iframeIdx) {
+    this.connectedPromises_[iframeIdx].then(() => {
+      this.messagingFor_[iframeIdx].sendRequest(
+        'visibilitychange',
+        {state: 'paused'},
+        true
+      );
+    });
   }
 }
 
@@ -260,3 +421,63 @@ self.onload = () => {
   const manager = new AmpStoryPlayerManager(self);
   manager.loadPlayers();
 };
+
+/**
+ * Manages the iframes used to host the stories inside the player. It keeps
+ * track of the iframes hosting stories, and what stories have an iframe.
+ */
+class IframePool {
+  /** @public */
+  constructor() {
+    /** @private @const {!Array<number>} */
+    this.iframePool_ = [];
+
+    /** @private @const {!Array<number>} */
+    this.storyIdsWithIframe_ = [];
+  }
+
+  /**
+   * @param {number} idx
+   * @public
+   */
+  addIframe(idx) {
+    this.iframePool_.push(idx);
+  }
+
+  /**
+   * @param {number} idx
+   * @public
+   */
+  addStory(idx) {
+    this.storyIdsWithIframe_.push(idx);
+  }
+
+  /**
+   * Shifts both the iframePool and the storyIdsWithIframe forward. It returns
+   * the index of the story which will be detached of an iframe.
+   * @param {number} nextStoryIdx
+   * @return {number}
+   */
+  shiftForwards(nextStoryIdx) {
+    const detachedStoryIdx = this.storyIdsWithIframe_.shift();
+    this.storyIdsWithIframe_.push(nextStoryIdx);
+
+    this.iframePool_.push(this.iframePool_.shift());
+    return detachedStoryIdx;
+  }
+
+  /**
+   * Shifts both the iframePool and the storyIdsWithIframe backwards. It returns
+   * the index of the story which will be detached of an iframe.
+   * @param {number} nextStoryIdx
+   * @return {number}
+   */
+  shiftBackwards(nextStoryIdx) {
+    const detachedStoryIdx = this.storyIdsWithIframe_.pop();
+    this.storyIdsWithIframe_.unshift(nextStoryIdx);
+
+    this.iframePool_.unshift(this.iframePool_.pop());
+
+    return detachedStoryIdx;
+  }
+}

From 3f15cc9852521c5410d2094e144d62fcc2e889fc Mon Sep 17 00:00:00 2001
From: Enrique Marroquin <artezan@google.com>
Date: Tue, 11 Feb 2020 20:32:31 -0500
Subject: [PATCH 2/8] renaming and jsdocs

---
 src/amp-story-player.js | 37 +++++++++++++++++++++----------------
 1 file changed, 21 insertions(+), 16 deletions(-)

diff --git a/src/amp-story-player.js b/src/amp-story-player.js
index 2af8ea0f9c12..5df591bdc09a 100644
--- a/src/amp-story-player.js
+++ b/src/amp-story-player.js
@@ -88,11 +88,11 @@ export class AmpStoryPlayer {
     /** @private {!IframePool} */
     this.iframePool_ = new IframePool();
 
-    /** @private @const {!Object<string, !Promise>} */
-    this.connectedPromises_ = map();
+    /** @private {!Object<string, !Promise>} */
+    this.handshakePromises_ = map();
 
-    /** @private @const {!Object<string, !Function>} */
-    this.connectedResolvers_ = map();
+    /** @private {!Object<string, !Function>} */
+    this.handshakeResolvers_ = map();
 
     /** @private {number} */
     this.currentIdx_ = 0;
@@ -174,15 +174,15 @@ export class AmpStoryPlayer {
     const iframeIdx = story[IFRAME_IDX];
 
     const deferred = new Deferred();
-    this.connectedPromises_[iframeIdx] = deferred.promise;
-    this.connectedResolvers_[iframeIdx] = deferred.resolve;
+    this.handshakePromises_[iframeIdx] = deferred.promise;
+    this.handshakeResolvers_[iframeIdx] = deferred.resolve;
 
     this.initializeHandshake_(story, iframeEl).then(
       messaging => {
         messaging.setDefaultHandler(() => Promise.resolve());
 
         this.messagingFor_[iframeIdx] = messaging;
-        this.connectedResolvers_[iframeIdx](iframeIdx);
+        this.handshakeResolvers_[iframeIdx](iframeIdx);
       },
       err => {
         console /*OK*/
@@ -377,7 +377,7 @@ export class AmpStoryPlayer {
    * @param {number} iframeIdx
    */
   displayStory_(iframeIdx) {
-    this.connectedPromises_[iframeIdx].then(() => {
+    this.handshakePromises_[iframeIdx].then(() => {
       this.messagingFor_[iframeIdx].sendRequest(
         'visibilitychange',
         {state: 'visible'},
@@ -392,7 +392,7 @@ export class AmpStoryPlayer {
    * @param {number} iframeIdx
    */
   preRenderStory_(iframeIdx) {
-    this.connectedPromises_[iframeIdx].then(() => {
+    this.handshakePromises_[iframeIdx].then(() => {
       this.messagingFor_[iframeIdx].sendRequest(
         'visibilitychange',
         {state: 'prerender'},
@@ -407,7 +407,7 @@ export class AmpStoryPlayer {
    * @param {number} iframeIdx
    */
   pauseStory_(iframeIdx) {
-    this.connectedPromises_[iframeIdx].then(() => {
+    this.handshakePromises_[iframeIdx].then(() => {
       this.messagingFor_[iframeIdx].sendRequest(
         'visibilitychange',
         {state: 'paused'},
@@ -453,24 +453,29 @@ class IframePool {
   }
 
   /**
-   * Shifts both the iframePool and the storyIdsWithIframe forward. It returns
-   * the index of the story which will be detached of an iframe.
+   * Shifts iframe allocation forwards. It takes the leftmost iframe and
+   * allocates it to the next story to the right without an iframe. It also
+   * updates the storyIdsWithIframe by removing the reference to the detached
+   * story and adds the new one.
    * @param {number} nextStoryIdx
-   * @return {number}
+   * @return {number} Index of the detached story.
    */
   shiftForwards(nextStoryIdx) {
     const detachedStoryIdx = this.storyIdsWithIframe_.shift();
     this.storyIdsWithIframe_.push(nextStoryIdx);
 
     this.iframePool_.push(this.iframePool_.shift());
+
     return detachedStoryIdx;
   }
 
   /**
-   * Shifts both the iframePool and the storyIdsWithIframe backwards. It returns
-   * the index of the story which will be detached of an iframe.
+   * Shifts iframe allocation backwards. It takes the rightmost iframe and
+   * allocates it to the next story to the left without an iframe. It also
+   * updates the storyIdsWithIframe by removing the reference to the detached
+   * story and adds the new one.
    * @param {number} nextStoryIdx
-   * @return {number}
+   * @return {number} Index of the detached story.
    */
   shiftBackwards(nextStoryIdx) {
     const detachedStoryIdx = this.storyIdsWithIframe_.pop();

From c1908bcfc37c14a8fb0cae21ba4dfec32f81e453 Mon Sep 17 00:00:00 2001
From: Enrique Marroquin <artezan@google.com>
Date: Wed, 12 Feb 2020 16:51:25 -0500
Subject: [PATCH 3/8] factorize visibilitystate methods, API renaming, remove
 unneeded resolvers

---
 src/amp-story-player.js | 116 ++++++++++++----------------------------
 1 file changed, 35 insertions(+), 81 deletions(-)

diff --git a/src/amp-story-player.js b/src/amp-story-player.js
index 5df591bdc09a..41aafabe8d54 100644
--- a/src/amp-story-player.js
+++ b/src/amp-story-player.js
@@ -24,7 +24,6 @@ import {
 } from './url';
 import {dict, map} from './utils/object';
 // Source for this constant is css/amp-story-player-iframe.css
-import {Deferred} from './utils/promise';
 import {cssText} from '../build/amp-story-player-iframe.css';
 import {setStyle} from './style';
 import {toArray} from './types';
@@ -91,9 +90,6 @@ export class AmpStoryPlayer {
     /** @private {!Object<string, !Promise>} */
     this.handshakePromises_ = map();
 
-    /** @private {!Object<string, !Function>} */
-    this.handshakeResolvers_ = map();
-
     /** @private {number} */
     this.currentIdx_ = 0;
   }
@@ -127,8 +123,8 @@ export class AmpStoryPlayer {
       story[IFRAME_IDX] = idx;
       this.setUpMessagingForIframe_(story, this.iframes_[idx]);
 
-      this.iframePool_.addIframe(idx);
-      this.iframePool_.addStory(idx);
+      this.iframePool_.addIframeIdx(idx);
+      this.iframePool_.addStoryIdx(idx);
     }
   }
 
@@ -173,22 +169,19 @@ export class AmpStoryPlayer {
   setUpMessagingForIframe_(story, iframeEl) {
     const iframeIdx = story[IFRAME_IDX];
 
-    const deferred = new Deferred();
-    this.handshakePromises_[iframeIdx] = deferred.promise;
-    this.handshakeResolvers_[iframeIdx] = deferred.resolve;
-
-    this.initializeHandshake_(story, iframeEl).then(
-      messaging => {
-        messaging.setDefaultHandler(() => Promise.resolve());
-
-        this.messagingFor_[iframeIdx] = messaging;
-        this.handshakeResolvers_[iframeIdx](iframeIdx);
-      },
-      err => {
-        console /*OK*/
-          .log({err});
-      }
-    );
+    this.handshakePromises_[iframeIdx] = new Promise(resolve => {
+      this.initializeHandshake_(story, iframeEl).then(
+        messaging => {
+          messaging.setDefaultHandler(() => Promise.resolve());
+          this.messagingFor_[iframeIdx] = messaging;
+          resolve();
+        },
+        err => {
+          console /*OK*/
+            .log({err});
+        }
+      );
+    });
   }
 
   /**
@@ -234,20 +227,11 @@ export class AmpStoryPlayer {
       return;
     }
 
-    // Display first story.
-    const firstStory = this.stories_[0];
-    const iframeIdx = firstStory[IFRAME_IDX];
-    const storyIframe = this.iframes_[iframeIdx];
-    this.layoutIframe_(firstStory, storyIframe);
-    this.displayStory_(iframeIdx);
-
-    // Pre-render next stories.
-    for (let idx = 1; idx < this.stories_.length && idx < MAX_IFRAMES; idx++) {
+    for (let idx = 0; idx < this.stories_.length && idx < MAX_IFRAMES; idx++) {
       const story = this.stories_[idx];
       const iframeIdx = story[IFRAME_IDX];
       const iframe = this.iframes_[iframeIdx];
-      this.layoutIframe_(story, iframe);
-      this.preRenderStory_(iframeIdx);
+      this.layoutIframe_(story, iframe, idx === 0 ? 'visible' : 'prerender');
     }
 
     this.isLaidOut_ = true;
@@ -265,10 +249,10 @@ export class AmpStoryPlayer {
     this.currentIdx_++;
 
     const previousStory = this.stories_[this.currentIdx_ - 1];
-    this.pauseStory_(previousStory[IFRAME_IDX]);
+    this.updateVisibilityState_(previousStory[IFRAME_IDX], 'paused');
 
     const currentStory = this.stories_[this.currentIdx_];
-    this.displayStory_(currentStory[IFRAME_IDX]);
+    this.updateVisibilityState_(currentStory[IFRAME_IDX], 'visible');
 
     const nextStoryIdx = this.currentIdx_ + 1;
     if (
@@ -276,7 +260,6 @@ export class AmpStoryPlayer {
       this.stories_[nextStoryIdx][IFRAME_IDX] === null
     ) {
       this.swapIframes_(nextStoryIdx);
-      this.preRenderStory_(this.stories_[nextStoryIdx][IFRAME_IDX]);
     }
   }
 
@@ -292,15 +275,14 @@ export class AmpStoryPlayer {
     this.currentIdx_--;
 
     const previousStory = this.stories_[this.currentIdx_ + 1];
-    this.pauseStory_(previousStory[IFRAME_IDX]);
+    this.updateVisibilityState_(previousStory[IFRAME_IDX], 'paused');
 
     const currentStory = this.stories_[this.currentIdx_];
-    this.displayStory_(currentStory[IFRAME_IDX]);
+    this.updateVisibilityState_(currentStory[IFRAME_IDX], 'visible');
 
     const nextStoryIdx = this.currentIdx_ - 1;
     if (nextStoryIdx >= 0 && this.stories_[nextStoryIdx][IFRAME_IDX] === null) {
       this.swapIframes_(nextStoryIdx, true /** backwards */);
-      this.preRenderStory_(this.stories_[nextStoryIdx][IFRAME_IDX]);
     }
   }
 
@@ -324,34 +306,36 @@ export class AmpStoryPlayer {
     detachedStory[IFRAME_IDX] = null;
 
     const nextIframe = this.iframes_[nextStory[IFRAME_IDX]];
-    this.layoutIframe_(nextStory, nextIframe);
+    this.layoutIframe_(nextStory, nextIframe, 'prerender');
     this.setUpMessagingForIframe_(nextStory, nextIframe);
   }
 
   /**
    * @param {!Element} story
    * @param {!Element} iframe
+   * @param {string} visibilityState
    * @private
    */
-  layoutIframe_(story, iframe) {
-    const {href} = this.getEncodedLocation_(story.href);
+  layoutIframe_(story, iframe, visibilityState) {
+    const {href} = this.getEncodedLocation_(story.href, visibilityState);
 
     iframe.setAttribute('src', href);
   }
 
   /**
-   * Gets encoded url for viewer usage.
+   * Gets encoded url for player usage.
    * @param {string} href
+   * @param {string} visibilityState
    * @return {!Location}
    * @private
    */
-  getEncodedLocation_(href) {
+  getEncodedLocation_(href, visibilityState = 'inactive') {
     const {location} = this.win_;
     const url = parseUrlWithA(this.cachedA_, location.href);
 
     const params = dict({
       'amp_js_v': '0.1',
-      'visibilityState': 'inactive',
+      'visibilityState': visibilityState,
       'origin': url.origin,
       'showStoryUrlInfo': '0',
       'storyPlayer': 'v0',
@@ -372,45 +356,15 @@ export class AmpStoryPlayer {
   }
 
   /**
-   * Sends a message to the story document to make it visible.
-   * @private
-   * @param {number} iframeIdx
-   */
-  displayStory_(iframeIdx) {
-    this.handshakePromises_[iframeIdx].then(() => {
-      this.messagingFor_[iframeIdx].sendRequest(
-        'visibilitychange',
-        {state: 'visible'},
-        true
-      );
-    });
-  }
-
-  /**
-   * Sends a message to the story document to pre-render it.
-   * @private
-   * @param {number} iframeIdx
-   */
-  preRenderStory_(iframeIdx) {
-    this.handshakePromises_[iframeIdx].then(() => {
-      this.messagingFor_[iframeIdx].sendRequest(
-        'visibilitychange',
-        {state: 'prerender'},
-        true
-      );
-    });
-  }
-
-  /**
-   * Sends a message to the story document to pause it.
-   * @private
+   * Updates the visibility state of the story inside the iframe.
    * @param {number} iframeIdx
+   * @param {string} visibilityState
    */
-  pauseStory_(iframeIdx) {
+  updateVisibilityState_(iframeIdx, visibilityState) {
     this.handshakePromises_[iframeIdx].then(() => {
       this.messagingFor_[iframeIdx].sendRequest(
         'visibilitychange',
-        {state: 'paused'},
+        {state: visibilityState},
         true
       );
     });
@@ -440,7 +394,7 @@ class IframePool {
    * @param {number} idx
    * @public
    */
-  addIframe(idx) {
+  addIframeIdx(idx) {
     this.iframePool_.push(idx);
   }
 
@@ -448,7 +402,7 @@ class IframePool {
    * @param {number} idx
    * @public
    */
-  addStory(idx) {
+  addStoryIdx(idx) {
     this.storyIdsWithIframe_.push(idx);
   }
 

From a14321f0ca6283cb18020d9590f1a518ab7fabb9 Mon Sep 17 00:00:00 2001
From: Enrique Marroquin <artezan@google.com>
Date: Wed, 12 Feb 2020 19:11:34 -0500
Subject: [PATCH 4/8] add tests, rename methods

---
 src/amp-story-player.js            | 22 ++++---
 test/unit/test-amp-story-player.js | 96 ++++++++++++++++++++++++++----
 2 files changed, 93 insertions(+), 25 deletions(-)

diff --git a/src/amp-story-player.js b/src/amp-story-player.js
index 41aafabe8d54..f631307c0cb2 100644
--- a/src/amp-story-player.js
+++ b/src/amp-story-player.js
@@ -296,8 +296,8 @@ export class AmpStoryPlayer {
    */
   swapIframes_(nextStoryIdx, backwards = false) {
     const detachedStoryIdx = backwards
-      ? this.iframePool_.shiftBackwards(nextStoryIdx)
-      : this.iframePool_.shiftForwards(nextStoryIdx);
+      ? this.iframePool_.rotateRight(nextStoryIdx)
+      : this.iframePool_.rotateLeft(nextStoryIdx);
 
     const detachedStory = this.stories_[detachedStoryIdx];
     const nextStory = this.stories_[nextStoryIdx];
@@ -407,14 +407,13 @@ class IframePool {
   }
 
   /**
-   * Shifts iframe allocation forwards. It takes the leftmost iframe and
-   * allocates it to the next story to the right without an iframe. It also
-   * updates the storyIdsWithIframe by removing the reference to the detached
-   * story and adds the new one.
+   * Takes the leftmost iframe and allocates it to the next story to the right
+   * without an iframe. It also updates the storyIdsWithIframe by removing the
+   * reference to the detached story and adds the new one.
    * @param {number} nextStoryIdx
    * @return {number} Index of the detached story.
    */
-  shiftForwards(nextStoryIdx) {
+  rotateLeft(nextStoryIdx) {
     const detachedStoryIdx = this.storyIdsWithIframe_.shift();
     this.storyIdsWithIframe_.push(nextStoryIdx);
 
@@ -424,14 +423,13 @@ class IframePool {
   }
 
   /**
-   * Shifts iframe allocation backwards. It takes the rightmost iframe and
-   * allocates it to the next story to the left without an iframe. It also
-   * updates the storyIdsWithIframe by removing the reference to the detached
-   * story and adds the new one.
+   * Takes the rightmost iframe and allocates it to the next story to the left
+   * without an iframe. It also updates the storyIdsWithIframe by removing the
+   * reference to the detached story and adds the new one.
    * @param {number} nextStoryIdx
    * @return {number} Index of the detached story.
    */
-  shiftBackwards(nextStoryIdx) {
+  rotateRight(nextStoryIdx) {
     const detachedStoryIdx = this.storyIdsWithIframe_.pop();
     this.storyIdsWithIframe_.unshift(nextStoryIdx);
 
diff --git a/test/unit/test-amp-story-player.js b/test/unit/test-amp-story-player.js
index 4f31b9710427..19fd586bedd0 100644
--- a/test/unit/test-amp-story-player.js
+++ b/test/unit/test-amp-story-player.js
@@ -14,7 +14,9 @@
  * limitations under the License.
  */
 
+import {AmpStoryPlayer} from '../../src/amp-story-player';
 import {AmpStoryPlayerManager} from '../../src/amp-story-player-manager';
+import {toArray} from '../../src/types';
 
 describes.realWin('AmpStoryPlayer', {amp: false}, env => {
   let win;
@@ -22,40 +24,43 @@ describes.realWin('AmpStoryPlayer', {amp: false}, env => {
   let url;
   let manager;
 
-  function buildStoryPlayer() {
+  function buildStoryPlayer(numStories = 1) {
     playerEl = win.document.createElement('amp-story-player');
-    const storyAnchor = win.document.createElement('a');
-    url =
-      'https://www-washingtonpost-com.cdn.ampproject.org/v/s/www.washingtonpost.com/graphics/2019/lifestyle/travel/amp-stories/a-locals-guide-to-what-to-eat-and-do-in-new-york-city/';
-    storyAnchor.setAttribute('href', url);
-    playerEl.appendChild(storyAnchor);
+    for (let i = 0; i < numStories; i++) {
+      const storyAnchor = win.document.createElement('a');
+      url =
+        'https://www-washingtonpost-com.cdn.ampproject.org/v/s/www.washingtonpost.com/graphics/2019/lifestyle/travel/amp-stories/a-locals-guide-to-what-to-eat-and-do-in-new-york-city/';
+      storyAnchor.setAttribute('href', url);
+      playerEl.appendChild(storyAnchor);
+    }
     win.document.body.appendChild(playerEl);
     manager = new AmpStoryPlayerManager(win);
   }
 
   beforeEach(() => {
     win = env.win;
-    buildStoryPlayer();
   });
 
   it('should build an iframe for each story', () => {
+    buildStoryPlayer();
     manager.loadPlayers();
 
     expect(playerEl.shadowRoot.querySelector('iframe')).to.exist;
   });
 
-  // TODO(#26308): unskip when messaging is enabled.
-  it.skip('should correctly append params at the end of the story url', () => {
+  it('should correctly append params at the end of the story url', () => {
+    buildStoryPlayer();
     manager.loadPlayers();
     const storyIframe = playerEl.shadowRoot.querySelector('iframe');
 
     expect(storyIframe.getAttribute('src')).to.equals(
-      url + '?amp_js_v=0.1#visibilityState=inactive&origin=about%3Asrcdoc'
+      url +
+        '?amp_js_v=0.1#visibilityState=visible&origin=about%3Asrcdoc&showStoryUrlInfo=0&storyPlayer=v0'
     );
   });
 
-  // TODO(#26308): unskip when messaging is enabled.
-  it.skip('should correctly append params at the end of a story url with existing params', () => {
+  it('should correctly append params at the end of a story url with existing params', () => {
+    buildStoryPlayer();
     url += '?testParam=true#myhash=hashValue';
     playerEl.firstElementChild.setAttribute('href', url);
 
@@ -63,7 +68,72 @@ describes.realWin('AmpStoryPlayer', {amp: false}, env => {
     const storyIframe = playerEl.shadowRoot.querySelector('iframe');
 
     expect(storyIframe.getAttribute('src')).to.equals(
-      url + '&amp_js_v=0.1#visibilityState=inactive&origin=about%3Asrcdoc'
+      url +
+        '&amp_js_v=0.1#visibilityState=visible&origin=about%3Asrcdoc&showStoryUrlInfo=0&storyPlayer=v0'
+    );
+  });
+
+  it('should set first story as visible', () => {
+    buildStoryPlayer(3);
+    manager.loadPlayers();
+
+    const storyIframes = playerEl.shadowRoot.querySelectorAll('iframe');
+    expect(storyIframes[0].getAttribute('src')).to.include(
+      '#visibilityState=visible'
+    );
+  });
+
+  it('should prerender next stories', () => {
+    buildStoryPlayer(3);
+    manager.loadPlayers();
+
+    const storyIframes = playerEl.shadowRoot.querySelectorAll('iframe');
+    expect(storyIframes[1].getAttribute('src')).to.include(
+      '#visibilityState=prerender'
     );
   });
+
+  it(
+    'should remove iframe from a story with distance > 1 from current story ' +
+      'and give it to a new story that is distance <= 1 when navigating',
+    () => {
+      buildStoryPlayer(4);
+      const stories = toArray(playerEl.querySelectorAll('a'));
+
+      const player = new AmpStoryPlayer(win, playerEl);
+      player.buildCallback();
+      player.layoutCallback();
+
+      // TODO: replace next_() with swipe.
+      player.next_();
+      expect(stories[0]['__AMP_IFRAME_IDX__']).to.eql(0);
+      expect(stories[3]['__AMP_IFRAME_IDX__']).to.eql(null);
+
+      // TODO: replace next_() with swipe.
+      player.next_();
+      expect(stories[0]['__AMP_IFRAME_IDX__']).to.eql(null);
+      expect(stories[3]['__AMP_IFRAME_IDX__']).to.eql(0);
+    }
+  );
+
+  it(
+    'should remove iframe from a story with distance > 1 from current story ' +
+      'and give it to a new story that is distance <= 1 when navigating backwards',
+    () => {
+      buildStoryPlayer(4);
+      const stories = toArray(playerEl.querySelectorAll('a'));
+
+      const player = new AmpStoryPlayer(win, playerEl);
+      player.buildCallback();
+      player.layoutCallback();
+
+      // TODO: replace next_() & previous_() with swipe.
+      player.next_();
+      player.next_();
+      player.previous_();
+
+      expect(stories[0]['__AMP_IFRAME_IDX__']).to.eql(0);
+      expect(stories[3]['__AMP_IFRAME_IDX__']).to.eql(null);
+    }
+  );
 });

From bc3b6c32274714755c9da35379566913e9bc9cac Mon Sep 17 00:00:00 2001
From: Enrique Marroquin <artezan@google.com>
Date: Thu, 13 Feb 2020 11:39:39 -0500
Subject: [PATCH 5/8] linter

---
 src/amp-story-player.js | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/src/amp-story-player.js b/src/amp-story-player.js
index f631307c0cb2..6d946730566d 100644
--- a/src/amp-story-player.js
+++ b/src/amp-story-player.js
@@ -240,6 +240,7 @@ export class AmpStoryPlayer {
   /**
    * Navigates to the next story in the player.
    * @private
+   * @visibleForTesting
    */
   next_() {
     if (this.currentIdx_ + 1 >= this.stories_.length) {
@@ -266,6 +267,7 @@ export class AmpStoryPlayer {
   /**
    * Navigates to the previous story in the player.
    * @private
+   * @visibleForTesting
    */
   previous_() {
     if (this.currentIdx_ - 1 < 0) {
@@ -282,7 +284,7 @@ export class AmpStoryPlayer {
 
     const nextStoryIdx = this.currentIdx_ - 1;
     if (nextStoryIdx >= 0 && this.stories_[nextStoryIdx][IFRAME_IDX] === null) {
-      this.swapIframes_(nextStoryIdx, true /** backwards */);
+      this.swapIframes_(nextStoryIdx, true /** reverse */);
     }
   }
 
@@ -291,11 +293,11 @@ export class AmpStoryPlayer {
    * the iframe from the story furthest away; depending where the user is
    * navigating and allocates it to a story that the user is close to seeing.
    * @param {number} nextStoryIdx
-   * @param {boolean} backwards
+   * @param {boolean} reverse
    * @private
    */
-  swapIframes_(nextStoryIdx, backwards = false) {
-    const detachedStoryIdx = backwards
+  swapIframes_(nextStoryIdx, reverse = false) {
+    const detachedStoryIdx = reverse
       ? this.iframePool_.rotateRight(nextStoryIdx)
       : this.iframePool_.rotateLeft(nextStoryIdx);
 

From 19008440c8667d35d93207e207c8ace5c8c74109 Mon Sep 17 00:00:00 2001
From: Enrique Marroquin <artezan@google.com>
Date: Tue, 18 Feb 2020 13:51:18 -0500
Subject: [PATCH 6/8] move iframepool to another file, add todos, replace for
 constants.

---
 src/OWNERS                          |   2 +-
 src/amp-story-player-iframe-pool.js |  80 +++++++++++++++++++
 src/amp-story-player.js             | 117 +++++++++-------------------
 test/unit/test-amp-story-player.js  |  22 +++---
 4 files changed, 129 insertions(+), 92 deletions(-)
 create mode 100644 src/amp-story-player-iframe-pool.js

diff --git a/src/OWNERS b/src/OWNERS
index edd2a4f862eb..e0a077f619d4 100644
--- a/src/OWNERS
+++ b/src/OWNERS
@@ -15,7 +15,7 @@
       owners: [{name: 'ampproject/wg-analytics'}],
     },
     {
-      pattern: '{amp-story-player,amp-story-player-manager}.js',
+      pattern: '{amp-story-*}.js',
       owners: [{name: 'newmuis'}, {name: 'gmajoulet'}, {name: 'enriqe'}],
     },
     {
diff --git a/src/amp-story-player-iframe-pool.js b/src/amp-story-player-iframe-pool.js
new file mode 100644
index 000000000000..4c1751ba0499
--- /dev/null
+++ b/src/amp-story-player-iframe-pool.js
@@ -0,0 +1,80 @@
+/**
+ * Copyright 2020 The AMP HTML Authors. All Rights Reserved.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS-IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+/**
+ * Manages the iframes used to host the stories inside the player. It keeps
+ * track of the iframes hosting stories, and what stories have an iframe.
+ */
+export class IframePool {
+  /** @public */
+  constructor() {
+    /** @private @const {!Array<number>} */
+    this.iframePool_ = [];
+
+    /** @private @const {!Array<number>} */
+    this.storyIdsWithIframe_ = [];
+  }
+
+  /**
+   * @param {number} idx
+   * @public
+   */
+  addIframeIdx(idx) {
+    this.iframePool_.push(idx);
+  }
+
+  /**
+   * @param {number} idx
+   * @public
+   */
+  addStoryIdx(idx) {
+    this.storyIdsWithIframe_.push(idx);
+  }
+
+  /**
+   * Takes the leftmost iframe and allocates it to the next story to the right
+   * without an iframe. It also updates the storyIdsWithIframe by removing the
+   * reference to the detached story and adds the new one.
+   * @param {number} nextStoryIdx
+   * @return {number} Index of the detached story.
+   * @public
+   */
+  rotateLeft(nextStoryIdx) {
+    const detachedStoryIdx = this.storyIdsWithIframe_.shift();
+    this.storyIdsWithIframe_.push(nextStoryIdx);
+
+    this.iframePool_.push(this.iframePool_.shift());
+
+    return detachedStoryIdx;
+  }
+
+  /**
+   * Takes the rightmost iframe and allocates it to the next story to the left
+   * without an iframe. It also updates the storyIdsWithIframe by removing the
+   * reference to the detached story and adds the new one.
+   * @param {number} nextStoryIdx
+   * @return {number} Index of the detached story.
+   * @public
+   */
+  rotateRight(nextStoryIdx) {
+    const detachedStoryIdx = this.storyIdsWithIframe_.pop();
+    this.storyIdsWithIframe_.unshift(nextStoryIdx);
+
+    this.iframePool_.unshift(this.iframePool_.pop());
+
+    return detachedStoryIdx;
+  }
+}
diff --git a/src/amp-story-player.js b/src/amp-story-player.js
index 6d946730566d..330d7153604f 100644
--- a/src/amp-story-player.js
+++ b/src/amp-story-player.js
@@ -24,6 +24,8 @@ import {
 } from './url';
 import {dict, map} from './utils/object';
 // Source for this constant is css/amp-story-player-iframe.css
+import {IframePool} from './amp-story-player-iframe-pool';
+import {VisibilityState} from './visibility-state';
 import {cssText} from '../build/amp-story-player-iframe.css';
 import {setStyle} from './style';
 import {toArray} from './types';
@@ -39,7 +41,7 @@ const LoadStateClass = {
 const MAX_IFRAMES = 3;
 
 /** @const {string} */
-const IFRAME_IDX = '__AMP_IFRAME_IDX__';
+export const IFRAME_IDX = '__AMP_IFRAME_IDX__';
 
 /**
  * Note that this is a vanilla JavaScript class and should not depend on AMP
@@ -112,10 +114,6 @@ export class AmpStoryPlayer {
 
   /** @private */
   initializeIframes_() {
-    this.stories_.forEach(story => {
-      story[IFRAME_IDX] = null;
-    });
-
     for (let idx = 0; idx < MAX_IFRAMES && idx < this.stories_.length; idx++) {
       const story = this.stories_[idx];
       this.buildIframe_(story);
@@ -231,7 +229,11 @@ export class AmpStoryPlayer {
       const story = this.stories_[idx];
       const iframeIdx = story[IFRAME_IDX];
       const iframe = this.iframes_[iframeIdx];
-      this.layoutIframe_(story, iframe, idx === 0 ? 'visible' : 'prerender');
+      this.layoutIframe_(
+        story,
+        iframe,
+        idx === 0 ? VisibilityState.VISIBLE : VisibilityState.PRERENDER
+      );
     }
 
     this.isLaidOut_ = true;
@@ -250,17 +252,23 @@ export class AmpStoryPlayer {
     this.currentIdx_++;
 
     const previousStory = this.stories_[this.currentIdx_ - 1];
-    this.updateVisibilityState_(previousStory[IFRAME_IDX], 'paused');
+    this.updateVisibilityState_(
+      previousStory[IFRAME_IDX],
+      VisibilityState.PAUSED
+    );
 
     const currentStory = this.stories_[this.currentIdx_];
-    this.updateVisibilityState_(currentStory[IFRAME_IDX], 'visible');
+    this.updateVisibilityState_(
+      currentStory[IFRAME_IDX],
+      VisibilityState.VISIBLE
+    );
 
     const nextStoryIdx = this.currentIdx_ + 1;
     if (
       nextStoryIdx < this.stories_.length &&
-      this.stories_[nextStoryIdx][IFRAME_IDX] === null
+      this.stories_[nextStoryIdx][IFRAME_IDX] === undefined
     ) {
-      this.swapIframes_(nextStoryIdx);
+      this.allocateIframeForStory_(nextStoryIdx);
     }
   }
 
@@ -277,14 +285,23 @@ export class AmpStoryPlayer {
     this.currentIdx_--;
 
     const previousStory = this.stories_[this.currentIdx_ + 1];
-    this.updateVisibilityState_(previousStory[IFRAME_IDX], 'paused');
+    this.updateVisibilityState_(
+      previousStory[IFRAME_IDX],
+      VisibilityState.PAUSED
+    );
 
     const currentStory = this.stories_[this.currentIdx_];
-    this.updateVisibilityState_(currentStory[IFRAME_IDX], 'visible');
+    this.updateVisibilityState_(
+      currentStory[IFRAME_IDX],
+      VisibilityState.VISIBLE
+    );
 
     const nextStoryIdx = this.currentIdx_ - 1;
-    if (nextStoryIdx >= 0 && this.stories_[nextStoryIdx][IFRAME_IDX] === null) {
-      this.swapIframes_(nextStoryIdx, true /** reverse */);
+    if (
+      nextStoryIdx >= 0 &&
+      this.stories_[nextStoryIdx][IFRAME_IDX] === undefined
+    ) {
+      this.allocateIframeForStory_(nextStoryIdx, true /** reverse */);
     }
   }
 
@@ -296,7 +313,7 @@ export class AmpStoryPlayer {
    * @param {boolean} reverse
    * @private
    */
-  swapIframes_(nextStoryIdx, reverse = false) {
+  allocateIframeForStory_(nextStoryIdx, reverse = false) {
     const detachedStoryIdx = reverse
       ? this.iframePool_.rotateRight(nextStoryIdx)
       : this.iframePool_.rotateLeft(nextStoryIdx);
@@ -305,10 +322,10 @@ export class AmpStoryPlayer {
     const nextStory = this.stories_[nextStoryIdx];
 
     nextStory[IFRAME_IDX] = detachedStory[IFRAME_IDX];
-    detachedStory[IFRAME_IDX] = null;
+    detachedStory[IFRAME_IDX] = undefined;
 
     const nextIframe = this.iframes_[nextStory[IFRAME_IDX]];
-    this.layoutIframe_(nextStory, nextIframe, 'prerender');
+    this.layoutIframe_(nextStory, nextIframe, VisibilityState.PRERENDER);
     this.setUpMessagingForIframe_(nextStory, nextIframe);
   }
 
@@ -331,7 +348,7 @@ export class AmpStoryPlayer {
    * @return {!Location}
    * @private
    */
-  getEncodedLocation_(href, visibilityState = 'inactive') {
+  getEncodedLocation_(href, visibilityState = VisibilityState.INACTIVE) {
     const {location} = this.win_;
     const url = parseUrlWithA(this.cachedA_, location.href);
 
@@ -361,6 +378,7 @@ export class AmpStoryPlayer {
    * Updates the visibility state of the story inside the iframe.
    * @param {number} iframeIdx
    * @param {string} visibilityState
+   * @private
    */
   updateVisibilityState_(iframeIdx, visibilityState) {
     this.handshakePromises_[iframeIdx].then(() => {
@@ -377,66 +395,3 @@ self.onload = () => {
   const manager = new AmpStoryPlayerManager(self);
   manager.loadPlayers();
 };
-
-/**
- * Manages the iframes used to host the stories inside the player. It keeps
- * track of the iframes hosting stories, and what stories have an iframe.
- */
-class IframePool {
-  /** @public */
-  constructor() {
-    /** @private @const {!Array<number>} */
-    this.iframePool_ = [];
-
-    /** @private @const {!Array<number>} */
-    this.storyIdsWithIframe_ = [];
-  }
-
-  /**
-   * @param {number} idx
-   * @public
-   */
-  addIframeIdx(idx) {
-    this.iframePool_.push(idx);
-  }
-
-  /**
-   * @param {number} idx
-   * @public
-   */
-  addStoryIdx(idx) {
-    this.storyIdsWithIframe_.push(idx);
-  }
-
-  /**
-   * Takes the leftmost iframe and allocates it to the next story to the right
-   * without an iframe. It also updates the storyIdsWithIframe by removing the
-   * reference to the detached story and adds the new one.
-   * @param {number} nextStoryIdx
-   * @return {number} Index of the detached story.
-   */
-  rotateLeft(nextStoryIdx) {
-    const detachedStoryIdx = this.storyIdsWithIframe_.shift();
-    this.storyIdsWithIframe_.push(nextStoryIdx);
-
-    this.iframePool_.push(this.iframePool_.shift());
-
-    return detachedStoryIdx;
-  }
-
-  /**
-   * Takes the rightmost iframe and allocates it to the next story to the left
-   * without an iframe. It also updates the storyIdsWithIframe by removing the
-   * reference to the detached story and adds the new one.
-   * @param {number} nextStoryIdx
-   * @return {number} Index of the detached story.
-   */
-  rotateRight(nextStoryIdx) {
-    const detachedStoryIdx = this.storyIdsWithIframe_.pop();
-    this.storyIdsWithIframe_.unshift(nextStoryIdx);
-
-    this.iframePool_.unshift(this.iframePool_.pop());
-
-    return detachedStoryIdx;
-  }
-}
diff --git a/test/unit/test-amp-story-player.js b/test/unit/test-amp-story-player.js
index 19fd586bedd0..e07ed410e725 100644
--- a/test/unit/test-amp-story-player.js
+++ b/test/unit/test-amp-story-player.js
@@ -14,7 +14,7 @@
  * limitations under the License.
  */
 
-import {AmpStoryPlayer} from '../../src/amp-story-player';
+import {AmpStoryPlayer, IFRAME_IDX} from '../../src/amp-story-player';
 import {AmpStoryPlayerManager} from '../../src/amp-story-player-manager';
 import {toArray} from '../../src/types';
 
@@ -100,19 +100,20 @@ describes.realWin('AmpStoryPlayer', {amp: false}, env => {
       buildStoryPlayer(4);
       const stories = toArray(playerEl.querySelectorAll('a'));
 
+      // TODO(#26308): Replace with manager.loadPlayers() when swipe is enabled.
       const player = new AmpStoryPlayer(win, playerEl);
       player.buildCallback();
       player.layoutCallback();
 
-      // TODO: replace next_() with swipe.
+      // TODO(#26308): replace next_() with swipe.
       player.next_();
-      expect(stories[0]['__AMP_IFRAME_IDX__']).to.eql(0);
-      expect(stories[3]['__AMP_IFRAME_IDX__']).to.eql(null);
+      expect(stories[0][IFRAME_IDX]).to.eql(0);
+      expect(stories[3][IFRAME_IDX]).to.eql(undefined);
 
-      // TODO: replace next_() with swipe.
+      // TODO(#26308): replace next_() with swipe.
       player.next_();
-      expect(stories[0]['__AMP_IFRAME_IDX__']).to.eql(null);
-      expect(stories[3]['__AMP_IFRAME_IDX__']).to.eql(0);
+      expect(stories[0][IFRAME_IDX]).to.eql(undefined);
+      expect(stories[3][IFRAME_IDX]).to.eql(0);
     }
   );
 
@@ -123,17 +124,18 @@ describes.realWin('AmpStoryPlayer', {amp: false}, env => {
       buildStoryPlayer(4);
       const stories = toArray(playerEl.querySelectorAll('a'));
 
+      // TODO(#26308): Replace with manager.loadPlayers() when swipe is enabled.
       const player = new AmpStoryPlayer(win, playerEl);
       player.buildCallback();
       player.layoutCallback();
 
-      // TODO: replace next_() & previous_() with swipe.
+      // TODO(#26308): replace next_() & previous_() with swipe.
       player.next_();
       player.next_();
       player.previous_();
 
-      expect(stories[0]['__AMP_IFRAME_IDX__']).to.eql(0);
-      expect(stories[3]['__AMP_IFRAME_IDX__']).to.eql(null);
+      expect(stories[0][IFRAME_IDX]).to.eql(0);
+      expect(stories[3][IFRAME_IDX]).to.eql(undefined);
     }
   );
 });

From 336c08bb4f3d8ba78bdf1ccbf06921e2f66f1ca5 Mon Sep 17 00:00:00 2001
From: Enrique Marroquin <artezan@google.com>
Date: Wed, 19 Feb 2020 11:07:13 -0500
Subject: [PATCH 7/8] jsdocs

---
 src/amp-story-player-iframe-pool.js |  8 ++++----
 src/amp-story-player.js             | 10 +++++-----
 2 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/src/amp-story-player-iframe-pool.js b/src/amp-story-player-iframe-pool.js
index 4c1751ba0499..930e0fd8f37f 100644
--- a/src/amp-story-player-iframe-pool.js
+++ b/src/amp-story-player-iframe-pool.js
@@ -45,7 +45,7 @@ export class IframePool {
   }
 
   /**
-   * Takes the leftmost iframe and allocates it to the next story to the right
+   * Takes the first iframe in the array and allocates it to the next story
    * without an iframe. It also updates the storyIdsWithIframe by removing the
    * reference to the detached story and adds the new one.
    * @param {number} nextStoryIdx
@@ -62,9 +62,9 @@ export class IframePool {
   }
 
   /**
-   * Takes the rightmost iframe and allocates it to the next story to the left
-   * without an iframe. It also updates the storyIdsWithIframe by removing the
-   * reference to the detached story and adds the new one.
+   * Takes the last iframe in the array and allocates it to the next incoming
+   * story without an iframe. It also updates the storyIdsWithIframe by removing
+   * the reference to the detached story and adds the new one.
    * @param {number} nextStoryIdx
    * @return {number} Index of the detached story.
    * @public
diff --git a/src/amp-story-player.js b/src/amp-story-player.js
index 330d7153604f..86af0a4336c2 100644
--- a/src/amp-story-player.js
+++ b/src/amp-story-player.js
@@ -254,7 +254,7 @@ export class AmpStoryPlayer {
     const previousStory = this.stories_[this.currentIdx_ - 1];
     this.updateVisibilityState_(
       previousStory[IFRAME_IDX],
-      VisibilityState.PAUSED
+      VisibilityState.INACTIVE
     );
 
     const currentStory = this.stories_[this.currentIdx_];
@@ -287,7 +287,7 @@ export class AmpStoryPlayer {
     const previousStory = this.stories_[this.currentIdx_ + 1];
     this.updateVisibilityState_(
       previousStory[IFRAME_IDX],
-      VisibilityState.PAUSED
+      VisibilityState.INACTIVE
     );
 
     const currentStory = this.stories_[this.currentIdx_];
@@ -332,7 +332,7 @@ export class AmpStoryPlayer {
   /**
    * @param {!Element} story
    * @param {!Element} iframe
-   * @param {string} visibilityState
+   * @param {!VisibilityState} visibilityState
    * @private
    */
   layoutIframe_(story, iframe, visibilityState) {
@@ -344,7 +344,7 @@ export class AmpStoryPlayer {
   /**
    * Gets encoded url for player usage.
    * @param {string} href
-   * @param {string} visibilityState
+   * @param {VisibilityState=} visibilityState
    * @return {!Location}
    * @private
    */
@@ -377,7 +377,7 @@ export class AmpStoryPlayer {
   /**
    * Updates the visibility state of the story inside the iframe.
    * @param {number} iframeIdx
-   * @param {string} visibilityState
+   * @param {!VisibilityState} visibilityState
    * @private
    */
   updateVisibilityState_(iframeIdx, visibilityState) {

From deb781e58003fddd809572a6a4c600d966142e22 Mon Sep 17 00:00:00 2001
From: Enrique Marroquin <artezan@google.com>
Date: Wed, 19 Feb 2020 11:13:27 -0500
Subject: [PATCH 8/8] replace rotateleft and right

---
 src/amp-story-player-iframe-pool.js | 4 ++--
 src/amp-story-player.js             | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/amp-story-player-iframe-pool.js b/src/amp-story-player-iframe-pool.js
index 930e0fd8f37f..c8832dce193b 100644
--- a/src/amp-story-player-iframe-pool.js
+++ b/src/amp-story-player-iframe-pool.js
@@ -52,7 +52,7 @@ export class IframePool {
    * @return {number} Index of the detached story.
    * @public
    */
-  rotateLeft(nextStoryIdx) {
+  rotateFirst(nextStoryIdx) {
     const detachedStoryIdx = this.storyIdsWithIframe_.shift();
     this.storyIdsWithIframe_.push(nextStoryIdx);
 
@@ -69,7 +69,7 @@ export class IframePool {
    * @return {number} Index of the detached story.
    * @public
    */
-  rotateRight(nextStoryIdx) {
+  rotateLast(nextStoryIdx) {
     const detachedStoryIdx = this.storyIdsWithIframe_.pop();
     this.storyIdsWithIframe_.unshift(nextStoryIdx);
 
diff --git a/src/amp-story-player.js b/src/amp-story-player.js
index 86af0a4336c2..f8ad4330603c 100644
--- a/src/amp-story-player.js
+++ b/src/amp-story-player.js
@@ -315,8 +315,8 @@ export class AmpStoryPlayer {
    */
   allocateIframeForStory_(nextStoryIdx, reverse = false) {
     const detachedStoryIdx = reverse
-      ? this.iframePool_.rotateRight(nextStoryIdx)
-      : this.iframePool_.rotateLeft(nextStoryIdx);
+      ? this.iframePool_.rotateLast(nextStoryIdx)
+      : this.iframePool_.rotateFirst(nextStoryIdx);
 
     const detachedStory = this.stories_[detachedStoryIdx];
     const nextStory = this.stories_[nextStoryIdx];