Settings Router Refactor: Replace route.subpage usage with route.path

chrome/browser/resources/settings/settings_page/settings_animated_pages.js

 // Copyright 2015 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.
 
 /**
  * @fileoverview
  * 'settings-animated-pages' is a container for a page and animated subpages.
  * It provides a set of common behaviors and animations.
  *
  * Example:
@@ skipping 54 matching lines @@
     if (!this.queuedRouteChange_)
       return;
     this.async(this.currentRouteChanged.bind(
         this,
         this.queuedRouteChange_.newRoute,
         this.queuedRouteChange_.oldRoute));
   },
 
   /** @protected */
   currentRouteChanged: function(newRoute, oldRoute) {
-    if (newRoute.section == this.section && newRoute.subpage.length > 0) {
+    if (newRoute.section == this.section && newRoute.isSubpage()) {
       this.switchToSubpage_(newRoute, oldRoute);
     } else {
       this.$.animatedPages.exitAnimation = 'fade-out-animation';
       this.$.animatedPages.entryAnimation = 'fade-in-animation';
       this.$.animatedPages.selected = 'main';
     }
   },
 
   /**
    * Selects the subpage specified by |newRoute|.
    * @param {!settings.Route} newRoute
    * @param {!settings.Route} oldRoute
    * @private
    */
   switchToSubpage_: function(newRoute, oldRoute) {

[michaelpg, 2016/08/09 03:07:48]

it looks like this function can be triggered for dialogs, which is not intended.

[tommycli, 2016/08/09 16:03:14]

Hmmm... should we change isSubpage() to return false for dialogs? (I would be
totally fine with that) I just followed the existing behavior.

[michaelpg, 2016/08/09 20:39:32]

I think the existing behavior is wrong, but I'm not sure what is right. If this
100% matches the previous behavior, we can deal with fixing it later.

Example: if I navigate from the main page to a dialog of a subpage, is that only
one currentRouteChanged? We'd need to animate the subpage, but also show the
dialog then.

OTOH, if the subpage doesn't change when navigating to a dialog, we shouldn't do
anything at all there.

[tommycli, 2016/08/10 17:21:33]

Okay, I tested this, and the best behavior is to make our only dialog (clear
browsing data) work right is to treat all dialogs as non-subpages.

Since there is only one instance, I'd recommend just removing the concept of
dialog-routes in a future CL and just special-casing the clear browsing data
dialog.

[michaelpg, 2016/08/10 18:45:51]

Not sure how that would work, given we still need to handle subpage lifecycle as
if a dialog were a subpage (when it appears in a subpage).

[tommycli, 2016/08/10 18:52:17]

The only navigable dialog is Clear Browsing Data, which is not off of a subpage.
There's talk of converting that to a subpage. I'll vote for the You Ain't Gonna
Need It on that feature.

     // Don't manipulate the light DOM until it's ready.
     if (!this.lightDomReady_) {
       this.queuedRouteChange_ = this.queuedRouteChange_ || {oldRoute: oldRoute};
       this.queuedRouteChange_.newRoute = newRoute;
       return;
     }
 
     this.ensureSubpageInstance_();
 
     if (oldRoute) {
-      var oldRouteIsSubpage = oldRoute.subpage.length > 0;
-      if (oldRouteIsSubpage && oldRoute.contains(newRoute)) {
+      if (oldRoute.isSubpage() && oldRoute.contains(newRoute)) {
         // Slide left for a descendant subpage.
         this.$.animatedPages.exitAnimation = 'slide-left-animation';
         this.$.animatedPages.entryAnimation = 'slide-from-right-animation';
       } else if (newRoute.contains(oldRoute)) {
         // Slide right for an ancestor subpage.
         this.$.animatedPages.exitAnimation = 'slide-right-animation';
         this.$.animatedPages.entryAnimation = 'slide-from-left-animation';
       } else {
         // The old route is not a subpage or is at the same level, so just fade.
         this.$.animatedPages.exitAnimation = 'fade-out-animation';
         this.$.animatedPages.entryAnimation = 'fade-in-animation';
 
-        if (!oldRouteIsSubpage) {
+        if (!oldRoute.isSubpage()) {
           // Set the height the expand animation should start at before
           // beginning the transition to the new subpage.
           // TODO(michaelpg): Remove MainPageBehavior's dependency on this
           // height being set.
           this.style.height = this.clientHeight + 'px';
           this.async(function() {
             this.style.height = '';
           });
         }
       }
     }
 
-    this.$.animatedPages.selected = newRoute.subpage.slice(-1)[0];
+    this.$.animatedPages.selected = newRoute.path;
   },
 
   /**
    * Ensures that the template enclosing the subpage is stamped.
    * @private
    */
   ensureSubpageInstance_: function() {
-    var id = settings.getCurrentRoute().subpage.slice(-1)[0];
+    var id = settings.getCurrentRoute().path;

[michaelpg, 2016/08/09 03:07:48]

an ID of "/foo/var" is... unusual, although apparently valid. have you confirmed
iron-selector and company, the polymer styling shim, etc. all have no qualms
with that (by asking the team)?

[tommycli, 2016/08/09 16:03:14]

Well... technically, the paths are all on dom-if names. We could just not use
ids at all, and set attrForSelected="route-path" on neon-animated pages. Does
that work better for you?

[michaelpg, 2016/08/09 20:39:32]

Yeah, let's do that!

BTW, thanks for the info, although it's even weirder in JS because you have to
escape the backslashes first:

document.querySelector('#\\/foo\\/bar')

[tommycli, 2016/08/10 17:21:33]

Done.

     var template = Polymer.dom(this).querySelector(
         'template[name="' + id + '"]');
 
     // Nothing to do if the subpage isn't wrapped in a <template> or the
     // template is already stamped.
     if (!template || template.if)
       return;
 
     // Set the subpage's id for use by neon-animated-pages.
     var subpage = /** @type {{_content: DocumentFragment}} */(template)._content
         .querySelector('settings-subpage');
     if (!subpage.id)
       subpage.id = id;
     // Carry over the 'no-search' attribute from the template to the stamped
     // instance, such that the stamped instance will also be ignored by the
     // searching algorithm.
     if (template.hasAttribute('no-search'))
       subpage.setAttribute('no-search', '');
 
     // Render synchronously so neon-animated-pages can select the subpage.
     template.if = true;
     template.render();
   },
 });