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

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

The neon-animated-pages elements that previously had subpage ids now use the path as ids. (including the slashes)

BUG=632382
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/1ce61a592e45865d01c17b36c86cbaf347dc5899
Cr-Commit-Position: refs/heads/master@{#411668}

[tommycli, 2016-08-08 21:45:06]

Description was changed from

==========
Settings Router Refactor: Remove route.subpage Array

BUG=632382
==========

to

==========
Settings Router Refactor: Remove route.subpage Array

BUG=632382
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[tommycli, 2016-08-08 21:49:23]

The CQ bit was checked by tommycli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-08 21:49:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-08 21:52:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-08 21:52:12]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[tommycli, 2016-08-08 22:07:35]

The CQ bit was checked by tommycli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-08 22:07:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-08 22:22:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-08 22:22:15]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[tommycli, 2016-08-08 22:25:33]

tommycli@chromium.org changed reviewers:
+ michaelpg@chromium.org

[tommycli, 2016-08-08 22:25:34]

michaelpg: PTAL, thanks!!

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

So the title should be "replace route.subpage with route.path", basically. Or at
least it would be good to mention in the description that we're changing subpage
ids from hyphenated-ids to /absolute/routePaths.

BTW, I just verified that "#/foo/bar" is not a valid CSS selector, so
querySelector would throw and CSS rules won't work. :-\

https://codereview.chromium.org/2224163002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/route.js (right):

https://codereview.chromium.org/2224163002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/route.js:90: return !!this.parent &&
this.parent.section == this.section;
doesn't this return true for dialogs as well? is that intentional?

https://codereview.chromium.org/2224163002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_page/settings_animated_pages.js
(right):

https://codereview.chromium.org/2224163002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_animated_pages.js:90:
switchToSubpage_: function(newRoute, oldRoute) {
it looks like this function can be triggered for dialogs, which is not intended.

https://codereview.chromium.org/2224163002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_animated_pages.js:135:
var id = settings.getCurrentRoute().path;
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 15:57:55]

Description was changed from

==========
Settings Router Refactor: Remove route.subpage Array

BUG=632382
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

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

The neon-animated-pages elements that previously had subpage ids now use the
path as ids. (including the slashes)

BUG=632382
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

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

michaelpg: Thanks.

BTW, CSS selectors can technically work on ids with slashes, they just look
funny:
https://jsfiddle.net/5hqpLx02/

https://codereview.chromium.org/2224163002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/route.js (right):

https://codereview.chromium.org/2224163002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/route.js:90: return !!this.parent &&
this.parent.section == this.section;
On 2016/08/09 03:07:48, michaelpg wrote:
> doesn't this return true for dialogs as well? is that intentional?

Yeah. It's intentional. All the existing logic treats dialogs as subpages as
well.

https://codereview.chromium.org/2224163002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_page/settings_animated_pages.js
(right):

https://codereview.chromium.org/2224163002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_animated_pages.js:90:
switchToSubpage_: function(newRoute, oldRoute) {
On 2016/08/09 03:07:48, michaelpg wrote:
> it looks like this function can be triggered for dialogs, which is not
intended.

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

https://codereview.chromium.org/2224163002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_animated_pages.js:135:
var id = settings.getCurrentRoute().path;
On 2016/08/09 03:07:48, michaelpg wrote:
> 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)?

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]

https://codereview.chromium.org/2224163002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_page/settings_animated_pages.js
(right):

https://codereview.chromium.org/2224163002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_animated_pages.js:90:
switchToSubpage_: function(newRoute, oldRoute) {
On 2016/08/09 16:03:14, tommycli wrote:
> On 2016/08/09 03:07:48, michaelpg wrote:
> > it looks like this function can be triggered for dialogs, which is not
> intended.
> 
> Hmmm... should we change isSubpage() to return false for dialogs? (I would be
> totally fine with that) I just followed the existing behavior.

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.

https://codereview.chromium.org/2224163002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_animated_pages.js:135:
var id = settings.getCurrentRoute().path;
On 2016/08/09 16:03:14, tommycli wrote:
> On 2016/08/09 03:07:48, michaelpg wrote:
> > 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)?
> 
> 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?

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 00:23:22]

