MD Settings: Allow trailing slashes in otherwise valid URLs.

MD Settings: Allow trailing slashes in otherwise valid URLs.

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

Committed: https://crrev.com/67c02ed84a243982e46ed96bdf392ee213b5fef1
Cr-Commit-Position: refs/heads/master@{#434816}

[dpapad, 2016-11-19 01:20:40]

Description was changed from

==========
MD Settings; Allow trailing slashes in otherwise valid URLs.

BUG=615242
==========

to

==========
MD Settings; Allow trailing slashes in otherwise valid URLs.

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

[dpapad, 2016-11-19 01:27:38]

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

[commit-bot: I haz the power, 2016-11-19 01:28: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-11-19 02:13:12]

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

[commit-bot: I haz the power, 2016-11-19 02:13:12]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[dpapad, 2016-11-23 19:26:47]

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

[commit-bot: I haz the power, 2016-11-23 19:27:51]

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

[dpapad, 2016-11-23 20:01:15]

Description was changed from

==========
MD Settings; Allow trailing slashes in otherwise valid URLs.

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

to

==========
MD Settings: Allow trailing slashes in otherwise valid URLs.

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

[commit-bot: I haz the power, 2016-11-23 21:22:40]

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

[commit-bot: I haz the power, 2016-11-23 21:22:41]

Dry run: This issue passed the CQ dry run.

[dpapad, 2016-11-23 21:32:07]

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

[dpapad, 2016-11-23 21:32:08]

Could not find an easy way to write a test for this. navigateTo() accepts Route
instances, not plain URLs, and initializeRouteFromUrl() looks at the URL, which
I can't tweak during a test.

[tommycli, 2016-11-23 22:07:08]

On 2016/11/23 21:32:08, dpapad wrote:
> Could not find an easy way to write a test for this. navigateTo() accepts
Route
> instances, not plain URLs, and initializeRouteFromUrl() looks at the URL,
which
> I can't tweak during a test.

Hi,

Since getRouteForPath is exposed in the public interface, you could write some
unit-test style tests like:

expectEq(settings.Route.PEOPLE, 'people');
expectEq(settings.Route.PEOPLE, 'people/');


and so forth.

Thanks!

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

thanks for the in-person explanation. production code lgtm. whole thing lgtm
assuming adding those tests to route_tests works. Some comments below.

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

https://codereview.chromium.org/2518763002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/route.js:252: return !!matchingKey ?
Route[matchingKey] : null;
I assume doing Route[undefined] is bad? (when there is no matching key)?

https://codereview.chromium.org/2518763002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/route.js:286: currentQueryParameters_ = new
URLSearchParams(window.location.search);
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 22:57:20]

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

https://codereview.chromium.org/2518763002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/route.js:249: return Route[key].path == (path
== '/' ? path : path.replace(/\/$/, ''));
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.

https://codereview.chromium.org/2518763002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/route.js:252: return !!matchingKey ?
Route[matchingKey] : null;
On 2016/11/23 at 22:33:46, tommycli wrote:
> I assume doing Route[undefined] is bad? (when there is no matching key)?

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.

[dpapad, 2016-11-23 23:51:56]

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

[commit-bot: I haz the power, 2016-11-23 23:52:27]

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

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

@tommycli: PTAL. I modified this CL since original LG, by beefing up the regular
expression and adding tests.

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

https://codereview.chromium.org/2518763002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/route.js:286: currentQueryParameters_ = new
URLSearchParams(window.location.search);
On 2016/11/23 at 22:33:46, tommycli wrote:
> 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.

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

[tommycli, 2016-11-24 00:02:52]

lgtm nit:

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

https://codereview.chromium.org/2518763002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/route.js:245: var CANONICAL_PATH_REGEX =
/(^\/)([\/-\w]{1,})(\/$)/;
Any particular reason to use {1,} instead of +? just curious

[tommycli, 2016-11-24 00:04:07]

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

https://codereview.chromium.org/2518763002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/route.js:245: var CANONICAL_PATH_REGEX =
/(^\/)([\/-\w]{1,})(\/$)/;
On 2016/11/24 00:02:51, tommycli wrote:
> Any particular reason to use {1,} instead of +? just curious

