MD Settings: Allow trailing slashes in otherwise valid URLs.

chrome/browser/resources/settings/route.js

 // Copyright 2016 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.
 
 cr.define('settings', function() {
   /**
    * Class for navigable routes. May only be instantiated within this file.
    * @constructor
    * @param {string} path
    * @private
@@ skipping 220 matching lines @@
      * @param {!settings.Route|undefined} opt_newRoute
      * @param {!settings.Route|undefined} opt_oldRoute
      * @abstract
      */
     currentRouteChanged: function(opt_newRoute, opt_oldRoute) {
       assertNotReached();
     },
   };
 
   /**
-   * Returns the matching canonical route, or null if none matches.
    * @param {string} path
-   * @return {?settings.Route}
+   * @return {?settings.Route} The matching canonical route, or null if none
+   *     matches.
    */
   var getRouteForPath = function(path) {
     // TODO(tommycli): Use Object.values once Closure compilation supports it.
     var matchingKey = Object.keys(Route).find(function(key) {
-      return Route[key].path == path;
+      // Allow trailing slash in paths.
+      return Route[key].path == (path == '/' ? path : path.replace(/\/$/, ''));

[dpapad, 2016/11/23 22:57:20]

As I am adding tests, I realized that getRouteForPath('//') returns BASIC
instead of null. Looking for a way to tweak the regexp to fix this.

     });
 
-    return Route[matchingKey] || null;
+    return !!matchingKey ? Route[matchingKey] : null;

[tommycli, 2016/11/23 22:33:46]

I assume doing Route[undefined] is bad? (when there is no matching key)?

[dpapad, 2016/11/23 22:57:20]

Yes. The effective behavior was the same, a fals-y value was returned by
getRouteForPath(), but using |undefined| as a key seemed odd to me. Current
version seems more explicit.

   };
 
   /**
    * The current active route. This updated is only by settings.navigateTo or
    * settings.initializeRouteFromUrl.
    * @private {!settings.Route}
    */
   var currentRoute_ = Route.BASIC;
 
   /**
@@ skipping 13 matching lines @@
    * Initialize the route and query params from the URL.
    */
   var initializeRouteFromUrl = function() {
     assert(!initializeRouteFromUrlCalled_);
     initializeRouteFromUrlCalled_ = true;
 
     var route = getRouteForPath(window.location.pathname);
     // Never allow direct navigation to ADVANCED.
     if (route && route != Route.ADVANCED) {
       currentRoute_ = route;
       currentQueryParameters_ = new URLSearchParams(window.location.search);

[tommycli, 2016/11/23 22:33:46]

Should we use replaceState here to canonicalize the paths?

i.e. when the user types in chrome://md-settings/searchEngines/, and presses
enter, on load, it changes to chrome://md-settings/searchEngines.

I think that would be a nicety, but not necessarily required. Maybe separate
patch or deferring that would be fine too.

[dpapad, 2016/11/23 23:55:04]

Leaving this for a separate CL, so that I can ensure first that others are OK
with this change (old Options does not seem to do that).

     } else {
       window.history.replaceState(undefined, '', Route.BASIC.path);
     }
   };
 
   /**
    * Helper function to set the current route and notify all observers.
    * @param {!settings.Route} route
    * @param {!URLSearchParams} queryParameters
    * @param {boolean} isPopstate
@@ skipping 75 matching lines @@
     RouteObserverBehavior: RouteObserverBehavior,
     getRouteForPath: getRouteForPath,
     initializeRouteFromUrl: initializeRouteFromUrl,
     getCurrentRoute: getCurrentRoute,
     getQueryParameters: getQueryParameters,
     lastRouteChangeWasPopstate: lastRouteChangeWasPopstate,
     navigateTo: navigateTo,
     navigateToPreviousRoute: navigateToPreviousRoute,
   };
 });