MD History: Refactored design for displaying history information

MD History: Refactored design for displaying history information.

Iron list now iterates over a list of history items to improve performance and
loading for infinite scroll as each item is similarly sized. History cards have
been completely removed.

BUG=425625
Committed: https://crrev.com/404625f55ce32a752ba7ab0944e0bfec26bd2272
Cr-Commit-Position: refs/heads/master@{#375133}

[yingran, 2016-01-27 06:54:04]

Description was changed from

==========
BUG=
==========

to

==========
Md History: Changed design of history cards

Refactored code so that each history item is now a history card to improve
infinite scroll performance and loading.

BUG=425625
==========

[yingran, 2016-01-27 06:58:53]

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

[yingran, 2016-01-27 06:59:30]

Description was changed from

==========
Md History: Changed design of history cards

Refactored code so that each history item is now a history card to improve
infinite scroll performance and loading.

BUG=425625
==========

to

==========
MD History: Changed design of history cards

Refactored code so that each history item is now a history card to improve
infinite scroll performance and loading.

BUG=425625
==========

[tsergeant, 2016-01-27 22:22:52]

Not going to do a proper review until other stuff starts landing, but:

1) Overall, you're deleting code (+319/-355), so nice work
2) Can you rename the polymer elements to reflect what they do now? history-card
and history-card-manager are not particularly good names.

[yingran, 2016-01-29 05:04:37]

Patchset #2 (id:20001) has been deleted

[yingran, 2016-01-29 05:18:54]

Patchset #2 (id:40001) has been deleted

[yingran, 2016-02-02 02:41:51]

Patchset #4 (id:100001) has been deleted

[yingran, 2016-02-02 02:59:01]

Patchset #4 (id:120001) has been deleted

[tsergeant, 2016-02-02 05:58:00]

https://codereview.chromium.org/1641543002/diff/160001/chrome/browser/browser...
File chrome/browser/browser_resources.grd (right):

https://codereview.chromium.org/1641543002/diff/160001/chrome/browser/browser...
chrome/browser/browser_resources.grd:209: <include
name="IDR_MD_HISTORY_HISTORY_HTML" file="resources\md_history\history.html"
type="BINDATA" />
history.html is out of order, sort these again.

https://codereview.chromium.org/1641543002/diff/160001/chrome/browser/resourc...
File chrome/browser/resources/md_history/compiled_resources.gyp (right):

https://codereview.chromium.org/1641543002/diff/160001/chrome/browser/resourc...
chrome/browser/resources/md_history/compiled_resources.gyp:16:
'history_list.js',
Sort these again.

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

https://codereview.chromium.org/1641543002/diff/160001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_item.html:54: padding: 8px 0 0 0;
If you use padding-top here...

https://codereview.chromium.org/1641543002/diff/160001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_item.html:58: padding: 0 0 8px 0;
And padding-bottom here...

https://codereview.chromium.org/1641543002/diff/160001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_item.html:61:
:host([is-first-card][is-last-card]) #item-container {
Then can you get rid of this block?

https://codereview.chromium.org/1641543002/diff/160001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_item.html:135: <div
id="date-accessed">[[historyDate]]</div>
Consider using

<div id="date-accessed" hidden$="{{isFirstCard}}">

to hide this when it is not needed, rather than doing the showing/hiding from
CSS.

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

https://codereview.chromium.org/1641543002/diff/160001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_item.js:37: websiteUrl: {
A bunch of these went from private to public. I'm not sure if it was intentional
or a result of the merge, but...I think it's okay? I don't think they should
have been private in the first place.

https://codereview.chromium.org/1641543002/diff/160001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_item.js:62: isFirstCard: {
Rename to something like isCardStart. isFirstCard makes it sound like it's the
first thing on the page.

https://codereview.chromium.org/1641543002/diff/160001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_item.js:85: checkboxSelected:
function() {
This rename should change back

https://codereview.chromium.org/1641543002/diff/160001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_item.js:105: openMenu: function(e) {
And this one

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

https://codereview.chromium.org/1641543002/diff/160001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_list.js:26: historyData: {
Minor nit: Maybe move this to the top of properties? It's the most important
property in the element.

https://codereview.chromium.org/1641543002/diff/160001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_list.js:92: results[i -
1].isLastItem = true;
Just like what you changed in the C++ layer last week, you need to be careful to
set default values for isLastItem and isFirstItem so that they are not
`undefined`. Otherwise, observers in Polymer won't always fire.

[yingran, 2016-02-02 06:49:11]

https://codereview.chromium.org/1641543002/diff/160001/chrome/browser/browser...
File chrome/browser/browser_resources.grd (right):

https://codereview.chromium.org/1641543002/diff/160001/chrome/browser/browser...
chrome/browser/browser_resources.grd:209: <include
name="IDR_MD_HISTORY_HISTORY_HTML" file="resources\md_history\history.html"
type="BINDATA" />
On 2016/02/02 05:58:00, tsergeant wrote:
> history.html is out of order, sort these again.

Done.

https://codereview.chromium.org/1641543002/diff/160001/chrome/browser/resourc...
File chrome/browser/resources/md_history/compiled_resources.gyp (right):

https://codereview.chromium.org/1641543002/diff/160001/chrome/browser/resourc...
chrome/browser/resources/md_history/compiled_resources.gyp:16:
'history_list.js',
On 2016/02/02 05:58:00, tsergeant wrote:
> Sort these again.

