NTP5: Implementing new specification for favicons of Most Visited Pages.

NTP5: Implementing new specification for favicons of Most Visited Pages.


BUG=


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=169194

[pedro (no code reviews), 2012-11-20 02:08:05]

Hey Evan, here's a small CL!

[Evan Stade, 2012-11-20 20:05:00]

https://codereview.chromium.org/11416091/diff/1/chrome/browser/resources/ntp_...
File chrome/browser/resources/ntp_search/thumbnail_page.css (right):

https://codereview.chromium.org/11416091/diff/1/chrome/browser/resources/ntp_...
chrome/browser/resources/ntp_search/thumbnail_page.css:87: left: 46px;
RTL?

https://codereview.chromium.org/11416091/diff/1/chrome/browser/resources/ntp_...
File chrome/browser/resources/ntp_search/thumbnail_page.js (right):

https://codereview.chromium.org/11416091/diff/1/chrome/browser/resources/ntp_...
chrome/browser/resources/ntp_search/thumbnail_page.js:89:
this.ownerDocument.createElement('div');
var favicon = this.querySelector('.thumbnail-favicon') ||
              this.ownerDocument.createElement('div');

[pedro (no code reviews), 2012-11-20 20:14:00]

https://codereview.chromium.org/11416091/diff/1/chrome/browser/resources/ntp_...
File chrome/browser/resources/ntp_search/thumbnail_page.css (right):

https://codereview.chromium.org/11416091/diff/1/chrome/browser/resources/ntp_...
chrome/browser/resources/ntp_search/thumbnail_page.css:87: left: 46px;
On 2012/11/20 20:05:00, Evan Stade wrote:
> RTL?

This is being centered, so RTL is not an issue here.

https://codereview.chromium.org/11416091/diff/1/chrome/browser/resources/ntp_...
File chrome/browser/resources/ntp_search/thumbnail_page.js (right):

https://codereview.chromium.org/11416091/diff/1/chrome/browser/resources/ntp_...
chrome/browser/resources/ntp_search/thumbnail_page.js:89:
this.ownerDocument.createElement('div');
On 2012/11/20 20:05:00, Evan Stade wrote:
> var favicon = this.querySelector('.thumbnail-favicon') ||
>               this.ownerDocument.createElement('div');

Done.

[Evan Stade, 2012-11-20 20:15:14]

https://codereview.chromium.org/11416091/diff/1/chrome/browser/resources/ntp_...
File chrome/browser/resources/ntp_search/thumbnail_page.css (right):

https://codereview.chromium.org/11416091/diff/1/chrome/browser/resources/ntp_...
chrome/browser/resources/ntp_search/thumbnail_page.css:87: left: 46px;
On 2012/11/20 20:14:00, pedrosimonetti wrote:
> On 2012/11/20 20:05:00, Evan Stade wrote:
> > RTL?
> 
> This is being centered, so RTL is not an issue here.

this is a fragile way to center something.

[pedro (no code reviews), 2012-11-20 21:38:45]

https://codereview.chromium.org/11416091/diff/1/chrome/browser/resources/ntp_...
File chrome/browser/resources/ntp_search/thumbnail_page.css (right):

https://codereview.chromium.org/11416091/diff/1/chrome/browser/resources/ntp_...
chrome/browser/resources/ntp_search/thumbnail_page.css:87: left: 46px;
On 2012/11/20 20:15:14, Evan Stade wrote:
> On 2012/11/20 20:14:00, pedrosimonetti wrote:
> > On 2012/11/20 20:05:00, Evan Stade wrote:
> > > RTL?
> > 
> > This is being centered, so RTL is not an issue here.
> 
> this is a fragile way to center something.

The favicon should be on top of the thumbnail, and half of it should "bleed off"
the thumbnail, which means we must use absolute position. Because of that, we
can't use "margin: 0 auto" to center the favicon, that is, we must position the
favicon manually using top and left properties.

[Evan Stade, 2012-11-20 21:44:18]

