[MD History] Add URL parameter for search.

[MD History] Add URL parameter for search.

This CL adds routing from the search term in the history toolbar to the
URL query. It also rewrites the old history page's #q=search queries into
?q=search to maintain compatibility.

BUG=619799
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation;master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/98fea99c967cbac5cd3959a3726d0a082c1acc93
Cr-Commit-Position: refs/heads/master@{#406515}

[calamity, 2016-06-14 08:03:45]

Description was changed from

==========
[MD History] Add URL parameter for search.

This CL adds routing from the search term in the history toolbar to the
URL query. It also rewrites the old history page's #q=search queries into
?q=search to maintain compatibility.

BUG=619799
==========

to

==========
[MD History] Add URL parameter for search.

This CL adds routing from the search term in the history toolbar to the
URL query. It also rewrites the old history page's #q=search queries into
?q=search to maintain compatibility.

BUG=619799
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[calamity, 2016-06-15 07:14:55]

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

[calamity, 2016-06-15 07:16:07]

Patchset #4 (id:60001) has been deleted

[calamity, 2016-06-15 07:18:12]

Patchset #4 (id:80001) has been deleted

[Dan Beam, 2016-06-15 22:08:00]

why do we need both ?q=search and #q=search?

[calamity, 2016-06-16 01:35:59]

On 2016/06/15 22:08:00, Dan Beam wrote:
> why do we need both ?q=search and #q=search?

It's probably nice to support anyone that bookmarked the old history page's
URLs. It's not a huge deal either way.

[Dan Beam, 2016-06-16 01:39:10]

On 2016/06/16 01:35:59, calamity wrote:
> On 2016/06/15 22:08:00, Dan Beam wrote:
> > why do we need both ?q=search and #q=search?
> 
> It's probably nice to support anyone that bookmarked the old history page's
> URLs. It's not a huge deal either way.

sorry, I guess that was ambiguous.

why do we need the *new* ?q=search URLs?  i.e. what's so much better about
?q=search that we'd support redirecting forever?

[tsergeant, 2016-06-16 02:00:15]

https://codereview.chromium.org/2068613002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/md_history/app.js (right):

https://codereview.chromium.org/2068613002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/md_history/app.js:69:
'searchQueryParamChanged_(route.__queryParams.q)',
app-route exposes a `queryParams` property. You should use that rather than bind
on something that's private.

[calamity, 2016-06-16 05:48:57]

On 2016/06/16 01:39:10, Dan Beam wrote:
> On 2016/06/16 01:35:59, calamity wrote:
> > On 2016/06/15 22:08:00, Dan Beam wrote:
> > > why do we need both ?q=search and #q=search?
> > 
> > It's probably nice to support anyone that bookmarked the old history page's
> > URLs. It's not a huge deal either way.
> 
> sorry, I guess that was ambiguous.
> 
> why do we need the *new* ?q=search URLs?  i.e. what's so much better about
> ?q=search that we'd support redirecting forever?

It's uhhh... supported by app-route =P

app-route doesn't have facilities to parse or modify the fragment (except in
this weird funky mode where it uses the fragment as the path).

We _could_ roll our own app-location that uses the hash as they query similar to
what happens for use-hash-as-path in
https://github.com/PolymerElements/app-route/blob/master/app-location.html at
the moment. I'm not sure if using the hash as the query is something they'd want
to support.

[calamity, 2016-06-16 06:49:14]

https://codereview.chromium.org/2068613002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/md_history/app.js (right):

https://codereview.chromium.org/2068613002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/md_history/app.js:69:
'searchQueryParamChanged_(route.__queryParams.q)',
On 2016/06/16 02:00:15, tsergeant wrote:
> app-route exposes a `queryParams` property. You should use that rather than
bind
> on something that's private.

Ugh! So _that's_ where that was. I was so confused about why I had to access a
private thing inside the route/tail. Why do we even _have_ that lever?

[calamity, 2016-07-14 05:21:51]

Description was changed from

==========
[MD History] Add URL parameter for search.

This CL adds routing from the search term in the history toolbar to the
URL query. It also rewrites the old history page's #q=search queries into
?q=search to maintain compatibility.

BUG=619799
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
[MD History] Add URL parameter for search.

This CL adds routing from the search term in the history toolbar to the
URL query. It also rewrites the old history page's #q=search queries into
?q=search to maintain compatibility.

BUG=619799
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation;master.tryserver.chromium.linux:closure_compilation
==========

[calamity, 2016-07-14 06:25:10]

Patchset #7 (id:160001) has been deleted

[calamity, 2016-07-18 04:24:01]

Patchset #8 (id:200001) has been deleted

[calamity, 2016-07-18 04:55:13]

Updated to work with the latest patch. PTAL, thanks.

