[ntp4] Bookmarks page implementation, first-pass.

chrome/browser/resources/ntp4/bookmarks_page.js

Index: chrome/browser/resources/ntp4/bookmarks_page.js
===================================================================
--- chrome/browser/resources/ntp4/bookmarks_page.js	(revision 96479)
+++ chrome/browser/resources/ntp4/bookmarks_page.js	(working copy)
@@ -5,6 +5,162 @@
 cr.define('ntp4', function() {
   'use strict';
 
+  var localStrings = new LocalStrings;
+  var tileID = 0;

[Evan Stade, 2011/08/12 21:13:30]

comment this

[arv (Not doing code reviews), 2011/08/13 01:02:07]

tileId for consistency

[csilv, 2011/08/13 01:09:21]

Done.

[csilv, 2011/08/13 01:48:17]

Done.

+
+  /**
+   * Creates a new bookmark object.
+   * @param {Object} data The url and title.
+   * @constructor
+   * @extends {HTMLDivElement}
+   */
+  function Bookmark(data) {
+    var el = cr.doc.createElement('div');
+    el.__proto__ = Bookmark.prototype;
+    el.data = data;
+    el.initialize();
+
+    return el;
+  }
+
+  Bookmark.prototype = {
+    __proto__: HTMLDivElement.prototype,
+
+    initialize: function() {
+      this.className = 'bookmark';
+      this.innerHTML =
+            '<a class="button bookmark-fills-parent">' +

[Evan Stade, 2011/08/12 21:13:30]

perhaps you could put this html in new_tab.html as a hidden template node and
then clone it. I don't have a particular preference for this approach but some
others apparently do.

(I should update most visited too)

[csilv, 2011/08/13 01:09:21]

Oh, that's a great idea!  Done.  Also did the same for the bookmarks page titles
header below.

+              '<div class="close-button"></div>' +
+              '<span class="button-frame bookmark-fills-parent"></span>' +
+              '<span class="favicon bookmark-fills-parent"></span>' +
+            '</a>' +
+          '<div class="color-stripe"></div>' +
+          '<a class="title"></a>';
+
+      var id = tileID++;
+      this.setAttribute('id', 'tile' + id);

[Evan Stade, 2011/08/12 21:13:30]

I think you can just do this.id =

(again, regardless of what most visited does)

however this is going to conflict with most visited IDs (which are also id +
number)

[csilv, 2011/08/13 01:09:21]

Done.  I also made updates so that the dominant color can be fetched for both
Most Visited and Bookmarks independently.  So the conflicting IDs are now
resolved.

+
+      var title = this.querySelector('.title');
+      title.textContent = this.data.title;
+
+      if (this.data.url) {
+        var button = this.querySelector('.button');
+        button.href = title.href = this.data.url;
+      }
+
+      var faviconDiv = this.querySelector('.favicon');
+      var faviconUrl;
+      if (this.data.url) {
+        faviconUrl = 'chrome://favicon/size/32/' + this.data.url;
+        if (this.data.faviconDominantColor)
+          this.setStripeColor(this.data.faviconDominantColor);

[Evan Stade, 2011/08/12 21:13:30]

add TODOs for the parts of the code that don't actually work yet (makes it
easier to review)

[arv (Not doing code reviews), 2011/08/13 01:02:07]

Use a setter whenever you see a setFoo.

[csilv, 2011/08/13 01:09:21]

Done.  This is now implemented, all other missing parts should be commented with
TODOs.

[csilv, 2011/08/13 01:48:17]

Done.

+        else
+          chrome.send('getFaviconDominantColor', [faviconUrl, id]);
+      } else {
+        // TODO(csilv): We need a large (32px) icon for this URL.
+        faviconUrl = 'chrome://theme/IDR_BOOKMARK_BAR_FOLDER';
+        this.setStripeColor('#919191');

[arv (Not doing code reviews), 2011/08/13 01:02:07]

The icon for the bookmark bar folder is different on different platforms...
should the stripe take that into account?

[csilv, 2011/08/13 01:48:17]

Perhaps, right now it's just using a neutral color for folders.  I've added a
TODO here so we can figure that out.

+      }
+      faviconDiv.style.backgroundImage = url(faviconUrl);
+
+      this.addEventListener('click', this.handleClick_.bind(this));
+    },
+
+    /**
+     * Sets the color of the favicon dominant color bar.
+     * @param {string} color The css-parsable value for the color.
+     */
+    setStripeColor: function(color) {
+      this.querySelector('.color-stripe').style.backgroundColor = color;
+    },
+
+    /**
+     * Set the size and position of the bookmark tile.
+     * @param {number} size The total size of |this|.
+     * @param {number} x The x-position.
+     * @param {number} y The y-position.
+     *     animate.
+     */
+    setBounds: function(size, x, y) {
+      this.style.width = this.style.height = size + 'px';
+      this.style.left = x + 'px';
+      this.style.top = y + 'px';
+    },
+
+    /**
+     * Invoked when a bookmark is clicked
+     * @param {Event} e The click event.
+     * @private
+     */
+    handleClick_: function(e) {
+      if (e.target.classList.contains('close-button')) {
+        this.handleDelete_();
+        e.preventDefault();
+      } else if (!this.data.url) {
+        chrome.send('getBookmarksData', [ this.data.id ]);

[arv (Not doing code reviews), 2011/08/13 01:02:07]

no ws after [ nor before ]

[csilv, 2011/08/13 01:48:17]

Done.

+        e.preventDefault();
+      }
+    },
+
+    /**
+     * Delete a bookmark from the data model.
+     * @private
+     */
+    handleDelete_: function() {
+      // TODO(csilv): add support for deleting bookmarks
+    },
+
+    /**
+     * The data and preferences for this app.
+     * @type {Object}
+     */
+    set data(data) {
+      this.data_ = data;

[arv (Not doing code reviews), 2011/08/13 01:02:07]

No need for a getter or setter her since there are no side effects.

[csilv, 2011/08/13 01:48:17]

Done.

+    },
+    get data() {
+      return this.data_;
+    },
+  };
+
+  /**
+   * Creates a new bookmark title object.
+   * @param {Object} data The url and title.
+   * @constructor
+   * @extends {HTMLDivElement}
+   */
+  function BookmarkTitle(data) {
+    var el = cr.doc.createElement('div');
+    el.__proto__ = BookmarkTitle.prototype;
+    el.initialize(data);
+
+    return el;
+  }
+
+  BookmarkTitle.prototype = {
+    __proto__: HTMLDivElement.prototype,
+
+    initialize: function(data) {
+      this.className = 'title-crumb';
+      this.folderId_ = data.id;
+
+      if (!data.parentId)
+        this.innerText = localStrings.getString('bookmarksPage');

[Evan Stade, 2011/08/12 21:13:30]

ternary operator

[arv (Not doing code reviews), 2011/08/13 01:02:07]

Use textContent instead of innerText

[csilv, 2011/08/13 01:09:21]

Done.

[csilv, 2011/08/13 01:48:17]

Done.

+      else
+        this.innerText = data.title;
+
+      this.addEventListener('click', this.handleClick_.bind(this));

[arv (Not doing code reviews), 2011/08/13 01:02:07]

no need for this bind since you are adding the event listener to 'this' already.

[csilv, 2011/08/13 01:48:17]

Done.

+    },
+
+    /**
+     * Invoked when a bookmark title is clicked
+     * @param {Event} e The click event.
+     * @private
+     */
+    handleClick_: function(e) {
+      chrome.send('getBookmarksData', [ this.folderId_ ]);

[arv (Not doing code reviews), 2011/08/13 01:02:07]

ws

[csilv, 2011/08/13 01:48:17]

Done.

+    },
+  };
+
   var TilePage = ntp4.TilePage;
 
   var bookmarksPageGridValues = {
@@ -14,9 +170,9 @@
     maxColCount: 6,
 
     // The smallest a tile can be.
-    minTileWidth: 100,
+    minTileWidth: 150,
     // The biggest a tile can be.
-    maxTileWidth: 100,
+    maxTileWidth: 150,
   };
   TilePage.initGridValues(bookmarksPageGridValues);
 
@@ -39,20 +195,94 @@
     initialize: function() {
       this.classList.add('bookmarks-page');
 
-      // TODO(csilv): Remove this placeholder.
-      var placeholder = this.ownerDocument.createElement('div');
-      placeholder.textContent = 'Bookmarks coming soon...';
-      placeholder.className = 'placeholder';
-      this.insertBefore(placeholder, this.firstChild);
+      var titleWrapper = this.ownerDocument.createElement('div');
+      titleWrapper.className = 'section-title-wrapper';
+      titleWrapper.setAttribute('id', 'bookmarks_title_wrapper');

[arv (Not doing code reviews), 2011/08/13 01:02:07]

Prefer DOM bindings over setAttribute

titleWrapper.id = 'bookmarks_title_wrapper';

[csilv, 2011/08/13 01:48:17]

Done.

+      this.insertBefore(titleWrapper, this.firstChild);
+
+      var titleMask = this.ownerDocument.createElement('div');
+      titleMask.className = 'section-title-mask';
+      titleWrapper.appendChild(titleMask);
+
+      var title = this.ownerDocument.createElement('div');
+      title.className = 'section-title';
+      titleWrapper.appendChild(title);
     },
 
+    /**
+     * Build the bookmark titles bar (ie, navigation hiearchy).
+     * @param {Array} items The parent hiearchy of the current folder.
+     * @private
+     */
+    updateBookmarkTitles_: function(items) {
+      var wrapper = $('bookmarks_title_wrapper');
+      var title = wrapper.getElementsByClassName('section-title')[0];

[arv (Not doing code reviews), 2011/08/13 01:02:07]

querySelector('.section-title')

[csilv, 2011/08/13 01:48:17]

Done.

+      title.innerHTML = '';
+
+      for (var i = items.length - 1; i > 0; i--) {
+        var titleCrumb = new BookmarkTitle(items[i]);
+        title.appendChild(titleCrumb);
+
+        var separator = document.createElement('div');

[arv (Not doing code reviews), 2011/08/13 01:02:07]

maybe use an hr here?

[csilv, 2011/08/13 01:48:17]

Done.

+        separator.className = 'bookmark-separator';
+        title.appendChild(separator);
+      }
+
+      var titleCrumb = new BookmarkTitle(items[0]);
+      titleCrumb.classList.add('title-crumb-active');
+      title.appendChild(titleCrumb);
+    },
+
+    /**
+     * Build the bookmark tiles.
+     * @param {Array} items The contents of the current folder.
+     * @private
+     */
+    updateBookmarkTiles_: function(items) {
+      this.removeAllTiles();
+      for (var i = 0; i < items.length; i++)
+        this.appendTile(new Bookmark(items[i]), false);
+    },
+
     /** @inheritDoc */
     shouldAcceptDrag: function(dataTransfer) {
       return false;
-    }
+    },
+
+    /**
+     * Set the bookmark data that should be displayed, replacing any existing
+     * data.
+     */
+    set data(data) {
+      this.updateBookmarkTiles_(data.items);
+      this.updateBookmarkTitles_(data.navigationItems);
+    },
   };
 
+  /**
+   * Initializes and renders the bookmark chevron canvas.  This needs to be
+   * performed after the page has been loaded so that we have access to the
+   * style sheet values.
+   */
+  function initBookmarkChevron() {
+    var wrapperStyle = window.getComputedStyle($('bookmarks_title_wrapper'));
+    var width = 10;
+    var height = parseInt(wrapperStyle.height);

[arv (Not doing code reviews), 2011/08/13 01:02:07]

always use two args to parseInt due to its strange behavior where it prefers to
parse things as octal.

[csilv, 2011/08/13 01:48:17]

Done.

+    var ctx = document.getCSSCanvasContext('2d', 'bookmark-chevron',
+                                           width, height);
+    ctx.strokeStyle = wrapperStyle.borderBottomColor;
+    ctx.beginPath();
+    ctx.moveTo(0, 0);
+    ctx.lineTo(width, height / 2);
+    ctx.lineTo(0, height);
+    ctx.stroke();
+  }
+
   return {
-    BookmarksPage: BookmarksPage
+    BookmarksPage: BookmarksPage,
+    initBookmarkChevron: initBookmarkChevron
   };
 });
+
+window.addEventListener('load', ntp4.initBookmarkChevron);
+