[ntp4] More UI refinements.

chrome/browser/resources/ntp4/bookmarks_page.js

Index: chrome/browser/resources/ntp4/bookmarks_page.js
===================================================================
--- chrome/browser/resources/ntp4/bookmarks_page.js	(revision 98657)
+++ chrome/browser/resources/ntp4/bookmarks_page.js	(working copy)
@@ -59,11 +59,10 @@
       var faviconDiv = this.querySelector('.favicon');
       var faviconUrl;
       if (this.data.url) {
-        faviconUrl = 'chrome://favicon/size/32/' + this.data.url;
+        faviconUrl = 'chrome://favicon/size/16/' + this.data.url;
         chrome.send('getFaviconDominantColor',
                     [faviconUrl, id, 'ntp4.setBookmarksFaviconDominantColor']);
       } else {
-        // TODO(csilv): We need a large (32px) icon for this URL.

[Evan Stade, 2011/08/30 01:06:24]

we do still need a large icon for this

[csilv, 2011/08/30 01:29:04]

Yes, but that's unrelated to fetching use of the favicon protocol, so that's why
this TODO is obsolete.

[Evan Stade, 2011/08/30 01:56:29]

what? this uses the chrome://theme protocol not the favicon protocol. We still
want a larger icon (bigger than 32x32)

[csilv, 2011/08/30 02:06:50]

Sorry, my reply was inaccurate.  What I meant to say was that this TODO was here
to remind me that we need a 32x32 icon for display in place of the favicon. 
That is no longer the case, we're using 16x16 favicon (for now).  The change to
use a large icon for folders is separate and not related to this segment of
code.  While the icon will be loaded via a theme URL, it will not be stored in
the favicon element, which is what this segment is doing.

         faviconUrl = 'chrome://theme/IDR_BOOKMARK_BAR_FOLDER';
         // TODO(csilv): Should we vary this color by platform?
         this.stripeColor = '#919191';
@@ -89,8 +88,11 @@
      *     animate.
      */
     setBounds: function(size, x, y) {
-      this.style.width = this.style.height = size + 'px';
+      this.style.width = size + 'px';
+      this.style.height = heightForWidth(size) + 'px';
+
       this.style.left = x + 'px';
+      this.style.right = x + 'px';
       this.style.top = y + 'px';
     },
 
@@ -173,6 +175,16 @@
   TilePage.initGridValues(bookmarksPageGridValues);
 
   /**
+   * Calculates the height for a bookmarks tile for a given width. The size
+   * is based on a desired size of 96x72 ratio.
+   * @return {number} The height.
+   */
+  function heightForWidth(width) {
+    // The 2s are for borders, the 31 is for the title.
+    return (width - 2) * 72 / 96 + 2 + 31;

[Evan Stade, 2011/08/30 01:06:24]

I imagine the 96x72 ratio applies to the whole thing including the border, i.e.
you don't need the 2s here

+  }
+
+  /**
    * Creates a new BookmarksPage object.
    * @constructor
    * @extends {TilePage}
@@ -275,6 +287,9 @@
       return false;
     },
 
+    /** @inheritDoc */
+    heightForWidth: heightForWidth,
+
     /**
      * Invoked before a batch import begins.  We will ignore added/changed
      * notifications while the operation is in progress.