MD History: Add basic material design history cards and history items

chrome/browser/resources/md_history/history-card-manager.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',
+
+  properties: {
+    // An array of objects sorted in reverse chronological order.
+    // Each object has a date and the history items belonging to that date.
+    historyDataByDay_: {
+      type: Array,
+      value: []

[Dan Beam, 2016/01/16 02:54:35]

should also probably be funtion() { return []; }

[yingran, 2016/01/18 05:40:28]

Done.

+    },
+    // The time of access of the last element of historyDataByDay_.
+    lastVisitedTime: {
+      type: Number,
+      value: 0
+    },
+    menuOpen: {
+      type: Boolean,
+      value: false,
+      reflectToAttribute: true
+    },
+    menuIdentifier: {
+      type: Number,
+      value: 0
+    }
+  },
+
+  /** @const */
+  X_OFFSET: 30,

[Dan Beam, 2016/01/16 02:54:35]

can this be @private?

[yingran, 2016/01/18 05:40:29]

Done.

+
+  listeners: {
+    'tap': 'closeMenu',
+    'toggle-menu': 'toggleMenu_'
+  },
+
+  /**
+   * Closes the overflow menu.
+   */
+  closeMenu: function() {
+    if (this.menuOpen) {
+      this.menuOpen = false;

[Dan Beam, 2016/01/16 02:54:35]

why not just always do this.menuOpen = false; and observe menuOpen for changes?

[yingran, 2016/01/18 05:40:28]

Done.

+    }

[Dan Beam, 2016/01/16 02:54:36]

no curlies

[yingran, 2016/01/18 05:40:29]

Done.

+  },
+
+  /**
+   * Opens the overflow menu unless the menu is already open and the same button
+   * is pressed.
+   * @param {Event} e The event with details of the menu item that was clicked.
+   * @private
+   */
+  toggleMenu_: function(e) {
+    var menu = this.$['overflow-menu'];
+
+    // Menu closes if the same button is clicked.
+    if (this.menuOpen && this.menuIdentifier == e.detail.accessTime) {
+      this.menuOpen = false;
+    } else {
+      this.menuOpen = true;
+      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. Makes sure the page stays in the same position when new content
+   * is loaded by saving the previous scroll bar position.
+   * @param {Array} results The new history results.
+   */
+  addNewResults: function(results) {
+    // Saves the current position of the scroll bar before the new list items
+    // are rendered.

[Dan Beam, 2016/01/16 02:54:35]

why is this necessary?

[yingran, 2016/01/18 05:40:28]

now that i test it - it doesn't seem to be necessary...

+    var scrollPosition = this.$['infinite-list'].scrollTop;
+
+    if (results.length == 0) {
+      return;
+    }

[Dan Beam, 2016/01/16 02:54:35]

no curlies

[yingran, 2016/01/18 05:40:28]

Done.

+
+    var dateSortedData = [];
+    var historyItems = [];
+    var currentDate = results[0].dateRelativeDay;
+
+    for (var i = 0; i < results.length; i++) {
+      results[i].selected = false;
+      if (results[i].dateRelativeDay != currentDate) {
+        this.appendHistoryData_(currentDate, historyItems);
+        currentDate = results[i].dateRelativeDay;
+        historyItems = [];
+      }
+      historyItems.push(results[i]);
+    }
+    this.appendHistoryData_(currentDate, historyItems);
+
+    this.lastVisitedTime = historyItems[historyItems.length - 1].time;
+    this.$['infinite-list'].scrollTop = scrollPosition;
+  },
+
+  /**
+   * Cycle through each entry in historyDataByDay_ and set all items to be
+   * unselected.
+   */
+  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;
+          }

[Dan Beam, 2016/01/16 02:54:35]

no curlies

[yingran, 2016/01/18 05:40:29]

Done.

+        }
+      }
+    }
+  },
+
+  /**
+   * 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} historyItems The list of history items for the current date.
+   * @private
+   */
+  appendHistoryData_: function(date, historyItems) {
+    var lastIndex = this.historyDataByDay_.length - 1;

[Dan Beam, 2016/01/16 02:54:35]

could this be oldestDay or lastDay?

[yingran, 2016/01/18 05:40:29]

Done.

+    if (lastIndex > 0 && date == this.historyDataByDay_[lastIndex].date) {
+      // A new object is required for the card to update due to iron-list's
+      // pass by reference implementation.

[Dan Beam, 2016/01/16 02:54:35]

can you give an example of when this happens or what problem exhibits?

[yingran, 2016/01/18 05:40:28]

Notify path only works if the oldValue!=newValue so polymer doesn't know that
the value changes if we just modify the array (even with a notifyPath()).

Example:
So if I do

      for(var i = 0; i < historyItems.length; i++)
        this.historyDataByDay_[lastDay].historyItems.push(historyItems[i]);
      
      this.notifyPath('historyDataByDay_.' + lastDay + '.historyItems',
this.historyDataByDay_[lastDay].historyItems);

then the path is not properly notified since
"this.historyDataByDay_[lastDay].historyItems" is still the same array. 

This caused confusion because we expected that modifying the elements of an
array and then notifying would update the Polymer elements. Using array concat
which creates a new array, we resolved the problem.

[Dan Beam, 2016/01/26 23:07:39]

did you try:

  this.notifyPath('historyDataByDay_.' + lastDay + '.historyItems.*');

?

[yingran, 2016/01/27 05:05:03]

Notify path for 'historyDataByDay_.' + lastDay + '.historyItems',
this.historyDataByDay_[lastDay].historyItems works if we remove the
oldValue!==newValue check on _notifyPath(). 

this.notifyPath('historyDataByDay_.' + lastDay + '.historyItems.*') doesn't work
with or without the check.

+      var items = this.historyDataByDay_[lastIndex]
+                      .historyItems.concat(historyItems);
+      this.splice('historyDataByDay_', lastIndex, 1, {

[Dan Beam, 2016/01/16 02:54:35]

how is splice('thing_', this.thing_.length - 1, 1, ...) different than
this.push('thing_', ...)?

[yingran, 2016/01/18 05:40:29]

Done.

this.push won't work because |historyItems| is a list so I need concat.

We didn't know you could do sub path indexing at the time.

Now it is changed to this.set()

+        date: date,
+        historyItems: items,
+      });
+    } else {
+      this.push('historyDataByDay_', {
+        date: date,
+        historyItems: historyItems
+      });
+    }
+  },
+
+  /**
+   * Called when the card manager is scrolled.
+   */
+  scrollHandler: function(e) {

[Dan Beam, 2016/01/16 02:54:35]

make @private (being called from the template is not reason to make it public,
IMO)

[yingran, 2016/01/18 05:40:28]

Done.

+    // Closes overflow menu on scroll.
+    this.closeMenu();
+
+    this.doInfiniteScroll_(e);
+  },
+
+  /**
+   * Requests the next list of results when the scrollbar is near the bottom
+   * of the window.

[Dan Beam, 2016/01/16 02:54:35]

@param

[yingran, 2016/01/18 05:40:29]

Removed param e since no longer needed as per comment below.

+   */
+  doInfiniteScroll_: function(e) {

[Dan Beam, 2016/01/16 02:54:35]

nit: loadMoreIfAtEnd_?  "doInfiniteScroll_" makes very little sense to me as a
name

[yingran, 2016/01/18 05:40:29]

Done.

+    var scrollOffset = 10;
+    var scrollElem = e.srcElement;

[Dan Beam, 2016/01/16 02:54:35]

srcElement -> target, but why are we using |e| at all?  why not just use a
reference to the element we know is the scroller?  i.e this.$['infinite-list']

[yingran, 2016/01/18 05:40:29]

Done.

+
+    if (scrollElem.scrollHeight <=
+        scrollElem.scrollTop + scrollElem.clientHeight + scrollOffset) {

[Dan Beam, 2016/01/16 02:54:35]

this looks a lot like the downloads implementation...

[yingran, 2016/01/18 05:40:28]

:)

+      chrome.send('queryHistory',
+          ['', 0, 0, this.lastVisitedTime, RESULTS_PER_PAGE]);
+    }
+  }
+});