Review changes

ampproject · Jan 22, 2020 · 202ed6d · 202ed6d
1 parent 6fb9245
commit 202ed6d
Show file tree

Hide file tree

Showing 2 changed files with 25 additions and 23 deletions.
diff --git a/extensions/amp-next-page/1.0/service.js b/extensions/amp-next-page/1.0/service.js
@@ -43,7 +43,7 @@ import VisibilityObserver, {ViewportRelativePos} from './visibility-observer';
 const TAG = 'amp-next-page';
 const PRERENDER_VIEWPORT_COUNT = 3;
 const NEAR_BOTTOM_VIEWPORT_COUNT = 1;
-const FORGET_PAGE_COUNT = 5;
+const PAUSE_PAGE_COUNT = 5;
 
 const NEXT_PAGE_CLASS = 'i-amphtml-next-page';
 const DOC_CLASS = 'i-amphtml-next-page-document';
@@ -221,7 +221,7 @@ export class NextPageService {
           page.setVisibility(VisibilityState.VISIBLE);
         }
         this.hidePreviousPages_(index);
-        this.resumeForgottenPages_(index);
+        this.resumePausedPages_(index);
       } else if (page.relativePos === ViewportRelativePos.OUTSIDE_VIEWPORT) {
         if (page.isVisible()) {
           page.setVisibility(VisibilityState.HIDDEN);
@@ -247,19 +247,19 @@ export class NextPageService {
   /**
    * Makes sure that all pages preceding the current page are
    * marked hidden if they are out of the viewport and additionally
-   * paused/forgotten if they are too far from the current page
+   * paused if they are too far from the current page
    * @param {number} index index of the page to start at
-   * @param {number=} forgetPageCountForTesting
+   * @param {number=} pausePageCountForTesting
    * @return {!Promise}
    * @private
    */
-  hidePreviousPages_(index, forgetPageCountForTesting) {
+  hidePreviousPages_(index, pausePageCountForTesting) {
     // The distance (in pages) to the currently visible page after which
     // we start unloading pages from memory
-    const forgetPageCount =
-      forgetPageCountForTesting === undefined
-        ? FORGET_PAGE_COUNT
-        : forgetPageCountForTesting;
+    const pausePageCount =
+      pausePageCountForTesting === undefined
+        ? PAUSE_PAGE_COUNT
+        : pausePageCountForTesting;
 
     const scrollingDown = this.scrollDirection_ === Direction.DOWN;
     // Hide the host (first) page if needed
@@ -287,7 +287,7 @@ export class NextPageService {
             page.setVisibility(VisibilityState.HIDDEN);
           }
           // Pause those that are too far away
-          if (away >= forgetPageCount) {
+          if (away >= pausePageCount) {
             return page.pause();
           }
         })
@@ -296,24 +296,27 @@ export class NextPageService {
 
   /**
    * Makes sure that all pages that are a few pages away from the
-   * currently visible page are re-inserted (if forgotten) and
+   * currently visible page are re-inserted (if paused) and
    * ready to become visible soon
    * @param {number} index index of the page to start at
-   * @param {number=} forgetPageCountForTesting
+   * @param {number=} pausePageCountForTesting
    * @private
    */
-  resumeForgottenPages_(index, forgetPageCountForTesting) {
+  resumePausedPages_(index, pausePageCountForTesting) {
     // The distance (in pages) to the currently visible page after which
     // we start unloading pages from memory
-    const forgetPageCount =
-      forgetPageCountForTesting === undefined
-        ? FORGET_PAGE_COUNT
-        : forgetPageCountForTesting;
+    const pausePageCount =
+      pausePageCountForTesting === undefined
+        ? PAUSE_PAGE_COUNT
+        : pausePageCountForTesting;
 
     // Get all the pages that should be resumed
     const nearViewportPages = this.pages_
       .slice(1) // Ignore host page
-      .slice(index - forgetPageCount - 1, index + forgetPageCount + 1)
+      .slice(
+        Math.max(0, index - pausePageCount - 1),
+        Math.min(this.pages_.length, index + pausePageCount + 1)
+      )
       .filter(page => page.isPaused());
 
     nearViewportPages.forEach(page => page.resume());
@@ -644,7 +647,6 @@ export class NextPageService {
           // we use initialUrl since the url can get updated if
           // the page issues a redirect
           if (this.pages_.some(p => p.initialUrl == page.url)) {
-            user().warn(TAG, 'Recommendation already queued');
             return;
           }
           // Queue the page for fetching

diff --git a/extensions/amp-next-page/1.0/test/test-amp-next-page.js b/extensions/amp-next-page/1.0/test/test-amp-next-page.js
@@ -375,7 +375,7 @@ describes.realWin(
 
         await service.hidePreviousPages_(
           0 /** index */,
-          0 /** forgetPageCountForTesting */
+          0 /** pausePageCountForTesting */
         );
 
         // Internally changes the state to paused
@@ -415,17 +415,17 @@ describes.realWin(
         service.scrollDirection_ = Direction.UP;
         await service.hidePreviousPages_(
           0 /** index */,
-          0 /** forgetPageCountForTesting */
+          0 /** pausePageCountForTesting */
         );
         expect(service.pages_[2].state_).to.equal(PageState.PAUSED);
         expect(service.pages_[2].visibilityState_).to.equal(
           VisibilityState.HIDDEN
         );
 
         service.scrollDirection_ = Direction.DOWN;
-        await service.resumeForgottenPages_(
+        await service.resumePausedPages_(
           1 /** index */,
-          0 /** forgetPageCountForTesting */
+          0 /** pausePageCountForTesting */
         );
 
         // Replaces the inserted placeholder with the page's content