MD Bookmarks: Remove '/?id=1' from URL when displaying Bookmarks Bar

MD Bookmarks: Remove '/?id=1' from URL when displaying Bookmarks Bar

This changes the default URL for MD Bookmarks to 'chrome://bookmarks',
like the previous bookmark manager. Upon navigating to a different
folder or performing a search, the URL will update as expected.

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

Review-Url: https://codereview.chromium.org/2960143002
Cr-Commit-Position: refs/heads/master@{#484481}
Committed: https://chromium.googlesource.com/chromium/src/+/4622c187b62c9cec74ff576bd8d02f486cc6f394

[tsergeant, 2017-06-28 07:39:20]

Description was changed from

==========
MD Bookmarks: Remove '/?id=1' from URL when displaying Bookmarks Bar

This changes the default URL for MD Bookmarks to 'chrome://bookmarks',
like the previous bookmark manager. Upon navigating to a different
folder or performing a search, the URL will update as expected.

BUG=658980
==========

to

==========
MD Bookmarks: Remove '/?id=1' from URL when displaying Bookmarks Bar

This changes the default URL for MD Bookmarks to 'chrome://bookmarks',
like the previous bookmark manager. Upon navigating to a different
folder or performing a search, the URL will update as expected.

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

[tsergeant, 2017-06-28 07:42:19]

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

[commit-bot: I haz the power, 2017-06-28 07:42:25]

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, 2017-06-28 08:45:12]

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

[commit-bot: I haz the power, 2017-06-28 08:45:13]

Dry run: This issue passed the CQ dry run.

[tsergeant, 2017-07-04 03:22:04]

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

[commit-bot: I haz the power, 2017-07-04 03:22:13]

Dry run: CQ is trying da patch.

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

[tsergeant, 2017-07-04 04:16:06]

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

[commit-bot: I haz the power, 2017-07-04 04:16:13]

Dry run: CQ is trying da patch.

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

[tsergeant, 2017-07-04 04:18:21]

tsergeant@chromium.org changed reviewers:
+ calamity@chromium.org

[commit-bot: I haz the power, 2017-07-04 05:29:41]

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

[commit-bot: I haz the power, 2017-07-04 05:29:42]

Dry run: This issue passed the CQ dry run.

[calamity, 2017-07-05 04:04:39]

https://codereview.chromium.org/2960143002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/router.html (right):

https://codereview.chromium.org/2960143002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/router.html:10: <iron-location
query="{{query}}" dwell-time="200"></iron-location>
Is this just because 2s felt too long?

https://codereview.chromium.org/2960143002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/router.js (right):

