MD History: Implement search functionality.

MD History: Implement search functionality.

Search bar in the toolbar sends dynamic search requests as the user is typing
and these results are displayed on one history-card. As you scroll, if there are
more results these are loaded, and the number of results found is updated.

Within each item: if the title contains the search term, this is bolded.

BUG=425625

[hsampson, 2016-01-28 05:32:02]

hsampson@google.com changed reviewers:
+ calamity@chromium.org, tsergeant@chromium.org

[tsergeant, 2016-02-02 02:57:36]

Here's a first pass

https://codereview.chromium.org/1643693003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_history/history_card.html (right):

https://codereview.chromium.org/1643693003/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_history/history_card.html:15: :host([hidden]) {
Is this needed? I can't see you setting hidden anywhere.

https://codereview.chromium.org/1643693003/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_history/history_card.html:52: <template is="dom-if"
if="{{!isNotSearchResult_(searchTerm)}}">
Can you get rid of the dom-ifs by having a single computed function which
depends on searchTerm, historyItems.length and historyDate? The if would be
internal to that function.

https://codereview.chromium.org/1643693003/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_history/history_card.html:54:
{{searchResultTitle_(historyItems.length,searchTerm)}}
Nit: space after comma

https://codereview.chromium.org/1643693003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_history/history_card_manager.html (right):

https://codereview.chromium.org/1643693003/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_history/history_card_manager.html:54: <template
is="dom-if" if="{{noResults_(historyDataByDay_.length, loading)}}">
These dom-ifs complicate the template a fair bit.

An alternative to dom-if is to use hidden$=. If you had

<template is="dom-if" if="{{expr}}">
  <div id="blah"></div>
</template>

Then you can do this instead:

<div id="blah" hidden$="{{!expr}}"></div>

dom-if is still better if the thing you're hiding is big and likely to never be
seen, but here it's fine to use hidden instead (See
https://www.polymer-project.org/1.0/docs/devguide/templates.html#dom-if for more
context on the differences)

https://codereview.chromium.org/1643693003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_history/history_card_manager.js (right):

