Fullscreen support for Lion.

Fullscreen support for Lion.

XIB changes:
- Add new "Enter Presentation Mode" menu item with Cmd-Shift-F.
- Change key equivalent for "Enter Full Screen" to Cmd-Opt-F.

On Lion:
This CL separates the concepts of "fullscreen mode" and "presentation mode."
Fullscreen mode uses Lion's fullscreen mode and places the window in its own
space.  Presentation mode is an extra option to additionally hide the UI when in
fullscreen mode.  On Lion, clicking on the fullscreen button takes you into
fullscreen mode, in which there is a button to toggle presentation mode on or
off.  While in normal windowed mode, there are two menu options to either "Enter
Full Screen" or "Enter Presentation Mode" directly.

On Snow Leopard:
This CL does not change much on Snow Leopard.  To maintain consistency with the
new Lion terminology, the "Enter Full Screen" menu item has been renamed to
"Enter Presentation Mode."

BUG=74065
TEST=Fullscreen works in Lion.  Fullscreen continues to work as before in Snow Leopard.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=95518

[rohitrao (ping after 24h), 2011-08-04 01:24:42]

I'm sending this out for a first review.  There are a couple bugs I still need
to fix:
1) The keyboard shortcuts in the xib file are wrong.
2) The spacing around the incognito man is off.

But I can fix both of those without affecting most of the CL, so I'll hit those
tomorrow morning.  In the meantime, if someone wants to get a head start on
looking at this, great.

There are also a few bugs with non-trivial fixes, and I have those written down
on a post-it note on my desk.  The biggest problem is that the z-ordering of
views is messed up, so there are cases where the findbar doesn't show up.  The
fix for that will take some experimentation, so I'd prefer to get this in first
and then follow up with another CL.  I'm sure other bugs will pop up once people
start using it for real.

[Robert Sesek, 2011-08-04 14:43:41]

http://codereview.chromium.org/7566016/diff/4007/chrome/browser/ui/cocoa/brow...
File chrome/browser/ui/cocoa/browser_window_controller.h (right):

http://codereview.chromium.org/7566016/diff/4007/chrome/browser/ui/cocoa/brow...
chrome/browser/ui/cocoa/browser_window_controller.h:130:
scoped_nsobject<NSWindow> fullscreenWindow_;
Make this a FullscreenWindow to appease the Clang build.

http://codereview.chromium.org/7566016/diff/4007/chrome/browser/ui/cocoa/brow...
File chrome/browser/ui/cocoa/browser_window_controller_private.h (right):

http://codereview.chromium.org/7566016/diff/4007/chrome/browser/ui/cocoa/brow...
chrome/browser/ui/cocoa/browser_window_controller_private.h:53:
OverlayMaxY:(CGFloat)maxY;
nit: lowercase O

http://codereview.chromium.org/7566016/diff/4007/chrome/browser/ui/cocoa/brow...
File chrome/browser/ui/cocoa/browser_window_controller_private.mm (right):

http://codereview.chromium.org/7566016/diff/4007/chrome/browser/ui/cocoa/brow...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:59: const CGFloat
kAvatarTabStripShrink = 18;
Is this used?

http://codereview.chromium.org/7566016/diff/4007/chrome/browser/ui/cocoa/brow...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:100:
setImage:[NSImage imageNamed:NSImageNameIChatTheaterTemplate]];
I was hoping you'd choose this one :)

