Mac tabpose: Add favicons and titles.

Mac tabpose: Add favicons and titles.

BUG=50307
TEST=Activate tabpose. The (white) thumbnails should have favicons and titles drawn below them.

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

[Nico, 2010-08-10 21:04:51]

Title & favicon could animate in nicer (ease-out opacity from transparent to
visible, don't animate height or width, keep favicon attached to left edge of
tab thumb and title attached to right edge of tab thumb while the animation
plays), but it's good enough for now.

The looks of this are cole-approved.

[Robert Sesek, 2010-08-10 21:28:12]

http://codereview.chromium.org/3159001/diff/10001/11002
File chrome/browser/cocoa/tabpose_window.mm (right):

http://codereview.chromium.org/3159001/diff/10001/11002#newcode125
chrome/browser/cocoa/tabpose_window.mm:125: // 1. View code queries desired font
size by calling |title_font_size()|
"View code" is repetitive.

http://codereview.chromium.org/3159001/diff/10001/11002#newcode150
chrome/browser/cocoa/tabpose_window.mm:150: TabContents* contents_;
weak?  What happens if you close a tab while in Tabpose?

http://codereview.chromium.org/3159001/diff/10001/11002#newcode160
chrome/browser/cocoa/tabpose_window.mm:160: SkBitmap Tile::favicon() const {
const SkBitmap?

http://codereview.chromium.org/3159001/diff/10001/11002#newcode169
chrome/browser/cocoa/tabpose_window.mm:169: void Tile::set_font_metrics(CGFloat
ascender, CGFloat descender) {
Copy the comment above this function to here.

http://codereview.chromium.org/3159001/diff/10001/11002#newcode197
chrome/browser/cocoa/tabpose_window.mm:197: Tile& tile_at(int index) { return
*tiles_[index]; }
tile_at(int index) const

http://codereview.chromium.org/3159001/diff/10001/11002#newcode212
chrome/browser/cocoa/tabpose_window.mm:212: tiles_[i]->contents_ =
source_model->GetTabContentsAt(i);
It's ugly that the TileSet reaches into the Tile.  Give the Tile class a
constructor?

http://codereview.chromium.org/3159001/diff/10001/11002#newcode328
chrome/browser/cocoa/tabpose_window.mm:328: // for now. The final position is
computed later by
Why can't the font metrics be computed here?

http://codereview.chromium.org/3159001/diff/10001/11002#newcode515
chrome/browser/cocoa/tabpose_window.mm:515: scoped_nsobject<NSImage>
defaultFavIcon(
Is scoping this necessary?  Doesn't the |rb| return it autoreleased?

http://codereview.chromium.org/3159001/diff/10001/11002#newcode665
chrome/browser/cocoa/tabpose_window.mm:665: 
Remove extra blank line

[Nico, 2010-08-10 21:37:00]

thanks!

http://codereview.chromium.org/3159001/diff/10001/11002
File chrome/browser/cocoa/tabpose_window.mm (right):

http://codereview.chromium.org/3159001/diff/10001/11002#newcode125
chrome/browser/cocoa/tabpose_window.mm:125: // 1. View code queries desired font
size by calling |title_font_size()|
On 2010/08/10 21:28:12, rsesek wrote:
> "View code" is repetitive.

Done.

http://codereview.chromium.org/3159001/diff/10001/11002#newcode150
chrome/browser/cocoa/tabpose_window.mm:150: TabContents* contents_;
On 2010/08/10 21:28:12, rsesek wrote:
> weak?  What happens if you close a tab while in Tabpose?

Weak (added note).

Short answer: You can't.
Longer answer: There's a TODO(thakis) below to add a TabStripModelObserver,
which will then remove the tile belonging to that contents

http://codereview.chromium.org/3159001/diff/10001/11002#newcode160
chrome/browser/cocoa/tabpose_window.mm:160: SkBitmap Tile::favicon() const {
On 2010/08/10 21:28:12, rsesek wrote:
> const SkBitmap?

You mean "const SkBitmap&"? SkBitmaps are effective to copy, so that's not
necessary.

http://codereview.chromium.org/3159001/diff/10001/11002#newcode169
chrome/browser/cocoa/tabpose_window.mm:169: void Tile::set_font_metrics(CGFloat
ascender, CGFloat descender) {
On 2010/08/10 21:28:12, rsesek wrote:
> Copy the comment above this function to here.

Done.

http://codereview.chromium.org/3159001/diff/10001/11002#newcode197
chrome/browser/cocoa/tabpose_window.mm:197: Tile& tile_at(int index) { return
*tiles_[index]; }
On 2010/08/10 21:28:12, rsesek wrote:
> tile_at(int index) const

No, it hands out a non-const reference. I know it's legal, but I think doing
this is wrong.

http://codereview.chromium.org/3159001/diff/10001/11002#newcode212
chrome/browser/cocoa/tabpose_window.mm:212: tiles_[i]->contents_ =
source_model->GetTabContentsAt(i);
On 2010/08/10 21:28:12, rsesek wrote:
> It's ugly that the TileSet reaches into the Tile.  Give the Tile class a
> constructor?

It reaches in there all the time in Layout(). Once I know what Tile is
responsible for (soon, once the thumbnail code is in probably), I'll fix all of
that, but not now.

http://codereview.chromium.org/3159001/diff/10001/11002#newcode328
chrome/browser/cocoa/tabpose_window.mm:328: // for now. The final position is
computed later by
On 2010/08/10 21:28:12, rsesek wrote:
> Why can't the font metrics be computed here?

Because I don't have a font object to give me ascender and descender (I don't
want to put stuff that's too osx-specific into Tile & TileSet, and gfx::Font
can't do what I want).

http://codereview.chromium.org/3159001/diff/10001/11002#newcode515
chrome/browser/cocoa/tabpose_window.mm:515: scoped_nsobject<NSImage>
defaultFavIcon(
On 2010/08/10 21:28:12, rsesek wrote:
> Is scoping this necessary?  Doesn't the |rb| return it autoreleased?

Done.

[Robert Sesek, 2010-08-10 22:19:54]

LGETM

http://codereview.chromium.org/3159001/diff/10001/11002
File chrome/browser/cocoa/tabpose_window.mm (right):

http://codereview.chromium.org/3159001/diff/10001/11002#newcode160
chrome/browser/cocoa/tabpose_window.mm:160: SkBitmap Tile::favicon() const {
On 2010/08/10 21:37:00, Nico wrote:
> On 2010/08/10 21:28:12, rsesek wrote:
> > const SkBitmap?
> 
> You mean "const SkBitmap&"? SkBitmaps are effective to copy, so that's not
> necessary.

Yes, I did mean &.  It's fine to copy; we just have some code that does one and
other code that does the other.

[viettrungluu, 2010-08-11 01:43:16]

On 2010/08/10 22:19:54, rsesek wrote:
> LGETM

I see that I'm being a bad influence. You should be positive, and say "Looks
Great To Me!".

[Nico, 2010-08-11 01:45:06]

On Tue, Aug 10, 2010 at 6:43 PM,  <viettrungluu@chromium.org> wrote:
> On 2010/08/10 22:19:54, rsesek wrote:
>>
>> LGETM
>
> I see that I'm being a bad influence. You should be positive, and say "Looks
> Great To Me!".

(Note how grandmaster Trung managed to be both condescending to you
_and_ not saying anything nice about my patch!)

>
> http://codereview.chromium.org/3159001/show
>