https://codereview.chromium.org/1643693003/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_history/history_card_manager.js:39:
noResultsMessage: {
Can this be private?

https://codereview.chromium.org/1643693003/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_history/history_card_manager.js:45: loading: {
And this?

https://codereview.chromium.org/1643693003/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_history/history_card_manager.js:107:
results[i].historySearchTerm = searchTerm;
What's this? I don't see it anywhere else

https://codereview.chromium.org/1643693003/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_history/history_card_manager.js:211: var scrollElem
= this.$$('#infinite-list');
If you use hidden$=, then you won't need to worry about this either.

https://codereview.chromium.org/1643693003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_history/history_item.js (right):

https://codereview.chromium.org/1643693003/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_history/history_item.js:107: setSearchedTextToBold_:
function() {
This is generally good. The thing that's a little messy is that you're changing
the contents of a node which is meant to be managed by Polymer.

I would get rid of the {{websiteTitle}} from this line in history_item.html:
<a href="{{websiteUrl_}}" id="title">{{websiteTitle_}}</a>

To make sure that this function is the only one responsible for the contents of
that node.

https://codereview.chromium.org/1643693003/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_history/history_item.js:112:
this.$['title'].textContent = '';
Just use this.$.title

https://codereview.chromium.org/1643693003/diff/20001/chrome/test/data/webui/...
File chrome/test/data/webui/md_history/history_card_manager_test.js (right):

https://codereview.chromium.org/1643693003/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/md_history/history_card_manager_test.js:73:
test('cancelling selection of multiple items', function(done) {
This should already be done, right? Make sure you rebase to get rid of it.

https://codereview.chromium.org/1643693003/diff/20001/chrome/test/data/webui/...
File chrome/test/data/webui/md_history/history_toolbar_test.js (right):

https://codereview.chromium.org/1643693003/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/md_history/history_toolbar_test.js:49: test('search term
gathered correctly from toolbar', function(done) {
So many tests \o/

https://codereview.chromium.org/1643693003/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/md_history/history_toolbar_test.js:65:
assertEquals(heading.textContent,
This feels kinda brittle -- you're testing the precise location of newline
characters. As a simple fix, can you use substr instead to check if the string
is in there?

[hsampson, 2016-02-03 02:37:59]

https://codereview.chromium.org/1643693003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_history/history_card.html (right):

https://codereview.chromium.org/1643693003/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_history/history_card.html:15: :host([hidden]) {
On 2016/02/02 02:57:36, tsergeant wrote:
> Is this needed? I can't see you setting hidden anywhere.

Yes this is needed so that the iron-list can hide cards it has rendered but
doesn't need, otherwise you end up with results from the previous display at the
bottom of your search results.

https://codereview.chromium.org/1643693003/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_history/history_card.html:52: <template is="dom-if"
if="{{!isNotSearchResult_(searchTerm)}}">
On 2016/02/02 02:57:36, tsergeant wrote:
> Can you get rid of the dom-ifs by having a single computed function which
> depends on searchTerm, historyItems.length and historyDate? The if would be
> internal to that function.

Done.

https://codereview.chromium.org/1643693003/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_history/history_card.html:54:
{{searchResultTitle_(historyItems.length,searchTerm)}}
On 2016/02/02 02:57:36, tsergeant wrote:
> Nit: space after comma

Done.

https://codereview.chromium.org/1643693003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_history/history_card_manager.html (right):

https://codereview.chromium.org/1643693003/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_history/history_card_manager.html:54: <template
is="dom-if" if="{{noResults_(historyDataByDay_.length, loading)}}">
On 2016/02/02 02:57:36, tsergeant wrote:
> These dom-ifs complicate the template a fair bit.
> 
> An alternative to dom-if is to use hidden$=. If you had
> 
> <template is="dom-if" if="{{expr}}">
>   <div id="blah"></div>
> </template>
> 
> Then you can do this instead:
> 
> <div id="blah" hidden$="{{!expr}}"></div>
> 
> dom-if is still better if the thing you're hiding is big and likely to never
be
> seen, but here it's fine to use hidden instead (See
> https://www.polymer-project.org/1.0/docs/devguide/templates.html#dom-if for
more
> context on the differences)

Done.

https://codereview.chromium.org/1643693003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_history/history_card_manager.js (right):

https://codereview.chromium.org/1643693003/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_history/history_card_manager.js:39:
noResultsMessage: {
On 2016/02/02 02:57:36, tsergeant wrote:
> Can this be private?

Done.

https://codereview.chromium.org/1643693003/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_history/history_card_manager.js:45: loading: {
On 2016/02/02 02:57:36, tsergeant wrote:
> And this?

Done.

https://codereview.chromium.org/1643693003/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_history/history_card_manager.js:107:
results[i].historySearchTerm = searchTerm;
On 2016/02/02 02:57:36, tsergeant wrote:
> What's this? I don't see it anywhere else

This is used in addNewResults() if the results returned are search results, as
the results get returned in a slightly different format.

https://codereview.chromium.org/1643693003/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_history/history_card_manager.js:211: var scrollElem
= this.$$('#infinite-list');
On 2016/02/02 02:57:36, tsergeant wrote:
> If you use hidden$=, then you won't need to worry about this either.

Done.

https://codereview.chromium.org/1643693003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_history/history_item.js (right):

https://codereview.chromium.org/1643693003/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_history/history_item.js:107: setSearchedTextToBold_:
function() {
On 2016/02/02 02:57:36, tsergeant wrote:
> This is generally good. The thing that's a little messy is that you're
changing
> the contents of a node which is meant to be managed by Polymer.
> 
> I would get rid of the {{websiteTitle}} from this line in history_item.html:
> <a href="{{websiteUrl_}}" id="title">{{websiteTitle_}}</a>
> 
> To make sure that this function is the only one responsible for the contents
of
> that node.

Done.

https://codereview.chromium.org/1643693003/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_history/history_item.js:112:
this.$['title'].textContent = '';
On 2016/02/02 02:57:36, tsergeant wrote:
> Just use this.$.title

Done.

https://codereview.chromium.org/1643693003/diff/20001/chrome/test/data/webui/...
File chrome/test/data/webui/md_history/history_card_manager_test.js (right):

https://codereview.chromium.org/1643693003/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/md_history/history_card_manager_test.js:73:
test('cancelling selection of multiple items', function(done) {
On 2016/02/02 02:57:36, tsergeant wrote:
> This should already be done, right? Make sure you rebase to get rid of it.

Done.

https://codereview.chromium.org/1643693003/diff/20001/chrome/test/data/webui/...
File chrome/test/data/webui/md_history/history_toolbar_test.js (right):

https://codereview.chromium.org/1643693003/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/md_history/history_toolbar_test.js:65:
assertEquals(heading.textContent,
On 2016/02/02 02:57:36, tsergeant wrote:
> This feels kinda brittle -- you're testing the precise location of newline
> characters. As a simple fix, can you use substr instead to check if the string
> is in there?

Done. Yup that's way better.

[tsergeant, 2016-02-04 02:28:50]

Attn: Chris, there's a question for you buried in here.

https://codereview.chromium.org/1643693003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_history/history_card.html (right):

https://codereview.chromium.org/1643693003/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_history/history_card.html:52: <template is="dom-if"
if="{{!isNotSearchResult_(searchTerm)}}">
On 2016/02/03 02:37:58, hsampson wrote:
> On 2016/02/02 02:57:36, tsergeant wrote:
> > Can you get rid of the dom-ifs by having a single computed function which
> > depends on searchTerm, historyItems.length and historyDate? The if would be
> > internal to that function.
> 
> Done.

Yay!

https://codereview.chromium.org/1643693003/diff/60001/chrome/browser/resource...
File chrome/browser/resources/md_history/history_card.html (right):

https://codereview.chromium.org/1643693003/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_history/history_card.html:53:
{{searchResultTitle_(historyItems.length, searchTerm, historyDate)}}
Can you rename searchResultTitle to cardTitle or something that doesn't
reference search (since it's not always a search)

https://codereview.chromium.org/1643693003/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_history/history_card.html:56: <template
is="dom-repeat" items="[[historyItems]]"
Make sure this is rebased correctly, so that bug doesn't crop up again

https://codereview.chromium.org/1643693003/diff/60001/chrome/browser/resource...
File chrome/browser/resources/md_history/history_card.js (right):

https://codereview.chromium.org/1643693003/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_history/history_card.js:60: if (search == '') {
Nit: Remove {}

Also use a ternary expression if you want, but it would be pretty long, so I'll
leave it up to you.

https://codereview.chromium.org/1643693003/diff/60001/chrome/browser/resource...
File chrome/browser/resources/md_history/history_card_manager.html (right):

https://codereview.chromium.org/1643693003/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_history/history_card_manager.html:60:
{{noResultsMessage_}}
Can this be a computed function instead of a property?

https://codereview.chromium.org/1643693003/diff/60001/chrome/browser/resource...
File chrome/browser/resources/md_history/history_card_manager.js (right):

https://codereview.chromium.org/1643693003/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_history/history_card_manager.js:71:
this.splice('historyDataByDay_', 0, this.historyDataByDay_.length);
Have you thought about not replacing the old results immediately, but when new
results come in? I'd expect that would have better performance, since you'd be
changing historyDataByData once rather than twice with a gap between them.

If you're not sure or it's too annoying right now, we can probably follow this
up later (maybe put in a TODO for it, though)

https://codereview.chromium.org/1643693003/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_history/history_card_manager.js:105:
results[i].dateRelativeDay = search;
On further thought, I don't like this pattern. It's clobbering the meaning of
these variables and making the code hard to read (dateRelativeDay is sometimes
not a date, dateTimeOfDay is either a time or a date).

I would prefer it if you made a new field for these things with a more generic
name, set them regardless of whether you're in a search or not, use those where
you need to, and leave the original fields alone.

For example, create something like results[i].visibleTimestamp which is set to
dateTimeOfDay for normal results, but dateShort for search results.

https://codereview.chromium.org/1643693003/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_history/history_card_manager.js:207: var scrollElem
= this.$$('#infinite-list');
infinite-list will always be present, so you can use this.$[] now.

https://codereview.chromium.org/1643693003/diff/60001/chrome/test/data/webui/...
File chrome/test/data/webui/md_history/history_toolbar_test.js (right):

https://codereview.chromium.org/1643693003/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/md_history/history_toolbar_test.js:68: var index =
heading.search('Found 1 search result for \'Google\'.');
Use .indexOf instead of search. indexOf uses a literal string, whereas search
uses a regex and so could theoretically cause false positives.

Also -- Chris, is it okay to check strings like this in a test? These tests
would fail if Chrome was run in another language, for example.

https://codereview.chromium.org/1643693003/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/md_history/history_toolbar_test.js:70: assertTrue(0 <=
index);
Probably assertTrue(index != -1), which is the normal pattern

https://codereview.chromium.org/1643693003/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/md_history/history_toolbar_test.js:81: var index =
message.search('No history entries found.');
As above

https://codereview.chromium.org/1643693003/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/md_history/history_toolbar_test.js:94: var index =
message.search('No search results found.');
As above

[calamity, 2016-02-05 02:30:10]

https://codereview.chromium.org/1643693003/diff/60001/chrome/browser/resource...
File chrome/browser/resources/md_history/history_card.js (right):

https://codereview.chromium.org/1643693003/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_history/history_card.js:40: if ((search == '') ||
(search == undefined)) {
(((Too) (many) parens))!

https://codereview.chromium.org/1643693003/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_history/history_card.js:57: searchResultTitle_:
function(numberOfItems, search, historyDate) {
This should be called something like 'cardTitle_' since it is used for both
search results _and_ normal cards.

The comment should be updated to reflect what the function does.

e.g

// Generates the title for this history card.

https://codereview.chromium.org/1643693003/diff/60001/chrome/browser/resource...
File chrome/browser/resources/md_history/history_card_manager.html (right):

https://codereview.chromium.org/1643693003/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_history/history_card_manager.html:52: display: flex;
Should be able to remove this.

https://codereview.chromium.org/1643693003/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_history/history_card_manager.html:53: font-size:
123.1%;
This is an interesting font size...

https://codereview.chromium.org/1643693003/diff/60001/chrome/browser/resource...
File chrome/browser/resources/md_history/history_card_manager.js (right):

https://codereview.chromium.org/1643693003/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_history/history_card_manager.js:209:
scrollElem.scrollTop + scrollElem.clientHeight + scrollOffset)) {
nit: less parens.

https://codereview.chromium.org/1643693003/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_history/history_card_manager.js:221:
resultsAvailable_: function(numberOfResults) {
Does this run? It needs an underscore?

https://codereview.chromium.org/1643693003/diff/60001/chrome/browser/resource...
File chrome/browser/resources/md_history/history_item.js (right):

https://codereview.chromium.org/1643693003/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_history/history_item.js:109: var i = 0;
Pull this.$.title into a local var here.

https://codereview.chromium.org/1643693003/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_history/history_item.js:110: if ((this.searchTerm !=
'') && (this.searchTerm != null)) {
nit: less parens.

https://codereview.chromium.org/1643693003/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_history/history_item.js:111: var searchText = new
RegExp(this.quoteString_(this.searchTerm), 'gim');
nit: TBH, I kinda like just calling this re. searchText can mean too many
things.

https://codereview.chromium.org/1643693003/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_history/history_item.js:115: if (match.index > i)
nit: This one needs braces since the content is 2 lines long.

https://codereview.chromium.org/1643693003/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_history/history_item.js:128:
this.$.title.textContent = this.websiteTitle_;
Invert the condition above and early return in this case. Do you remember
DeMoivre's theorem? =P

https://codereview.chromium.org/1643693003/diff/60001/chrome/browser/resource...
File chrome/browser/resources/md_history/history_toolbar.js (right):

https://codereview.chromium.org/1643693003/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_history/history_toolbar.js:23: searchTerm: {
Comment.

https://codereview.chromium.org/1643693003/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_history/history_toolbar.js:48: this.async(function()
{
Why async?

https://codereview.chromium.org/1643693003/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_history/history_toolbar.js:50:
this.$$('#search-input').setDelegate(this.searchFieldDelegate_);
I believe this can also become this.$['search-input'].

Actually, I'm super surprised that the this keyword has bound correctly here.
*shrug*

[hsampson, 2016-02-08 04:40:23]

https://codereview.chromium.org/1643693003/diff/60001/chrome/browser/resource...
File chrome/browser/resources/md_history/history_card.html (right):

https://codereview.chromium.org/1643693003/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_history/history_card.html:53:
{{searchResultTitle_(historyItems.length, searchTerm, historyDate)}}
On 2016/02/04 02:28:49, tsergeant wrote:
> Can you rename searchResultTitle to cardTitle or something that doesn't
> reference search (since it's not always a search)

Done.

https://codereview.chromium.org/1643693003/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_history/history_card.html:56: <template
is="dom-repeat" items="[[historyItems]]"
On 2016/02/04 02:28:49, tsergeant wrote:
> Make sure this is rebased correctly, so that bug doesn't crop up again

Done.

https://codereview.chromium.org/1643693003/diff/60001/chrome/browser/resource...
File chrome/browser/resources/md_history/history_card.js (right):

https://codereview.chromium.org/1643693003/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_history/history_card.js:40: if ((search == '') ||
(search == undefined)) {
On 2016/02/05 02:30:10, calamity wrote:
> (((Too) (many) parens))!

Done.

https://codereview.chromium.org/1643693003/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_history/history_card.js:57: searchResultTitle_:
function(numberOfItems, search, historyDate) {
On 2016/02/05 02:30:10, calamity wrote:
> This should be called something like 'cardTitle_' since it is used for both
> search results _and_ normal cards.
> 
> The comment should be updated to reflect what the function does.
> 
> e.g
> 
> // Generates the title for this history card.

Done.

https://codereview.chromium.org/1643693003/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_history/history_card.js:60: if (search == '') {
On 2016/02/04 02:28:49, tsergeant wrote:
> Nit: Remove {}
> 
> Also use a ternary expression if you want, but it would be pretty long, so
I'll
> leave it up to you.

Done.

https://codereview.chromium.org/1643693003/diff/60001/chrome/browser/resource...
File chrome/browser/resources/md_history/history_card_manager.html (right):

https://codereview.chromium.org/1643693003/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_history/history_card_manager.html:52: display: flex;
On 2016/02/05 02:30:10, calamity wrote:
> Should be able to remove this.

It looks like it but if I try removing any of the flex stuff it changes the
layout too much.

https://codereview.chromium.org/1643693003/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_history/history_card_manager.html:53: font-size:
123.1%;
On 2016/02/05 02:30:10, calamity wrote:
> This is an interesting font size...

Yea just copied it from md_downloads as there are no specs for this. Have
changed it to the same size as the card titles.

https://codereview.chromium.org/1643693003/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_history/history_card_manager.html:60:
{{noResultsMessage_}}
On 2016/02/04 02:28:49, tsergeant wrote:
> Can this be a computed function instead of a property?

Done.

https://codereview.chromium.org/1643693003/diff/60001/chrome/browser/resource...
File chrome/browser/resources/md_history/history_card_manager.js (right):

https://codereview.chromium.org/1643693003/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_history/history_card_manager.js:71:
this.splice('historyDataByDay_', 0, this.historyDataByDay_.length);
On 2016/02/04 02:28:49, tsergeant wrote:
> Have you thought about not replacing the old results immediately, but when new
> results come in? I'd expect that would have better performance, since you'd be
> changing historyDataByData once rather than twice with a gap between them.
> 
> If you're not sure or it's too annoying right now, we can probably follow this
> up later (maybe put in a TODO for it, though)

Acknowledged.

https://codereview.chromium.org/1643693003/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_history/history_card_manager.js:105:
results[i].dateRelativeDay = search;
On 2016/02/04 02:28:49, tsergeant wrote:
> On further thought, I don't like this pattern. It's clobbering the meaning of
> these variables and making the code hard to read (dateRelativeDay is sometimes
> not a date, dateTimeOfDay is either a time or a date).
> 
> I would prefer it if you made a new field for these things with a more generic
> name, set them regardless of whether you're in a search or not, use those
where
> you need to, and leave the original fields alone.
> 
> For example, create something like results[i].visibleTimestamp which is set to
> dateTimeOfDay for normal results, but dateShort for search results.

Done.

https://codereview.chromium.org/1643693003/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_history/history_card_manager.js:207: var scrollElem
= this.$$('#infinite-list');
On 2016/02/04 02:28:49, tsergeant wrote:
> infinite-list will always be present, so you can use this.$[] now.

Done.

https://codereview.chromium.org/1643693003/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_history/history_card_manager.js:209:
scrollElem.scrollTop + scrollElem.clientHeight + scrollOffset)) {
On 2016/02/05 02:30:10, calamity wrote:
> nit: less parens.

Done.

https://codereview.chromium.org/1643693003/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_history/history_card_manager.js:221:
resultsAvailable_: function(numberOfResults) {
On 2016/02/05 02:30:10, calamity wrote:
> Does this run? It needs an underscore?

Yes it runs.

https://codereview.chromium.org/1643693003/diff/60001/chrome/browser/resource...
File chrome/browser/resources/md_history/history_toolbar.js (right):

https://codereview.chromium.org/1643693003/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_history/history_toolbar.js:23: searchTerm: {
On 2016/02/05 02:30:10, calamity wrote:
> Comment.

Done.

https://codereview.chromium.org/1643693003/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_history/history_toolbar.js:48: this.async(function()
{
On 2016/02/05 02:30:10, calamity wrote:
> Why async?

Because otherwise this function was being called before the search bar had been
created and this is what was breaking the refresh when devtools was open.

https://codereview.chromium.org/1643693003/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_history/history_toolbar.js:50:
this.$$('#search-input').setDelegate(this.searchFieldDelegate_);
On 2016/02/05 02:30:10, calamity wrote:
> I believe this can also become this.$['search-input'].
> 
> Actually, I'm super surprised that the this keyword has bound correctly here.
> *shrug*

Done.

https://codereview.chromium.org/1643693003/diff/60001/chrome/test/data/webui/...
File chrome/test/data/webui/md_history/history_toolbar_test.js (right):

https://codereview.chromium.org/1643693003/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/md_history/history_toolbar_test.js:68: var index =
heading.search('Found 1 search result for \'Google\'.');
On 2016/02/04 02:28:50, tsergeant wrote:
> Use .indexOf instead of search. indexOf uses a literal string, whereas search
> uses a regex and so could theoretically cause false positives.
> 
> Also -- Chris, is it okay to check strings like this in a test? These tests
> would fail if Chrome was run in another language, for example.

Done.

https://codereview.chromium.org/1643693003/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/md_history/history_toolbar_test.js:70: assertTrue(0 <=
index);
On 2016/02/04 02:28:50, tsergeant wrote:
> Probably assertTrue(index != -1), which is the normal pattern

Done.

https://codereview.chromium.org/1643693003/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/md_history/history_toolbar_test.js:81: var index =
message.search('No history entries found.');
On 2016/02/04 02:28:49, tsergeant wrote:
> As above

Done.

https://codereview.chromium.org/1643693003/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/md_history/history_toolbar_test.js:94: var index =
message.search('No search results found.');
On 2016/02/04 02:28:50, tsergeant wrote:
> As above

Done.

[tsergeant, 2016-02-08 22:59:01]

Partial code review before I run off to a meeting.

Be aware that several of the comments are on an old patchset, so don't get lost
in the confusing Rietveld UI.

https://codereview.chromium.org/1643693003/diff/60001/chrome/browser/resource...
File chrome/browser/resources/md_history/history_card_manager.html (right):

https://codereview.chromium.org/1643693003/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_history/history_card_manager.html:60:
{{noResultsMessage_}}
On 2016/02/08 04:40:23, hsampson wrote:
> On 2016/02/04 02:28:49, tsergeant wrote:
> > Can this be a computed function instead of a property?
> 
> Done.

Meep, what I meant here was to create a function like

{{noResultsMessage_(searchTerm)}}

That way, the message would be updated automatically rather than having to call
noResults_ manually.

https://codereview.chromium.org/1643693003/diff/60001/chrome/browser/resource...
File chrome/browser/resources/md_history/history_card_manager.js (right):

https://codereview.chromium.org/1643693003/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_history/history_card_manager.js:107:
results[i].historySearchTerm = search;
What happened to historySearchTerm? Was it unused?

https://codereview.chromium.org/1643693003/diff/60001/chrome/browser/resource...
File chrome/browser/resources/md_history/history_item.js (right):

https://codereview.chromium.org/1643693003/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_history/history_item.js:109: var i = 0;
On 2016/02/05 02:30:10, calamity wrote:
> Pull this.$.title into a local var here.

Bump on the comments in this file.

https://codereview.chromium.org/1643693003/diff/60001/chrome/browser/resource...
File chrome/browser/resources/md_history/history_toolbar.js (right):

https://codereview.chromium.org/1643693003/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_history/history_toolbar.js:48: this.async(function()
{
On 2016/02/08 04:40:23, hsampson wrote:
> On 2016/02/05 02:30:10, calamity wrote:
> > Why async?
> 
> Because otherwise this function was being called before the search bar had
been
> created and this is what was breaking the refresh when devtools was open.

This -should- be fixed by removing the dom-ifs from the toolbar. Have you tried
removing the async and seeing if it works?

https://codereview.chromium.org/1643693003/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/md_history/history_card.html (right):

https://codereview.chromium.org/1643693003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_card.html:68: <template is="dom-if"
if="[[needsTimeGap_(index, historyItem, searchTerm)]]">
Nit: Wrap to 80 characters

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

https://codereview.chromium.org/1643693003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_card.js:9: // The date of these
history items.
Update this comment

https://codereview.chromium.org/1643693003/diff/100001/chrome/test/data/webui...
File chrome/test/data/webui/md_history/history_toolbar_test.js (right):

https://codereview.chromium.org/1643693003/diff/100001/chrome/test/data/webui...
chrome/test/data/webui/md_history/history_toolbar_test.js:60: test('search
results display with correct title', function(done) {
Chris and I had a chat about these tests the other day, and agree that
unfortunately they probably can't stay. The tests are entirely
language-dependent, and annoyingly brittle if we ever change the strings.

Ultimately, the tests are mostly checking that databinding has been done
correctly in the .html files, which means they're not extremely critical tests,
but they are nice to have. 

So, try and think of something to replace them which has more or less the same
effect without being entirely dependent on a loadTimeData string.

For example, you can test that "<b>Google</b>" is the result item, since it
doesn't depend on an external string.

You can also check that #no-results is visible (probably), but can't actually
check the contents.

[hsampson, 2016-02-09 02:17:51]

https://codereview.chromium.org/1643693003/diff/60001/chrome/browser/resource...
File chrome/browser/resources/md_history/history_card_manager.html (right):

https://codereview.chromium.org/1643693003/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_history/history_card_manager.html:60:
{{noResultsMessage_}}
On 2016/02/08 22:59:01, tsergeant wrote:
> On 2016/02/08 04:40:23, hsampson wrote:
> > On 2016/02/04 02:28:49, tsergeant wrote:
> > > Can this be a computed function instead of a property?
> > 
> > Done.
> 
> Meep, what I meant here was to create a function like
> 
> {{noResultsMessage_(searchTerm)}}
> 
> That way, the message would be updated automatically rather than having to
call
> noResults_ manually.

Done.

https://codereview.chromium.org/1643693003/diff/60001/chrome/browser/resource...
File chrome/browser/resources/md_history/history_card_manager.js (right):

https://codereview.chromium.org/1643693003/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_history/history_card_manager.js:107:
results[i].historySearchTerm = search;
On 2016/02/08 22:59:01, tsergeant wrote:
> What happened to historySearchTerm? Was it unused?

Yes it wasn't being used anywhere.

https://codereview.chromium.org/1643693003/diff/60001/chrome/browser/resource...
File chrome/browser/resources/md_history/history_item.js (right):

https://codereview.chromium.org/1643693003/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_history/history_item.js:109: var i = 0;
On 2016/02/05 02:30:10, calamity wrote:
> Pull this.$.title into a local var here.

Done.

https://codereview.chromium.org/1643693003/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_history/history_item.js:110: if ((this.searchTerm !=
'') && (this.searchTerm != null)) {
On 2016/02/05 02:30:10, calamity wrote:
> nit: less parens.

Done.

https://codereview.chromium.org/1643693003/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_history/history_item.js:111: var searchText = new
RegExp(this.quoteString_(this.searchTerm), 'gim');
On 2016/02/05 02:30:10, calamity wrote:
> nit: TBH, I kinda like just calling this re. searchText can mean too many
> things.

Done.

https://codereview.chromium.org/1643693003/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_history/history_item.js:115: if (match.index > i)
On 2016/02/05 02:30:10, calamity wrote:
> nit: This one needs braces since the content is 2 lines long.

NA now.

https://codereview.chromium.org/1643693003/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_history/history_item.js:128:
this.$.title.textContent = this.websiteTitle_;
On 2016/02/05 02:30:10, calamity wrote:
> Invert the condition above and early return in this case. Do you remember
> DeMoivre's theorem? =P

No idea what that is :P but done I think.

https://codereview.chromium.org/1643693003/diff/60001/chrome/browser/resource...
File chrome/browser/resources/md_history/history_toolbar.js (right):

https://codereview.chromium.org/1643693003/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_history/history_toolbar.js:48: this.async(function()
{
On 2016/02/08 22:59:01, tsergeant wrote:
> On 2016/02/08 04:40:23, hsampson wrote:
> > On 2016/02/05 02:30:10, calamity wrote:
> > > Why async?
> > 
> > Because otherwise this function was being called before the search bar had
> been
> > created and this is what was breaking the refresh when devtools was open.
> 
> This -should- be fixed by removing the dom-ifs from the toolbar. Have you
tried
> removing the async and seeing if it works?

If the async is removed and you refresh the page with devtools open the search
bar no longer works. Refreshing the page normally works, but I think this is the
same bug as last time.

https://codereview.chromium.org/1643693003/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/md_history/history_card.html (right):

https://codereview.chromium.org/1643693003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_card.html:68: <template is="dom-if"
if="[[needsTimeGap_(index, historyItem, searchTerm)]]">
On 2016/02/08 22:59:01, tsergeant wrote:
> Nit: Wrap to 80 characters

Done.

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

https://codereview.chromium.org/1643693003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_card.js:9: // The date of these
history items.
On 2016/02/08 22:59:01, tsergeant wrote:
> Update this comment

Done.

https://codereview.chromium.org/1643693003/diff/100001/chrome/test/data/webui...
File chrome/test/data/webui/md_history/history_toolbar_test.js (right):

https://codereview.chromium.org/1643693003/diff/100001/chrome/test/data/webui...
chrome/test/data/webui/md_history/history_toolbar_test.js:60: test('search
results display with correct title', function(done) {
On 2016/02/08 22:59:01, tsergeant wrote:
> Chris and I had a chat about these tests the other day, and agree that
> unfortunately they probably can't stay. The tests are entirely
> language-dependent, and annoyingly brittle if we ever change the strings.
> 
> Ultimately, the tests are mostly checking that databinding has been done
> correctly in the .html files, which means they're not extremely critical
tests,
> but they are nice to have. 
> 
> So, try and think of something to replace them which has more or less the same
> effect without being entirely dependent on a loadTimeData string.
> 
> For example, you can test that "<b>Google</b>" is the result item, since it
> doesn't depend on an external string.
> 
> You can also check that #no-results is visible (probably), but can't actually
> check the contents.

Done.

[tsergeant, 2016-02-09 04:28:54]

https://codereview.chromium.org/1643693003/diff/60001/chrome/browser/resource...
File chrome/browser/resources/md_history/history_card_manager.html (right):

https://codereview.chromium.org/1643693003/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_history/history_card_manager.html:60:
{{noResultsMessage_}}
On 2016/02/09 02:17:51, hsampson wrote:
> On 2016/02/08 22:59:01, tsergeant wrote:
> > On 2016/02/08 04:40:23, hsampson wrote:
> > > On 2016/02/04 02:28:49, tsergeant wrote:
> > > > Can this be a computed function instead of a property?
> > > 
> > > Done.
> > 
> > Meep, what I meant here was to create a function like
> > 
> > {{noResultsMessage_(searchTerm)}}
> > 
> > That way, the message would be updated automatically rather than having to
> call
> > noResults_ manually.
> 
> Done.

This still isn't quite what I intended, but it does have its benefits. I'll
explain in person.

https://codereview.chromium.org/1643693003/diff/160001/chrome/browser/resourc...
File chrome/browser/resources/md_history/history_card_manager.html (right):

https://codereview.chromium.org/1643693003/diff/160001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_card_manager.html:69:
hidden$="{{noResultsAvailable_(historyDataByDay_.length, loading_)}}">
As discussed, try to rename these functions so that there's no confusion between
resultsAvailable_ and noResultsAvailable_

https://codereview.chromium.org/1643693003/diff/160001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_card_manager.html:71: <history-card
history-title="{{historyDay.cardTitle}}"
As discussed, revert this back to passing down the date instead of the title

https://codereview.chromium.org/1643693003/diff/160001/chrome/browser/resourc...
File chrome/browser/resources/md_history/history_card_manager.js (right):

https://codereview.chromium.org/1643693003/diff/160001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_card_manager.js:40: type: Boolean,
Should loading_ be true to start with while initial results are loaded in?

https://codereview.chromium.org/1643693003/diff/160001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_card_manager.js:133:
Polymer.dom.flush();
Why is this here?

https://codereview.chromium.org/1643693003/diff/160001/chrome/test/data/webui...
File chrome/test/data/webui/md_history/history_toolbar_test.js (right):

https://codereview.chromium.org/1643693003/diff/160001/chrome/test/data/webui...
chrome/test/data/webui/md_history/history_toolbar_test.js:60: test('search
results display with correct item title', function(done) {
These next few tests should be moved to history_card_manager_test since there's
no mention of the toolbar in them.

[hsampson, 2016-02-11 02:46:26]

https://codereview.chromium.org/1643693003/diff/160001/chrome/browser/resourc...
File chrome/browser/resources/md_history/history_card_manager.html (right):

https://codereview.chromium.org/1643693003/diff/160001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_card_manager.html:69:
hidden$="{{noResultsAvailable_(historyDataByDay_.length, loading_)}}">
On 2016/02/09 04:28:53, tsergeant wrote:
> As discussed, try to rename these functions so that there's no confusion
between
> resultsAvailable_ and noResultsAvailable_

Now that we have figured out how to get loadTimeData to be ready earlier, we can
now make this one div and set the displayed message using a function as the
previous comments suggested.

https://codereview.chromium.org/1643693003/diff/160001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_card_manager.html:71: <history-card
history-title="{{historyDay.cardTitle}}"
On 2016/02/09 04:28:53, tsergeant wrote:
> As discussed, revert this back to passing down the date instead of the title

Done.

https://codereview.chromium.org/1643693003/diff/160001/chrome/browser/resourc...
File chrome/browser/resources/md_history/history_card_manager.js (right):

https://codereview.chromium.org/1643693003/diff/160001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_card_manager.js:40: type: Boolean,
On 2016/02/09 04:28:53, tsergeant wrote:
> Should loading_ be true to start with while initial results are loaded in?

Yes it should. Now when the page loads this is set to be true until the results
are properly set.

https://codereview.chromium.org/1643693003/diff/160001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_card_manager.js:133:
Polymer.dom.flush();
On 2016/02/09 04:28:53, tsergeant wrote:
> Why is this here?

Unnecessary now.

https://codereview.chromium.org/1643693003/diff/160001/chrome/test/data/webui...
File chrome/test/data/webui/md_history/history_toolbar_test.js (right):

https://codereview.chromium.org/1643693003/diff/160001/chrome/test/data/webui...
chrome/test/data/webui/md_history/history_toolbar_test.js:60: test('search
results display with correct item title', function(done) {
On 2016/02/09 04:28:53, tsergeant wrote:
> These next few tests should be moved to history_card_manager_test since
there's
> no mention of the toolbar in them.

Done.

[tsergeant, 2016-02-12 00:03:53]

You need to do a rebase as well

https://codereview.chromium.org/1643693003/diff/200001/chrome/browser/resourc...
File chrome/browser/resources/md_history/history_card.js (right):

https://codereview.chromium.org/1643693003/diff/200001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_card.js:59: if (search == '' ||
search == undefined) {
Can search actually ever be undefined?
Can you do
  if (search) {
  }

?

Also, remove these {} or add them back on the else clause as well

(if you have braces at all, you have to have them on every branch)

https://codereview.chromium.org/1643693003/diff/200001/chrome/browser/resourc...
File chrome/browser/resources/md_history/history_card_manager.js (right):

https://codereview.chromium.org/1643693003/diff/200001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_card_manager.js:41: value: false
Can you change to `value: true` instead of your change to history.js?

https://codereview.chromium.org/1643693003/diff/200001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_card_manager.js:111: if (currentDate
== undefined)
I'm not sure what the intention is here? Nothing in the HistoryEntry will ever
be undefined -- it will be something like the empty string instead.

https://codereview.chromium.org/1643693003/diff/200001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_card_manager.js:129: //
this.lastVisitedTime = historyItems[historyItems.length - 1].time;
Is this meant to be commented?

[hsampson, 2016-02-12 06:45:43]

https://codereview.chromium.org/1643693003/diff/200001/chrome/browser/resourc...
File chrome/browser/resources/md_history/history_card.js (right):

https://codereview.chromium.org/1643693003/diff/200001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_card.js:59: if (search == '' ||
search == undefined) {
On 2016/02/12 00:03:52, tsergeant wrote:
> Can search actually ever be undefined?
> Can you do
>   if (search) {
>   }
> 
> ?
> 
> Also, remove these {} or add them back on the else clause as well
> 
> (if you have braces at all, you have to have them on every branch)
> 

No I don't think it can.

https://codereview.chromium.org/1643693003/diff/200001/chrome/browser/resourc...
File chrome/browser/resources/md_history/history_card_manager.js (right):

https://codereview.chromium.org/1643693003/diff/200001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_card_manager.js:41: value: false
On 2016/02/12 00:03:52, tsergeant wrote:
> Can you change to `value: true` instead of your change to history.js?

Done.

https://codereview.chromium.org/1643693003/diff/200001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_card_manager.js:111: if (currentDate
== undefined)
On 2016/02/12 00:03:53, tsergeant wrote:
> I'm not sure what the intention is here? Nothing in the HistoryEntry will ever
> be undefined -- it will be something like the empty string instead.

I think this may just be leftover from a different architecture and doesn't do
anything anymore.

https://codereview.chromium.org/1643693003/diff/200001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_card_manager.js:129: //
this.lastVisitedTime = historyItems[historyItems.length - 1].time;
On 2016/02/12 00:03:52, tsergeant wrote:
> Is this meant to be commented?

Done.