MD History: Use computed functions for card borders and time gaps

MD History: Use computed functions for card borders and time gaps

This replaces values which were updated manually any time the history
data changed with values which are automatically computed by Polymer.
This simplifies the code and reduces the chance of errors. Due to the
limitations of computed functions in Polymer, these functions have much
wider dependencies than necessary, however, this does not appear to have
a noticable impact on performance.

BUG=425625
Committed: https://crrev.com/0e7617f648602b15e8a2faf2a0c3118eee9d5f4e
Cr-Commit-Position: refs/heads/master@{#386883}

[tsergeant, 2016-04-11 23:23:19]

Description was changed from

==========
MD History: Use computed functions for card borders and time gaps

This replaces values which were updated manually any time the history
data changed with values which are automatically computed by Polymer.
This simplifies the code and reduces the chance of errors.

We also switch item deletion to use notifySplices, which is
significantly quicker when deleting several items at once.

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

to

==========
MD History: Use computed functions for card borders and time gaps

This replaces values which were updated manually any time the history
data changed with values which are automatically computed by Polymer.
This simplifies the code and reduces the chance of errors. Due to the
limitations of computed functions in Polymer, these functions have much
wider dependencies than necessary, however, this does not appear to have
a noticable impact on performance.

We also switch item deletion to use notifySplices, which is
significantly quicker when deleting several items at once.

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

[tsergeant, 2016-04-11 23:23:41]

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

[tsergeant, 2016-04-11 23:23:41]

dbeam@, please take a look!

[Dan Beam, 2016-04-12 01:28:16]

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

https://codereview.chromium.org/1880783002/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_history/history_list.js:159: var spliceinfo = [];
nit: splices

https://codereview.chromium.org/1880783002/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_history/history_list.js:179:
this.notifySplices('historyData', spliceinfo);
why are you using notifySplices?
https://www.polymer-project.org/1.0/docs/devguide/properties.html#notifysplices

https://codereview.chromium.org/1880783002/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_history/history_list.js:232: * @param {number}
length The length of |historyData|
nit: periods after [semi-complete] sentences

https://codereview.chromium.org/1880783002/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_history/history_list.js:237: if (index >= length - 1
|| length == 0)
when should index be longer than the length of the list?

https://codereview.chromium.org/1880783002/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_history/history_list.js:260: 
nit: jsdoc for both of these methods?

[tsergeant, 2016-04-12 05:40:27]

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

https://codereview.chromium.org/1880783002/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_history/history_list.js:159: var spliceinfo = [];
On 2016/04/12 01:28:16, Dan Beam wrote:
> nit: splices

Done.

https://codereview.chromium.org/1880783002/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_history/history_list.js:179:
this.notifySplices('historyData', spliceinfo);
On 2016/04/12 01:28:16, Dan Beam wrote:
> why are you using notifySplices?
>
https://www.polymer-project.org/1.0/docs/devguide/properties.html#notifysplices

The old way we were doing this, with `this.splice('historyData', ...)`, resulted
in a bunch of individual calls to notifyPath, each of which did a lot of work to
shift the array around before another item got deleted and the array changed
again.

notifySplices batches all of the notifications into one update, so there's only
one notifyPath and one update to the array. The time spent in
removeDeletedHistory goes from 160ms to 15ms when deleting 20 items as a result
of this change.

Anyway, there's no extern definition for notifySplices at the moment, so I'll
pull this change out and deal with it in a separate CL.

https://codereview.chromium.org/1880783002/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_history/history_list.js:232: * @param {number}
length The length of |historyData|
On 2016/04/12 01:28:16, Dan Beam wrote:
> nit: periods after [semi-complete] sentences

Done.

https://codereview.chromium.org/1880783002/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_history/history_list.js:237: if (index >= length - 1
|| length == 0)
On 2016/04/12 01:28:16, Dan Beam wrote:
> when should index be longer than the length of the list?

Strangely, the iron-list will still compute these functions for physical items
that have no corresponding virtual item.

So, if there are 10 physical <history-items>s stamped, but only one item in
historyData, needsTimeGap_ will be called for each of the 9 extra physical
items.

It's weird behavior, but I'm not sure whether it's actually a bug in Polymer or
iron-list or intended behavior.

https://codereview.chromium.org/1880783002/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_history/history_list.js:260: 
On 2016/04/12 01:28:16, Dan Beam wrote:
> nit: jsdoc for both of these methods?

Done.

[tsergeant, 2016-04-12 05:57:21]

Description was changed from

==========
MD History: Use computed functions for card borders and time gaps

