Mac tabpose: Add thumbnails

Mac tabpose: Add thumbnails

Most things actually work.

Missing from this CL:
* Reloading thumbnails for tabs that change. This is required to show non-white thumbnails for thumbnails that are still waiting on the net when tabpose is opened.
* Showing infobars / bookmark bar in the thumbnail
* Showing accelerated surfaces (youtube videos on 10.6, compositor on 10.6)

BUG=50307
TEST=Enable tabpose. Should see thumbnails for all tabs (some loaded after a delay). Thumbnails should animate in correctly even if a tab has info bars, a detached NTP, or docked devtools. Tabs that haven't been frontmost since the window was last resized should look good. Opening many tabs and then immediately jumping into expose shouldn't crash.

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

[Nico, 2010-08-30 08:18:52]

For some reason, this was super-annoying to write. Not sure why. Seems like a
good chunk to get reviewed and checked in.

[viettrungluu, 2010-08-30 15:12:51]

Some preliminary comments/questions/nits....

http://codereview.chromium.org/3163003/diff/34004/47004
File chrome/browser/cocoa/tabpose_window.mm (right):

http://codereview.chromium.org/3163003/diff/34004/47004#newcode76
chrome/browser/cocoa/tabpose_window.mm:76: // renderer process, it' stored in
|thumbnail_|.
s/'/'s/

http://codereview.chromium.org/3163003/diff/34004/47004#newcode99
chrome/browser/cocoa/tabpose_window.mm:99: private:
insert blank line above

http://codereview.chromium.org/3163003/diff/34004/47004#newcode101
chrome/browser/cocoa/tabpose_window.mm:101: ~ThumbnailLoader() {
ResetPaintingObserver(); }
Might as well split it across multiple lines.