http://codereview.chromium.org/7566016/diff/4007/chrome/browser/ui/cocoa/brow...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:102:
[presentationModeToggleButton_
[[button cell] setHighlightsBy:NSContentsCellMask];
[[button cell] setShowsStateBy:NSContentsCellMask];

http://codereview.chromium.org/7566016/diff/4007/chrome/browser/ui/cocoa/brow...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:594:
DCHECK(!base::mac::IsOSLionOrLater());
IsOSSnowLeopardOrEarlier()

http://codereview.chromium.org/7566016/diff/4007/chrome/browser/ui/cocoa/brow...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:599: // Close the
bookmark bubble, if it's open.  We use |-ok:| instead of
Mark is going to tell you to not use 'we'

http://codereview.chromium.org/7566016/diff/4007/chrome/browser/ui/cocoa/brow...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:704:
DCHECK(!base::mac::IsOSLionOrLater());
IsOSSnowLeopardOrEarlier();

http://codereview.chromium.org/7566016/diff/4007/chrome/browser/ui/cocoa/brow...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:739:
DCHECK(!base::mac::IsOSLionOrLater());
here too

[Mark Mentovai, 2011-08-04 15:07:19]

When I first enter full-screen, either presentation or not, mouseovers don’t
work. I don’t get a status bubble when I hover over links in a Google search
result page, and the mouse cursor doesn’t give me the finger. Once I hit the
toggle to switch in or out of presentation mode, mouseovers begin working.

[rohitrao (ping after 24h), 2011-08-04 18:18:26]

New diffs up.  These fix:
1) rsesek's comments.
2) Key equivalents.
3) Incognito guy spacing.
4) Instant crashy.

http://codereview.chromium.org/7566016/diff/4007/chrome/browser/ui/cocoa/brow...
File chrome/browser/ui/cocoa/browser_window_controller.h (right):

http://codereview.chromium.org/7566016/diff/4007/chrome/browser/ui/cocoa/brow...
chrome/browser/ui/cocoa/browser_window_controller.h:130:
scoped_nsobject<NSWindow> fullscreenWindow_;
On 2011/08/04 14:43:41, rsesek wrote:
> Make this a FullscreenWindow to appease the Clang build.

The clang bot was unhappy when this was a FullscreenWindow, but it seems happy
enough with the NSWindow.

http://codereview.chromium.org/7566016/diff/4007/chrome/browser/ui/cocoa/brow...
File chrome/browser/ui/cocoa/browser_window_controller_private.h (right):

http://codereview.chromium.org/7566016/diff/4007/chrome/browser/ui/cocoa/brow...
chrome/browser/ui/cocoa/browser_window_controller_private.h:53:
OverlayMaxY:(CGFloat)maxY;
On 2011/08/04 14:43:41, rsesek wrote:
> nit: lowercase O

Done.

http://codereview.chromium.org/7566016/diff/4007/chrome/browser/ui/cocoa/brow...
File chrome/browser/ui/cocoa/browser_window_controller_private.mm (right):

http://codereview.chromium.org/7566016/diff/4007/chrome/browser/ui/cocoa/brow...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:59: const CGFloat
kAvatarTabStripShrink = 18;
On 2011/08/04 14:43:41, rsesek wrote:
> Is this used?

It is now =)

http://codereview.chromium.org/7566016/diff/4007/chrome/browser/ui/cocoa/brow...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:102:
[presentationModeToggleButton_
On 2011/08/04 14:43:41, rsesek wrote:
> [[button cell] setHighlightsBy:NSContentsCellMask];
> [[button cell] setShowsStateBy:NSContentsCellMask];

Done.

http://codereview.chromium.org/7566016/diff/4007/chrome/browser/ui/cocoa/brow...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:594:
DCHECK(!base::mac::IsOSLionOrLater());
On 2011/08/04 14:43:41, rsesek wrote:
> IsOSSnowLeopardOrEarlier()

Cool, I meant to go look for that function, but forgot to.  Fixed everywhere.

http://codereview.chromium.org/7566016/diff/4007/chrome/browser/ui/cocoa/brow...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:599: // Close the
bookmark bubble, if it's open.  We use |-ok:| instead of
On 2011/08/04 14:43:41, rsesek wrote:
> Mark is going to tell you to not use 'we'

Ook.

[rohitrao (ping after 24h), 2011-08-04 18:29:13]

On 2011/08/04 15:07:19, Mark Mentovai wrote:
> When I first enter full-screen, either presentation or not, mouseovers don’t
> work. I don’t get a status bubble when I hover over links in a Google search
> result page, and the mouse cursor doesn’t give me the finger. Once I hit the
> toggle to switch in or out of presentation mode, mouseovers begin working.

This seems to be related to the RWHVCocoa changing size.  I dropped some logging
in and we're not getting any mouseMoved: events at all after going fullscreen.

This seems to be triggered mostly if you go fullscreen while the mouse is
outside the web content area.  The bug also somewhat exists on SL and in the old
fullscreen mode, as we lose cursor changes there as well.

[Mark Mentovai, 2011-08-04 19:18:16]

I love you.

http://codereview.chromium.org/7566016/diff/17001/chrome/browser/ui/cocoa/boo...
File chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm (right):

http://codereview.chromium.org/7566016/diff/17001/chrome/browser/ui/cocoa/boo...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:37: #import
"chrome/browser/ui/cocoa/presentation_mode_controller.h"
Sort, presentation_mode_controller < themed_window.

http://codereview.chromium.org/7566016/diff/17001/chrome/browser/ui/cocoa/bro...
File chrome/browser/ui/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/7566016/diff/17001/chrome/browser/ui/cocoa/bro...
chrome/browser/ui/cocoa/browser_window_controller.mm:2062: if ([[self window]
respondsToSelector:@selector(toggleSystemFullScreen)])
Can you do this as an isKindOfClass check with a cast to the right type, instead
of a respondsToSelector check with a cast to id? Again on 2119 and 2135.

http://codereview.chromium.org/7566016/diff/17001/chrome/browser/ui/cocoa/bro...
File chrome/browser/ui/cocoa/browser_window_controller_private.mm (right):

http://codereview.chromium.org/7566016/diff/17001/chrome/browser/ui/cocoa/bro...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:105: [button
setImage:[NSImage imageNamed:NSImageNameIChatTheaterTemplate]];
I think this was a great choice for now, at least until we get Cole art.