Instead of just "+"

Question mark is for the question, not for the non-greedy modifier lol

[dpapad, 2016-11-24 00:07:41]

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

https://codereview.chromium.org/2518763002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/route.js:245: var CANONICAL_PATH_REGEX =
/(^\/)([\/-\w]{1,})(\/$)/;
On 2016/11/24 at 00:04:07, tommycli wrote:
> On 2016/11/24 00:02:51, tommycli wrote:
> > Any particular reason to use {1,} instead of +? just curious
> 
> Instead of just "+"
> 
> Question mark is for the question, not for the non-greedy modifier lol

Yes. I need to ensure that there is at least on character between the leading
and trailing slashes, otherwise this assertion in my test fails,

assertEquals(null, settings.getRouteForPath('//'));

because it returns BASIC instead of null. The user visible bug is that if user
navigates to 
chrome://md-settings// is considered a valid path, where as the expected
behavior is to be forwarded to
chrome://md-settings.

[dpapad, 2016-11-24 00:12:41]

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

[commit-bot: I haz the power, 2016-11-24 00:13:47]

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

[dpapad, 2016-11-24 01:54:09]

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

https://codereview.chromium.org/2518763002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/route.js:245: var CANONICAL_PATH_REGEX =
/(^\/)([\/-\w]{1,})(\/$)/;
On 2016/11/24 at 00:07:41, dpapad wrote:
> On 2016/11/24 at 00:04:07, tommycli wrote:
> > On 2016/11/24 00:02:51, tommycli wrote:
> > > Any particular reason to use {1,} instead of +? just curious
> > 
> > Instead of just "+"
> > 
> > Question mark is for the question, not for the non-greedy modifier lol
> 
> Yes. I need to ensure that there is at least on character between the leading
and trailing slashes, otherwise this assertion in my test fails,
> 
> assertEquals(null, settings.getRouteForPath('//'));
> 
> because it returns BASIC instead of null. The user visible bug is that if user
navigates to 
> chrome://md-settings// is considered a valid path, where as the expected
behavior is to be forwarded to
> chrome://md-settings.

Done. I double checked and {1,} is equivalent to + (I must have tried * instead
of + initially).

[dpapad, 2016-11-24 01:54:18]

The CQ bit was checked by dpapad@chromium.org

[dpapad, 2016-11-24 01:54:18]

The patchset sent to the CQ was uploaded after l-g-t-m from
tommycli@chromium.org
Link to the patchset: https://codereview.chromium.org/2518763002/#ps80001
(title: "Nit per Tommy's comment.")

[commit-bot: I haz the power, 2016-11-24 01:55:17]

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-11-24 03:56:33]

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

[commit-bot: I haz the power, 2016-11-24 03:56:35]

Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  closure_compilation on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)

[dpapad, 2016-11-28 19:05:04]

The CQ bit was checked by dpapad@chromium.org

[commit-bot: I haz the power, 2016-11-28 19:05:39]

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-11-28 19:40:40]

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

[commit-bot: I haz the power, 2016-11-28 19:40:41]

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

[dpapad, 2016-11-29 00:27:48]

The CQ bit was checked by dpapad@chromium.org

[commit-bot: I haz the power, 2016-11-29 00:28:34]

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-11-29 01:41:15]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1480379268081130,
"parent_rev": "e400993781978e47f01392bda00965e343126355", "commit_rev":
"14cea14c8c32887f2e5745ba0fca5960f6cd1c3d"}

[commit-bot: I haz the power, 2016-11-29 01:41:44]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-11-29 01:48:00]

Description was changed from

==========
MD Settings: Allow trailing slashes in otherwise valid URLs.

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

to

==========
MD Settings: Allow trailing slashes in otherwise valid URLs.

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

Committed: https://crrev.com/67c02ed84a243982e46ed96bdf392ee213b5fef1
Cr-Commit-Position: refs/heads/master@{#434816}
==========

[commit-bot: I haz the power, 2016-11-29 01:48:01]

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