On 2012/11/20 21:38:45, pedrosimonetti wrote:
>
https://codereview.chromium.org/11416091/diff/1/chrome/browser/resources/ntp_...
> File chrome/browser/resources/ntp_search/thumbnail_page.css (right):
> 
>
https://codereview.chromium.org/11416091/diff/1/chrome/browser/resources/ntp_...
> chrome/browser/resources/ntp_search/thumbnail_page.css:87: left: 46px;
> On 2012/11/20 20:15:14, Evan Stade wrote:
> > On 2012/11/20 20:14:00, pedrosimonetti wrote:
> > > On 2012/11/20 20:05:00, Evan Stade wrote:
> > > > RTL?
> > > 
> > > This is being centered, so RTL is not an issue here.
> > 
> > this is a fragile way to center something.
> 
> The favicon should be on top of the thumbnail, and half of it should "bleed
off"
> the thumbnail, which means we must use absolute position. Because of that, we
> can't use "margin: 0 auto" to center the favicon, that is, we must position
the
> favicon manually using top and left properties.

you mean like this? http://jsfiddle.net/pzkHp/

[pedro (no code reviews), 2012-11-20 23:46:02]

On 2012/11/20 21:44:18, Evan Stade wrote:
> On 2012/11/20 21:38:45, pedrosimonetti wrote:
> >
>
https://codereview.chromium.org/11416091/diff/1/chrome/browser/resources/ntp_...
> > File chrome/browser/resources/ntp_search/thumbnail_page.css (right):
> > 
> >
>
https://codereview.chromium.org/11416091/diff/1/chrome/browser/resources/ntp_...
> > chrome/browser/resources/ntp_search/thumbnail_page.css:87: left: 46px;
> > On 2012/11/20 20:15:14, Evan Stade wrote:
> > > On 2012/11/20 20:14:00, pedrosimonetti wrote:
> > > > On 2012/11/20 20:05:00, Evan Stade wrote:
> > > > > RTL?
> > > > 
> > > > This is being centered, so RTL is not an issue here.
> > > 
> > > this is a fragile way to center something.
> > 
> > The favicon should be on top of the thumbnail, and half of it should "bleed
> off"
> > the thumbnail, which means we must use absolute position. Because of that,
we
> > can't use "margin: 0 auto" to center the favicon, that is, we must position
> the
> > favicon manually using top and left properties.
> 
> you mean like this? http://jsfiddle.net/pzkHp/

You're right. I thought I had to use absolute position. I just changed the code
and now I'm using relative position with "margin: 0 auto".

I also noticed I have to increase the threshold for displaying the Bottom Panel
by 8 pixels, since the thumbnail titles have moved down by 8 pixels.

[Evan Stade, 2012-11-20 23:52:05]

lgtm with final nit addressed

https://codereview.chromium.org/11416091/diff/8001/chrome/browser/resources/n...
File chrome/browser/resources/ntp_search/thumbnail_page.css (right):

https://codereview.chromium.org/11416091/diff/8001/chrome/browser/resources/n...
chrome/browser/resources/ntp_search/thumbnail_page.css:78: bottom: 25px;
you specify this twice. Better to doing so rather than relying on rule order.

[Evan Stade, 2012-11-21 00:02:37]

"to avoid doing so"

[pedro (no code reviews), 2012-11-21 00:14:03]

Thanks!

https://codereview.chromium.org/11416091/diff/8001/chrome/browser/resources/n...
File chrome/browser/resources/ntp_search/thumbnail_page.css (right):

https://codereview.chromium.org/11416091/diff/8001/chrome/browser/resources/n...
chrome/browser/resources/ntp_search/thumbnail_page.css:78: bottom: 25px;
On 2012/11/20 23:52:05, Evan Stade wrote:
> you specify this twice. Better to doing so rather than relying on rule order.

Done.

[pedro (no code reviews), 2012-11-21 00:16:07]

Hey Holloway, this is another one. Could you please take a look at
chrome/browser/ui/search/search_ui.cc?

I'm cc'ing Kuan since I'm touching the BMB code.

[dhollowa, 2012-11-21 22:42:10]

lgtm

[pedro (no code reviews), 2012-11-22 00:17:37]

Thanks!

[commit-bot: I haz the power, 2012-11-22 00:17:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pedrosimonetti@chromium.org/11416091/1...

[commit-bot: I haz the power, 2012-11-22 02:14:13]

Change committed as 169194