Mac: Add tabpose window

Mac: Add tabpose window

The window doesn't have any contents yet, which makes its appearance look a bit janky for now.

BUG=50307
TEST=
* All the following happens only if --enable-expose-for-tabs is passed in, else all of it should be disabled.
* In a browser window, hit cmd-f10 or three-finger-swipe down. A grey overlay with a gradient at the top should appear.
* The overlay should cover tab contents, eventual info bars, the bookmarks bar if it's detached (but not the bookmarks bar if it's not detached), and eventual attached inspector windows. It should not cover the download shelf if it's open.
* The window should block clicks on the tab strip and the download shelf for now.
* The overlay should close on three-finger-swipe up, click, esc, enter, and space.
* Every open browser window should have its own overlay, and they should be independent of each other.
* If a browser window with an overlay window is active, most menu items should be greyed out, and all browser-related keyboard shortcuts should be disabled.
* In particular, hitting cmd-f10 twice should open only one overlay per browser window
* The overlay should have the correct size with a UI scale factor > 1

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

[Robert Sesek, 2010-07-28 04:05:38]

http://codereview.chromium.org/2819070/diff/7001/8001
File chrome/browser/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/2819070/diff/7001/8001#newcode922
chrome/browser/cocoa/browser_window_controller.mm:922: NSInteger command =
[sender tag];
Add a comment as to what's going on here because |-openTabpose:|'s parameter
isn't really named.

http://codereview.chromium.org/2819070/diff/7001/8003
File chrome/browser/cocoa/browser_window_controller_private.mm (right):

http://codereview.chromium.org/2819070/diff/7001/8003#newcode29
chrome/browser/cocoa/browser_window_controller_private.mm:29: #include
"chrome/common/chrome_switches.h"
Alphabetize

http://codereview.chromium.org/2819070/diff/7001/8003#newcode514
chrome/browser/cocoa/browser_window_controller_private.mm:514: // Cover info
bars, inspector window, detached bookmark bar on NTP.
, and

http://codereview.chromium.org/2819070/diff/7001/8004
File chrome/browser/cocoa/tabpose_window.h (right):

http://codereview.chromium.org/2819070/diff/7001/8004#newcode6
chrome/browser/cocoa/tabpose_window.h:6: #define
CHROME_BROWSER_COCOA_TABPOSE_WINDOW_H_
#pragma once? Mac only, so it doesn't matter technically; but is consistency
good?

http://codereview.chromium.org/2819070/diff/7001/8004#newcode14
chrome/browser/cocoa/tabpose_window.h:14: CALayer* selectedLayer_;
Document each of these ivars. And strong or weak?

http://codereview.chromium.org/2819070/diff/7001/8005
File chrome/browser/cocoa/tabpose_window.mm (right):

http://codereview.chromium.org/2819070/diff/7001/8005#newcode13
chrome/browser/cocoa/tabpose_window.mm:13: // CAGradientLayer is 10.6-only --
roll our own
nit: Full-stop.

http://codereview.chromium.org/2819070/diff/7001/8005#newcode20
chrome/browser/cocoa/tabpose_window.mm:20: scoped_cftyperef<CGColorSpaceRef>
grayColorspace(
Keep their capitalization: grayColorSpace

http://codereview.chromium.org/2819070/diff/7001/8005#newcode25
chrome/browser/cocoa/tabpose_window.mm:25: grayColorspace, grays, locations,
arraysize(grays)));
grayColorSpace.get()

http://codereview.chromium.org/2819070/diff/7001/8005#newcode27
chrome/browser/cocoa/tabpose_window.mm:27: CGContextDrawLinearGradient(context,
gradient, topLeft, CGPointZero, 0);
gradient.get()

http://codereview.chromium.org/2819070/diff/7001/8006
File chrome/browser/cocoa/tabpose_window_unittest.mm (right):

http://codereview.chromium.org/2819070/diff/7001/8006#newcode9
chrome/browser/cocoa/tabpose_window_unittest.mm:9: #import
"chrome/browser/cocoa/browser_test_helper.h"
Alphabetize

[Nico, 2010-07-28 16:41:34]

http://codereview.chromium.org/2819070/diff/7001/8001
File chrome/browser/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/2819070/diff/7001/8001#newcode922
chrome/browser/cocoa/browser_window_controller.mm:922: NSInteger command =
[sender tag];
On 2010/07/28 04:05:38, rsesek wrote:
> Add a comment as to what's going on here because |-openTabpose:|'s parameter
> isn't really named.