http://codereview.chromium.org/7566016/diff/17001/chrome/browser/ui/cocoa/bro...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:218: (floor((1 -
floatingBarShownFraction_) * floatingBarHeight) -
#include <cmath> and use std::floor, here and on line 225.

http://codereview.chromium.org/7566016/diff/17001/chrome/browser/ui/cocoa/bro...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:324: // tabstrip.
tab strip is two words.

http://codereview.chromium.org/7566016/diff/17001/chrome/browser/ui/cocoa/bro...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:591: // This method
is only for Snow Leopard and earlier.
So name it with SnowLeopardOrEarlier like you’ve done with other methods?

You should really consider doing this for any method that begins with a
DCHECK(IsOSAllays()).

http://codereview.chromium.org/7566016/diff/17001/chrome/browser/ui/cocoa/fra...
File chrome/browser/ui/cocoa/framed_browser_window.mm (right):

http://codereview.chromium.org/7566016/diff/17001/chrome/browser/ui/cocoa/fra...
chrome/browser/ui/cocoa/framed_browser_window.mm:295: [delegate
handleLionToggleFullscreen];
Is there a decent way to preserve |sender| through the call so that you can use
it when you call back into the system at line 300? I don’t know if the system
cares or not, but it might want to do something based on the value of sender.
Like determine if the command came from a menu or the window button. I don’t
know.

Not a huge deal, but if it’s easy, it’s easy. If we’re guaranteed to have
everything on the stack, you can even just stash sender in a member variable.

http://codereview.chromium.org/7566016/diff/17001/chrome/browser/ui/cocoa/pre...
File chrome/browser/ui/cocoa/presentation_mode_prefs.h (right):

http://codereview.chromium.org/7566016/diff/17001/chrome/browser/ui/cocoa/pre...
chrome/browser/ui/cocoa/presentation_mode_prefs.h:7: 
#pragma once

[rohitrao (ping after 24h), 2011-08-04 19:49:17]

http://codereview.chromium.org/7566016/diff/17001/chrome/browser/ui/cocoa/boo...
File chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm (right):

http://codereview.chromium.org/7566016/diff/17001/chrome/browser/ui/cocoa/boo...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:37: #import
"chrome/browser/ui/cocoa/presentation_mode_controller.h"
On 2011/08/04 19:18:17, Mark Mentovai wrote:
> Sort, presentation_mode_controller < themed_window.

Done.