The CQ bit was checked by tommycli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-10 00:23:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-10 00:27:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-10 00:27:37]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[tommycli, 2016-08-10 00:29:46]

The CQ bit was checked by tommycli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-10 00:30:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-10 00:49:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-10 00:49:18]

Dry run: Try jobs failed on following builders:
  closure_compilation on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)

[tommycli, 2016-08-10 17:01:23]

The CQ bit was checked by tommycli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-10 17:02:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

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

michaelpg: Thanks. I merged, so the delta might look big. See below

https://codereview.chromium.org/2224163002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_page/settings_animated_pages.js
(right):

https://codereview.chromium.org/2224163002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_animated_pages.js:90:
switchToSubpage_: function(newRoute, oldRoute) {
On 2016/08/09 20:39:32, michaelpg wrote:
> On 2016/08/09 16:03:14, tommycli wrote:
> > On 2016/08/09 03:07:48, michaelpg wrote:
> > > it looks like this function can be triggered for dialogs, which is not
> > intended.
> > 
> > Hmmm... should we change isSubpage() to return false for dialogs? (I would
be
> > totally fine with that) I just followed the existing behavior.
> 
> 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.

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.

https://codereview.chromium.org/2224163002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_animated_pages.js:135:
var id = settings.getCurrentRoute().path;
On 2016/08/09 20:39:32, michaelpg wrote:
> On 2016/08/09 16:03:14, tommycli wrote:
> > On 2016/08/09 03:07:48, michaelpg wrote:
> > > 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)?
> > 
> > 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?
> 
> 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')

Done.

[commit-bot: I haz the power, 2016-08-10 17:51:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-10 17:51:08]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tommycli, 2016-08-10 18:16:06]

The CQ bit was checked by tommycli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-10 18:17:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tommycli, 2016-08-10 18:18:35]

The CQ bit was checked by tommycli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-10 18:19:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[michaelpg, 2016-08-10 18:45:52]

https://codereview.chromium.org/2224163002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_page/settings_animated_pages.js
(right):

https://codereview.chromium.org/2224163002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_animated_pages.js:90:
switchToSubpage_: function(newRoute, oldRoute) {
On 2016/08/10 17:21:33, tommycli wrote:
> On 2016/08/09 20:39:32, michaelpg wrote:
> > On 2016/08/09 16:03:14, tommycli wrote:
> > > On 2016/08/09 03:07:48, michaelpg wrote:
> > > > it looks like this function can be triggered for dialogs, which is not
> > > intended.
> > > 
> > > Hmmm... should we change isSubpage() to return false for dialogs? (I would
> be
> > > totally fine with that) I just followed the existing behavior.
> > 
> > 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.
> 
> 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.

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).
> 
> 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.

https://codereview.chromium.org/2224163002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/settings/route.js (right):

https://codereview.chromium.org/2224163002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/route.js:86: * Returns true if this route is a
descendant of a section.
updated comment ("subpage of a"?)

https://codereview.chromium.org/2224163002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_main/settings_main.js (right):

https://codereview.chromium.org/2224163002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_main/settings_main.js:127:
this.style.height = this.inSubpage_ ? '100%' : '';
inSubpage_ really means whether a subpage contains this route, dialogs included
(when opened from a subpage) -- maybe it would be easier now to use the
opposite, e.g. this.onRootPage_

for instance, i'd expect this to cause style problems if the dialog is opened
from a subpage

https://codereview.chromium.org/2224163002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_page/main_page_behavior.js
(right):

https://codereview.chromium.org/2224163002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:89: if
(currentRoute.isSubpage())
again, if we deep-link to a dialog inside a subpage this still needs to run if
the subpage itself is opening (was closed)

https://codereview.chromium.org/2224163002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_page/settings_animated_pages.js
(right):