Done.

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

https://codereview.chromium.org/1641543002/diff/160001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_item.html:54: padding: 8px 0 0 0;
On 2016/02/02 05:58:00, tsergeant wrote:
> If you use padding-top here...

Done.

https://codereview.chromium.org/1641543002/diff/160001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_item.html:58: padding: 0 0 8px 0;
On 2016/02/02 05:58:00, tsergeant wrote:
> And padding-bottom here...

Done.

https://codereview.chromium.org/1641543002/diff/160001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_item.html:61:
:host([is-first-card][is-last-card]) #item-container {
On 2016/02/02 05:58:00, tsergeant wrote:
> Then can you get rid of this block?

Done.

https://codereview.chromium.org/1641543002/diff/160001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_item.html:135: <div
id="date-accessed">[[historyDate]]</div>
On 2016/02/02 05:58:00, tsergeant wrote:
> Consider using
> 
> <div id="date-accessed" hidden$="{{isFirstCard}}">
> 
> to hide this when it is not needed, rather than doing the showing/hiding from
> CSS.

Acknowledged.

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

https://codereview.chromium.org/1641543002/diff/160001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_item.js:37: websiteUrl: {
On 2016/02/02 05:58:00, tsergeant wrote:
> A bunch of these went from private to public. I'm not sure if it was
intentional
> or a result of the merge, but...I think it's okay? I don't think they should
> have been private in the first place.

dbeam asked me to change them to private but yeah, i don't see why they have to
be private either...

https://codereview.chromium.org/1641543002/diff/160001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_item.js:62: isFirstCard: {
On 2016/02/02 05:58:00, tsergeant wrote:
> Rename to something like isCardStart. isFirstCard makes it sound like it's the
> first thing on the page.

Done.

https://codereview.chromium.org/1641543002/diff/160001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_item.js:85: checkboxSelected:
function() {
On 2016/02/02 05:58:00, tsergeant wrote:
> This rename should change back

Done.

https://codereview.chromium.org/1641543002/diff/160001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_item.js:105: openMenu: function(e) {
On 2016/02/02 05:58:00, tsergeant wrote:
> And this one

Done.

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

https://codereview.chromium.org/1641543002/diff/160001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_list.js:26: historyData: {
On 2016/02/02 05:58:00, tsergeant wrote:
> Minor nit: Maybe move this to the top of properties? It's the most important
> property in the element.

Done.

https://codereview.chromium.org/1641543002/diff/160001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_list.js:92: results[i -
1].isLastItem = true;
On 2016/02/02 05:58:00, tsergeant wrote:
> Just like what you changed in the C++ layer last week, you need to be careful
to
> set default values for isLastItem and isFirstItem so that they are not
> `undefined`. Otherwise, observers in Polymer won't always fire.

Done.

[tsergeant, 2016-02-02 23:47:33]

https://codereview.chromium.org/1641543002/diff/180001/chrome/browser/resourc...
File chrome/browser/resources/md_history/history_item.js (right):

https://codereview.chromium.org/1641543002/diff/180001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_item.js:68: isLastCard: {
Forgot to mention this explicitly, but you should change this too (isCardEnd)

https://codereview.chromium.org/1641543002/diff/180001/chrome/browser/resourc...
File chrome/browser/resources/md_history/history_list.html (right):

https://codereview.chromium.org/1641543002/diff/180001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_list.html:55:
is-card-start="{{historyDay.isFirstItem}}"
Can these be one-way bound?

https://codereview.chromium.org/1641543002/diff/180001/chrome/browser/resourc...
File chrome/browser/resources/md_history/history_list.js (right):

https://codereview.chromium.org/1641543002/diff/180001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_list.js:81: var scrollPosition =
this.$['infinite-list'].scrollTop;
The scroll position stuff had been removed previously. Do we need it to come
back?

https://codereview.chromium.org/1641543002/diff/180001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_list.js:98: results[i].isFirstItem =
results[i].isFirstItem || false;
This is a slightly confusing way to do this.