http://codereview.chromium.org/7566016/diff/17001/chrome/browser/ui/cocoa/bro...
File chrome/browser/ui/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/7566016/diff/17001/chrome/browser/ui/cocoa/bro...
chrome/browser/ui/cocoa/browser_window_controller.mm:2062: if ([[self window]
respondsToSelector:@selector(toggleSystemFullScreen)])
On 2011/08/04 19:18:17, Mark Mentovai wrote:
> Can you do this as an isKindOfClass check with a cast to the right type,
instead
> of a respondsToSelector check with a cast to id? Again on 2119 and 2135.

Done.  Why is this better?

http://codereview.chromium.org/7566016/diff/17001/chrome/browser/ui/cocoa/bro...
File chrome/browser/ui/cocoa/browser_window_controller_private.mm (right):

http://codereview.chromium.org/7566016/diff/17001/chrome/browser/ui/cocoa/bro...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:218: (floor((1 -
floatingBarShownFraction_) * floatingBarHeight) -
On 2011/08/04 19:18:17, Mark Mentovai wrote:
> #include <cmath> and use std::floor, here and on line 225.

Done.

http://codereview.chromium.org/7566016/diff/17001/chrome/browser/ui/cocoa/bro...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:324: // tabstrip.
On 2011/08/04 19:18:17, Mark Mentovai wrote:
> tab strip is two words.

Done.

http://codereview.chromium.org/7566016/diff/17001/chrome/browser/ui/cocoa/bro...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:591: // This method
is only for Snow Leopard and earlier.
On 2011/08/04 19:18:17, Mark Mentovai wrote:
> So name it with SnowLeopardOrEarlier like you’ve done with other methods?
> 
> You should really consider doing this for any method that begins with a
> DCHECK(IsOSAllays()).

Done.

http://codereview.chromium.org/7566016/diff/17001/chrome/browser/ui/cocoa/fra...
File chrome/browser/ui/cocoa/framed_browser_window.mm (right):

http://codereview.chromium.org/7566016/diff/17001/chrome/browser/ui/cocoa/fra...
chrome/browser/ui/cocoa/framed_browser_window.mm:295: [delegate
handleLionToggleFullscreen];
On 2011/08/04 19:18:17, Mark Mentovai wrote:
> Is there a decent way to preserve |sender| through the call so that you can
use
> it when you call back into the system at line 300? I don’t know if the system
> cares or not, but it might want to do something based on the value of sender.
> Like determine if the command came from a menu or the window button. I don’t
> know.
> 
> Not a huge deal, but if it’s easy, it’s easy. If we’re guaranteed to have
> everything on the stack, you can even just stash sender in a member variable.


Saving it on the stack looks like the most straightforward way of keeping it
around, since we can't pass any context into Browser::ExecuteCommand().  I think
there might be a way for the Browser to issue a fullscreen command without
anything going through BWC, which would make saving anything scary.

http://codereview.chromium.org/7566016/diff/17001/chrome/browser/ui/cocoa/pre...
File chrome/browser/ui/cocoa/presentation_mode_prefs.h (right):

http://codereview.chromium.org/7566016/diff/17001/chrome/browser/ui/cocoa/pre...
chrome/browser/ui/cocoa/presentation_mode_prefs.h:7: 
On 2011/08/04 19:18:17, Mark Mentovai wrote:
> #pragma once

Done.

[Mark Mentovai, 2011-08-04 19:56:52]

I’m going to LGTM this now because the code is clean enough, the behaviors are
good enough, and future work can be done in subsequent bite-sized patches. It’s
certainly in good enough shape to go to the canary now.

http://codereview.chromium.org/7566016/diff/17001/chrome/browser/ui/cocoa/bro...
File chrome/browser/ui/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/7566016/diff/17001/chrome/browser/ui/cocoa/bro...
chrome/browser/ui/cocoa/browser_window_controller.mm:2062: if ([[self window]
respondsToSelector:@selector(toggleSystemFullScreen)])
rohitrao wrote:
> Done.  Why is this better?

In case you ever start making more calls into the window, you might as well have
it in a pointer of the right type. It’s about even for a single call, but once
you have more than that, it’s a win, and it discourages anyone from leaving it
in an id pointer in the future.

[commit-bot: I haz the power, 2011-08-04 21:38:42]

Can't process patch for file
chrome/browser/ui/cocoa/presentation_mode_controller.mm.
A  +