Settings: Fix Site Details subpage routing

Settings: Fix Site Details subpage routing

Goal is: A site details subpage that's accessible both from /siteSettings/all, /siteSettings/bluetooth, /siteSettings/plugins, etc. (all the categories).

Previous way it was done was: Separate route for each category, so there was /siteSettings/all/details, /siteSettings/bluetooth/details, /siteSettings/plugins/details.

And it relied on a quirk of how settings-animated-pages used to work to coalesce all those routes to a single card.

New way I'm proposing: One single /siteSettings/siteDetails route.

But here are some extra needed changes:
 - settings-animated-pages must slide right / left based on route depth instead of based on .contains()
 - subpage-back button needs to call window.history.back() whenever there is a previous route, even if the previous route doesn't strictly contain the current route.

I think the above two changes are harmless and the new version is overall an improvement.

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

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

[tommycli, 2016-08-16 00:08:53]

Description was changed from

==========
Settings: Fix Site Details subpage routing

BUG=637865
==========

to

==========
Settings: Fix Site Details subpage routing

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

[tommycli, 2016-08-16 00:09:41]

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

[tommycli, 2016-08-16 00:09:41]

finnur: PTAL. This fixes the Site Details subpage for me. If this fix looks good
to you, feel free to CQ. Thanks for the report!

[tommycli, 2016-08-16 00:09:54]

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

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

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-16 01:08:48]

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

[commit-bot: I haz the power, 2016-08-16 01:08:49]

Dry run: This issue passed the CQ dry run.

[Finnur, 2016-08-16 10:32:35]

This is a minor improvement, since it enables the details page again for All
Sites, but it unfortunately fails for the other categories that show Site
Details so I think it is not the right fix.

To repro:
Go to http://permission.site and enable Location (or something).
Now go to chrome://md-settings/siteSettings/location and try to click on the
permission.site entry that you just created.
End result: Details page is blank

But... lets for a moment pretend that the Site Details page worked after this
change. I suspect we'd still have a problem because if you take this route:
Advanced -> Site Settings -> Location -> Site Details
... when you press Back, you'd end up on All Sites, not Location -- as would
have been expected. 
Right?

[tommycli, 2016-08-16 16:51:49]

Description was changed from

==========
Settings: Fix Site Details subpage routing

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

to

==========
Settings: Fix Site Details subpage routing

Goal is: A site details subpage that's accessible both from /siteSettings/all,
/siteSettings/bluetooth, /siteSettings/plugins, etc. (all the categories).

Previous way it was done was: Separate route for each category, so there was
/siteSettings/all/details, /siteSettings/bluetooth/details,
/siteSettings/plugins/details.

And it relied on a quirk of how settings-animated-pages used to work to coalesce
all those routes to a single card.

New way I'm proposing: One single /siteSettings/siteDetails route.

But here are some extra needed changes:
 - settings-animated-pages must slide right / left based on route depth instead
of based on .contains()
 - subpage-back button needs to call window.history.back() whenever there is a
previous route, even if the previous route doesn't strictly contain the current
route.

I think the above two changes are harmless and the new version is overall an
improvement.


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

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

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

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

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-16 16:54:31]

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

[commit-bot: I haz the power, 2016-08-16 16:54:32]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  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-16 16:54:34]

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

[tommycli, 2016-08-16 16:54:35]

finnur: Okay I (finally) understand what the issue is, and I wrote it up in the
CL description.

I made some more extensive changes to routing, and settings-animated-pages, to
support the Site Settings details use case.

michaelpg / dbeam: PTAL and give me some feedback on the routing and
subpage-back update. I think this is overall a cleaner way to implement the site
details page.

[tommycli, 2016-08-16 16:58:41]

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

[commit-bot: I haz the power, 2016-08-16 16:58:52]

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-16 17:33:49]

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

[commit-bot: I haz the power, 2016-08-16 17:33:50]

Dry run: Try jobs failed on following builders:
  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-16 19:09:18]

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

[commit-bot: I haz the power, 2016-08-16 19:09:45]

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-16 19:54:27]

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