https://codereview.chromium.org/2960143002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/router.js:54: if (searchTerm !=
(this.searchTerm_ || '')) {
This is a wild line.

https://codereview.chromium.org/2960143002/diff/40001/chrome/browser/ui/chrom...
File chrome/browser/ui/chrome_pages.cc (right):

https://codereview.chromium.org/2960143002/diff/40001/chrome/browser/ui/chrom...
chrome/browser/ui/chrome_pages.cc:174: params.path_behavior =
NavigateParams::IGNORE_AND_NAVIGATE;
This would be wayyy better as IGNORE_AND_STAY_PUT actually. Possibly ditto for
History now that I think about it..

https://codereview.chromium.org/2960143002/diff/40001/chrome/test/data/webui/...
File chrome/test/data/webui/md_bookmarks/router_test.js (right):

https://codereview.chromium.org/2960143002/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/md_bookmarks/router_test.js:81:
assertEquals('chrome://bookmarks/', window.location.href);
Assert above this that the route was ?id=2 at some point.

[tsergeant, 2017-07-05 07:07:00]

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

[commit-bot: I haz the power, 2017-07-05 07:07:18]

Dry run: CQ is trying da patch.

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

[tsergeant, 2017-07-05 07:37:36]

https://codereview.chromium.org/2960143002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/router.html (right):

https://codereview.chromium.org/2960143002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/router.html:10: <iron-location
query="{{query}}" dwell-time="200"></iron-location>
On 2017/07/05 04:04:38, calamity wrote:
> Is this just because 2s felt too long?

Yeah. I was hitting it occasionally, and it's really jarring when you go Back
and skip past a folder that you know you visited.

https://codereview.chromium.org/2960143002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/router.js (right):

https://codereview.chromium.org/2960143002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/router.js:54: if (searchTerm !=
(this.searchTerm_ || '')) {
On 2017/07/05 04:04:38, calamity wrote:
> This is a wild line.

Fixed. The OR case was only used during startup where this.searchTerm_ was
undefined. I've changed it so that the property just has a default value.

https://codereview.chromium.org/2960143002/diff/40001/chrome/browser/ui/chrom...
File chrome/browser/ui/chrome_pages.cc (right):

https://codereview.chromium.org/2960143002/diff/40001/chrome/browser/ui/chrom...
chrome/browser/ui/chrome_pages.cc:174: params.path_behavior =
NavigateParams::IGNORE_AND_NAVIGATE;
On 2017/07/05 04:04:39, calamity wrote:
> This would be wayyy better as IGNORE_AND_STAY_PUT actually. Possibly ditto for
> History now that I think about it..

Done for Bookmarks.

I was really on the fence about this and flipped back and forward while writing
the patch, but I'm happy to go with it.

Less sure about History -- because it doesn't real-time update,
IGNORE_AND_NAVIGATE ensures that you see fresh data.

https://codereview.chromium.org/2960143002/diff/40001/chrome/test/data/webui/...
File chrome/test/data/webui/md_bookmarks/router_test.js (right):

https://codereview.chromium.org/2960143002/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/md_bookmarks/router_test.js:81:
assertEquals('chrome://bookmarks/', window.location.href);
On 2017/07/05 04:04:39, calamity wrote:
> Assert above this that the route was ?id=2 at some point.

Done, by moving this extra check up into the test which already did that.

[commit-bot: I haz the power, 2017-07-05 08:02:06]

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

[commit-bot: I haz the power, 2017-07-05 08:02:07]

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

[calamity, 2017-07-06 04:16:29]

lgtm

https://codereview.chromium.org/2960143002/diff/40001/chrome/browser/ui/chrom...
File chrome/browser/ui/chrome_pages.cc (right):

https://codereview.chromium.org/2960143002/diff/40001/chrome/browser/ui/chrom...
chrome/browser/ui/chrome_pages.cc:174: params.path_behavior =
NavigateParams::IGNORE_AND_NAVIGATE;
On 2017/07/05 07:37:36, tsergeant wrote:
> On 2017/07/05 04:04:39, calamity wrote:
> > This would be wayyy better as IGNORE_AND_STAY_PUT actually. Possibly ditto
for
> > History now that I think about it..
> 
> Done for Bookmarks.
> 
> I was really on the fence about this and flipped back and forward while
writing
> the patch, but I'm happy to go with it.
> 
> Less sure about History -- because it doesn't real-time update,
> IGNORE_AND_NAVIGATE ensures that you see fresh data.

I just find it weird when I want to go back to the page, and it takes me there,
but it also destroys my state.

[tsergeant, 2017-07-06 04:51:04]

tsergeant@chromium.org changed reviewers:
+ thestig@chromium.org

[tsergeant, 2017-07-06 04:51:07]

+thestig@, PTAL at chrome_pages.cc

https://codereview.chromium.org/2960143002/diff/40001/chrome/browser/ui/chrom...
File chrome/browser/ui/chrome_pages.cc (right):

https://codereview.chromium.org/2960143002/diff/40001/chrome/browser/ui/chrom...
chrome/browser/ui/chrome_pages.cc:174: params.path_behavior =
NavigateParams::IGNORE_AND_NAVIGATE;
On 2017/07/06 04:16:29, calamity wrote:
> On 2017/07/05 07:37:36, tsergeant wrote:
> > On 2017/07/05 04:04:39, calamity wrote:
> > > This would be wayyy better as IGNORE_AND_STAY_PUT actually. Possibly ditto
> for
> > > History now that I think about it..
> > 
> > Done for Bookmarks.
> > 
> > I was really on the fence about this and flipped back and forward while
> writing
> > the patch, but I'm happy to go with it.
> > 
> > Less sure about History -- because it doesn't real-time update,
> > IGNORE_AND_NAVIGATE ensures that you see fresh data.
> 
> I just find it weird when I want to go back to the page, and it takes me
there,
> but it also destroys my state.

I understand, but there's precedent for the current behavior -- settings,
extensions, etc all work like this. I'll file a bug and we can discuss there.

[Lei Zhang, 2017-07-06 05:13:57]

On 2017/07/06 04:51:07, tsergeant wrote:
> +thestig@, PTAL at chrome_pages.cc

rs lgtm

[tsergeant, 2017-07-06 05:23:42]

The CQ bit was checked by tsergeant@chromium.org

[commit-bot: I haz the power, 2017-07-06 05:23:57]

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, 2017-07-06 06:35:26]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1499318622563550,
"parent_rev": "71bda89f989e04e5393a9e2bd33e6482ad6d2bff", "commit_rev":
"4622c187b62c9cec74ff576bd8d02f486cc6f394"}

[commit-bot: I haz the power, 2017-07-06 06:35:41]

Description was changed from

==========
MD Bookmarks: Remove '/?id=1' from URL when displaying Bookmarks Bar

This changes the default URL for MD Bookmarks to 'chrome://bookmarks',
like the previous bookmark manager. Upon navigating to a different
folder or performing a search, the URL will update as expected.

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

to

==========
MD Bookmarks: Remove '/?id=1' from URL when displaying Bookmarks Bar

This changes the default URL for MD Bookmarks to 'chrome://bookmarks',
like the previous bookmark manager. Upon navigating to a different
folder or performing a search, the URL will update as expected.

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

Review-Url: https://codereview.chromium.org/2960143002
Cr-Commit-Position: refs/heads/master@{#484481}
Committed:
https://chromium.googlesource.com/chromium/src/+/4622c187b62c9cec74ff576bd8d0...
==========

[commit-bot: I haz the power, 2017-07-06 06:35:42]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/4622c187b62c9cec74ff576bd8d0...