Setting Router Refactor: Implement RouteObserverBehavior.

chrome/browser/resources/settings/route.js

Index: chrome/browser/resources/settings/route.js
diff --git a/chrome/browser/resources/settings/route.js b/chrome/browser/resources/settings/route.js
index 0ef6c3fab622800f2120279fc2d99e7fcd9ec704..802ecc7f637a1c252a633207da4da49f4488d224 100644
--- a/chrome/browser/resources/settings/route.js
+++ b/chrome/browser/resources/settings/route.js
@@ -263,15 +263,84 @@ cr.define('settings', function() {
   r.DETAILED_BUILD_INFO.section = 'about';
 </if>
 
+  var routeObservers_ = new Set();
+
+  /** @polymerBehavior */
+  var RouteObserverBehavior = {
+    /** @override */
+    attached: function() {
+      assert(!routeObservers_.has(this));
+      routeObservers_.add(this);
+    },
+
+    /** @override */
+    detached: function() {
+      assert(routeObservers_.has(this));
+      routeObservers_.delete(this);

[Dan Beam, 2016/07/30 01:13:02]

assert(routeObservers_.delete(this));

[tommycli, 2016/08/01 17:33:24]

Done.

+    },
+

[Dan Beam, 2016/07/30 01:13:02]

nit: /** @abstract */

[tommycli, 2016/08/01 17:33:24]

Done. Ideally I could use @override in settings-router.js (and other classes
that mixin this behavior) but it doesn't seem to work. If I try, I get:

(ERROR) Error in: settings_router.js
##
/mnt/src/chromium/src/chrome/browser/resources/settings/settings_page/settings_router.js:34:
ERROR - property currentRouteChanged not defined on any superclass of
SettingsRouterElement
##   currentRouteChanged: function() {
##                        ^

+    currentRouteChanged_: function() {

[Dan Beam, 2016/07/30 01:13:02]

this should not be named with _, as that is for @private (this should arguably
be @protected or just public)

[tommycli, 2016/08/01 17:33:24]

Done.

+      assertNotReached('Elements that mixin RouteObserverBehavior should ' +
+                       'override currentRouteChanged_.');

[Dan Beam, 2016/07/30 01:13:02]

i don't know that this message adds much, but I guess you feel it does

otherwise the code could just be:

  currentRouteChanged: assertNotReached,

[tommycli, 2016/08/01 17:33:24]

Done.

+    },
+  };
+
+  /**
+   * Returns the matching canonical route, or the default route if none matches.
+   * @param {string} path
+   * @return {!settings.Route}
+   * @private
+   */
+  var getRouteFor = function(path) {

[Dan Beam, 2016/07/30 01:13:02]

in my utopia:

return Object.values(Routes).find(r => r.path == path) || Routes.Basic;

[tommycli, 2016/08/01 17:33:24]

Acknowledged.

+    // TODO(tommycli): Use Object.values once Closure compilation supports it.
+    var matchingKey = Object.keys(Route).find(function(key) {
+      return Route[key].path == path;
+    });
+
+    if (!matchingKey)
+      return Route.BASIC;
+
+    return Route[matchingKey];

[Dan Beam, 2016/07/30 01:13:02]

return Route[matchingKey] || Route.BASIC; 

[tommycli, 2016/08/01 17:33:24]

Done.

[tommycli, 2016/08/01 20:06:35]

FYI, in a different patch we changed the behavior to return null for a missing
route, and that got merged in.

+  };
+
   /**
-   * Use this function (and only this function) to navigate within Settings.
-   * This function is set by settings-router once it is created.
-   * @type {?function(!settings.Route):void}
+   * The current active route. This may only be updated via the global
+   * function settings.navigateTo.
+   * @private {!settings.Route}
    */
-  var navigateTo = null;
+  var currentRoute_ = getRouteFor(window.location.pathname);
+
+  /** @return {!settings.Route} */
+  var getCurrentRoute = function() { return currentRoute_; };
+
+  /**
+   * Navigates to a canonical route.
+   * @param {!settings.Route} route
+   * @private
+   */
+  var navigateTo = function(route) {
+    assert(!!route);

[Dan Beam, 2016/07/30 01:13:02]

assert(!!thing) is the same as assert(thing)

the only reason you need to do !! is when you're using assertTrue(!!truthy) in
unit tests as it checks for === true or something

[Dan Beam, 2016/07/30 01:13:02]

nit: if (assert(route) == currentRoute_)

[tommycli, 2016/08/01 17:33:23]

Done.

[tommycli, 2016/08/01 17:33:23]

Done.

+    if (route == currentRoute_)
+      return;
+
+    window.history.pushState(undefined, document.title, route.path);
+    currentRoute_ = route;
+    for (var observer of routeObservers_)
+      observer.currentRouteChanged_();

[Dan Beam, 2016/07/30 01:13:02]

nit:

var setCurrentRoute = function(route) {
  currentRoute_ = route;
  for (var observer of routeObservers_)
    observer.currentRouteChanged_();
};

and call from navigateTo and 'popstate'

[tommycli, 2016/08/01 17:33:23]

Done.

+  };
+
+  window.addEventListener('popstate', function(event) {
+    // On pop state, do not push the state onto the window.history again.
+    currentRoute_ = getRouteFor(window.location.pathname);
+    for (var observer of routeObservers_)
+      observer.currentRouteChanged_();
+  });
 
   return {
     Route: Route,
+    RouteObserverBehavior: RouteObserverBehavior,
+    getRouteFor: getRouteFor,
+    getCurrentRoute: getCurrentRoute,
     navigateTo: navigateTo,
   };
 });