[commit-bot: I haz the power, 2016-08-16 19:54:28]

Dry run: This issue passed the CQ dry run.

[michaelpg, 2016-08-17 00:31:13]

> But here are some extra needed changes:
>  - settings-animated-pages must slide right / left based on
> route depth instead of based on .contains()
>  - subpage-back button needs to call window.history.back()
> whenever there is a previous route, even if the previous
> route doesn't strictly contain the current route.

When we last talked about this in a standup, the informal consensus was that
what we're doing now is fine: subpage-back (unlike the browser back action) goes
up a route, e.g. from a child subpage to a parent subpage, or a subpage to a
root page.

Just like browsing your file system in a file explorer (including chrome...)
where [up arrow] goes up a level but [back arrow] goes back in history.

I'm not opposed to making this change, but would prefer it be a deliberate (and
final) decision rather than happenstance as a convenient side effect of another
change.

[michaelpg, 2016-08-17 00:40:50]

> Goal is: A site details subpage that's accessible both from
> /siteSettings/all, /siteSettings/bluetooth, /siteSettings/plugins,
> etc. (all the categories).
> 
> Previous way it was done was: Separate route for each category, so
> there was /siteSettings/all/details, /siteSettings/bluetooth/details,
> /siteSettings/plugins/details.
> 
> And it relied on a quirk of how settings-animated-pages used to work
> to coalesce all those routes to a single card.

> New way I'm proposing: One single /siteSettings/siteDetails route.

Can you clarify -- I think this is what you mean by "coalesce... to a
single card" -- did all these Routes, ignoring that they were implemented
as individual settings.Route instances, did they all actually point to
the same logical page? There's only one site-details page, and the [R]oute
was nothing more than an artifact of the [r]oute we took to get to that
page?

Unrelatedly -- will this make it easier or harder to make the site used
in the details page correctly change when navigating back/forward in history?

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

https://codereview.chromium.org/2249873003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_animated_pages.js:95:
if (oldRoute.isSubpage() && newRoute.depth > oldRoute.depth) {
Wait a minute, isn't this exactly what we had previously (comparing
subpage.length)?

https://codereview.chromium.org/2249873003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_animated_pages.js:100:
// Slide right for an shallower subpage.
s/an/a

[Finnur, 2016-08-17 11:29:52]

From a Site Settings view this LGTM. I've verified this fixes the issue properly
(Back goes back to the category you were viewing).

Thanks!

[tommycli, 2016-08-17 16:17:00]

Hi michaelpg:

Re: deliberate change on subpage-back arrow. Yeah I'm pretty much changing it
because it's convenient, but I also think it's harmless.

Re: coalescing into a single card. Yes, previously all the category details
routes pointed to the same actual page. Since they all ended with the
"site-details" as the last string on the subpage Array, it all pointed to the
same item in settings-animated-pages.

Re: Back/Fwd on Site Details. This patch shouldn't make things either better or
worse. The real solution to that is to use the new URLSearchParams feature of
routing to make it look like: /siteSettings/siteDetails?origin=foo.com - which
is up to finnur :D

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

https://codereview.chromium.org/2249873003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_animated_pages.js:95:
if (oldRoute.isSubpage() && newRoute.depth > oldRoute.depth) {
On 2016/08/17 00:40:49, michaelpg wrote:
> Wait a minute, isn't this exactly what we had previously (comparing
> subpage.length)?

Previously we checked if the subpage.length > 0, but didn't compare the lengths
of oldRoute vs newRoute.

https://codereview.chromium.org/2249873003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_animated_pages.js:100:
// Slide right for an shallower subpage.
On 2016/08/17 00:40:49, michaelpg wrote:
> s/an/a

Done.

[michaelpg, 2016-08-17 18:06:12]

thanks for all the clarification; lgtm assuming "subpage back arrow same as
browser back" is OK

[tommycli, 2016-08-17 18:10:59]

On 2016/08/17 18:06:12, michaelpg wrote:
> thanks for all the clarification; lgtm assuming "subpage back arrow same as
> browser back" is OK