https://codereview.chromium.org/2224163002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/settings_animated_pages.js:74:
this.$.animatedPages.selected = 'default';
THANK YOU. i've learned never to use "main" or "page" again, they're like
"status" or "data" but for UI.

[tommycli, 2016-08-10 18:52:10]

The CQ bit was checked by tommycli@chromium.org to run a CQ dry run

[tommycli, 2016-08-10 18:52:18]

https://codereview.chromium.org/2224163002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_page/settings_animated_pages.js
(right):

https://codereview.chromium.org/2224163002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_animated_pages.js:90:
switchToSubpage_: function(newRoute, oldRoute) {
On 2016/08/10 18:45:51, michaelpg wrote:
> On 2016/08/10 17:21:33, tommycli wrote:
> > On 2016/08/09 20:39:32, michaelpg wrote:
> > > On 2016/08/09 16:03:14, tommycli wrote:
> > > > On 2016/08/09 03:07:48, michaelpg wrote:
> > > > > it looks like this function can be triggered for dialogs, which is not
> > > > intended.
> > > > 
> > > > Hmmm... should we change isSubpage() to return false for dialogs? (I
would
> > be
> > > > totally fine with that) I just followed the existing behavior.
> > > 
> > > 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.
> > 
> > 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.
> 
> 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).
> > 
> > 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.
> 

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.

https://codereview.chromium.org/2224163002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/settings/route.js (right):

https://codereview.chromium.org/2224163002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/route.js:86: * Returns true if this route is a
descendant of a section.
On 2016/08/10 18:45:52, michaelpg wrote:
> updated comment ("subpage of a"?)

Done.

https://codereview.chromium.org/2224163002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_main/settings_main.js (right):

https://codereview.chromium.org/2224163002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_main/settings_main.js:127:
this.style.height = this.inSubpage_ ? '100%' : '';
On 2016/08/10 18:45:52, michaelpg wrote:
> inSubpage_ really means whether a subpage contains this route, dialogs
included
> (when opened from a subpage) -- maybe it would be easier now to use the
> opposite, e.g. this.onRootPage_
> 
> for instance, i'd expect this to cause style problems if the dialog is opened
> from a subpage

Yeah, I'll just suggest that dialogs can never be from within subpages.

https://codereview.chromium.org/2224163002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_page/main_page_behavior.js
(right):

https://codereview.chromium.org/2224163002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:89: if
(currentRoute.isSubpage())
On 2016/08/10 18:45:52, michaelpg wrote:
> again, if we deep-link to a dialog inside a subpage this still needs to run if
> the subpage itself is opening (was closed)

Same. It seems like we can realize major simplicity if we demand that navigable
dialogs can never be from within subpages.

https://codereview.chromium.org/2224163002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_page/settings_animated_pages.js
(right):

https://codereview.chromium.org/2224163002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/settings_animated_pages.js:74:
this.$.animatedPages.selected = 'default';
On 2016/08/10 18:45:52, michaelpg wrote:
> THANK YOU. i've learned never to use "main" or "page" again, they're like
> "status" or "data" but for UI.

Acknowledged. I debated with myself for a while as to whether to rename it.

[commit-bot: I haz the power, 2016-08-10 18:52:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-10 19:45:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-10 19:46:00]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tommycli, 2016-08-10 20:43:21]

The CQ bit was checked by tommycli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-10 20:43:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-10 21:20:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-10 21:20:46]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tommycli, 2016-08-10 21:38:19]

The CQ bit was checked by tommycli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-10 21:39:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-10 22:30:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-10 22:30:46]

Dry run: Try jobs failed on following builders:
  closure_compilation on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)

[michaelpg, 2016-08-11 03:41:13]

lgtm, see comments

https://codereview.chromium.org/2224163002/diff/180001/chrome/browser/resourc...
File chrome/browser/resources/settings/route.js (right):

https://codereview.chromium.org/2224163002/diff/180001/chrome/browser/resourc...
chrome/browser/resources/settings/route.js:90: return !this.dialog &&
!!this.parent &&
I suggest removing the !this.dialog check (and comment): if dialog routes are
never in sections, then it's redundant. Removing the check means we're still
safe if we *do* add a subpage dialog -- the transitions won't get confused and
broken.

