[MD extensions] clear removed extension id url from history

This CL fixes an issue with the page history. If an extension is removed
and the user tries to navigate back to the details page for that
extension, they may get caught in a loop (unable to go back). This CL
will navigate back or replace the history element (there are two different
cases: removing an extension from the details page; or navigating to an
invalid extension ID).

FYI, the case where the extension is removed from the main list page is
not affected (it doesn't have the issue).

Bug: 789891
Cq-Include-Trybots: master.tryserver.chromium.linux:closure_compilation
Change-Id: I45eb0f2917452da55e0e07c67356e24ae5bd56bb
Reviewed-on: https://chromium-review.googlesource.com/801975
Commit-Queue: Dave Schuyler <dschuyler@chromium.org>
Reviewed-by: Demetrios Papadopoulos <dpapad@chromium.org>
Cr-Commit-Position: refs/heads/master@{#521483}

chrome/browser/resources/md_extensions/manager.js

@@ -384,7 +384,7 @@ cr.define('extensions', function() {
            this.currentPage_.page == Page.ERRORS) &&
           this.currentPage_.extensionId == itemId) {
         // Leave the details page (the 'list' page is a fine choice).
-        extensions.navigation.navigateTo({page: Page.LIST});
+        extensions.navigation.replaceWith({page: Page.LIST});
       }
     },
 
@@ -428,7 +428,7 @@ cr.define('extensions', function() {
         data = this.getData_(newPage.extensionId);
         if (!data) {
           // Attempting to view an invalid (removed?) app or extension ID.
-          extensions.navigation.navigateTo({page: Page.LIST});
+          extensions.navigation.replaceWith({page: Page.LIST});
           return;
         }
       }

chrome/browser/resources/md_extensions/navigation_helper.js

@@ -29,6 +29,16 @@ let PageState;
 cr.define('extensions', function() {
   'use strict';
 
+  /**
+   * @param {!PageState} a
+   * @param {!PageState} b
+   * @return {boolean} Whether a and b are equal.
+   */
+  function isPageStateEqual(a, b) {
+    return a.page == b.page && a.subpage == b.subpage &&
+        a.extensionId == b.extensionId;
+  }
+
   /**
    * Regular expression that captures the leading slash, the content and the
    * trailing slash in three different groups.
@@ -51,6 +61,9 @@ cr.define('extensions', function() {
       /** @private {!Map<number, function(!PageState)>} */
       this.listeners_ = new Map();
 
+      /** @private {!PageState} */
+      this.previousPage_;
+
       window.addEventListener('popstate', () => {
         this.notifyRouteChanged_(this.getCurrentPage());
       });
@@ -137,21 +150,33 @@ cr.define('extensions', function() {
      */
     navigateTo(newPage) {
       let currentPage = this.getCurrentPage();
-      if (currentPage && currentPage.page == newPage.page &&
-          currentPage.subpage == newPage.subpage &&
-          currentPage.extensionId == newPage.extensionId) {
+      if (currentPage && isPageStateEqual(currentPage, newPage)) {
         return;
       }
 
-      this.updateHistory(newPage);
+      this.updateHistory(newPage, false /* replaceState */);
+      this.notifyRouteChanged_(newPage);
+    }
+
+    /**
+     * @param {!PageState} newPage the page to replace the current page with.
+     */
+    replaceWith(newPage) {
+      this.updateHistory(newPage, true /* replaceState */);
+      if (this.previousPage_ && isPageStateEqual(this.previousPage_, newPage)) {
+        // Skip the duplicate history entry.
+        history.back();
+        return;
+      }
       this.notifyRouteChanged_(newPage);
     }
 
     /**
      * Called when a page changes, and pushes state to history to reflect it.
      * @param {!PageState} entry
+     * @param {boolean} replaceState
      */
-    updateHistory(entry) {
+    updateHistory(entry, replaceState) {
       let path;
       switch (entry.page) {
         case Page.LIST:
@@ -181,10 +206,12 @@ cr.define('extensions', function() {
       // a dialog. As such, we replace state rather than pushing a new state
       // on the stack so that hitting the back button doesn't just toggle the
       // dialog.
-      if (isDialogNavigation)
+      if (replaceState || isDialogNavigation) {
         history.replaceState(state, '', path);
-      else
+      } else {
+        this.previousPage_ = currentPage;
         history.pushState(state, '', path);
+      }
     }
   }
 

chrome/test/data/webui/extensions/extension_manager_unit_test.js

@@ -175,7 +175,8 @@ cr.define('extension_manager_tests', function() {
 
       service.itemStateChangedTarget.callListeners({
         event_type: chrome.developerPrivate.EventType.UNINSTALLED,
-        // When an extension is unistalled, only the ID is passed back from C++.
+        // When an extension is uninstalled, only the ID is passed back from
+        // C++.
         item_id: extension.id,
       });
 
@@ -189,7 +190,7 @@ cr.define('extension_manager_tests', function() {
 
     // Test case where an extension is uninstalled from the details view. User
     // should be forwarded to the main view.
-    test(assert(TestNames.UninstallFromDetails), function() {
+    test(assert(TestNames.UninstallFromDetails), function(done) {
       const extension = extension_test_util.createExtensionInfo(
           {location: 'FROM_STORE', name: 'Alpha', id: 'a'.repeat(32)});
       simulateExtensionInstall(extension);
@@ -199,13 +200,17 @@ cr.define('extension_manager_tests', function() {
       Polymer.dom.flush();
       assertViewActive('extensions-detail-view');
 
+      window.addEventListener('popstate', () => {
+        assertViewActive('extensions-item-list');
+        done();
+      });
+
       service.itemStateChangedTarget.callListeners({
         event_type: chrome.developerPrivate.EventType.UNINSTALLED,
-        // When an extension is unistalled, only the ID is passed back from C++.
+        // When an extension is uninstalled, only the ID is passed back from
+        // C++.
         item_id: extension.id,
       });
-
-      assertViewActive('extensions-item-list');
     });
 
     test(assert(TestNames.ToggleIncognitoMode), function() {

chrome/test/data/webui/extensions/extension_navigation_helper_test.js

@@ -157,6 +157,12 @@ cr.define('extension_navigation_helper_tests', function() {
       navigationHelper.updateHistory(
           {page: Page.DETAILS, extensionId: id2});
       expectEquals(++expectedLength, history.length);
+
+      // Using replaceWith, which passes true for replaceState should not push
+      // state.
+      navigationHelper.updateHistory(
+          {page: Page.DETAILS, extensionId: id1}, true /* replaceState */);
+      expectEquals(expectedLength, history.length);
     });
 
     test(assert(TestNames.SupportedRoutes), function() {