michaelpg: Thank you sir. Sending in to unblock Site Settings. We can revisit
the subpage-back issue if subpage-back = browserback is bad in Canary testing.

[tommycli, 2016-08-17 18:11:04]

The CQ bit was checked by tommycli@chromium.org

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

The patchset sent to the CQ was uploaded after l-g-t-m from finnur@chromium.org
Link to the patchset: https://codereview.chromium.org/2249873003/#ps80001
(title: "fix comment typo")

[commit-bot: I haz the power, 2016-08-17 18:11:22]

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-17 19:06:20]

Description was changed from

==========
Settings: Fix Site Details subpage routing

Goal is: A site details subpage that's accessible both from /siteSettings/all,
/siteSettings/bluetooth, /siteSettings/plugins, etc. (all the categories).

Previous way it was done was: Separate route for each category, so there was
/siteSettings/all/details, /siteSettings/bluetooth/details,
/siteSettings/plugins/details.

And it relied on a quirk of how settings-animated-pages used to work to coalesce
all those routes to a single card.

New way I'm proposing: One single /siteSettings/siteDetails route.

But here are some extra needed changes:
 - settings-animated-pages must slide right / left based on route depth instead
of based on .contains()
 - subpage-back button needs to call window.history.back() whenever there is a
previous route, even if the previous route doesn't strictly contain the current
route.

I think the above two changes are harmless and the new version is overall an
improvement.


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

to

==========
Settings: Fix Site Details subpage routing

Goal is: A site details subpage that's accessible both from /siteSettings/all,
/siteSettings/bluetooth, /siteSettings/plugins, etc. (all the categories).

Previous way it was done was: Separate route for each category, so there was
/siteSettings/all/details, /siteSettings/bluetooth/details,
/siteSettings/plugins/details.

And it relied on a quirk of how settings-animated-pages used to work to coalesce
all those routes to a single card.

New way I'm proposing: One single /siteSettings/siteDetails route.

But here are some extra needed changes:
 - settings-animated-pages must slide right / left based on route depth instead
of based on .contains()
 - subpage-back button needs to call window.history.back() whenever there is a
previous route, even if the previous route doesn't strictly contain the current
route.

I think the above two changes are harmless and the new version is overall an
improvement.


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

[commit-bot: I haz the power, 2016-08-17 19:06:21]

Committed patchset #5 (id:80001)

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

Description was changed from

==========
Settings: Fix Site Details subpage routing

Goal is: A site details subpage that's accessible both from /siteSettings/all,
/siteSettings/bluetooth, /siteSettings/plugins, etc. (all the categories).

Previous way it was done was: Separate route for each category, so there was
/siteSettings/all/details, /siteSettings/bluetooth/details,
/siteSettings/plugins/details.

And it relied on a quirk of how settings-animated-pages used to work to coalesce
all those routes to a single card.

New way I'm proposing: One single /siteSettings/siteDetails route.

But here are some extra needed changes:
 - settings-animated-pages must slide right / left based on route depth instead
of based on .contains()
 - subpage-back button needs to call window.history.back() whenever there is a
previous route, even if the previous route doesn't strictly contain the current
route.

I think the above two changes are harmless and the new version is overall an
improvement.


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

to

==========
Settings: Fix Site Details subpage routing

Goal is: A site details subpage that's accessible both from /siteSettings/all,
/siteSettings/bluetooth, /siteSettings/plugins, etc. (all the categories).

Previous way it was done was: Separate route for each category, so there was
/siteSettings/all/details, /siteSettings/bluetooth/details,
/siteSettings/plugins/details.

And it relied on a quirk of how settings-animated-pages used to work to coalesce
all those routes to a single card.

New way I'm proposing: One single /siteSettings/siteDetails route.

But here are some extra needed changes:
 - settings-animated-pages must slide right / left based on route depth instead
of based on .contains()
 - subpage-back button needs to call window.history.back() whenever there is a
previous route, even if the previous route doesn't strictly contain the current
route.

I think the above two changes are harmless and the new version is overall an
improvement.

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

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

[commit-bot: I haz the power, 2016-08-17 19:08:16]

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