MD History: Refactored design for displaying history information

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-card-manager',
+  is: 'history-list',
 
   properties: {
-    // An array of objects sorted in reverse chronological order.
-    // Each object has a date and the history items belonging to that date.
-    historyDataByDay_: {
+    // An array of history entries in reverse chronological order.
+    historyData: {
       type: Array,
       value: function() { return []; }
     },
 
-    // The time of access of the last element of historyDataByDay_.
+    // The time in seconds of the last history item in historyData.
     lastVisitedTime: {
       type: Number,
       value: 0
     },
 
     menuOpen: {
       type: Boolean,
       value: false,
       reflectToAttribute: true
     },
@@ skipping 36 matching lines @@
       this.menuIdentifier = e.detail.accessTime;
 
       cr.ui.positionPopupAtPoint(e.detail.x + this.X_OFFSET_, e.detail.y, menu,
           cr.ui.AnchorType.BEFORE);
 
       menu.focus();
     }
   },
 
   /**
-   * Split the newly updated history results into history items sorted via day
-   * accessed.
+   * Adds the newly updated history results into historyData. Adds new fields
+   * for each result.
    * @param {!Array<!HistoryEntry>} results The new history results.
    */
   addNewResults: function(results) {
     if (results.length == 0)
       return;
 
-    var dateSortedData = [];
-    var historyItems = [];
     var currentDate = results[0].dateRelativeDay;
 
     for (var i = 0; i < results.length; i++) {
-      if (!currentDate)
-        continue;
+      // 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].selected = false;
       if (results[i].dateRelativeDay != currentDate) {
-        this.appendHistoryData_(currentDate, historyItems);
+        results[i - 1].isLastItem = true;
+        results[i].isFirstItem = true;
         currentDate = results[i].dateRelativeDay;
-        historyItems = [];
       }
-      historyItems.push(results[i]);
+      results[i].needsTimeGap = this.needsTimeGap_(results, i);
     }
 
-    if (currentDate)
-      this.appendHistoryData_(currentDate, historyItems);
+    // If it's the first time we get data, the first item will always be the
+    // first card.
+    if (this.historyData.length == 0)
+      results[0].isFirstItem = true;
 
-    this.lastVisitedTime = historyItems[historyItems.length - 1].time;
+    results.unshift('historyData');

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

Would be good to have a comment here explaining what is going on.

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

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.

[yingran, 2016/02/11 00:41:42]

Done.

[yingran, 2016/02/11 00:41:42]

Done.

+    this.push.apply(this, results);

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

Should use `Polymer.Base.push.apply`

(Right, Chris?)

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

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

+
+    this.lastVisitedTime = this.historyData[this.historyData.length - 1].time;
   },
 
   /**
-   * Cycle through each entry in historyDataByDay_ and set all items to be
+   * Cycle through each entry in historyData and set all items to be
    * unselected.
+   * @param {number} overallItemCount The number of checkboxes selected.
    */
   unselectAllItems: function(overallItemCount) {
-    var historyCardData = this.historyDataByDay_;
-
-    for (var i = 0; i < historyCardData.length; i++) {
-      var items = historyCardData[i].historyItems;
-      for (var j = 0; j < items.length; j++) {
-        if (items[j].selected) {
-          this.set('historyDataByDay_.' + i + '.historyItems.' + j +
-              '.selected', false);
-          overallItemCount--;
-          if (overallItemCount == 0)
-            break;
-        }
+    for (var i = 0; i < this.historyData.length; i++) {
+      if (this.historyData[i].selected) {
+        this.set('historyData.' + i + '.selected', false);
+        overallItemCount--;
+        if (overallItemCount == 0)
+          break;
       }
     }
   },
 
   /**
-   * Adds the given items into historyDataByDay_. Adds items to the last
-   * existing day if the date matches, creates a new element otherwise.
-   * @param {string} date The date of the history items.
-   * @param {!Array<!HistoryEntry>} historyItems The list of history items for
-   * the current date.
-   * @private
-   */
-  appendHistoryData_: function(date, historyItems) {
-    var lastDay = this.historyDataByDay_.length - 1;
-    if (lastDay > 0 && date == this.historyDataByDay_[lastDay].date) {
-      this.set('historyDataByDay_.' + lastDay + '.historyItems',
-          this.historyDataByDay_[lastDay].historyItems.concat(historyItems));
-    } else {
-      this.push('historyDataByDay_', {
-        date: date,
-        historyItems: historyItems
-      });
-    }
-  },
-
-  /**
    * Called when the card manager is scrolled.
    * @private
    */
   scrollHandler_: function() {
     // Close overflow menu on scroll.
     this.closeMenu();
 
     // Requests the next list of results when the scrollbar is near the bottom
     // of the window.
     var scrollOffset = 10;
     var scrollElem = this.$['infinite-list'];
 
     if (scrollElem.scrollHeight <=
         scrollElem.scrollTop + scrollElem.clientHeight + scrollOffset) {
       chrome.send('queryHistory',
           ['', 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 {number} index The index number of the first item being compared.

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

Missing a param.

[yingran, 2016/02/11 00:41:42]

Done.

+   * @return {boolean} Whether or not time gap separator is required.
+   * @private
+   */
+  needsTimeGap_: function(results, index) {
+   var items = results;

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

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:42]

Done.

+   return index + 1 < items.length &&
+       items[index].time - items[index + 1].time > BROWSING_GAP_TIME &&
+       items[index].dateRelativeDay == items[index + 1].dateRelativeDay;
   }
 });