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

chrome/browser/resources/md_history/history_list.js

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 Polymer({
   is: 'history-list',
 
   properties: {
     // An array of history entries in reverse chronological order.
     historyData: {
@@ skipping 87 matching lines @@
 
     if (historyResults.length == 0)
       return;
 
     // Creates a copy of historyResults to prevent accidentally modifying this
     // field.
     var results = historyResults.slice();
 
     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
       // existing array.
       results.unshift('historyData');
       this.push.apply(this, results);
     } else {
       // The first time we receive data, use set() to ensure the iron-list is
       // initialized correctly.
       this.set('historyData', results);
@@ skipping 21 matching lines @@
     }
   },
 
   /**
    * Remove all selected items from the overall array so that they are also
    * removed from view. Make sure that the card length and positioning is
    * updated accordingly.
    * @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.

   },
 
   /**
    * Based on which items are selected, collect an array of the info required
    * for chrome.send('removeHistory', ...).
    * @param {number} count The number of items that are selected.
    * @return {Array<HistoryEntry>} toBeRemoved An array of objects which contain
    * information on which history-items should be deleted.
    */
   getSelectedItems: function(count) {
@@ skipping 30 matching lines @@
         scrollElem.scrollTop + scrollElem.clientHeight + scrollOffset) {
       this.loading_ = true;
       chrome.send('queryHistory',
           [this.searchTerm, 0, 0, this.lastVisitedTime, RESULTS_PER_PAGE]);
     }
   },
 
   /**
    * 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;
 
     return currentItem.time - nextItem.time > BROWSING_GAP_TIME &&
         currentItem.dateRelativeDay == nextItem.dateRelativeDay;
   },
 
   hasResults: function(historyDataLength) {
     return historyDataLength > 0;
   },
 
   noResultsMessage_: function(searchTerm, isLoading) {
     if (isLoading)
       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;
+  },
 });