Done.

http://codereview.chromium.org/2819070/diff/7001/8003
File chrome/browser/cocoa/browser_window_controller_private.mm (right):

http://codereview.chromium.org/2819070/diff/7001/8003#newcode29
chrome/browser/cocoa/browser_window_controller_private.mm:29: #include
"chrome/common/chrome_switches.h"
On 2010/07/28 04:05:38, rsesek wrote:
> Alphabetize

Done.

http://codereview.chromium.org/2819070/diff/7001/8003#newcode514
chrome/browser/cocoa/browser_window_controller_private.mm:514: // Cover info
bars, inspector window, detached bookmark bar on NTP.
On 2010/07/28 04:05:38, rsesek wrote:
> , and

Done.

http://codereview.chromium.org/2819070/diff/7001/8004
File chrome/browser/cocoa/tabpose_window.h (right):

http://codereview.chromium.org/2819070/diff/7001/8004#newcode6
chrome/browser/cocoa/tabpose_window.h:6: #define
CHROME_BROWSER_COCOA_TABPOSE_WINDOW_H_
On 2010/07/28 04:05:38, rsesek wrote:
> #pragma once? Mac only, so it doesn't matter technically; but is consistency
> good?

Done.

what idiot came up with that #pragma once shit

http://codereview.chromium.org/2819070/diff/7001/8004#newcode14
chrome/browser/cocoa/tabpose_window.h:14: CALayer* selectedLayer_;
On 2010/07/28 04:05:38, rsesek wrote:
> Document each of these ivars. And strong or weak?
Done.

Weak, else they'd be scoped_nsobjects. (Well, weak in a sense. They're owned by
the window's content view. But these references are weak.)

http://codereview.chromium.org/2819070/diff/7001/8005
File chrome/browser/cocoa/tabpose_window.mm (right):

http://codereview.chromium.org/2819070/diff/7001/8005#newcode13
chrome/browser/cocoa/tabpose_window.mm:13: // CAGradientLayer is 10.6-only --
roll our own
On 2010/07/28 04:05:38, rsesek wrote:
> nit: Full-stop.

Done.

http://codereview.chromium.org/2819070/diff/7001/8005#newcode20
chrome/browser/cocoa/tabpose_window.mm:20: scoped_cftyperef<CGColorSpaceRef>
grayColorspace(
On 2010/07/28 04:05:38, rsesek wrote:
> Keep their capitalization: grayColorSpace

Done.

http://codereview.chromium.org/2819070/diff/7001/8005#newcode25
chrome/browser/cocoa/tabpose_window.mm:25: grayColorspace, grays, locations,
arraysize(grays)));
On 2010/07/28 04:05:38, rsesek wrote:
> grayColorSpace.get()

Done.

http://codereview.chromium.org/2819070/diff/7001/8005#newcode27
chrome/browser/cocoa/tabpose_window.mm:27: CGContextDrawLinearGradient(context,
gradient, topLeft, CGPointZero, 0);
On 2010/07/28 04:05:38, rsesek wrote:
> gradient.get()

Done.

http://codereview.chromium.org/2819070/diff/7001/8006
File chrome/browser/cocoa/tabpose_window_unittest.mm (right):

http://codereview.chromium.org/2819070/diff/7001/8006#newcode9
chrome/browser/cocoa/tabpose_window_unittest.mm:9: #import
"chrome/browser/cocoa/browser_test_helper.h"
On 2010/07/28 04:05:38, rsesek wrote:
> Alphabetize

Done.

[pink (ping after 24hrs), 2010-07-28 16:47:42]

