Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(3831)

Unified Diff: chrome/browser/resources/settings/route.js

Issue 2206613003: Settings Router Refactor: Support dynamic parameters (Closed) Base URL: https://chromium.googlesource.com/chromium/src.git@223-settings-router-handle-random-urls-better
Patch Set: fix Created 4 years, 4 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
« no previous file with comments | « no previous file | chrome/test/data/webui/settings/cr_settings_browsertest.js » ('j') | no next file with comments »
Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
Index: chrome/browser/resources/settings/route.js
diff --git a/chrome/browser/resources/settings/route.js b/chrome/browser/resources/settings/route.js
index 70b3570064f87252fd36fbd789b157c632940bd3..691306180ffaea81cd3003b7c3bcd3d2976d7561 100644
--- a/chrome/browser/resources/settings/route.js
+++ b/chrome/browser/resources/settings/route.js
@@ -292,8 +292,7 @@ cr.define('settings', function() {
};
/**
- * The current active route. This may only be updated via the global
- * function settings.navigateTo.
+ * The current active route. This updated only by settings.navigateTo.
* @private {!settings.Route}
*/
var currentRoute_ = (function() {
@@ -302,16 +301,35 @@ cr.define('settings', function() {
return route;
// Reset the URL path to '/' if the user navigates to a nonexistent URL.
- window.history.replaceState(undefined, '', Route.BASIC.path);
+ window.history.replaceState({}, '', Route.BASIC.path);
return Route.BASIC;
})();
/**
+ * The current query parameters. This updated only by settings.navigateTo.
+ * @private {!Object}
stevenjb 2016/08/02 22:17:17 Can/should we type this?
tommycli 2016/08/02 22:46:34 Done. I changed it to use the URLSearchParams type
+ */
+ var currentQueryParameters_ = (function() {
+ if (window.location.search.length < 2)
+ return {};
+
+ var params = {};
+ window.location.search.substring(1).split('&').forEach(function(pair) {
Dan Beam 2016/08/02 21:44:50 OH NO YOU DI'NT https://developer.mozilla.org/en-
tommycli 2016/08/02 22:46:33 Thanks! I ended up using the URLSearchParams type
+ var [key, value] = pair.split('=').map(decodeURIComponent);
+ params[key] = value;
+ });
+ window.history.replaceState(params, '', currentRoute_.path);
+ return params;
+ })();
+
+ /**
* Helper function to set the current route and notify all observers.
* @param {!settings.Route} route
+ * @param {!Object} queryParameters
*/
- var setCurrentRoute = function(route) {
+ var setCurrentRoute = function(route, queryParameters) {
currentRoute_ = route;
+ currentQueryParameters_ = queryParameters;
for (var observer of routeObservers_)
observer.currentRouteChanged();
};
@@ -319,22 +337,41 @@ cr.define('settings', function() {
/** @return {!settings.Route} */
var getCurrentRoute = function() { return currentRoute_; };
+ /** @return {!Object} */
+ var getQueryParameters = function() { return currentQueryParameters_; };
+
/**
* Navigates to a canonical route and pushes a new history entry.
* @param {!settings.Route} route
+ * @param {Object=} opt_dynamicParameters A new history entry is always pushed
+ * when this parameter is truthy, even when the parameters are unchanged.
* @private
*/
- var navigateTo = function(route) {
- if (assert(route) == currentRoute_)
+ var navigateTo = function(route, opt_dynamicParameters) {
+ if (assert(route) == currentRoute_ && !opt_dynamicParameters)
return;
- window.history.pushState(undefined, '', route.path);
- setCurrentRoute(route);
+ var queryString = '';
+ if (opt_dynamicParameters) {
+ queryString = '?' + Object.keys(opt_dynamicParameters).map(function(key) {
+ var value = opt_dynamicParameters[key];
+ assert(typeof value == 'string',
+ 'Non-string dynamic parameter values cannot be correctly ' +
+ 'serialized and deserialized from the URL query string.');
+
+ return encodeURIComponent(key) + '=' + encodeURIComponent(value);
+ }).join('&');
+ }
+
+ setCurrentRoute(route, opt_dynamicParameters || {});
+ window.history.pushState(
+ currentQueryParameters_, '', route.path + queryString);
};
window.addEventListener('popstate', function(event) {
// On pop state, do not push the state onto the window.history again.
- setCurrentRoute(getRouteForPath(window.location.pathname) || Route.BASIC);
+ setCurrentRoute(getRouteForPath(window.location.pathname) || Route.BASIC,
+ event.state || {});
});
return {
@@ -342,6 +379,7 @@ cr.define('settings', function() {
RouteObserverBehavior: RouteObserverBehavior,
getRouteForPath: getRouteForPath,
getCurrentRoute: getCurrentRoute,
+ getQueryParameters: getQueryParameters,
navigateTo: navigateTo,
};
});
« no previous file with comments | « no previous file | chrome/test/data/webui/settings/cr_settings_browsertest.js » ('j') | no next file with comments »

Powered by Google App Engine
This is Rietveld 408576698