http://codereview.chromium.org/3163003/diff/34004/47004#newcode110
chrome/browser/cocoa/tabpose_window.mm:110: if (rwh_->painting_observer() !=
NULL) {
!= NULL is redundant

http://codereview.chromium.org/3163003/diff/34004/47004#newcode133
chrome/browser/cocoa/tabpose_window.mm:133: // clicks at in the end. But we
don't don't which thumbnail that will be, so
s/at/on/

http://codereview.chromium.org/3163003/diff/34004/47004#newcode134
chrome/browser/cocoa/tabpose_window.mm:134: // keep it simple and request full
thumbnails for everything.
This sounds like it'll consume a tonne of memory in the case that the user has
30+ tabs (which is when one needs tabposé most), is that right?

Couldn't you just fake it by blowing up the selected thumbnail at the end? (It
might be a bit ugly, but it'd bound memory use.)

http://codereview.chromium.org/3163003/diff/34004/47004#newcode145
chrome/browser/cocoa/tabpose_window.mm:145: /*prefer_backing_store=*/false,
Ouch, and we might not be able to use existing backing stores....

http://codereview.chromium.org/3163003/diff/34004/47004#newcode164
chrome/browser/cocoa/tabpose_window.mm:164: contents_ = contents;
Should you also have a CHECK() here?

http://codereview.chromium.org/3163003/diff/34004/47004#newcode175
chrome/browser/cocoa/tabpose_window.mm:175: -
(void)drawInContext:(CGContextRef)context {
This method should be split up into smaller chunks.

http://codereview.chromium.org/3163003/diff/34004/47004#newcode192
chrome/browser/cocoa/tabpose_window.mm:192: // Medium term, we want to show
thumbs of the actual info bar views, which
s/Medium term/Medium-term/

Also, this sounds like a TODO to me.

http://codereview.chromium.org/3163003/diff/34004/47004#newcode194
chrome/browser/cocoa/tabpose_window.mm:194: // the height from that controller.
Until then, hardcode :-/
s/hardcode/&./

http://codereview.chromium.org/3163003/diff/34004/47004#newcode198
chrome/browser/cocoa/tabpose_window.mm:198: bool always_show_bookmark_bar =
Doing all this yourself is somewhat painful. :(

http://codereview.chromium.org/3163003/diff/34004/47004#newcode272
chrome/browser/cocoa/tabpose_window.mm:272: // To fill with bg color.
s/To f/F/

http://codereview.chromium.org/3163003/diff/34004/47004#newcode277
chrome/browser/cocoa/tabpose_window.mm:277: CGContextDrawImage(context,
destRect, thumbnail_.get());
Can you combine drawing using a thumbnail and using a backing store?

http://codereview.chromium.org/3163003/diff/34004/47004#newcode347
chrome/browser/cocoa/tabpose_window.mm:347: Tile() {}
s/^/ /

http://codereview.chromium.org/3163003/diff/34004/47004#newcode376
chrome/browser/cocoa/tabpose_window.mm:376: private:
Insert blank line above.

http://codereview.chromium.org/3163003/diff/34004/47004#newcode447
chrome/browser/cocoa/tabpose_window.mm:447: void MoveTileFromTo(int from, int
to);
Should that be from_index, to_index, respectively?

http://codereview.chromium.org/3163003/diff/34004/47004#newcode448
chrome/browser/cocoa/tabpose_window.mm:448: private:
Insert blank line above.

http://codereview.chromium.org/3163003/diff/34004/47004#newcode630
chrome/browser/cocoa/tabpose_window.mm:630: void TileSet::MoveTileFromTo(int
from, int to) {
Some more comments would be nice. What is this used for?

http://codereview.chromium.org/3163003/diff/34004/47004#newcode711
chrome/browser/cocoa/tabpose_window.mm:711: 
---- stopped here

[Nico, 2010-08-30 15:35:25]

Answered all the non-nits comments.

Will address the rest soon.

http://codereview.chromium.org/3163003/diff/34004/47004
File chrome/browser/cocoa/tabpose_window.mm (right):

http://codereview.chromium.org/3163003/diff/34004/47004#newcode134
chrome/browser/cocoa/tabpose_window.mm:134: // keep it simple and request full
thumbnails for everything.
On 2010/08/30 15:12:51, viettrungluu wrote:
> This sounds like it'll consume a tonne of memory in the case that the user has
> 30+ tabs (which is when one needs tabposé most), is that right?

It'd consume a ton of memory in that case, yes. I don't think that's when one
needs this most, since most people don't have 30 tabs.

> Couldn't you just fake it by blowing up the selected thumbnail at the end? (It
> might be a bit ugly, but it'd bound memory use.)

I tried that, it looks really bad.

I also tried selecting the tab early enough so that we can use the tab's backing
store when zooming in, but in most cases the backing store wasn't ready fast
enough.

I could do something like "if tab count > 12 then make thumbnails smaller, then
it looks nice for most people and doesn't need all that much memory for the
occasional weirdo. I'd prefer to do that in a followup CL (that I intend to
write tonight).

http://codereview.chromium.org/3163003/diff/34004/47004#newcode145
chrome/browser/cocoa/tabpose_window.mm:145: /*prefer_backing_store=*/false,
On 2010/08/30 15:12:51, viettrungluu wrote:
> Ouch, and we might not be able to use existing backing stores....

We are, I do that manually below. The reason for that is that ThumbnailGenerator
does unnecessary memcpys on Mac (converts stuff to an SkBitmap, paints that to
somewhere else), while I can just draw from the CGLayerRef directly if I do the
painting myself.

http://codereview.chromium.org/3163003/diff/34004/47004#newcode192
chrome/browser/cocoa/tabpose_window.mm:192: // Medium term, we want to show
thumbs of the actual info bar views, which
On 2010/08/30 15:12:51, viettrungluu wrote:
> s/Medium term/Medium-term/
> 
> Also, this sounds like a TODO to me.

There's one further below, where I envision that drawing to happen.

http://codereview.chromium.org/3163003/diff/34004/47004#newcode198
chrome/browser/cocoa/tabpose_window.mm:198: bool always_show_bookmark_bar =
On 2010/08/30 15:12:51, viettrungluu wrote:
> Doing all this yourself is somewhat painful. :(

Yes, hence my consulting you 2 days ago. I think it's the best alternative we
have at this point.

http://codereview.chromium.org/3163003/diff/34004/47004#newcode277
chrome/browser/cocoa/tabpose_window.mm:277: CGContextDrawImage(context,
destRect, thumbnail_.get());
On 2010/08/30 15:12:51, viettrungluu wrote:
> Can you combine drawing using a thumbnail and using a backing store?

Huh?

[viettrungluu, 2010-08-30 18:23:27]

Mostly looks good, with some nits. Still have to review the unit test.

http://codereview.chromium.org/3163003/diff/34004/47004
File chrome/browser/cocoa/tabpose_window.mm (right):

http://codereview.chromium.org/3163003/diff/34004/47004#newcode154
chrome/browser/cocoa/tabpose_window.mm:154: -
(id)initWithTabContents:(TabContents*)contents fullSize:(NSSize)fullSize {
nit: Blank line above.

http://codereview.chromium.org/3163003/diff/34004/47004#newcode277
chrome/browser/cocoa/tabpose_window.mm:277: CGContextDrawImage(context,
destRect, thumbnail_.get());
On 2010/08/30 15:35:26, Nico wrote:
> On 2010/08/30 15:12:51, viettrungluu wrote:
> > Can you combine drawing using a thumbnail and using a backing store?
> 
> Huh?

Just the call to CGContextDrawImage(). I have a slight preference for all the
drawing to happen in one spot. (Or in very nearby spots. Not separated by the
thumbnail request stuff.)

http://codereview.chromium.org/3163003/diff/34004/47004#newcode279
chrome/browser/cocoa/tabpose_window.mm:279: @end
nit: blank line above

http://codereview.chromium.org/3163003/diff/34004/47004#newcode708
chrome/browser/cocoa/tabpose_window.mm:708: -
(void)selectTileAtIndex:(int)newIndex;
Consider merging this and the NoAnimation variant.

http://codereview.chromium.org/3163003/diff/34004/47004#newcode709
chrome/browser/cocoa/tabpose_window.mm:709: -
(void)selectTileAtIndexNoAnimation:(int)newIndex;
This is a pretty bad name for a method.

http://codereview.chromium.org/3163003/diff/34004/47004#newcode800
chrome/browser/cocoa/tabpose_window.mm:800: NSImage* ns_favicon =
gfx::SkBitmapToNSImage(tile.favicon());
Obj-C naming convention applies here

http://codereview.chromium.org/3163003/diff/34004/47004#newcode804
chrome/browser/cocoa/tabpose_window.mm:804: NSImage* defaultFavIcon =
ResourceBundle::GetSharedInstance().GetNSImageNamed(
80 chars

http://codereview.chromium.org/3163003/diff/34004/47004#newcode930
chrome/browser/cocoa/tabpose_window.mm:930: #if 0
Hmmm. You should probably not check this in.

http://codereview.chromium.org/3163003/diff/34004/47004#newcode1000
chrome/browser/cocoa/tabpose_window.mm:1000: // Move the selected layer on top
of all other layers.
Consider breaking this file up.

http://codereview.chromium.org/3163003/diff/34004/47004#newcode1022
chrome/browser/cocoa/tabpose_window.mm:1022: // the initial too)
Needs punctuation at the end. And I don't really understand the parenthetical
comment.

http://codereview.chromium.org/3163003/diff/34004/47004#newcode1099
chrome/browser/cocoa/tabpose_window.mm:1099: showZoom:NO
s/^/  /

http://codereview.chromium.org/3163003/diff/34004/47004#newcode1100
chrome/browser/cocoa/tabpose_window.mm:1100: slomo:NO
"

http://codereview.chromium.org/3163003/diff/34004/47004#newcode1101
chrome/browser/cocoa/tabpose_window.mm:1101: animationDelegate:nil];
"

http://codereview.chromium.org/3163003/diff/34004/47004#newcode1104
chrome/browser/cocoa/tabpose_window.mm:1104: DCHECK_EQ(tabStripModel_->count(),
(int)[allThumbnailLayers_ count]);
Apparently, we frown on C-style casts.

http://codereview.chromium.org/3163003/diff/34004/47004#newcode1169
chrome/browser/cocoa/tabpose_window.mm:1169: scoped_nsobject<CALayer>
thumbLayer(
Could you add a comment saying what this does?

[Nico, 2010-08-30 18:31:03]

Let me put you my thanks.

http://codereview.chromium.org/3163003/diff/34004/47004
File chrome/browser/cocoa/tabpose_window.mm (right):

http://codereview.chromium.org/3163003/diff/34004/47004#newcode76
chrome/browser/cocoa/tabpose_window.mm:76: // renderer process, it' stored in
|thumbnail_|.
On 2010/08/30 15:12:51, viettrungluu wrote:
> s/'/'s/

Done.

http://codereview.chromium.org/3163003/diff/34004/47004#newcode99
chrome/browser/cocoa/tabpose_window.mm:99: private:
On 2010/08/30 15:12:51, viettrungluu wrote:
> insert blank line above

Done.

http://codereview.chromium.org/3163003/diff/34004/47004#newcode101
chrome/browser/cocoa/tabpose_window.mm:101: ~ThumbnailLoader() {
ResetPaintingObserver(); }
On 2010/08/30 15:12:51, viettrungluu wrote:
> Might as well split it across multiple lines.

Done.

http://codereview.chromium.org/3163003/diff/34004/47004#newcode110
chrome/browser/cocoa/tabpose_window.mm:110: if (rwh_->painting_observer() !=
NULL) {
On 2010/08/30 15:12:51, viettrungluu wrote:
> != NULL is redundant

Doesn't hurt though. Couldn't find anything in the style guide.

http://codereview.chromium.org/3163003/diff/34004/47004#newcode133
chrome/browser/cocoa/tabpose_window.mm:133: // clicks at in the end. But we
don't don't which thumbnail that will be, so
On 2010/08/30 15:12:51, viettrungluu wrote:
> s/at/on/

Done.

http://codereview.chromium.org/3163003/diff/34004/47004#newcode134
chrome/browser/cocoa/tabpose_window.mm:134: // keep it simple and request full
thumbnails for everything.
On 2010/08/30 15:35:26, Nico wrote:
> On 2010/08/30 15:12:51, viettrungluu wrote:
> > This sounds like it'll consume a tonne of memory in the case that the user
has
> > 30+ tabs (which is when one needs tabposé most), is that right?
> 
> It'd consume a ton of memory in that case, yes. I don't think that's when one
> needs this most, since most people don't have 30 tabs.
> 
> > Couldn't you just fake it by blowing up the selected thumbnail at the end?
(It
> > might be a bit ugly, but it'd bound memory use.)
> 
> I tried that, it looks really bad.
> 
> I also tried selecting the tab early enough so that we can use the tab's
backing
> store when zooming in, but in most cases the backing store wasn't ready fast
> enough.
> 
> I could do something like "if tab count > 12 then make thumbnails smaller,
then
> it looks nice for most people and doesn't need all that much memory for the
> occasional weirdo. I'd prefer to do that in a followup CL (that I intend to
> write tonight).

Added a TODO, real fix will be out hopefully later today.

http://codereview.chromium.org/3163003/diff/34004/47004#newcode164
chrome/browser/cocoa/tabpose_window.mm:164: contents_ = contents;
On 2010/08/30 15:12:51, viettrungluu wrote:
> Should you also have a CHECK() here?

Shouldn't be necessary, drawInContext: will happily crash on its own

http://codereview.chromium.org/3163003/diff/34004/47004#newcode175
chrome/browser/cocoa/tabpose_window.mm:175: -
(void)drawInContext:(CGContextRef)context {
On 2010/08/30 15:12:51, viettrungluu wrote:
> This method should be split up into smaller chunks.

Done?

http://codereview.chromium.org/3163003/diff/34004/47004#newcode194
chrome/browser/cocoa/tabpose_window.mm:194: // the height from that controller.
Until then, hardcode :-/
On 2010/08/30 15:12:51, viettrungluu wrote:
> s/hardcode/&./

Done.

http://codereview.chromium.org/3163003/diff/34004/47004#newcode272
chrome/browser/cocoa/tabpose_window.mm:272: // To fill with bg color.
On 2010/08/30 15:12:51, viettrungluu wrote:
> s/To f/F/

Done.

http://codereview.chromium.org/3163003/diff/34004/47004#newcode347
chrome/browser/cocoa/tabpose_window.mm:347: Tile() {}
On 2010/08/30 15:12:51, viettrungluu wrote:
> s/^/ /

Done.

http://codereview.chromium.org/3163003/diff/34004/47004#newcode376
chrome/browser/cocoa/tabpose_window.mm:376: private:
On 2010/08/30 15:12:51, viettrungluu wrote:
> Insert blank line above.

Done.

http://codereview.chromium.org/3163003/diff/34004/47004#newcode447
chrome/browser/cocoa/tabpose_window.mm:447: void MoveTileFromTo(int from, int
to);
On 2010/08/30 15:12:51, viettrungluu wrote:
> Should that be from_index, to_index, respectively?

Done.

http://codereview.chromium.org/3163003/diff/34004/47004#newcode448
chrome/browser/cocoa/tabpose_window.mm:448: private:
On 2010/08/30 15:12:51, viettrungluu wrote:
> Insert blank line above.

Done.

http://codereview.chromium.org/3163003/diff/34004/47004#newcode630
chrome/browser/cocoa/tabpose_window.mm:630: void TileSet::MoveTileFromTo(int
from, int to) {
On 2010/08/30 15:12:51, viettrungluu wrote:
> Some more comments would be nice. What is this used for?

Done.

http://codereview.chromium.org/3163003/diff/34004/47004#newcode711
chrome/browser/cocoa/tabpose_window.mm:711: 
On 2010/08/30 15:12:51, viettrungluu wrote:
> ---- stopped here

I think the rest is mostly harmless, all the tricky bits were above this line.

[Nico, 2010-08-30 18:46:30]

http://codereview.chromium.org/3163003/diff/34004/47004
File chrome/browser/cocoa/tabpose_window.mm (right):

http://codereview.chromium.org/3163003/diff/34004/47004#newcode154
chrome/browser/cocoa/tabpose_window.mm:154: -
(id)initWithTabContents:(TabContents*)contents fullSize:(NSSize)fullSize {
On 2010/08/30 18:23:28, viettrungluu wrote:
> nit: Blank line above.

Eww! Seriously?

http://codereview.chromium.org/3163003/diff/34004/47004#newcode277
chrome/browser/cocoa/tabpose_window.mm:277: CGContextDrawImage(context,
destRect, thumbnail_.get());
On 2010/08/30 18:23:28, viettrungluu wrote:
> On 2010/08/30 15:35:26, Nico wrote:
> > On 2010/08/30 15:12:51, viettrungluu wrote:
> > > Can you combine drawing using a thumbnail and using a backing store?
> > 
> > Huh?
> 
> Just the call to CGContextDrawImage(). I have a slight preference for all the
> drawing to happen in one spot. (Or in very nearby spots. Not separated by the
> thumbnail request stuff.)
> 
> 

Done.

http://codereview.chromium.org/3163003/diff/34004/47004#newcode279
chrome/browser/cocoa/tabpose_window.mm:279: @end
On 2010/08/30 18:23:28, viettrungluu wrote:
> nit: blank line above

Likewise: Eww!

http://codereview.chromium.org/3163003/diff/34004/47004#newcode708
chrome/browser/cocoa/tabpose_window.mm:708: -
(void)selectTileAtIndex:(int)newIndex;
On 2010/08/30 18:23:28, viettrungluu wrote:
> Consider merging this and the NoAnimation variant.

I did consider it, but if I make this a parameter, then the method either can't
use the ScopedCAActionDisabler ('cause that would be in the if block before the
actual code), have duplicate code (same code in both branches), or can't use the
ScopedCAActionDisabler. Having two methods seemed better to me.

http://codereview.chromium.org/3163003/diff/34004/47004#newcode709
chrome/browser/cocoa/tabpose_window.mm:709: -
(void)selectTileAtIndexNoAnimation:(int)newIndex;
On 2010/08/30 18:23:28, viettrungluu wrote:
> This is a pretty bad name for a method.

Did s/No/Without/ :-D

http://codereview.chromium.org/3163003/diff/34004/47004#newcode800
chrome/browser/cocoa/tabpose_window.mm:800: NSImage* ns_favicon =
gfx::SkBitmapToNSImage(tile.favicon());
On 2010/08/30 18:23:28, viettrungluu wrote:
> Obj-C naming convention applies here

Done.

http://codereview.chromium.org/3163003/diff/34004/47004#newcode804
chrome/browser/cocoa/tabpose_window.mm:804: NSImage* defaultFavIcon =
ResourceBundle::GetSharedInstance().GetNSImageNamed(
On 2010/08/30 18:23:28, viettrungluu wrote:
> 80 chars

Done.

http://codereview.chromium.org/3163003/diff/34004/47004#newcode930
chrome/browser/cocoa/tabpose_window.mm:930: #if 0
On 2010/08/30 18:23:28, viettrungluu wrote:
> Hmmm. You should probably not check this in.

Done.

http://codereview.chromium.org/3163003/diff/34004/47004#newcode1000
chrome/browser/cocoa/tabpose_window.mm:1000: // Move the selected layer on top
of all other layers.
On 2010/08/30 18:23:28, viettrungluu wrote:
> Consider breaking this file up.

Yes. Soon.

http://codereview.chromium.org/3163003/diff/34004/47004#newcode1022
chrome/browser/cocoa/tabpose_window.mm:1022: // the initial too)
On 2010/08/30 18:23:28, viettrungluu wrote:
> Needs punctuation at the end. And I don't really understand the parenthetical
> comment.

When you zoom out, the current tab gets a start rect of more or less (0, 0,
contents_width, contents_height) and becomes smaller. All other tabs are
positioned relative to that initial current tab: They fly in from the left,
right, top, bottom.

Now, when a layer is selected, all the start rects need to become end rects.
This happens by making them relative to the final selected tab.

If that's still not clear, maybe I can draw it on some whiteboard and then you
can lend me your English expertise to sculpt a better comment.

http://codereview.chromium.org/3163003/diff/34004/47004#newcode1099
chrome/browser/cocoa/tabpose_window.mm:1099: showZoom:NO
On 2010/08/30 18:23:28, viettrungluu wrote:
> s/^/  /

Done.

http://codereview.chromium.org/3163003/diff/34004/47004#newcode1100
chrome/browser/cocoa/tabpose_window.mm:1100: slomo:NO
On 2010/08/30 18:23:28, viettrungluu wrote:
> "

Done.

http://codereview.chromium.org/3163003/diff/34004/47004#newcode1101
chrome/browser/cocoa/tabpose_window.mm:1101: animationDelegate:nil];
On 2010/08/30 18:23:28, viettrungluu wrote:
> "

Done.

(0C-vjjI  <esc> on the first line would've been more economical)

http://codereview.chromium.org/3163003/diff/34004/47004#newcode1104
chrome/browser/cocoa/tabpose_window.mm:1104: DCHECK_EQ(tabStripModel_->count(),
(int)[allThumbnailLayers_ count]);
On 2010/08/30 18:23:28, viettrungluu wrote:
> Apparently, we frown on C-style casts.

Done.

http://codereview.chromium.org/3163003/diff/34004/47004#newcode1169
chrome/browser/cocoa/tabpose_window.mm:1169: scoped_nsobject<CALayer>
thumbLayer(
On 2010/08/30 18:23:28, viettrungluu wrote:
> Could you add a comment saying what this does?

Made the existing comment more explicit

[viettrungluu, 2010-08-30 19:52:41]

LA(lmost)GTLM.

http://codereview.chromium.org/3163003/diff/34004/47004
File chrome/browser/cocoa/tabpose_window.mm (right):

http://codereview.chromium.org/3163003/diff/34004/47004#newcode154
chrome/browser/cocoa/tabpose_window.mm:154: -
(id)initWithTabContents:(TabContents*)contents fullSize:(NSSize)fullSize {
On 2010/08/30 18:46:31, Nico wrote:
> On 2010/08/30 18:23:28, viettrungluu wrote:
> > nit: Blank line above.
> 
> Eww! Seriously?

Yes, in general, if an @implementation section is nontrivial, we nearly always
have this blank line. (Just like for namespaces and such.)

http://codereview.chromium.org/3163003/diff/53001/40012
File chrome/browser/cocoa/tabpose_window_unittest.mm (right):

http://codereview.chromium.org/3163003/diff/53001/40012#newcode49
chrome/browser/cocoa/tabpose_window_unittest.mm:49:
tabStripModel:browser_helper_.browser()->tabstrip_model()];
Cheater!

http://codereview.chromium.org/3163003/diff/53001/40012#newcode77
chrome/browser/cocoa/tabpose_window_unittest.mm:77: model->MoveTabContentsAt(0,
1, /*select_after_move=*/false);
Could you exercise a bunch of different move tab scenarios (to < from, to >
from, to = from)? Also with different selected tabs....

http://codereview.chromium.org/3163003/diff/53001/40012#newcode86
chrome/browser/cocoa/tabpose_window_unittest.mm:86: model->CloseTabContentsAt(0,
TabStripModel::CLOSE_NONE);
Ditto.

[viettrungluu, 2010-08-30 19:53:07]

Let me try that again:

LA(lmost)GE(nough)TM.

[Nico, 2010-08-30 20:38:16]

On 2010/08/30 19:53:07, viettrungluu wrote:
> LAGETM.

That sounds a bit like an airport.

[viettrungluu, 2010-08-30 20:44:05]

LGETM.

[Nico, 2010-08-30 20:44:16]

right, "reply" doesn't send comments

http://codereview.chromium.org/3163003/diff/53001/40012
File chrome/browser/cocoa/tabpose_window_unittest.mm (right):

http://codereview.chromium.org/3163003/diff/53001/40012#newcode77
chrome/browser/cocoa/tabpose_window_unittest.mm:77: model->MoveTabContentsAt(0,
1, /*select_after_move=*/false);
On 2010/08/30 19:52:41, viettrungluu wrote:
> Could you exercise a bunch of different move tab scenarios (to < from, to >
> from, to = from)? Also with different selected tabs....

That's a _great_ idea! I'm very excited about having the opportunity to work on
this.

Also, done.

http://codereview.chromium.org/3163003/diff/53001/40012#newcode86
chrome/browser/cocoa/tabpose_window_unittest.mm:86: model->CloseTabContentsAt(0,
TabStripModel::CLOSE_NONE);
On 2010/08/30 19:52:41, viettrungluu wrote:
> Ditto.

Done as well. (The excitement wore off a little.)

[viettrungluu, 2010-08-31 02:50:17]

I'm happy to see you working on your enthusiasm.

On 2010/08/30 20:44:16, Nico wrote:
> right, "reply" doesn't send comments
> 
> http://codereview.chromium.org/3163003/diff/53001/40012
> File chrome/browser/cocoa/tabpose_window_unittest.mm (right):
> 
> http://codereview.chromium.org/3163003/diff/53001/40012#newcode77
> chrome/browser/cocoa/tabpose_window_unittest.mm:77:
model->MoveTabContentsAt(0,
> 1, /*select_after_move=*/false);
> On 2010/08/30 19:52:41, viettrungluu wrote:
> > Could you exercise a bunch of different move tab scenarios (to < from, to >
> > from, to = from)? Also with different selected tabs....
> 
> That's a _great_ idea! I'm very excited about having the opportunity to work
on
> this.
> 
> Also, done.
> 
> http://codereview.chromium.org/3163003/diff/53001/40012#newcode86
> chrome/browser/cocoa/tabpose_window_unittest.mm:86:
model->CloseTabContentsAt(0,
> TabStripModel::CLOSE_NONE);
> On 2010/08/30 19:52:41, viettrungluu wrote:
> > Ditto.
> 
> Done as well. (The excitement wore off a little.)