[MD Extensions] Add support for URL navigation

chrome/browser/resources/md_extensions/navigation_helper.js

+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+/**
+ * The different pages that can be shown at a time.
+ * Note: This must remain in sync with the order in manager.html!
+ * @enum {string}
+ */
+var Page = {
+  LIST: '0',
+  DETAILS: '1',
+  SHORTCUTS: '2',
+  ERRORS: '3',
+};
+
+/** @enum {string} */
+var Dialog = {
+  OPTIONS: 'options',
+};
+
+/** @typedef {{page: Page,
+               id: (string|undefined),
+               subpage: (!Dialog|undefined)}} */
+var PageState;
+
+cr.define('extensions', function() {
+  'use strict';
+
+  /**
+   * A helper object to manage in-page navigations. Since the extensions page
+   * needs to support different urls for different subpages (like the details
+   * page), we use this object to manage the history and url conversions.
+   * @param {!function(!PageState:void)} changePage A function to call when the

[michaelpg, 2017/04/18 22:03:11]

It also takes a boolean (maybe)

[Devlin, 2017/04/27 13:41:26]

It doesn't.  Below was a mistake.

+   *     page has changed as a result of the user going back or forward in
+   *     history; called with the new active page.
+   * @constructor */
+  function NavigationHelper(changePage) {

[michaelpg, 2017/04/18 22:03:12]

nit: changePageObserver? onChangePage?

[Devlin, 2017/04/27 13:41:26]

onHistoryChange, to match below?

+    this.changePage_ = changePage;
+    window.addEventListener('popstate', this.onPopState_.bind(this));
+  }
+
+  NavigationHelper.prototype = {
+    /** @private */
+    onPopState_: function() {
+      this.changePage_(this.getCurrentPage(), false);

[michaelpg, 2017/04/18 22:03:12]

Is |false| unused here?

I think the interface is clearer if this function is only called with the new
page state. We already know it means that the user navigates the browser
somewhere; it should be up to the manager how to handle that.

And I think that's actually what you already have, from where Manager constructs
NavigationHelper -- it's a detail the Manager should, and already does, keep
internally (by currying a |true| argument)

[Devlin, 2017/04/27 13:41:25]

Yes, false is unused and shouldn't be there.  Removed.

+    },
+
+    /**
+     * Returns the page that should be displayed for the current URL.
+     * @return {!PageState}
+     */
+    getCurrentPage: function() {
+      if (location.search == '' && location.pathname == '/')
+        return {page: Page.LIST};
+
+      var search = new URLSearchParams(location.search);
+      var id = search.get('id');
+      if (id)
+        return {page: Page.DETAILS, id: id};
+      id = search.get('options');
+      if (id)
+        return {page: Page.DETAILS, id: id, subpage: Dialog.OPTIONS};
+      id = search.get('errors');
+      if (id)
+        return {page: Page.ERRORS, id: id};
+
+      if (location.pathname == '/shortcuts')
+        return {page: Page.SHORTCUTS};
+
+      return {page: Page.LIST};
+    },
+
+    /**
+     * Called when a page changes, and pushes state to history to reflect it.
+     * @param {!PageState} entry
+     */
+    onPageChange: function(entry) {

[michaelpg, 2017/04/18 22:03:12]

If you don't mind some more naming bikeshedding, how about "updateHistory"? It's
not actually observing an event (unlike onPopState_), it's a public method to be
called by clients to do a thing.

That would also make me less likely to confuse the purpose of this.changePage_
and this.onPageChange.

[Devlin, 2017/04/27 13:41:26]

Sure!  Done.

+      var relative;
+      switch (entry.page) {
+        case Page.LIST:
+          relative = '/';
+          break;
+        case Page.DETAILS:
+          if (entry.subpage) {
+            assert(entry.subpage == Dialog.OPTIONS);
+            relative = '/?options=' + entry.id;
+          } else {
+            relative = '/?id=' + entry.id;
+          }
+          break;
+        case Page.SHORTCUTS:
+          relative = '/shortcuts';
+          break;
+        case Page.ERRORS:
+          relative = '/?errors=' + entry.id;
+      }

[michaelpg, 2017/04/18 22:03:12]

instead of `if (relative)`, maybe `default: return;` to make the control flow
clearer. also something the C++ style guide mandates, but that's not directly
relevant because we don't have a real compiler.

[Devlin, 2017/04/27 13:41:26]

I think we can actually assert(relative) here since all Page.* options are
handled, and C++ wouldn't require it for the same reason.

+      if (relative) {
+        var state = {url: relative};
+        var currentPage = this.getCurrentPage();
+        var isDialogNavigation =
+            currentPage.page == entry.page &&
+            currentPage.id == entry.id;
+        // Navigating to a dialog doesn't visually change pages; it just opens
+        // 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)
+          history.replaceState(state, '', relative);
+        else
+          history.pushState(state, '', relative);
+

[michaelpg, 2017/04/18 22:03:12]

remove line

[Devlin, 2017/04/27 13:41:26]

Whoops, done.

+      }
+    },
+  };
+
+  return {NavigationHelper: NavigationHelper};
+});