One way to improve it is to get rid of the else and have these lines always run
(maybe with a comment to explain that it's to get rid of `undefined`)

You could also try shuffling around the entire for loop to make things a bit
more intuitive. For example, try setting these unconditionally to false at the
start of the loop, and then move your `if length == 0` check to -after- the for
loop.

https://codereview.chromium.org/1641543002/diff/180001/chrome/test/data/webui...
File chrome/test/data/webui/md_history/history_card_manager_test.js (left):

https://codereview.chromium.org/1641543002/diff/180001/chrome/test/data/webui...
chrome/test/data/webui/md_history/history_card_manager_test.js:1: // Copyright
2016 The Chromium Authors. All rights reserved.
I wonder if we can convince git/rietveld that this file was moved.

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

https://codereview.chromium.org/1641543002/diff/180001/chrome/test/data/webui...
chrome/test/data/webui/md_history/history_list_test.js:103: flush(function() {
Any test which uses flush() needs to use the `done` parameter.

Otherwise, the callback will be placed on the queue, execution will come back to
the main test, and it will count as passed. If you use `done`, then it won't
pass until `done()` is called.

[tsergeant, 2016-02-02 23:54:16]

Also, please update your CL description to use the new names and be a little
more detailed. Feel free to ask if you want more concrete advice about how to
change it.

[yingran, 2016-02-04 02:13:42]

https://codereview.chromium.org/1641543002/diff/180001/chrome/browser/resourc...
File chrome/browser/resources/md_history/history_item.js (right):

https://codereview.chromium.org/1641543002/diff/180001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_item.js:68: isLastCard: {
On 2016/02/02 23:47:33, tsergeant wrote:
> Forgot to mention this explicitly, but you should change this too (isCardEnd)

Done.

https://codereview.chromium.org/1641543002/diff/180001/chrome/browser/resourc...
File chrome/browser/resources/md_history/history_list.html (right):

https://codereview.chromium.org/1641543002/diff/180001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_list.html:55:
is-card-start="{{historyDay.isFirstItem}}"
On 2016/02/02 23:47:33, tsergeant wrote:
> Can these be one-way bound?

Done.

https://codereview.chromium.org/1641543002/diff/180001/chrome/browser/resourc...
File chrome/browser/resources/md_history/history_list.js (right):

https://codereview.chromium.org/1641543002/diff/180001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_list.js:81: var scrollPosition =
this.$['infinite-list'].scrollTop;
On 2016/02/02 23:47:33, tsergeant wrote:
> The scroll position stuff had been removed previously. Do we need it to come
> back?

yep, otherwise, it just goes back all the way to the top

https://codereview.chromium.org/1641543002/diff/180001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_list.js:98: results[i].isFirstItem =
results[i].isFirstItem || false;
On 2016/02/02 23:47:33, tsergeant wrote:
> This is a slightly confusing way to do this.
> 
> One way to improve it is to get rid of the else and have these lines always
run
> (maybe with a comment to explain that it's to get rid of `undefined`)
> 
> You could also try shuffling around the entire for loop to make things a bit
> more intuitive. For example, try setting these unconditionally to false at the
> start of the loop, and then move your `if length == 0` check to -after- the
for
> loop.

Not sure about the second point, but tried the first one

https://codereview.chromium.org/1641543002/diff/180001/chrome/test/data/webui...
File chrome/test/data/webui/md_history/history_card_manager_test.js (left):

https://codereview.chromium.org/1641543002/diff/180001/chrome/test/data/webui...
chrome/test/data/webui/md_history/history_card_manager_test.js:1: // Copyright
2016 The Chromium Authors. All rights reserved.
On 2016/02/02 23:47:33, tsergeant wrote:
> I wonder if we can convince git/rietveld that this file was moved.

we can try....

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

https://codereview.chromium.org/1641543002/diff/180001/chrome/test/data/webui...
chrome/test/data/webui/md_history/history_list_test.js:103: flush(function() {
On 2016/02/02 23:47:33, tsergeant wrote:
> Any test which uses flush() needs to use the `done` parameter.
> 
> Otherwise, the callback will be placed on the queue, execution will come back
to
> the main test, and it will count as passed. If you use `done`, then it won't
> pass until `done()` is called.

Done.

[tsergeant, 2016-02-04 03:11:51]

Generally looking good. Make sure this is rebased, as well, since some stuff has
changed.

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

https://codereview.chromium.org/1641543002/diff/240001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_list.js:104: this.set('historyData',
this.historyData.concat(results));
Can you do this.push to avoid resetting the scroll position?

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

https://codereview.chromium.org/1641543002/diff/240001/chrome/test/data/webui...
chrome/test/data/webui/md_history/history_item_test.js:31:
.querySelectorAll('history-item');
          var items =
              Polymer.dom(element.root).querySelectorAll('history-item');

https://codereview.chromium.org/1641543002/diff/240001/chrome/test/data/webui...
chrome/test/data/webui/md_history/history_item_test.js:34:
assertFalse(items[2].hasTimeGap);
This test needs to use done() as well.

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

https://codereview.chromium.org/1641543002/diff/240001/chrome/test/data/webui...
chrome/test/data/webui/md_history/history_list_test.js:118: element.historyData=
[];
Nit: Space before equals

[yingran, 2016-02-05 00:17:32]

Patchset #9 (id:240001) has been deleted

[yingran, 2016-02-05 00:20:22]

Patchset #10 (id:280001) has been deleted

[yingran, 2016-02-05 04:55:27]

yingran@google.com changed reviewers:
+ dbeam@chromium.org

[yingran, 2016-02-05 04:55:27]

+dbeam for browser_resources.grd OWNERS.

PTAL thanks :)

[Dan Beam, 2016-02-06 03:33:45]

lgtm

[tsergeant, 2016-02-08 00:10:54]

Mostly looks good. A couple of extra things:

1. Can you try adding borders to the history-items in place of shadows, as we
discussed?

2. Please update the CL description. It would be better if you mention how
history-item is now iterated over for iron-list, and how history-card has been
removed.

https://codereview.chromium.org/1641543002/diff/320001/chrome/browser/resourc...
File chrome/browser/resources/md_history/history_list.js (right):

https://codereview.chromium.org/1641543002/diff/320001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_list.js:73: * for each result.
Preserves the scroll position of the window when new data
You can get rid of the comment about scroll position if we're sticking with push

https://codereview.chromium.org/1641543002/diff/320001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_list.js:99: if
(this.historyData.length == 0) {
Nit: No {}

https://codereview.chromium.org/1641543002/diff/320001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_list.js:103: for (var i = 0; i <
results.length; i++) {
We got this working with apply and no for loop on Friday afternoon, right?

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

https://codereview.chromium.org/1641543002/diff/320001/chrome/test/data/webui...
chrome/test/data/webui/md_history/history_list_test.js:75: 
Nit: Remove the bonus newline

https://codereview.chromium.org/1641543002/diff/320001/chrome/test/data/webui...
chrome/test/data/webui/md_history/history_list_test.js:118: element.historyData=
[];
Nit: Space before equals

[tsergeant, 2016-02-08 02:23:59]

https://codereview.chromium.org/1641543002/diff/320001/chrome/browser/resourc...
File chrome/browser/resources/md_history/history_item.html (right):

https://codereview.chromium.org/1641543002/diff/320001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_item.html:147: <a
href="{{websiteUrl}}" id="title">{{websiteTitle}}</a>
While you're here,

when you use polymer to bind to a native attribute, you're supposed to use $=

So this should be <a href$="{{websiteUrl}}" ...>

[yingran, 2016-02-08 02:55:08]

Description was changed from

==========
MD History: Changed design of history cards

Refactored code so that each history item is now a history card to improve
infinite scroll performance and loading.

BUG=425625
==========

to

==========
MD History: Refactored design for displaying history information.

Iron list now iterates over a list of history items to improve performance and
loading for infinite scroll as each item is similarly sized. History cards have
been completely removed. 

BUG=425625
==========

[yingran, 2016-02-08 03:20:36]

https://codereview.chromium.org/1641543002/diff/320001/chrome/browser/resourc...
File chrome/browser/resources/md_history/history_item.html (right):

https://codereview.chromium.org/1641543002/diff/320001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_item.html:147: <a
href="{{websiteUrl}}" id="title">{{websiteTitle}}</a>
On 2016/02/08 02:23:59, tsergeant wrote:
> While you're here,
> 
> when you use polymer to bind to a native attribute, you're supposed to use $=
> 
> So this should be <a href$="{{websiteUrl}}" ...>

Done.

https://codereview.chromium.org/1641543002/diff/320001/chrome/browser/resourc...
File chrome/browser/resources/md_history/history_list.js (right):

https://codereview.chromium.org/1641543002/diff/320001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_list.js:73: * for each result.
Preserves the scroll position of the window when new data
On 2016/02/08 00:10:54, tsergeant wrote:
> You can get rid of the comment about scroll position if we're sticking with
push

Done.

https://codereview.chromium.org/1641543002/diff/320001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_list.js:99: if
(this.historyData.length == 0) {
On 2016/02/08 00:10:54, tsergeant wrote:
> Nit: No {}

Done.

https://codereview.chromium.org/1641543002/diff/320001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_list.js:103: for (var i = 0; i <
results.length; i++) {
On 2016/02/08 00:10:54, tsergeant wrote:
> We got this working with apply and no for loop on Friday afternoon, right?

Yes- this'll be uploaded next patch

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

https://codereview.chromium.org/1641543002/diff/320001/chrome/test/data/webui...
chrome/test/data/webui/md_history/history_list_test.js:75: 
On 2016/02/08 00:10:54, tsergeant wrote:
> Nit: Remove the bonus newline

Done.

https://codereview.chromium.org/1641543002/diff/320001/chrome/test/data/webui...
chrome/test/data/webui/md_history/history_list_test.js:118: element.historyData=
[];
On 2016/02/08 00:10:54, tsergeant wrote:
> Nit: Space before equals

Done.

[tsergeant, 2016-02-08 03:44:51]

Two more things:

1) Please rebase from master. I wasn't able to apply your patch locally.

2) Wrap your CL description to 80 characters wide (It's annoying because of how
Rietveld works. I can show you how to do it in vim if you like)

https://codereview.chromium.org/1641543002/diff/340001/chrome/browser/resourc...
File chrome/browser/resources/md_history/history_item.html (right):

https://codereview.chromium.org/1641543002/diff/340001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_item.html:22: border-left: 1px solid
rgba(0, 0, 0, 0.14);
Split this apart to reduce duplication:

border-color: rgba(...)
border-style: solid
border-width: 0 1px

Then below you can just do

border-top-width: 1px;

and

border-bottom-width: 2px;

https://codereview.chromium.org/1641543002/diff/340001/chrome/browser/resourc...
File chrome/browser/resources/md_history/history_list.js (right):

https://codereview.chromium.org/1641543002/diff/340001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_list.js:101:
results.unshift('historyData');
Would be good to have a comment here explaining what is going on.

https://codereview.chromium.org/1641543002/diff/340001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_list.js:102: this.push.apply(this,
results);
Should use `Polymer.Base.push.apply`

(Right, Chris?)

[Dan Beam, 2016-02-08 23:50:47]

https://codereview.chromium.org/1641543002/diff/340001/chrome/browser/resourc...
File chrome/browser/resources/md_history/history_list.js (right):

https://codereview.chromium.org/1641543002/diff/340001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_list.js:102: this.push.apply(this,
results);
On 2016/02/08 03:44:51, tsergeant wrote:
> Should use `Polymer.Base.push.apply`
> 
> (Right, Chris?)

no, that's only if you're @override-ing push() yourself

[calamity, 2016-02-09 12:25:29]

https://codereview.chromium.org/1641543002/diff/340001/chrome/browser/resourc...
File chrome/browser/resources/md_history/history_item.html (right):

https://codereview.chromium.org/1641543002/diff/340001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_item.html:144: 
nit: stray newline.

https://codereview.chromium.org/1641543002/diff/340001/chrome/browser/resourc...
File chrome/browser/resources/md_history/history_item.js (right):

https://codereview.chromium.org/1641543002/diff/340001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_item.js:15: timeAccessed: {
I recommend you change this to cardTitle in anticipation of hsampson's change. I
believe she's going to using this field for the search result card title.

https://codereview.chromium.org/1641543002/diff/340001/chrome/browser/resourc...
File chrome/browser/resources/md_history/history_list.html (right):

https://codereview.chromium.org/1641543002/diff/340001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_list.html:28: 
This block seems to have moved. May as well leave it where it was.

https://codereview.chromium.org/1641543002/diff/340001/chrome/browser/resourc...
File chrome/browser/resources/md_history/history_list.js (right):

https://codereview.chromium.org/1641543002/diff/340001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_list.js:101:
results.unshift('historyData');
It's safer to slice the results at the beginning in case someone calls this
without it expecting to modify the input. The main saving here is the dom stuff
which is orders of magnitude more inefficient anyway.

Also, this caused a test to fail.

https://codereview.chromium.org/1641543002/diff/340001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_list.js:146: * @param {number} index
The index number of the first item being compared.
Missing a param.

https://codereview.chromium.org/1641543002/diff/340001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_list.js:151: var items = results;
Can we just remove this..?

Actually, on second thought, make a var currentItem and nextItem.

Move the first condition onto the first line. 

if (index + 1 < ...)
  return false;

[yingran, 2016-02-11 00:41:43]

https://codereview.chromium.org/1641543002/diff/340001/chrome/browser/resourc...
File chrome/browser/resources/md_history/history_item.html (right):

https://codereview.chromium.org/1641543002/diff/340001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_item.html:22: border-left: 1px solid
rgba(0, 0, 0, 0.14);
On 2016/02/08 03:44:51, tsergeant wrote:
> Split this apart to reduce duplication:
> 
> border-color: rgba(...)
> border-style: solid
> border-width: 0 1px
> 
> Then below you can just do
> 
> border-top-width: 1px;
> 
> and
> 
> border-bottom-width: 2px;

Done.

https://codereview.chromium.org/1641543002/diff/340001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_item.html:144: 
On 2016/02/09 12:25:29, calamity wrote:
> nit: stray newline.

Done.

https://codereview.chromium.org/1641543002/diff/340001/chrome/browser/resourc...
File chrome/browser/resources/md_history/history_item.js (right):

https://codereview.chromium.org/1641543002/diff/340001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_item.js:15: timeAccessed: {
On 2016/02/09 12:25:29, calamity wrote:
> I recommend you change this to cardTitle in anticipation of hsampson's change.
I
> believe she's going to using this field for the search result card title.

??? do u mean historyDate -> cardTitle? timeAccessed is still in her search
thing...

https://codereview.chromium.org/1641543002/diff/340001/chrome/browser/resourc...
File chrome/browser/resources/md_history/history_list.html (right):

https://codereview.chromium.org/1641543002/diff/340001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_list.html:28: 
On 2016/02/09 12:25:29, calamity wrote:
> This block seems to have moved. May as well leave it where it was.

am confused

https://codereview.chromium.org/1641543002/diff/340001/chrome/browser/resourc...
File chrome/browser/resources/md_history/history_list.js (right):

https://codereview.chromium.org/1641543002/diff/340001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_list.js:101:
results.unshift('historyData');
On 2016/02/09 12:25:29, calamity wrote:
> It's safer to slice the results at the beginning in case someone calls this
> without it expecting to modify the input. The main saving here is the dom
stuff
> which is orders of magnitude more inefficient anyway.
> 
> Also, this caused a test to fail.

Done.

https://codereview.chromium.org/1641543002/diff/340001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_list.js:101:
results.unshift('historyData');
On 2016/02/08 03:44:51, tsergeant wrote:
> Would be good to have a comment here explaining what is going on.

Done.

https://codereview.chromium.org/1641543002/diff/340001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_list.js:146: * @param {number} index
The index number of the first item being compared.
On 2016/02/09 12:25:29, calamity wrote:
> Missing a param.

Done.

https://codereview.chromium.org/1641543002/diff/340001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_list.js:151: var items = results;
On 2016/02/09 12:25:29, calamity wrote:
> Can we just remove this..?
> 
> Actually, on second thought, make a var currentItem and nextItem.
> 
> Move the first condition onto the first line. 
> 
> if (index + 1 < ...)
>   return false;

Done.

[tsergeant, 2016-02-11 02:43:02]

Comments are mostly on patchset 13

https://codereview.chromium.org/1641543002/diff/340001/chrome/browser/resourc...
File chrome/browser/resources/md_history/history_item.js (right):

https://codereview.chromium.org/1641543002/diff/340001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_item.js:15: timeAccessed: {
On 2016/02/11 00:41:42, yingran wrote:
> On 2016/02/09 12:25:29, calamity wrote:
> > I recommend you change this to cardTitle in anticipation of hsampson's
change.
> I
> > believe she's going to using this field for the search result card title.
> 
> ??? do u mean historyDate -> cardTitle? timeAccessed is still in her search
> thing...

I think that this will no longer need to change, yeah. Hannah and I went back on
that and historyDate should still exist.

https://codereview.chromium.org/1641543002/diff/340001/chrome/browser/resourc...
File chrome/browser/resources/md_history/history_list.html (right):

https://codereview.chromium.org/1641543002/diff/340001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_list.html:28: 
On 2016/02/11 00:41:42, yingran wrote:
> On 2016/02/09 12:25:29, calamity wrote:
> > This block seems to have moved. May as well leave it where it was.
> 
> am confused

My $0.02: Leave this block where it is now. It modifies the behavior of the
#overflow-menu block, so it makes more sense for it to be after it rather than
before it.

https://codereview.chromium.org/1641543002/diff/360001/chrome/browser/resourc...
File chrome/browser/resources/md_history/history_item.js (right):

https://codereview.chromium.org/1641543002/diff/360001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_item.js:119: // return false;
Remove this.

https://codereview.chromium.org/1641543002/diff/360001/chrome/browser/resourc...
File chrome/browser/resources/md_history/history_list.js (right):

https://codereview.chromium.org/1641543002/diff/360001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_list.js:80: // Creates a copy of
historyResults to prevent accidentally modifying the
Nit: "modifying the this"

https://codereview.chromium.org/1641543002/diff/360001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_list.js:138: // Resets the first
history item.
This is a bit gnarly. It feels very manual and error-prone, and Polymer is meant
to help us with that.

Have you thought about using computed properties to help with this? Something
like

<history-item ...
    is-first-card="[[isFirstCard_(index, item, historyData.length)]]"

I think you should be able to make it so that it will recompute at the right
time and get everything right with less effort.

(I don't want to give you a comment that's going to take half a day to fix when
you have a day left. If you don't get it working fairly quickly, let me know and
we can TODO it instead).

https://codereview.chromium.org/1641543002/diff/360001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_list.js:153: if
(this.historyData[i].needsTimeGap && i > 0) {
Nit: Remove {}

https://codereview.chromium.org/1641543002/diff/360001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_list.js:161: if (overallItemCount ==
0) {
{} here too

https://codereview.chromium.org/1641543002/diff/360001/chrome/test/data/webui...
File chrome/test/data/webui/md_history/history_list_test.js (left):

https://codereview.chromium.org/1641543002/diff/360001/chrome/test/data/webui...
chrome/test/data/webui/md_history/history_list_test.js:138: test('removeVisits
for multiple items', function(done) {
What happened to this test?

https://codereview.chromium.org/1641543002/diff/360001/chrome/test/data/webui...
chrome/test/data/webui/md_history/history_list_test.js:167: test('deleting
multiple items from view', function(done) {
And this one?

https://codereview.chromium.org/1641543002/diff/380001/chrome/browser/resourc...
File chrome/browser/resources/md_history/history_list.js (right):

https://codereview.chromium.org/1641543002/diff/380001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_list.js:108: results[i -
1].isLastItem = true;
\o/ yay!

[yingran, 2016-02-12 02:03:05]

https://codereview.chromium.org/1641543002/diff/340001/chrome/browser/resourc...
File chrome/browser/resources/md_history/history_item.js (right):

https://codereview.chromium.org/1641543002/diff/340001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_item.js:15: timeAccessed: {
On 2016/02/11 02:43:02, tsergeant wrote:
> On 2016/02/11 00:41:42, yingran wrote:
> > On 2016/02/09 12:25:29, calamity wrote:
> > > I recommend you change this to cardTitle in anticipation of hsampson's
> change.
> > I
> > > believe she's going to using this field for the search result card title.
> > 
> > ??? do u mean historyDate -> cardTitle? timeAccessed is still in her search
> > thing...
> 
> I think that this will no longer need to change, yeah. Hannah and I went back
on
> that and historyDate should still exist.

coolies

https://codereview.chromium.org/1641543002/diff/340001/chrome/browser/resourc...
File chrome/browser/resources/md_history/history_list.html (right):

https://codereview.chromium.org/1641543002/diff/340001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_list.html:28: 
On 2016/02/11 02:43:02, tsergeant wrote:
> On 2016/02/11 00:41:42, yingran wrote:
> > On 2016/02/09 12:25:29, calamity wrote:
> > > This block seems to have moved. May as well leave it where it was.
> > 
> > am confused
> 
> My $0.02: Leave this block where it is now. It modifies the behavior of the
> #overflow-menu block, so it makes more sense for it to be after it rather than
> before it.

Done.

https://codereview.chromium.org/1641543002/diff/360001/chrome/browser/resourc...
File chrome/browser/resources/md_history/history_item.js (right):

https://codereview.chromium.org/1641543002/diff/360001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_item.js:119: // return false;
On 2016/02/11 02:43:02, tsergeant wrote:
> Remove this.

Done.

https://codereview.chromium.org/1641543002/diff/360001/chrome/browser/resourc...
File chrome/browser/resources/md_history/history_list.js (right):

https://codereview.chromium.org/1641543002/diff/360001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_list.js:80: // Creates a copy of
historyResults to prevent accidentally modifying the
On 2016/02/11 02:43:02, tsergeant wrote:
> Nit: "modifying the this"

Done.

https://codereview.chromium.org/1641543002/diff/360001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_list.js:138: // Resets the first
history item.
On 2016/02/11 02:43:02, tsergeant wrote:
> This is a bit gnarly. It feels very manual and error-prone, and Polymer is
meant
> to help us with that.
> 
> Have you thought about using computed properties to help with this? Something
> like
> 
> <history-item ...
>     is-first-card="[[isFirstCard_(index, item, historyData.length)]]"
> 
> I think you should be able to make it so that it will recompute at the right
> time and get everything right with less effort.
> 
> (I don't want to give you a comment that's going to take half a day to fix
when
> you have a day left. If you don't get it working fairly quickly, let me know
and
> we can TODO it instead).

Acknowledged.

https://codereview.chromium.org/1641543002/diff/360001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_list.js:153: if
(this.historyData[i].needsTimeGap && i > 0) {
On 2016/02/11 02:43:02, tsergeant wrote:
> Nit: Remove {}

Done.

https://codereview.chromium.org/1641543002/diff/360001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_list.js:161: if (overallItemCount ==
0) {
On 2016/02/11 02:43:02, tsergeant wrote:
> {} here too

Done.

https://codereview.chromium.org/1641543002/diff/360001/chrome/test/data/webui...
File chrome/test/data/webui/md_history/history_list_test.js (left):

https://codereview.chromium.org/1641543002/diff/360001/chrome/test/data/webui...
chrome/test/data/webui/md_history/history_list_test.js:138: test('removeVisits
for multiple items', function(done) {
On 2016/02/11 02:43:02, tsergeant wrote:
> What happened to this test?

Done.

https://codereview.chromium.org/1641543002/diff/360001/chrome/test/data/webui...
chrome/test/data/webui/md_history/history_list_test.js:167: test('deleting
multiple items from view', function(done) {
On 2016/02/11 02:43:02, tsergeant wrote:
> And this one?

Done.

https://codereview.chromium.org/1641543002/diff/380001/chrome/browser/resourc...
File chrome/browser/resources/md_history/history_list.js (right):

https://codereview.chromium.org/1641543002/diff/380001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_list.js:108: results[i -
1].isLastItem = true;
On 2016/02/11 02:43:02, tsergeant wrote:
> \o/ yay!

Acknowledged.

[yingran, 2016-02-12 02:18:00]

Description was changed from

==========
MD History: Refactored design for displaying history information.

Iron list now iterates over a list of history items to improve performance and
loading for infinite scroll as each item is similarly sized. History cards have
been completely removed. 

BUG=425625
==========

to

==========
MD History: Refactored design for displaying history information.

Iron list now iterates over a list of history items to improve performance and
loading for infinite scroll as each item is similarly sized. History cards have
been completely removed.

BUG=425625
==========

[calamity, 2016-02-12 02:50:23]

https://codereview.chromium.org/1641543002/diff/400001/chrome/browser/resourc...
File chrome/browser/resources/md_history/history_list.js (right):

https://codereview.chromium.org/1641543002/diff/400001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_list.js:15: // The time in seconds
of the last history item in historyData.
// The time of access of the last history item in historyData.

https://codereview.chromium.org/1641543002/diff/400001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_list.js:91: this.set('historyData.'
+ parseInt(this.historyData.length - 1) +
Remove parseInt

https://codereview.chromium.org/1641543002/diff/400001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_list.js:106: results[i].needsTimeGap
= this.needsTimeGap_(results, i);
Move this above the if block. It's weird for it to be down here.

https://codereview.chromium.org/1641543002/diff/400001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_list.js:157: this.set('historyData.'
+ parseInt(i + 1) + '.isFirstItem', true);
I don't think these parseInts are necessary either. Just use parens around the
numbers and it'll be fine.

https://codereview.chromium.org/1641543002/diff/400001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_list.js:170: 
Missed the deletion after case for time gaps here.

https://codereview.chromium.org/1641543002/diff/400001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_list.js:200: }
Put return toBeRemoved back here and change the other return to a break;

Seeing possible no-return paths through the code gives me night terrors.

[tsergeant, 2016-02-12 03:05:52]

https://codereview.chromium.org/1641543002/diff/400001/chrome/test/data/webui...
File chrome/test/data/webui/md_history/history_item_test.js (right):

https://codereview.chromium.org/1641543002/diff/400001/chrome/test/data/webui...
chrome/test/data/webui/md_history/history_item_test.js:42: test('separator
insertion after deletion', function(done) {
I think this test probably belongs in history_list_test, since you're testing
code that is in history-list.

Also, add a test case for the case that wasn't working.

[yingran, 2016-02-12 03:26:56]

Patchset #17 (id:440001) has been deleted

[yingran, 2016-02-12 03:30:32]

https://codereview.chromium.org/1641543002/diff/400001/chrome/browser/resourc...
File chrome/browser/resources/md_history/history_list.js (right):

https://codereview.chromium.org/1641543002/diff/400001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_list.js:15: // The time in seconds
of the last history item in historyData.
On 2016/02/12 02:50:22, calamity wrote:
> // The time of access of the last history item in historyData.

Done.

https://codereview.chromium.org/1641543002/diff/400001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_list.js:91: this.set('historyData.'
+ parseInt(this.historyData.length - 1) +
On 2016/02/12 02:50:22, calamity wrote:
> Remove parseInt

Done.

https://codereview.chromium.org/1641543002/diff/400001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_list.js:106: results[i].needsTimeGap
= this.needsTimeGap_(results, i);
On 2016/02/12 02:50:22, calamity wrote:
> Move this above the if block. It's weird for it to be down here.

Done.

https://codereview.chromium.org/1641543002/diff/400001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_list.js:157: this.set('historyData.'
+ parseInt(i + 1) + '.isFirstItem', true);
On 2016/02/12 02:50:23, calamity wrote:
> I don't think these parseInts are necessary either. Just use parens around the
> numbers and it'll be fine.

Done.

https://codereview.chromium.org/1641543002/diff/400001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_list.js:170: 
On 2016/02/12 02:50:23, calamity wrote:
> Missed the deletion after case for time gaps here.

Done.

https://codereview.chromium.org/1641543002/diff/400001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_list.js:200: }
On 2016/02/12 02:50:23, calamity wrote:
> Put return toBeRemoved back here and change the other return to a break;
> 
> Seeing possible no-return paths through the code gives me night terrors.

Done.

https://codereview.chromium.org/1641543002/diff/400001/chrome/test/data/webui...
File chrome/test/data/webui/md_history/history_item_test.js (right):

https://codereview.chromium.org/1641543002/diff/400001/chrome/test/data/webui...
chrome/test/data/webui/md_history/history_item_test.js:42: test('separator
insertion after deletion', function(done) {
On 2016/02/12 03:05:52, tsergeant wrote:
> I think this test probably belongs in history_list_test, since you're testing
> code that is in history-list.
> 
> Also, add a test case for the case that wasn't working.

Done.

Uhhh... i'm gunna leave it where it is for now since the separators are based on
this set of TEST_HISTORY_RESULTS, but the tests are added

[calamity, 2016-02-12 04:11:10]

lgtm

[tsergeant, 2016-02-12 04:14:30]

Very close

https://codereview.chromium.org/1641543002/diff/460001/chrome/browser/resourc...
File chrome/browser/resources/md_history/history_list.js (right):

https://codereview.chromium.org/1641543002/diff/460001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_list.js:166: if (this.historyData[i
- 1].needsTimeGap) {
Nit: {}

https://codereview.chromium.org/1641543002/diff/460001/chrome/test/data/webui...
File chrome/test/data/webui/md_history/history_item_test.js (right):

https://codereview.chromium.org/1641543002/diff/460001/chrome/test/data/webui...
chrome/test/data/webui/md_history/history_item_test.js:56:
element.set('historyData.3.selected', true);
Nit: A comment would be nice here

[tsergeant, 2016-02-12 04:22:20]

lgtm ヾ(⌐■_■)ノ♪

Triple check the closure compile before you commit -- you don't want this to get
reverted because of something going wrong there.

[yingran, 2016-02-12 04:37:43]

https://codereview.chromium.org/1641543002/diff/460001/chrome/browser/resourc...
File chrome/browser/resources/md_history/history_list.js (right):

https://codereview.chromium.org/1641543002/diff/460001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_list.js:166: if (this.historyData[i
- 1].needsTimeGap) {
On 2016/02/12 04:14:30, tsergeant wrote:
> Nit: {}

Done.

https://codereview.chromium.org/1641543002/diff/460001/chrome/test/data/webui...
File chrome/test/data/webui/md_history/history_item_test.js (right):

https://codereview.chromium.org/1641543002/diff/460001/chrome/test/data/webui...
chrome/test/data/webui/md_history/history_item_test.js:56:
element.set('historyData.3.selected', true);
On 2016/02/12 04:14:30, tsergeant wrote:
> Nit: A comment would be nice here

Done.

[yingran, 2016-02-12 04:41:54]

The CQ bit was checked by yingran@google.com

[yingran, 2016-02-12 04:41:54]

The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org,
tsergeant@chromium.org, calamity@chromium.org
Link to the patchset: https://codereview.chromium.org/1641543002/#ps500001
(title: "closure")

[commit-bot: I haz the power, 2016-02-12 04:43:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1641543002/500001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1641543002/500001

[commit-bot: I haz the power, 2016-02-12 05:54:43]

Committed patchset #19 (id:500001)

[commit-bot: I haz the power, 2016-02-16 22:41:57]

Description was changed from

==========
MD History: Refactored design for displaying history information.

Iron list now iterates over a list of history items to improve performance and
loading for infinite scroll as each item is similarly sized. History cards have
been completely removed.

BUG=425625
==========

to

==========
MD History: Refactored design for displaying history information.

Iron list now iterates over a list of history items to improve performance and
loading for infinite scroll as each item is similarly sized. History cards have
been completely removed.

BUG=425625

Committed: https://crrev.com/404625f55ce32a752ba7ab0944e0bfec26bd2272
Cr-Commit-Position: refs/heads/master@{#375133}
==========

[commit-bot: I haz the power, 2016-02-16 22:41:58]

Patchset 19 (id:??) landed as
https://crrev.com/404625f55ce32a752ba7ab0944e0bfec26bd2272
Cr-Commit-Position: refs/heads/master@{#375133}