IOW: if we don't have navigable subpage dialogs, `!this.dialog &&` has no
effect; if we do, things break. That was my original point, anyway -- we're
replacing subpage.length > 0 with isSubpage(), so I'd prefer if they stayed
equal in this CL.

https://codereview.chromium.org/2224163002/diff/180001/chrome/test/data/webui...
File chrome/test/data/webui/settings/device_page_tests.js (right):

https://codereview.chromium.org/2224163002/diff/180001/chrome/test/data/webui...
chrome/test/data/webui/settings/device_page_tests.js:229:
assertTrue(settings.getCurrentRoute() == settings.Route.POINTERS);
err, why not assertEquals, here and the others below?

[tommycli, 2016-08-11 18:54:05]

The CQ bit was checked by tommycli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-11 18:54:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tommycli, 2016-08-11 19:15:02]

michaelpg: Thanks for the feedback. In the last patchset I just remove the
createDialog method, since it's only used in one place.

I hope that addresses your comment. Let me know if it's objectionable.

Thx

https://codereview.chromium.org/2224163002/diff/180001/chrome/browser/resourc...
File chrome/browser/resources/settings/route.js (right):

https://codereview.chromium.org/2224163002/diff/180001/chrome/browser/resourc...
chrome/browser/resources/settings/route.js:90: return !this.dialog &&
!!this.parent &&
On 2016/08/11 03:41:13, michaelpg wrote:
> I suggest removing the !this.dialog check (and comment): if dialog routes are
> never in sections, then it's redundant. Removing the check means we're still
> safe if we *do* add a subpage dialog -- the transitions won't get confused and
> broken.
> 
> IOW: if we don't have navigable subpage dialogs, `!this.dialog &&` has no
> effect; if we do, things break. That was my original point, anyway -- we're
> replacing subpage.length > 0 with isSubpage(), so I'd prefer if they stayed
> equal in this CL.

Removing !this.dialog doesn't work, since we changed the isSubpage test from
looking at the length of the subpage array to checking if the current route's
section is equal to the parent route's section.

In the latest patchset, I just removed the concept of a dialog route and special
cased the CLEAR_BROWSING_DATA instance. Is that clearer?

https://codereview.chromium.org/2224163002/diff/180001/chrome/test/data/webui...
File chrome/test/data/webui/settings/device_page_tests.js (right):

https://codereview.chromium.org/2224163002/diff/180001/chrome/test/data/webui...
chrome/test/data/webui/settings/device_page_tests.js:229:
assertTrue(settings.getCurrentRoute() == settings.Route.POINTERS);
On 2016/08/11 03:41:13, michaelpg wrote:
> err, why not assertEquals, here and the others below?

Done.

[tommycli, 2016-08-11 19:19:59]

On 2016/08/11 19:15:02, tommycli wrote:
> michaelpg: Thanks for the feedback. In the last patchset I just remove the
> createDialog method, since it's only used in one place.
> 
> I hope that addresses your comment. Let me know if it's objectionable.
> 
> Thx
> 
>
https://codereview.chromium.org/2224163002/diff/180001/chrome/browser/resourc...
> File chrome/browser/resources/settings/route.js (right):
> 
>
https://codereview.chromium.org/2224163002/diff/180001/chrome/browser/resourc...
> chrome/browser/resources/settings/route.js:90: return !this.dialog &&
> !!this.parent &&
> On 2016/08/11 03:41:13, michaelpg wrote:
> > I suggest removing the !this.dialog check (and comment): if dialog routes
are
> > never in sections, then it's redundant. Removing the check means we're still
> > safe if we *do* add a subpage dialog -- the transitions won't get confused
and
> > broken.
> > 
> > IOW: if we don't have navigable subpage dialogs, `!this.dialog &&` has no
> > effect; if we do, things break. That was my original point, anyway -- we're
> > replacing subpage.length > 0 with isSubpage(), so I'd prefer if they stayed
> > equal in this CL.
> 
> Removing !this.dialog doesn't work, since we changed the isSubpage test from
> looking at the length of the subpage array to checking if the current route's
> section is equal to the parent route's section.
> 
> In the latest patchset, I just removed the concept of a dialog route and
special
> cased the CLEAR_BROWSING_DATA instance. Is that clearer?
> 
>
https://codereview.chromium.org/2224163002/diff/180001/chrome/test/data/webui...
> File chrome/test/data/webui/settings/device_page_tests.js (right):
> 
>
https://codereview.chromium.org/2224163002/diff/180001/chrome/test/data/webui...
> chrome/test/data/webui/settings/device_page_tests.js:229:
> assertTrue(settings.getCurrentRoute() == settings.Route.POINTERS);
> On 2016/08/11 03:41:13, michaelpg wrote:
> > err, why not assertEquals, here and the others below?
> 
> Done.

