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

chrome/browser/resources/md_history/history_list.js

Index: chrome/browser/resources/md_history/history_list.js
diff --git a/chrome/browser/resources/md_history/history_list.js b/chrome/browser/resources/md_history/history_list.js
index 791a9acbe0cdae7bd0ca27bc43fc0c978ee8cc49..8be183f563baa7e1244d71c3d63af8b3db9971d5 100644
--- a/chrome/browser/resources/md_history/history_list.js
+++ b/chrome/browser/resources/md_history/history_list.js
@@ -105,35 +105,16 @@ Polymer({
 
     var currentDate = results[0].dateRelativeDay;
 
-    // Resets the last history item for the currentDate if new history results
-    // for currentDate is loaded.
-    if (this.historyData && this.historyData.length > 0) {
-      var lastHistoryItem = this.historyData[this.historyData.length - 1];
-      if (lastHistoryItem && lastHistoryItem.dateRelativeDay == currentDate) {
-        this.set('historyData.' + (this.historyData.length - 1) +
-            '.isLastItem', false);
-      }
-    }
-
     for (var i = 0; i < results.length; i++) {
       // Sets the default values for these fields to prevent undefined types.
       results[i].selected = false;
-      results[i].isLastItem = false;
-      results[i].isFirstItem = false;
-      results[i].needsTimeGap = this.needsTimeGap_(results, i);
       results[i].readableTimestamp =
           searchTerm == '' ? results[i].dateTimeOfDay : results[i].dateShort;
 
       if (results[i].dateRelativeDay != currentDate) {
-        results[i - 1].isLastItem = true;
-        results[i].isFirstItem = true;
         currentDate = results[i].dateRelativeDay;
       }
     }
-    results[i - 1].isLastItem = true;
-
-    if (!this.historyData || this.historyData.length == 0)
-      results[0].isFirstItem = true;
 
     if (this.historyData) {
       // If we have previously received data, push the new items onto the
@@ -175,42 +156,27 @@ Polymer({
    * @param {number} overallItemCount The number of items selected.
    */
   removeDeletedHistory: function(overallItemCount) {
+    var spliceinfo = [];

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

nit: splices

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

Done.

     for (var i = this.historyData.length - 1; i >= 0; i--) {
       if (!this.historyData[i].selected)
         continue;
 
-      // TODO: Change to using computed properties to recompute the first and
-      // last cards.
-
-      // Resets the first history item.
-      if (this.historyData[i].isFirstItem &&
-          (i + 1) < this.historyData.length &&
-          this.historyData[i].dateRelativeDay ==
-          this.historyData[i + 1].dateRelativeDay) {
-        this.set('historyData.' + (i + 1) + '.isFirstItem', true);
-      }
-
-      // Resets the last history item.
-      if (this.historyData[i].isLastItem && i > 0 &&
-          this.historyData[i].dateRelativeDay ==
-          this.historyData[i - 1].dateRelativeDay) {
-        this.set('historyData.' + (i - 1) + '.isLastItem', true);
-
-        if (this.historyData[i - 1].needsTimeGap)
-          this.set('historyData.' + (i - 1) + '.needsTimeGap', false);
-      }
-
-      // Makes sure that the time gap separators are preserved.
-      if (this.historyData[i].needsTimeGap && i > 0)
-        this.set('historyData.' + (i - 1) + '.needsTimeGap', true);
-
-      // Removes the selected item from historyData.
-      this.splice('historyData', i, 1);
+      // Removes the selected item from historyData. Use unshift so spliceInfo
+      // ends up in index order.
+      spliceinfo.unshift({
+        index: i,
+        removed: [this.historyData[i]],
+        addedCount: 0,
+        object: this.historyData,
+        type: 'splice'
+      });
+      this.historyData.splice(i, 1);
 
       overallItemCount--;
       if (overallItemCount == 0)
         break;
     }
+    this.notifySplices('historyData', spliceinfo);

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

why are you using notifySplices?
https://www.polymer-project.org/1.0/docs/devguide/properties.html#notifysplices

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

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.

   },
 
   /**
@@ -261,19 +227,18 @@ Polymer({
   /**
    * Check whether the time difference between the given history item and the
    * next one is large enough for a spacer to be required.
-   * @param {Array<HistoryEntry>} results A list of history results.
-   * @param {number} index The index number of the first item being compared.
+   * @param {HistoryEntry} item
+   * @param {number} index The index of |item| in |historyData|
+   * @param {number} length The length of |historyData|

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

nit: periods after [semi-complete] sentences

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

Done.

    * @return {boolean} Whether or not time gap separator is required.
    * @private
    */
-  needsTimeGap_: function(results, index) {
-    // TODO(tsergeant): Allow the final item from one batch of results to have a
-    // timegap once more results are added.
-    if (index == results.length - 1)
+  needsTimeGap_: function(item, index, length) {
+    if (index >= length - 1 || length == 0)

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

when should index be longer than the length of the list?

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

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.

       return false;
 
-    var currentItem = results[index];
-    var nextItem = results[index + 1];
+    var currentItem = this.historyData[index];
+    var nextItem = this.historyData[index + 1];
 
     if (this.searchTerm)
       return currentItem.dateShort != nextItem.dateShort;
@@ -291,5 +256,21 @@ Polymer({
       return '';
     var messageId = searchTerm !== '' ? 'noSearchResults' : 'noResults';
     return loadTimeData.getString(messageId);
-  }
+  },
+

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

nit: jsdoc for both of these methods?

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

Done.

+  isCardStart_: function(item, i, length) {
+    if (length == 0 || i > length - 1)
+      return false;
+    return i == 0 ||
+        this.historyData[i].dateRelativeDay !=
+        this.historyData[i - 1].dateRelativeDay;
+  },
+
+  isCardEnd_: function(item, i, length) {
+    if (length == 0 || i > length - 1)
+      return false;
+    return i == length - 1 ||
+        this.historyData[i].dateRelativeDay !=
+        this.historyData[i + 1].dateRelativeDay;
+  },
 });