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. Preserves the scroll position of the window when new data
+   * is loaded.
    * @param {!Array<!HistoryEntry>} results The new history results.
    */
   addNewResults: function(results) {
     if (results.length == 0)
       return;
 
-    var dateSortedData = [];
-    var historyItems = [];
+    var scrollPosition = this.$['infinite-list'].scrollTop;

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

The scroll position stuff had been removed previously. Do we need it to come
back?

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

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

+    // 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;
+    }
+
     var currentDate = results[0].dateRelativeDay;
 
     for (var i = 0; i < results.length; i++) {
-      if (!currentDate)
-        continue;
-
       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 = [];
+      } else {
+        results[i].isLastItem = results[i].isLastItem || false;
+        results[i].isFirstItem = results[i].isFirstItem || false;

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

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.

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

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

       }
-      historyItems.push(results[i]);
+      results[i].needsTimeGap = this.needsTimeGap_(results, i);
     }
 
-    if (currentDate)
-      this.appendHistoryData_(currentDate, historyItems);
-
-    this.lastVisitedTime = historyItems[historyItems.length - 1].time;
+    this.set('historyData', this.historyData.concat(results));
+    this.lastVisitedTime = this.historyData[this.historyData.length - 1].time;
+    this.$['infinite-list'].scrollTop = scrollPosition;
   },
 
   /**
-   * 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.
+   * @return {boolean} Whether or not time gap separator is required.
+   * @private
+   */
+  needsTimeGap_: function(results, index) {
+   var items = results;
+   return index + 1 < items.length &&
+       items[index].time - items[index + 1].time > BROWSING_GAP_TIME &&
+       items[index].dateRelativeDay == items[index + 1].dateRelativeDay;
   }
 });