[ntp4] More UI refinements.

[ntp4] More UI refinements.

* Constraint favicon size for bookmarks & most-visited to 16x16.
* Make color strip on bookmarks and most-visited full opacity.
* Set padding space between icon and title for bookmarks & most-visited to 8px.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=98859

[csilv, 2011-08-29 21:25:30]

+estade for review

[Evan Stade, 2011-08-29 21:28:53]

http://codereview.chromium.org/7794005/diff/1/chrome/browser/resources/ntp4/m...
File chrome/browser/resources/ntp4/most_visited_page.css (right):

http://codereview.chromium.org/7794005/diff/1/chrome/browser/resources/ntp4/m...
chrome/browser/resources/ntp4/most_visited_page.css:65: background-size: 16px;
I think you can do background-size 100% maybe

http://codereview.chromium.org/7794005/diff/1/chrome/browser/resources/ntp4/m...
chrome/browser/resources/ntp4/most_visited_page.css:90: padding-top: 8px;
just add this to line height. And then you'll need to update the JS
heightForWidth function.

[csilv, 2011-08-29 22:56:52]

http://codereview.chromium.org/7794005/diff/1/chrome/browser/resources/ntp4/m...
File chrome/browser/resources/ntp4/most_visited_page.css (right):

http://codereview.chromium.org/7794005/diff/1/chrome/browser/resources/ntp4/m...
chrome/browser/resources/ntp4/most_visited_page.css:65: background-size: 16px;
On 2011/08/29 21:28:53, Evan Stade wrote:
> I think you can do background-size 100% maybe

Wouldn't that display the globe icon at 32x32?  In the meeting we decided to
contraint to 16px

http://codereview.chromium.org/7794005/diff/1/chrome/browser/resources/ntp4/m...
chrome/browser/resources/ntp4/most_visited_page.css:90: padding-top: 8px;
On 2011/08/29 21:28:53, Evan Stade wrote:
> just add this to line height. And then you'll need to update the JS
> heightForWidth function.

line-height doesn't quite work because the text will be aligned to the vertical
center.  We'd have to add 16 to the line-height in order to add 8px to the
spacing.  However, that would give us unwanted padding on the bottom.

I've fixed the implementaiton for heightForWidth and added a similar function
for bookmarks.

[Evan Stade, 2011-08-29 23:06:45]

http://codereview.chromium.org/7794005/diff/1/chrome/browser/resources/ntp4/m...
File chrome/browser/resources/ntp4/most_visited_page.css (right):

http://codereview.chromium.org/7794005/diff/1/chrome/browser/resources/ntp4/m...
chrome/browser/resources/ntp4/most_visited_page.css:65: background-size: 16px;
On 2011/08/29 22:56:52, csilv wrote:
> On 2011/08/29 21:28:53, Evan Stade wrote:
> > I think you can do background-size 100% maybe

or just leave it off

> 
> Wouldn't that display the globe icon at 32x32?  In the meeting we decided to
> contraint to 16px

we want to request the 16x16 globe icon anyway so we don't have to scale

http://codereview.chromium.org/7794005/diff/1/chrome/browser/resources/ntp4/m...
chrome/browser/resources/ntp4/most_visited_page.css:90: padding-top: 8px;
On 2011/08/29 22:56:52, csilv wrote:
> On 2011/08/29 21:28:53, Evan Stade wrote:
> > just add this to line height. And then you'll need to update the JS
> > heightForWidth function.
> 
> line-height doesn't quite work because the text will be aligned to the
vertical
> center.  We'd have to add 16 to the line-height in order to add 8px to the
> spacing.  However, that would give us unwanted padding on the bottom.
> 
> I've fixed the implementaiton for heightForWidth and added a similar function
> for bookmarks.

if that's the case then I'm sure we can either bottom-align the text or we
should remove the line-height property altogether and adjust padding-top as
appropriate.

http://codereview.chromium.org/7794005/diff/4002/chrome/browser/resources/ntp...
File chrome/browser/resources/ntp4/bookmarks_page.js (right):

http://codereview.chromium.org/7794005/diff/4002/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/bookmarks_page.js:292: bookmarksHeightForWidth:
bookmarksHeightForWidth,
huh? shouldn't this be heightForWidth?

[csilv, 2011-08-30 00:45:28]

http://codereview.chromium.org/7794005/diff/1/chrome/browser/resources/ntp4/m...
File chrome/browser/resources/ntp4/most_visited_page.css (right):

http://codereview.chromium.org/7794005/diff/1/chrome/browser/resources/ntp4/m...
chrome/browser/resources/ntp4/most_visited_page.css:65: background-size: 16px;
On 2011/08/29 23:06:45, Evan Stade wrote:
> On 2011/08/29 22:56:52, csilv wrote:
> > On 2011/08/29 21:28:53, Evan Stade wrote:
> > > I think you can do background-size 100% maybe
> 
> or just leave it off
> 
> > 
> > Wouldn't that display the globe icon at 32x32?  In the meeting we decided to
> > contraint to 16px
> 
> we want to request the 16x16 globe icon anyway so we don't have to scale

Done.

http://codereview.chromium.org/7794005/diff/1/chrome/browser/resources/ntp4/m...
chrome/browser/resources/ntp4/most_visited_page.css:90: padding-top: 8px;
There's not a general CSS way to make text align to the bottom putting the text
into a sub-element.  I've taken your advice and got rid of the line-height
attribute since it's not really necessary.

