Make thumbnails closable in tabpose

Make thumbnails closable in tabpose

Also make the thumbnails left-aligned. The reason for this is that when thumbnails are closed, the reshuffling algorithm looks a bit busy. We want to eventually slide thumbs out on the left side on the window and then slide them back in from the right.

BUG=50307
TEST=
* Hover a thumb. It should show a close button.
* Press opt. All thumbs should show a close button.
* Press a close button. Thumb should close, the rest of the tabs should reshuffle.

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

[viettrungluu, 2010-12-30 01:16:48]

The CL description needs work too.

http://codereview.chromium.org/5960008/diff/7001/chrome/browser/ui/cocoa/tabp...
File chrome/browser/ui/cocoa/tabpose_window.mm (right):

http://codereview.chromium.org/5960008/diff/7001/chrome/browser/ui/cocoa/tabp...
chrome/browser/ui/cocoa/tabpose_window.mm:919: -
(void)setUpLayersInSlomo:(BOOL)slomo;
These things all need comments describing what they do, either here or where
they're implemented.

http://codereview.chromium.org/5960008/diff/7001/chrome/browser/ui/cocoa/tabp...
chrome/browser/ui/cocoa/tabpose_window.mm:920: - (void)fadeAway:(BOOL)slomo;
I don't know how I ever let you get away with misnaming this method; it
should've been -fadeAwayInSlomo:. Instead, I need to look up the declaration to
understand what the argument is for.

http://codereview.chromium.org/5960008/diff/7001/chrome/browser/ui/cocoa/tabp...
chrome/browser/ui/cocoa/tabpose_window.mm:977: -
(void)addLayersForTile:(tabpose::Tile&)tile
Yes, this method is in fact too long.

http://codereview.chromium.org/5960008/diff/7001/chrome/browser/ui/cocoa/tabp...
chrome/browser/ui/cocoa/tabpose_window.mm:1013: // FIXME: do this conversion
only once, not once per layer
FIXME?

http://codereview.chromium.org/5960008/diff/7001/chrome/browser/ui/cocoa/tabp...
chrome/browser/ui/cocoa/tabpose_window.mm:1016: closeLayer.contents =
(id)closeIcon.get();
What's with the cast?

http://codereview.chromium.org/5960008/diff/7001/chrome/browser/ui/cocoa/tabp...
chrome/browser/ui/cocoa/tabpose_window.mm:1018: closeLayer.bounds = closeBounds;
I don't know when we started using the . notation, but using setters explicitly
seems more common in Chrom{e,ium} code....

http://codereview.chromium.org/5960008/diff/7001/chrome/browser/ui/cocoa/tabp...
chrome/browser/ui/cocoa/tabpose_window.mm:1265:
-(void)selectTileFromMouseEvent:(NSEvent*)event {
missing space here

http://codereview.chromium.org/5960008/diff/7001/chrome/browser/ui/cocoa/tabp...
chrome/browser/ui/cocoa/tabpose_window.mm:1290: layer.hidden = (state_ !=
tabpose::kFadedIn ||
This isn't sufficiently self-explanatory, so you should either write a comment
(or write code which is more self documenting).

http://codereview.chromium.org/5960008/diff/7001/chrome/browser/ui/cocoa/tabp...
chrome/browser/ui/cocoa/tabpose_window.mm:1630: for (int i = std::min(from, to);
i <= std::max(from, to); ++i)
You're missing the #include to use std::{min,max}.

[Nico, 2011-01-12 19:51:58]

Thanks!

Another change is that the las row of tiles is now left-aligned instead of
centered.

http://codereview.chromium.org/5960008/diff/7001/chrome/browser/ui/cocoa/tabp...
File chrome/browser/ui/cocoa/tabpose_window.mm (right):

http://codereview.chromium.org/5960008/diff/7001/chrome/browser/ui/cocoa/tabp...
chrome/browser/ui/cocoa/tabpose_window.mm:919: -
(void)setUpLayersInSlomo:(BOOL)slomo;
On 2010/12/30 01:16:49, viettrungluu wrote:
> These things all need comments describing what they do, either here or where
> they're implemented.

Done.

http://codereview.chromium.org/5960008/diff/7001/chrome/browser/ui/cocoa/tabp...
chrome/browser/ui/cocoa/tabpose_window.mm:920: - (void)fadeAway:(BOOL)slomo;
On 2010/12/30 01:16:49, viettrungluu wrote:
> I don't know how I ever let you get away with misnaming this method; it
> should've been -fadeAwayInSlomo:. Instead, I need to look up the declaration
to
> understand what the argument is for.

Done.

http://codereview.chromium.org/5960008/diff/7001/chrome/browser/ui/cocoa/tabp...
chrome/browser/ui/cocoa/tabpose_window.mm:1013: // FIXME: do this conversion
only once, not once per layer
On 2010/12/30 01:16:49, viettrungluu wrote:
> FIXME?

Done.

http://codereview.chromium.org/5960008/diff/7001/chrome/browser/ui/cocoa/tabp...
chrome/browser/ui/cocoa/tabpose_window.mm:1016: closeLayer.contents =
(id)closeIcon.get();
On 2010/12/30 01:16:49, viettrungluu wrote:
> What's with the cast?

It's required. I guess you want me to do a static_cast?

http://codereview.chromium.org/5960008/diff/7001/chrome/browser/ui/cocoa/tabp...
chrome/browser/ui/cocoa/tabpose_window.mm:1018: closeLayer.bounds = closeBounds;
On 2010/12/30 01:16:49, viettrungluu wrote:
> I don't know when we started using the . notation, but using setters
explicitly
> seems more common in Chrom{e,ium} code....

I agree, but it's not more common in this file, the google objc guide allows it
iirc, and coreanimation code usually uses . notation fervently. Just trying to
blend in.

http://codereview.chromium.org/5960008/diff/7001/chrome/browser/ui/cocoa/tabp...
chrome/browser/ui/cocoa/tabpose_window.mm:1265:
-(void)selectTileFromMouseEvent:(NSEvent*)event {
On 2010/12/30 01:16:49, viettrungluu wrote:
> missing space here

Done.

http://codereview.chromium.org/5960008/diff/7001/chrome/browser/ui/cocoa/tabp...
chrome/browser/ui/cocoa/tabpose_window.mm:1290: layer.hidden = (state_ !=
tabpose::kFadedIn ||
On 2010/12/30 01:16:49, viettrungluu wrote:
> This isn't sufficiently self-explanatory, so you should either write a comment
> (or write code which is more self documenting).

Done?

http://codereview.chromium.org/5960008/diff/7001/chrome/browser/ui/cocoa/tabp...
chrome/browser/ui/cocoa/tabpose_window.mm:1630: for (int i = std::min(from, to);
i <= std::max(from, to); ++i)
On 2010/12/30 01:16:49, viettrungluu wrote:
> You're missing the #include to use std::{min,max}.

Done.

[viettrungluu, 2011-01-12 20:21:03]

LGEishTM.