History: Add search browser test.

History: Add search browser test.

Updated the query stub to filter by search term and added a test for searching
in the history page.

R=dubroy@chromium.org
BUG=196568
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=192552

[Patrick Dubroy, 2013-04-03 09:52:10]

Thanks a lot for doing this. Some comments below.

https://codereview.chromium.org/13523002/diff/1/chrome/test/data/webui/histor...
File chrome/test/data/webui/history_browsertest.js (right):

https://codereview.chromium.org/13523002/diff/1/chrome/test/data/webui/histor...
chrome/test/data/webui/history_browsertest.js:182: var results = new Array();
Oh, I just noticed this. Should be "[]" instead of the Array constructor.

https://codereview.chromium.org/13523002/diff/1/chrome/test/data/webui/histor...
chrome/test/data/webui/history_browsertest.js:197: if (searchText) {
I feel like this should probably be moved to above "if (beginTime)", so that
maxCount, etc. will still apply.

Maybe you could do something like:

  if (searchText) {
    ...
  } else {
    results = history;  // Make a copy.
  }

And then change the code inside the "if (beginTime)" block to be results =
results.slice(...).

Alternatively, maybe use console.error() to ensure that searchText is never set
when maxCount or beginTime/endTime is, and put a note in that the stub is
deliberately limited.

But, it shouldn't be too difficult to get it all working together.

https://codereview.chromium.org/13523002/diff/1/chrome/test/data/webui/histor...
chrome/test/data/webui/history_browsertest.js:198: var newResults = new Array();
Use "[]" instead of the Array constructor.

[Sergiu, 2013-04-03 13:43:39]

https://codereview.chromium.org/13523002/diff/1/chrome/test/data/webui/histor...
File chrome/test/data/webui/history_browsertest.js (right):

https://codereview.chromium.org/13523002/diff/1/chrome/test/data/webui/histor...
chrome/test/data/webui/history_browsertest.js:182: var results = new Array();
On 2013/04/03 09:52:10, dubroy wrote:
> Oh, I just noticed this. Should be "[]" instead of the Array constructor.

Done.

https://codereview.chromium.org/13523002/diff/1/chrome/test/data/webui/histor...
chrome/test/data/webui/history_browsertest.js:197: if (searchText) {
On 2013/04/03 09:52:10, dubroy wrote:
> I feel like this should probably be moved to above "if (beginTime)", so that
> maxCount, etc. will still apply.
> 
> Maybe you could do something like:
> 
>   if (searchText) {
>     ...
>   } else {
>     results = history;  // Make a copy.
>   }
> 
> And then change the code inside the "if (beginTime)" block to be results =
> results.slice(...).
> 
> Alternatively, maybe use console.error() to ensure that searchText is never
set
> when maxCount or beginTime/endTime is, and put a note in that the stub is
> deliberately limited.
> 
> But, it shouldn't be too difficult to get it all working together.

Makes sense, fixed.

https://codereview.chromium.org/13523002/diff/1/chrome/test/data/webui/histor...
chrome/test/data/webui/history_browsertest.js:198: var newResults = new Array();
On 2013/04/03 09:52:10, dubroy wrote:
> Use "[]" instead of the Array constructor.

Done.

[Patrick Dubroy, 2013-04-03 14:12:33]

https://codereview.chromium.org/13523002/diff/7001/chrome/test/data/webui/his...
File chrome/test/data/webui/history_browsertest.js (right):

https://codereview.chromium.org/13523002/diff/7001/chrome/test/data/webui/his...
chrome/test/data/webui/history_browsertest.js:177: // Search only by title in
this stub.
nit: I'd move this comment down above the "if history[k].." statement.

https://codereview.chromium.org/13523002/diff/7001/chrome/test/data/webui/his...
chrome/test/data/webui/history_browsertest.js:220: finished: (searchText ||
history.length <= i + results.length),
Might as well make this correct too.

[Sergiu, 2013-04-03 17:04:39]

https://codereview.chromium.org/13523002/diff/7001/chrome/test/data/webui/his...
File chrome/test/data/webui/history_browsertest.js (right):

https://codereview.chromium.org/13523002/diff/7001/chrome/test/data/webui/his...
chrome/test/data/webui/history_browsertest.js:177: // Search only by title in
this stub.
On 2013/04/03 14:12:33, dubroy wrote:
> nit: I'd move this comment down above the "if history[k].." statement.

Done.

https://codereview.chromium.org/13523002/diff/7001/chrome/test/data/webui/his...
chrome/test/data/webui/history_browsertest.js:220: finished: (searchText ||
history.length <= i + results.length),
On 2013/04/03 14:12:33, dubroy wrote:
> Might as well make this correct too.

Done.

[Patrick Dubroy, 2013-04-03 17:08:38]

lgtm

[commit-bot: I haz the power, 2013-04-04 16:59:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergiu@chromium.org/13523002/16001

[commit-bot: I haz the power, 2013-04-04 17:42:43]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac&number...

[commit-bot: I haz the power, 2013-04-05 08:44:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergiu@chromium.org/13523002/24002

[commit-bot: I haz the power, 2013-04-05 13:05:34]

Change committed as 192552