http://codereview.chromium.org/7794005/diff/4002/chrome/browser/resources/ntp...
File chrome/browser/resources/ntp4/bookmarks_page.js (right):

http://codereview.chromium.org/7794005/diff/4002/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/bookmarks_page.js:292: bookmarksHeightForWidth:
bookmarksHeightForWidth,
On 2011/08/29 23:06:46, Evan Stade wrote:
> huh? shouldn't this be heightForWidth?

Done.

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

http://codereview.chromium.org/7794005/diff/5/chrome/browser/resources/ntp4/b...
File chrome/browser/resources/ntp4/bookmarks_page.css (right):

http://codereview.chromium.org/7794005/diff/5/chrome/browser/resources/ntp4/b...
chrome/browser/resources/ntp4/bookmarks_page.css:64: text-overflow: ellipsis;
also get rid of underline?

http://codereview.chromium.org/7794005/diff/5/chrome/browser/resources/ntp4/b...
File chrome/browser/resources/ntp4/bookmarks_page.js (left):

http://codereview.chromium.org/7794005/diff/5/chrome/browser/resources/ntp4/b...
chrome/browser/resources/ntp4/bookmarks_page.js:66: // TODO(csilv): We need a
large (32px) icon for this URL.
we do still need a large icon for this

http://codereview.chromium.org/7794005/diff/5/chrome/browser/resources/ntp4/b...
File chrome/browser/resources/ntp4/bookmarks_page.js (right):

http://codereview.chromium.org/7794005/diff/5/chrome/browser/resources/ntp4/b...
chrome/browser/resources/ntp4/bookmarks_page.js:184: return (width - 2) * 72 /
96 + 2 + 31;
I imagine the 96x72 ratio applies to the whole thing including the border, i.e.
you don't need the 2s here

http://codereview.chromium.org/7794005/diff/5/chrome/browser/resources/ntp4/m...
File chrome/browser/resources/ntp4/most_visited_page.css (right):

http://codereview.chromium.org/7794005/diff/5/chrome/browser/resources/ntp4/m...
chrome/browser/resources/ntp4/most_visited_page.css:76: /* Matches height of
title.  */
comment not quite accurate any more, maybe make the height greater and use
margin instead of padding below?

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

http://codereview.chromium.org/7794005/diff/5/chrome/browser/resources/ntp4/b...
File chrome/browser/resources/ntp4/bookmarks_page.css (right):

http://codereview.chromium.org/7794005/diff/5/chrome/browser/resources/ntp4/b...
chrome/browser/resources/ntp4/bookmarks_page.css:64: text-overflow: ellipsis;
On 2011/08/30 01:06:24, Evan Stade wrote:
> also get rid of underline?

That's already there (text-decoration: none).

http://codereview.chromium.org/7794005/diff/5/chrome/browser/resources/ntp4/b...
File chrome/browser/resources/ntp4/bookmarks_page.js (left):

http://codereview.chromium.org/7794005/diff/5/chrome/browser/resources/ntp4/b...
chrome/browser/resources/ntp4/bookmarks_page.js:66: // TODO(csilv): We need a
large (32px) icon for this URL.
On 2011/08/30 01:06:24, Evan Stade wrote:
> we do still need a large icon for this

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

http://codereview.chromium.org/7794005/diff/5/chrome/browser/resources/ntp4/m...
File chrome/browser/resources/ntp4/most_visited_page.css (right):

http://codereview.chromium.org/7794005/diff/5/chrome/browser/resources/ntp4/m...
chrome/browser/resources/ntp4/most_visited_page.css:76: /* Matches height of
title.  */
On 2011/08/30 01:06:24, Evan Stade wrote:
> comment not quite accurate any more, maybe make the height greater and use
> margin instead of padding below?

Done.

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

http://codereview.chromium.org/7794005/diff/5/chrome/browser/resources/ntp4/b...
File chrome/browser/resources/ntp4/bookmarks_page.js (left):

http://codereview.chromium.org/7794005/diff/5/chrome/browser/resources/ntp4/b...
chrome/browser/resources/ntp4/bookmarks_page.js:66: // TODO(csilv): We need a
large (32px) icon for this URL.
On 2011/08/30 01:29:04, csilv wrote:
> On 2011/08/30 01:06:24, Evan Stade wrote:
> > we do still need a large icon for this
> 
> Yes, but that's unrelated to fetching use of the favicon protocol, so that's
why
> this TODO is obsolete.

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]

http://codereview.chromium.org/7794005/diff/5/chrome/browser/resources/ntp4/b...
File chrome/browser/resources/ntp4/bookmarks_page.js (left):

http://codereview.chromium.org/7794005/diff/5/chrome/browser/resources/ntp4/b...
chrome/browser/resources/ntp4/bookmarks_page.js:66: // TODO(csilv): We need a
large (32px) icon for this URL.
On 2011/08/30 01:56:29, Evan Stade wrote:
> On 2011/08/30 01:29:04, csilv wrote:
> > On 2011/08/30 01:06:24, Evan Stade wrote:
> > > we do still need a large icon for this
> > 
> > Yes, but that's unrelated to fetching use of the favicon protocol, so that's
> why
> > this TODO is obsolete.
> 
> what? this uses the chrome://theme protocol not the favicon protocol. We still
> want a larger icon (bigger than 32x32)

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.

[Evan Stade, 2011-08-30 19:28:43]

ok lgtm