BTW, thinking about it: The most correct way to implement navigable dialogs (if
we ever bring them back) is to set: this.inSubpage = this.parent.inSubpage
during the createDialog method.

[tommycli, 2016-08-11 19:27:14]

The CQ bit was checked by tommycli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-11 19:27:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-11 20:32:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-11 20:32:11]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tommycli, 2016-08-11 22:02:36]

The CQ bit was checked by tommycli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-11 22:05:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-11 23:19:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-11 23:19:04]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tommycli, 2016-08-11 23:33:11]

The CQ bit was checked by tommycli@chromium.org

[tommycli, 2016-08-11 23:33:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
michaelpg@chromium.org
Link to the patchset: https://codereview.chromium.org/2224163002/#ps240001
(title: "fix devices test")

[commit-bot: I haz the power, 2016-08-11 23:35:19]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-12 00:28:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-12 00:28:41]

Failed to apply patch for
chrome/browser/resources/settings/people_page/people_page.html:
While running git apply --index -3 -p1;
  error: patch failed:
chrome/browser/resources/settings/people_page/people_page.html:240
  Falling back to three-way merge...
  Applied patch to
'chrome/browser/resources/settings/people_page/people_page.html' with conflicts.
  U chrome/browser/resources/settings/people_page/people_page.html

Patch:       chrome/browser/resources/settings/people_page/people_page.html
Index: chrome/browser/resources/settings/people_page/people_page.html
diff --git a/chrome/browser/resources/settings/people_page/people_page.html
b/chrome/browser/resources/settings/people_page/people_page.html
index
13b2f0a167460beee9f9b2e587217266ba58a8fa..b906fa9d085e162b6a70601d40acb0d05cd78e7a
100644
--- a/chrome/browser/resources/settings/people_page/people_page.html
+++ b/chrome/browser/resources/settings/people_page/people_page.html
@@ -73,7 +73,7 @@
       }
     </style>
     <settings-animated-pages id="pages" section="people">
-      <neon-animatable id="main">
+      <neon-animatable route-path="default">
         <div id="picture-subpage-trigger" class="settings-box first two-line">
           <img id="profile-icon" src="[[profileIconUrl_]]"
               on-tap="onPictureTap_" actionable>
@@ -240,7 +240,7 @@
         </template>
       </neon-animatable>
       <template is="dom-if"
if="[[isAdvancedSyncSettingsVisible_(syncStatus)]]">
-        <template is="dom-if" name="sync">
+        <template is="dom-if" route-path="/syncSetup">
           <settings-subpage
               associated-control="[[$$('#customize-sync')]]"
               page-title="$i18n{syncPageTitle}">
@@ -250,7 +250,7 @@
       </template>
 <if expr="chromeos">
       <template is="dom-if" if="[[quickUnlockEnabled_]]">
-        <template is="dom-if" name="lockScreen">
+        <template is="dom-if" route-path="/lockScreen" no-search>
           <settings-subpage page-title="$i18n{lockScreenTitle}">
             <settings-lock-screen
               prefs="{{prefs}}">