http://codereview.chromium.org/2819070/diff/12001/13001
File chrome/browser/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/2819070/diff/12001/13001#newcode924
chrome/browser/cocoa/browser_window_controller.mm:924: case IDC_TABPOSE: {
rather than trapping this mac-only, shouldn't this be plumbed through the
browser to platform-specific impls, of which we only now have Mac?

http://codereview.chromium.org/2819070/diff/12001/13001#newcode1486
chrome/browser/cocoa/browser_window_controller.mm:1486: [self
openTabpose:([event modifierFlags] & NSShiftKeyMask) != 0];
check with cole to see what we want to do here. The whole point of not landing
something is that we dont want to have to take it away when we unify our
gestures with chromeOS, not simply because we couldn't think of anything to put
here.

http://codereview.chromium.org/2819070/diff/12001/13004
File chrome/browser/cocoa/tabpose_window.h (right):

http://codereview.chromium.org/2819070/diff/12001/13004#newcode12
chrome/browser/cocoa/tabpose_window.h:12: // A TabposeWindow shows an overview
of open tabs and lets the user select a new
i dont' follow why this has to be a full NSWindow. Why can't it just be a view
that doesn't allow any key or mouseclicks to go to its subviews? If it has to
be, can you add a comment as to why?

http://codereview.chromium.org/2819070/diff/12001/13004#newcode16
chrome/browser/cocoa/tabpose_window.h:16: CALayer* selectedLayer_;
note these are weak references, per style guide.

http://codereview.chromium.org/2819070/diff/12001/13005
File chrome/browser/cocoa/tabpose_window.mm (right):

http://codereview.chromium.org/2819070/diff/12001/13005#newcode72
chrome/browser/cocoa/tabpose_window.mm:72: CALayer* gradientLayer =
[DarkGradientLayer layer];
Why now wrap this such that you can use a CAGradientLayer on 10.6 where
available and DarkGradientLayer on 10.5? That way we can get rid of it entirely
when we move on w/out having to re-write it at that point.

http://codereview.chromium.org/2819070/diff/12001/13005#newcode127
chrome/browser/cocoa/tabpose_window.mm:127: // Disable all browser-related menu
items.
Am I allowed to switch tabs? If so, i should be able to create a new tab or open
a new window, right?

[pink (ping after 24hrs), 2010-07-28 16:49:29]

Also, all the design decisions  you put in the TEST= area should be documented
somewhere in the code. They seem to be pretty important, not just something for
QA.

[Nico, 2010-07-28 17:51:45]

On 2010/07/28 16:49:29, pink wrote:
> Also, all the design decisions  you put in the TEST= area should be documented
> somewhere in the code. They seem to be pretty important, not just something
for
> QA.

Any comment in particular you're referring to? I've folded two into the
TabposeWindow class comment, and some of the others are already somewhere in the
code (the parts of the window that are covered are documented in BWC's
-openTabpose). The rest seems more like stuff that should be mention in the TEST
section and on the bug (where it will arrive once this gets committed)

[Nico, 2010-07-28 17:52:57]

(Note that this is still behind a flag and I'm not 100% sure how this will look
once it's done)

http://codereview.chromium.org/2819070/diff/12001/13001
File chrome/browser/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/2819070/diff/12001/13001#newcode924
chrome/browser/cocoa/browser_window_controller.mm:924: case IDC_TABPOSE: {
On 2010/07/28 16:47:42, pink wrote:
> rather than trapping this mac-only, shouldn't this be plumbed through the
> browser to platform-specific impls, of which we only now have Mac?

I wasn't sure about that. It's quite a bunch of extra plumbing for no particular
benefit.

Well, done now. Reminded me of adding uma stuff, so that was good.

http://codereview.chromium.org/2819070/diff/12001/13001#newcode1486
chrome/browser/cocoa/browser_window_controller.mm:1486: [self
openTabpose:([event modifierFlags] & NSShiftKeyMask) != 0];
On 2010/07/28 16:47:42, pink wrote:
> check with cole to see what we want to do here. The whole point of not landing
> something is that we dont want to have to take it away when we unify our
> gestures with chromeOS, not simply because we couldn't think of anything to
put
> here.

I checked with Cole, he thinks this is good.

http://codereview.chromium.org/2819070/diff/12001/13004
File chrome/browser/cocoa/tabpose_window.h (right):

http://codereview.chromium.org/2819070/diff/12001/13004#newcode12
chrome/browser/cocoa/tabpose_window.h:12: // A TabposeWindow shows an overview
of open tabs and lets the user select a new
On 2010/07/28 16:47:42, pink wrote:
> i dont' follow why this has to be a full NSWindow. Why can't it just be a view
> that doesn't allow any key or mouseclicks to go to its subviews? If it has to
> be, can you add a comment as to why?

It seemed to me that putting this into a separate window makes things much
simpler. 

* I want to block clicks everywhere in the browser window (resize rect, traffic
light buttons, etc)
* It's more flexible: maybe some UI guy decides that the tabpose effect should
cover the whole screen, not just the window it's for
* Putting a view that covers all other views in the browser window seems pretty
intrusive; a child window is more separate

Not sure how to succinctly put this into a comment

http://codereview.chromium.org/2819070/diff/12001/13004#newcode16
chrome/browser/cocoa/tabpose_window.h:16: CALayer* selectedLayer_;
On 2010/07/28 16:47:42, pink wrote:
> note these are weak references, per style guide.

Done.

http://codereview.chromium.org/2819070/diff/12001/13005
File chrome/browser/cocoa/tabpose_window.mm (right):

http://codereview.chromium.org/2819070/diff/12001/13005#newcode72
chrome/browser/cocoa/tabpose_window.mm:72: CALayer* gradientLayer =
[DarkGradientLayer layer];
On 2010/07/28 16:47:42, pink wrote:
> Why now wrap this such that you can use a CAGradientLayer on 10.6 where
> available and DarkGradientLayer on 10.5? That way we can get rid of it
entirely
> when we move on w/out having to re-write it at that point.

That'd mean this would only compile with the 10.6 SDK (and the machine I'm
currently on doesn't have 10.6 :-P). There are other reasons (CAGradientLayer
saves maybe 10 lines over the current solution, and the 10.5/10.6 detection
logic is already half that), but that's the main reason

http://codereview.chromium.org/2819070/diff/12001/13005#newcode127
chrome/browser/cocoa/tabpose_window.mm:127: // Disable all browser-related menu
items.
On 2010/07/28 16:47:42, pink wrote:
> Am I allowed to switch tabs?

Not using the tab strip, at least for now.

> If so, i should be able to create a new tab or open
> a new window, right?

I'm not planning to support this atm either, but that depends on how this feels
once it's done.

Currently, my think is that this is a pretty shortlived thing. People open
tabpose, click a tab thumbnail, and away it goes again. That's all it's meant to
support for now (but that's just my opinion!)

It might be necessary to let browser_window_controller know about its current
TabposeWindow down the road, and then this logic could live there, but for now
I'm trying to keep things simple and not change browser_window_controller too
much.

[Robert Sesek, 2010-07-28 20:09:08]

These points should be integrated into the class comments, I think:

* Every open browser window should have its own overlay, and they should be
independent of each other. 
* If a browser window with an overlay window is active, most menu items should
be greyed out, and all browser-related keyboard shortcuts should be disabled. 
* In particular, hitting cmd-f10 twice should open only one overlay per browser
window

http://codereview.chromium.org/2819070/diff/29001/30001
File chrome/browser/browser.cc (right):

http://codereview.chromium.org/2819070/diff/29001/30001#newcode1352
chrome/browser/browser.cc:1352: #endif
#else
NOTIMPLEMENTED()?

http://codereview.chromium.org/2819070/diff/29001/30012
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/2819070/diff/29001/30012#newcode991
chrome/chrome_browser.gypi:991: 'browser/cocoa/tabpose_window.mm',
Alphabetize. p > l

http://codereview.chromium.org/2819070/diff/29001/30013
File chrome/chrome_tests.gypi (right):

http://codereview.chromium.org/2819070/diff/29001/30013#newcode883
chrome/chrome_tests.gypi:883: 'browser/cocoa/tabpose_window_unittest.mm',
Here too

[pink (ping after 24hrs), 2010-07-30 14:07:27]

lgtm

[Nico, 2010-07-30 18:28:57]

Thanks!

I noticed that I get all kinds of leaks in CoreAnimation when I run this under
valgrind locally. I get the same leaks when I run AnimatableImageTest under
valgrind, which seems to be fine on the bots (maybe because the bots have
different graphics cards and hence hit different paths), so maybe it'll work.
Else, I'll need to add suppressions.

http://codereview.chromium.org/2819070/diff/29001/30001
File chrome/browser/browser.cc (right):

http://codereview.chromium.org/2819070/diff/29001/30001#newcode1352
chrome/browser/browser.cc:1352: #endif
On 2010/07/28 20:09:08, rsesek wrote:
> #else
> NOTIMPLEMENTED()?

Since the assumption is that linux/win won't have that, NOTREACHED is better I
think. Done.

http://codereview.chromium.org/2819070/diff/29001/30012
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/2819070/diff/29001/30012#newcode991
chrome/chrome_browser.gypi:991: 'browser/cocoa/tabpose_window.mm',
On 2010/07/28 20:09:08, rsesek wrote:
> Alphabetize. p > l

Done.

http://codereview.chromium.org/2819070/diff/29001/30013
File chrome/chrome_tests.gypi (right):

http://codereview.chromium.org/2819070/diff/29001/30013#newcode883
chrome/chrome_tests.gypi:883: 'browser/cocoa/tabpose_window_unittest.mm',
On 2010/07/28 20:09:08, rsesek wrote:
> Here too

Done.