This replaces values which were updated manually any time the history
data changed with values which are automatically computed by Polymer.
This simplifies the code and reduces the chance of errors. Due to the
limitations of computed functions in Polymer, these functions have much
wider dependencies than necessary, however, this does not appear to have
a noticable impact on performance.

We also switch item deletion to use notifySplices, which is
significantly quicker when deleting several items at once.

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

to

==========
MD History: Use computed functions for card borders and time gaps

This replaces values which were updated manually any time the history
data changed with values which are automatically computed by Polymer.
This simplifies the code and reduces the chance of errors. Due to the
limitations of computed functions in Polymer, these functions have much
wider dependencies than necessary, however, this does not appear to have
a noticable impact on performance.

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

[Dan Beam, 2016-04-12 19:08:29]

lgtm

https://codereview.chromium.org/1880783002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/md_history/history_list.js (left):

https://codereview.chromium.org/1880783002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_history/history_list.js:207: // Removes the selected
item from historyData.
mayyyyyyyyyybe keep this comment?

https://codereview.chromium.org/1880783002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/md_history/history_list.js (right):

https://codereview.chromium.org/1880783002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_history/history_list.js:253: * @param {number} i
d




ocument more for 1 character variables, I'd recommmend ;)

https://codereview.chromium.org/1880783002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_history/history_list.js:269: * @param {number} i
s



ame

[tsergeant, 2016-04-12 23:44:00]

https://codereview.chromium.org/1880783002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/md_history/history_list.js (left):

https://codereview.chromium.org/1880783002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_history/history_list.js:207: // Removes the selected
item from historyData.
On 2016/04/12 19:08:29, Dan Beam wrote:
> mayyyyyyyyyybe keep this comment?

Done.

https://codereview.chromium.org/1880783002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/md_history/history_list.js (right):

https://codereview.chromium.org/1880783002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_history/history_list.js:253: * @param {number} i
On 2016/04/12 19:08:29, Dan Beam wrote:
> d
> 
> 
> 
> 
> ocument more for 1 character variables, I'd recommmend ;)

Done.

https://codereview.chromium.org/1880783002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_history/history_list.js:269: * @param {number} i
On 2016/04/12 19:08:29, Dan Beam wrote:
> s
> 
> 
> 
> ame

Done.

[tsergeant, 2016-04-12 23:44:06]

The CQ bit was checked by tsergeant@chromium.org

[tsergeant, 2016-04-12 23:44:06]

The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/1880783002/#ps60001
(title: "Review comments")

[commit-bot: I haz the power, 2016-04-12 23:44:37]

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

[commit-bot: I haz the power, 2016-04-13 01:17:51]

Description was changed from

==========
MD History: Use computed functions for card borders and time gaps

This replaces values which were updated manually any time the history
data changed with values which are automatically computed by Polymer.
This simplifies the code and reduces the chance of errors. Due to the
limitations of computed functions in Polymer, these functions have much
wider dependencies than necessary, however, this does not appear to have
a noticable impact on performance.

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

to

==========
MD History: Use computed functions for card borders and time gaps

This replaces values which were updated manually any time the history
data changed with values which are automatically computed by Polymer.
This simplifies the code and reduces the chance of errors. Due to the
limitations of computed functions in Polymer, these functions have much
wider dependencies than necessary, however, this does not appear to have
a noticable impact on performance.

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

[commit-bot: I haz the power, 2016-04-13 01:17:52]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-04-13 01:19:17]

Description was changed from

==========
MD History: Use computed functions for card borders and time gaps

This replaces values which were updated manually any time the history
data changed with values which are automatically computed by Polymer.
This simplifies the code and reduces the chance of errors. Due to the
limitations of computed functions in Polymer, these functions have much
wider dependencies than necessary, however, this does not appear to have
a noticable impact on performance.

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

to

==========
MD History: Use computed functions for card borders and time gaps

This replaces values which were updated manually any time the history
data changed with values which are automatically computed by Polymer.
This simplifies the code and reduces the chance of errors. Due to the
limitations of computed functions in Polymer, these functions have much
wider dependencies than necessary, however, this does not appear to have
a noticable impact on performance.

BUG=425625

Committed: https://crrev.com/0e7617f648602b15e8a2faf2a0c3118eee9d5f4e
Cr-Commit-Position: refs/heads/master@{#386883}
==========

[commit-bot: I haz the power, 2016-04-13 01:19:18]

Patchset 4 (id:??) landed as
https://crrev.com/0e7617f648602b15e8a2faf2a0c3118eee9d5f4e
Cr-Commit-Position: refs/heads/master@{#386883}