[tsergeant, 2016-07-19 03:32:20]

https://codereview.chromium.org/2068613002/diff/240001/chrome/browser/resourc...
File chrome/browser/resources/md_history/app.js (right):

https://codereview.chromium.org/2068613002/diff/240001/chrome/browser/resourc...
chrome/browser/resources/md_history/app.js:83: if (window.location.hash) {
Should have a comment here explaining this.

https://codereview.chromium.org/2068613002/diff/240001/chrome/browser/resourc...
chrome/browser/resources/md_history/app.js:145: 
Nit: Undo this

https://codereview.chromium.org/2068613002/diff/240001/chrome/test/data/webui...
File chrome/test/data/webui/md_history/history_list_test.js (right):

https://codereview.chromium.org/2068613002/diff/240001/chrome/test/data/webui...
chrome/test/data/webui/md_history/history_list_test.js:240: flush().then(done);
You should just be able to

return flush();

without the done param.

https://codereview.chromium.org/2068613002/diff/240001/chrome/test/data/webui...
File chrome/test/data/webui/md_history/history_routing_test.js (right):

https://codereview.chromium.org/2068613002/diff/240001/chrome/test/data/webui...
chrome/test/data/webui/md_history/history_routing_test.js:101:
console.log(info[0]);
Delete this log

https://codereview.chromium.org/2068613002/diff/240001/chrome/test/data/webui...
File chrome/test/data/webui/md_history/md_history_browsertest.js (right):

https://codereview.chromium.org/2068613002/diff/240001/chrome/test/data/webui...
chrome/test/data/webui/md_history/md_history_browsertest.js:131: // things
going.
oh BOY

I -think- you can simplify this a bit by having the test take over this listener
when it is ready.

Here, do:

registerMessageCallback('queryHistory', this, function (info) {
  window.historyQueryInfo = info;
});

In the test, do:

if (window.historyQueryInfo) {
  verifyFunction(window.historyQueryInfo);
  return;
}

registerMessageCallback('queryHistory', this, verifyFunction);


I'm not 100% certain this will work, but it seems okay. If it doesn't, I would
suggest using a regular callback (window.historyQueriedCallback) instead of
`chrome.send('historyQueried')`, since chrome.send has a specific purpose
already.

https://codereview.chromium.org/2068613002/diff/240001/chrome/test/data/webui...
chrome/test/data/webui/md_history/md_history_browsertest.js:133:
console.log(info);
Delete this log

[calamity, 2016-07-19 04:34:26]

Patchset #10 (id:260001) has been deleted

[calamity, 2016-07-19 04:35:33]

https://codereview.chromium.org/2068613002/diff/240001/chrome/browser/resourc...
File chrome/browser/resources/md_history/app.js (right):

https://codereview.chromium.org/2068613002/diff/240001/chrome/browser/resourc...
chrome/browser/resources/md_history/app.js:83: if (window.location.hash) {
On 2016/07/19 03:32:20, tsergeant wrote:
> Should have a comment here explaining this.

Done.

https://codereview.chromium.org/2068613002/diff/240001/chrome/browser/resourc...
chrome/browser/resources/md_history/app.js:145: 
On 2016/07/19 03:32:20, tsergeant wrote:
> Nit: Undo this

Done.

https://codereview.chromium.org/2068613002/diff/240001/chrome/test/data/webui...
File chrome/test/data/webui/md_history/history_list_test.js (right):

https://codereview.chromium.org/2068613002/diff/240001/chrome/test/data/webui...
chrome/test/data/webui/md_history/history_list_test.js:240: flush().then(done);
On 2016/07/19 03:32:20, tsergeant wrote:
> You should just be able to
> 
> return flush();
> 
> without the done param.

Done.

https://codereview.chromium.org/2068613002/diff/240001/chrome/test/data/webui...
File chrome/test/data/webui/md_history/history_routing_test.js (right):

https://codereview.chromium.org/2068613002/diff/240001/chrome/test/data/webui...
chrome/test/data/webui/md_history/history_routing_test.js:101:
console.log(info[0]);
On 2016/07/19 03:32:20, tsergeant wrote:
> Delete this log

Done.

https://codereview.chromium.org/2068613002/diff/240001/chrome/test/data/webui...
File chrome/test/data/webui/md_history/md_history_browsertest.js (right):

https://codereview.chromium.org/2068613002/diff/240001/chrome/test/data/webui...
chrome/test/data/webui/md_history/md_history_browsertest.js:131: // things
going.
On 2016/07/19 03:32:20, tsergeant wrote:
> oh BOY
> 
> I -think- you can simplify this a bit by having the test take over this
listener
> when it is ready.
> 
> Here, do:
> 
> registerMessageCallback('queryHistory', this, function (info) {
>   window.historyQueryInfo = info;
> });
> 
> In the test, do:
> 
> if (window.historyQueryInfo) {
>   verifyFunction(window.historyQueryInfo);
>   return;
> }
> 
> registerMessageCallback('queryHistory', this, verifyFunction);
> 
> 
> I'm not 100% certain this will work, but it seems okay. If it doesn't, I would
> suggest using a regular callback (window.historyQueriedCallback) instead of
> `chrome.send('historyQueried')`, since chrome.send has a specific purpose
> already.

That does seem to work! I tested by strategically setting timeouts to test the
different cases, seems fine.

https://codereview.chromium.org/2068613002/diff/240001/chrome/test/data/webui...
chrome/test/data/webui/md_history/md_history_browsertest.js:133:
console.log(info);
On 2016/07/19 03:32:20, tsergeant wrote:
> Delete this log

Done.

[tsergeant, 2016-07-19 04:52:38]

lgtm with a nit

https://codereview.chromium.org/2068613002/diff/300001/chrome/test/data/webui...
File chrome/test/data/webui/md_history/md_history_browsertest.js (right):

https://codereview.chromium.org/2068613002/diff/300001/chrome/test/data/webui...
chrome/test/data/webui/md_history/md_history_browsertest.js:137: return
waitForUpgrade($('history-app')).then(function() {
`upgraded` is no longer used, so you can just do `return
waitForUpgrade($('history-app'))` and delete the variable above.

[calamity, 2016-07-19 06:48:56]

https://codereview.chromium.org/2068613002/diff/300001/chrome/test/data/webui...
File chrome/test/data/webui/md_history/md_history_browsertest.js (right):

https://codereview.chromium.org/2068613002/diff/300001/chrome/test/data/webui...
chrome/test/data/webui/md_history/md_history_browsertest.js:137: return
waitForUpgrade($('history-app')).then(function() {
On 2016/07/19 04:52:38, tsergeant wrote:
> `upgraded` is no longer used, so you can just do `return
> waitForUpgrade($('history-app'))` and delete the variable above.

Done.

[tsergeant, 2016-07-19 07:06:54]

https://codereview.chromium.org/2068613002/diff/320001/chrome/test/data/webui...
File chrome/test/data/webui/md_history/md_history_browsertest.js (right):

https://codereview.chromium.org/2068613002/diff/320001/chrome/test/data/webui...
chrome/test/data/webui/md_history/md_history_browsertest.js:135: return
waitForUpgrade($('history-app')).then(function() {
You can still get rid of the then() clause here.

[calamity, 2016-07-20 04:59:07]

https://codereview.chromium.org/2068613002/diff/320001/chrome/test/data/webui...
File chrome/test/data/webui/md_history/md_history_browsertest.js (right):

https://codereview.chromium.org/2068613002/diff/320001/chrome/test/data/webui...
chrome/test/data/webui/md_history/md_history_browsertest.js:135: return
waitForUpgrade($('history-app')).then(function() {
On 2016/07/19 07:06:54, tsergeant wrote:
> You can still get rid of the then() clause here.

Oops, Done.

[calamity, 2016-07-20 07:33:54]

The CQ bit was checked by calamity@chromium.org

[calamity, 2016-07-20 07:33:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
tsergeant@chromium.org
Link to the patchset: https://codereview.chromium.org/2068613002/#ps340001
(title: "remove_var")

[commit-bot: I haz the power, 2016-07-20 07:34:16]

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-07-20 07:34:18]

Your CL relies on deprecated CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
     tryserver.chromium.linux
   For more details, see http://crbug.com/617627.

[commit-bot: I haz the power, 2016-07-20 08:16:05]

Committed patchset #13 (id:340001)

[commit-bot: I haz the power, 2016-07-20 08:16:14]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-20 08:18:56]

Description was changed from

==========
[MD History] Add URL parameter for search.

This CL adds routing from the search term in the history toolbar to the
URL query. It also rewrites the old history page's #q=search queries into
?q=search to maintain compatibility.

BUG=619799
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation;master.tryserver.chromium.linux:closure_compilation
==========

to

==========
[MD History] Add URL parameter for search.

This CL adds routing from the search term in the history toolbar to the
URL query. It also rewrites the old history page's #q=search queries into
?q=search to maintain compatibility.

BUG=619799
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation;master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/98fea99c967cbac5cd3959a3726d0a082c1acc93
Cr-Commit-Position: refs/heads/master@{#406515}
==========

[commit-bot: I haz the power, 2016-07-20 08:18:58]

Patchset 13 (id:??) landed as
https://crrev.com/98fea99c967cbac5cd3959a3726d0a082c1acc93
Cr-Commit-Position: refs/heads/master@{#406515}