@@ -258,14 +258,14 @@
           </settings-subpage>
         </template>
       </template>
-      <template is="dom-if" name="users">
+      <template is="dom-if" route-path="/accounts">
         <settings-subpage
             associated-control="[[$$('#manage-other-people-subpage-trigger')]]"
             page-title="$i18n{usersPageTitle}">
           <settings-users-page prefs="{{prefs}}"></settings-users-page>
         </settings-subpage>
       </template>
-      <template is="dom-if" name="changePicture">
+      <template is="dom-if" route-path="/changePicture">
         <settings-subpage
           associated-control="[[$$('#picture-subpage-trigger')]]"
           page-title="$i18n{changePictureTitle}">
@@ -274,7 +274,7 @@
       </template>
 </if>
 <if expr="not chromeos">
-      <template is="dom-if" name="manageProfile">
+      <template is="dom-if" route-path="/manageProfile">
         <settings-subpage
             associated-control="[[$$('#picture-subpage-trigger')]]"
             page-title="$i18n{editPerson}">

[tommycli, 2016-08-12 14:44:37]

The CQ bit was checked by tommycli@chromium.org

[tommycli, 2016-08-12 14:44:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
michaelpg@chromium.org
Link to the patchset: https://codereview.chromium.org/2224163002/#ps260001
(title: "merge")

[commit-bot: I haz the power, 2016-08-12 14:44:52]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-12 16:20:13]

Description was changed from

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

The neon-animated-pages elements that previously had subpage ids now use the
path as ids. (including the slashes)

BUG=632382
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

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

The neon-animated-pages elements that previously had subpage ids now use the
path as ids. (including the slashes)

BUG=632382
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[commit-bot: I haz the power, 2016-08-12 16:20:15]

Committed patchset #14 (id:260001)

[commit-bot: I haz the power, 2016-08-12 16:21:59]

Description was changed from

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

The neon-animated-pages elements that previously had subpage ids now use the
path as ids. (including the slashes)

BUG=632382
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

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

The neon-animated-pages elements that previously had subpage ids now use the
path as ids. (including the slashes)

BUG=632382
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/1ce61a592e45865d01c17b36c86cbaf347dc5899
Cr-Commit-Position: refs/heads/master@{#411668}
==========

[commit-bot: I haz the power, 2016-08-12 16:22:00]

Patchset 14 (id:??) landed as
https://crrev.com/1ce61a592e45865d01c17b36c86cbaf347dc5899
Cr-Commit-Position: refs/heads/master@{#411668}

[michaelpg, 2016-08-12 22:43:05]

https://codereview.chromium.org/2224163002/diff/180001/chrome/browser/resourc...
File chrome/browser/resources/settings/route.js (right):

https://codereview.chromium.org/2224163002/diff/180001/chrome/browser/resourc...
chrome/browser/resources/settings/route.js:90: return !this.dialog &&
!!this.parent &&
On 2016/08/11 19:15:02, tommycli wrote:
> On 2016/08/11 03:41:13, michaelpg wrote:
> > I suggest removing the !this.dialog check (and comment): if dialog routes
are
> > never in sections, then it's redundant. Removing the check means we're still
> > safe if we *do* add a subpage dialog -- the transitions won't get confused
and
> > broken.
> > 
> > IOW: if we don't have navigable subpage dialogs, `!this.dialog &&` has no
> > effect; if we do, things break. That was my original point, anyway -- we're
> > replacing subpage.length > 0 with isSubpage(), so I'd prefer if they stayed
> > equal in this CL.
> 
> Removing !this.dialog doesn't work, since we changed the isSubpage test from
> looking at the length of the subpage array to checking if the current route's
> section is equal to the parent route's section.

Right, so this would have returned *true* for clear-browser-data, which is what
I wanted. I'll open a bug on the behavior that I see now, so we can start with
concrete behavior :-)

> 
> In the latest patchset, I just removed the concept of a dialog route and
special
> cased the CLEAR_BROWSING_DATA instance. Is that clearer?