Mac fullscreen mode (with pkasting).

Mac fullscreen mode (with pkasting).

TEST=Launch Chrome.  Create a 2nd tab.  Close bookmark bar.
Cmd-F11 to enter fullscreen; make sure content is centered and both
  bookmark bar and toolbar are gone.  Make sure menubar gone.
Cmd-Opt-arrows to switch tabs; make sure still OK.
Cmd-F11 to go back; make sure things look normal.
Open bookmark bar.
Cmd-F11; make sure gone.  Cmd-F11 again; make sure it comes back.
Confirm View-->Fullscreen menu item works.
While in fullscreen, Cmd-T to create new tab and click on a fav tile.
  Make sure page loads.
While in fullscreen, try window hotkeys (Cmd-N and Cmd-W) to make sure
  they work.  Cmd-` to switch windows; switch back, then Cmd-F11 to
  undo fullscreen.
Move the mouse to to the top of the screen; make sure menubar appears.
Move the mouse down; make sure menubar goes away.



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

[John Grabowski, 2009-06-17 21:55:13]

+cc avi,jeremy for Cocoa key advice.

I know this is a post-beta feature but I accidentally implemented it with
pkasting.  To be clear, this is implemented like windows (web content goes
fullscreen with no toolbar visible).

F11 (fullscreen for Win) is the Expose' default so I cannot reuse.  I'm
currently using Cmd-F11 since I can't find a better answer.  (Neither
FireFox/Mac nor Safari have a fullscreen mode available from a menu option).

Thoughts?

[jeremy, 2009-06-17 22:00:04]

lgtm

http://codereview.chromium.org/126294/diff/1/7
File chrome/browser/cocoa/tab_contents_controller.mm (right):

http://codereview.chromium.org/126294/diff/1/7#newcode42
Line 42: // TODO(jrg): info bubble mentioning hotkey to get out of fullscreen
mode
Could you file a bug for this and reference it from here so we can track this?

[Peter Kasting, 2009-06-17 22:19:11]

LGTM other than comment below

http://codereview.chromium.org/126294/diff/1011/18
File chrome/browser/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/126294/diff/1011/18#newcode562
Line 562: [tabStripController_ setFullscreen:fullscreen];
Is there a way of just getting the selected tab contents (to get its controller)
here, and then calling setFullscreen on the controller directly?  It seems
somehow wrong for the tabStripController to know about fullscreen mode at all.

(Same comment for isFullscreen, obviously)

[John Grabowski, 2009-06-18 02:42:34]

http://codereview.chromium.org/126294/diff/1/7
File chrome/browser/cocoa/tab_contents_controller.mm (right):

http://codereview.chromium.org/126294/diff/1/7#newcode42
Line 42: // TODO(jrg): info bubble mentioning hotkey to get out of fullscreen
mode
On 2009/06/17 22:00:04, jeremy wrote:
> Could you file a bug for this and reference it from here so we can track this?

Added (14471)

http://codereview.chromium.org/126294/diff/1011/18
File chrome/browser/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/126294/diff/1011/18#newcode562
Line 562: [tabStripController_ setFullscreen:fullscreen];
On 2009/06/17 22:19:11, pkasting wrote:
> Is there a way of just getting the selected tab contents (to get its
controller)
> here, and then calling setFullscreen on the controller directly?  It seems
> somehow wrong for the tabStripController to know about fullscreen mode at all.
> 
> (Same comment for isFullscreen, obviously)

Yes, I could have written a -[TabStripController getCurrentTabController]. 
However, the tab controllers themselves are not exposed so I was leery of
opening up the abstraction.
The NSViews embedded in the controllers are in fact exposed, but only as an
implementation detail of some low-level issues (e.g. resizing).
I am open to changing this if you really think it's the wrong model, but I tend
to like encapsulation even if a tad inconvenient.

[Peter Kasting, 2009-06-18 02:59:43]

> On 2009/06/17 22:19:11, pkasting wrote:
> > Is there a way of just getting the selected tab contents (to get its
> controller)
> > here, and then calling setFullscreen on the controller directly?  It seems
> > somehow wrong for the tabStripController to know about fullscreen mode at
all.
> > 
> > (Same comment for isFullscreen, obviously)
> 
> Yes, I could have written a -[TabStripController getCurrentTabController]. 
> However, the tab controllers themselves are not exposed so I was leery of
> opening up the abstraction.
> The NSViews embedded in the controllers are in fact exposed, but only as an
> implementation detail of some low-level issues (e.g. resizing).
> I am open to changing this if you really think it's the wrong model, but I
tend
> to like encapsulation even if a tad inconvenient.

Yeah, the tabstrip really shouldn't know about fullscreen mode.  TBH, maybe
neither should the tabcontents?  You could just have the browser controller get
the right contents view to make fullscreen and then call using that.  Not sure.

[Peter Kasting, 2009-06-18 05:32:23]

Something else here -- when in fullscreen mode, you still want to respond to
various commands, specifically command-tab (or whatever Mac's "next tab"
shortcut is).  If you fullscreened the content view for a specific tab this is
going to be messy.  Is there a view that represents "the whole content area"
that individual tab contents get put inside?  If you can fullscreen that it
might be better?

[pink (ping after 24hrs), 2009-06-18 17:35:49]

I too wonder about things like the download shelf and the find bar, which should
continue to work. There is a "whole content area", the BWC has a -tabContentArea
method that holds that view. Maybe we want to make that fullscreen instead?

Also, when that view is fullscreen, what is the firstResponder? Does the BWC
still get a crack at all the commands? Do menu commands still work? Do we have
to enable/disable menu items based on whether or not that view is in full-screen
mode?

[Peter Kasting, 2009-06-18 17:40:37]

On 2009/06/18 17:35:49, pink wrote:
> Do menu commands still work? Do we have
> to enable/disable menu items based on whether or not that view is in
full-screen
> mode?

Yes, but I think that's already done for you?  browser.cc has a function called
UpdateCommandsForFullscreenMode() that gets called that should take care of
things.

Overall it might make sense to make "the whole window" fullscreen and also turn
off some bits of unwanted UI, which is sort of like what Windows does.

Another thing to be careful of here: if focus is in the address bar when you go
fullscreen, we need to move focus out, since if the user can continue to type
into a control they can't see very weird things happen.

All of this stuff can be discovered by tracing through the Windows fullscreen
mode, I'm sorry I didn't mention a lot of it earlier.  I just wasn't thinking.

[John Grabowski, 2009-06-23 23:45:27]

On 2009/06/18 17:40:37, pkasting wrote:
> On 2009/06/18 17:35:49, pink wrote:
> > Do menu commands still work? Do we have
> > to enable/disable menu items based on whether or not that view is in
> full-screen
> > mode?
> 
> Yes, but I think that's already done for you?  browser.cc has a function
called
> UpdateCommandsForFullscreenMode() that gets called that should take care of
> things.
> 
> Overall it might make sense to make "the whole window" fullscreen and also
turn
> off some bits of unwanted UI, which is sort of like what Windows does.
> 
> Another thing to be careful of here: if focus is in the address bar when you
go
> fullscreen, we need to move focus out, since if the user can continue to type
> into a control they can't see very weird things happen.
> 
> All of this stuff can be discovered by tracing through the Windows fullscreen
> mode, I'm sorry I didn't mention a lot of it earlier.  I just wasn't thinking.

New diffs uploaded.  Quick replies:
- tab switch hotkeys (cmd-opt-arrows) still work.
- find bar still works when fullscreen.
- responder chain is fine.  There is no option to be typing in the address bar
once fullscreen.
- BWC does get a crack at commands.  Although UpdateCommandsForFullscreenMode()
takes care of most things, I'm also disabling window commands (NEW_WINDOW /
CLOSE_WINDOW) since they screw up Cocoa's fullscreen mode.  I don't know why
these aren't disabled on Windows; if OK with you pkasting I'll move the
disabling to browser.cc.

[Peter Kasting, 2009-06-24 06:40:13]

On 2009/06/23 23:45:27, John Grabowski wrote:
> I'm also disabling window commands (NEW_WINDOW /
> CLOSE_WINDOW) since they screw up Cocoa's fullscreen mode.  I don't know why
> these aren't disabled on Windows; if OK with you pkasting I'll move the
> disabling to browser.cc.

I'm on vacation and thus don't really have a chance to review, but as a
drive-by, we should not be disabling window commands in fullscreen mode. 
There's nothing wrong with hitting ctrl-N to create a new window (it works fine,
and should work fine, in Windows today).  Always think from the perspective of
someone with two monitors and a fullscreen window on one; should they be able to
hit ctrl-n or ctrl-w to open or close a window, perhaps on their other screen? 
Clearly yes.

I'm not sure what it would take to make these work fine for Mac so maybe
disabling them for now is the only feasible thing, but I wouldn't want them
disabled for Windows (or, really, for Mac long-term).

[pink (ping after 24hrs), 2009-06-24 17:47:46]

I like this approach much better than the last version.

http://codereview.chromium.org/126294/diff/1011/14
File chrome/browser/cocoa/browser_window_cocoa.mm (right):

http://codereview.chromium.org/126294/diff/1011/14#newcode129
Line 129: return !![controller_ isFullscreen];
!!? huh?

http://codereview.chromium.org/126294/diff/1011/17
File chrome/browser/cocoa/tab_strip_controller.mm (right):

http://codereview.chromium.org/126294/diff/1011/17#newcode81
Line 81: 
omit blank line

http://codereview.chromium.org/126294/diff/4030/5037
File chrome/browser/cocoa/bookmark_bar_controller.mm (right):

http://codereview.chromium.org/126294/diff/4030/5037#newcode49
Line 49: [self showBookmarkBar:YES immediately:NO];
shouldn't this be YES? We don't want the animation when the window first opens.
It looks janky (several people have complained).

http://codereview.chromium.org/126294/diff/4030/5031
File chrome/browser/cocoa/browser_window_cocoa.mm (right):

http://codereview.chromium.org/126294/diff/4030/5031#newcode130
Line 130: return !![controller_ isFullscreen];
Use ternary operator here or |!= NO|. General consensus on irc is !! is way too
cryptic.

http://codereview.chromium.org/126294/diff/4030/5033
File chrome/browser/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/126294/diff/4030/5033#newcode348
Line 348: // Disable window commands if in fullscreen mode.
Here's what quicktime does on leopard: "new" is still enabled when playing a
movie in fullscreen. The menubar appears and the window shows like normal, the
previous window is still full-screen behind it. Clicking the full window
re-hides the menubar. Looks like we should probably find a way to duplicate that
behavior.

http://codereview.chromium.org/126294/diff/4030/5033#newcode593
Line 593: [bookmarkBarController_ toggleBookmarkBarNow];
does doing this affect the persisted state of the bookmark bar? shouldn't we be
able to later ask the bar controller (or prefs) for whether or not it should be
displayed, rather than caching it? Esp if we allow multiple windows to be open
while in full screen, another window could change this setting.

http://codereview.chromium.org/126294/diff/4030/5033#newcode690
Line 690: 
omit extra line

[John Grabowski, 2009-06-25 18:38:46]

Wow... a quick little CL to help pkasting out with some Mac stuff has gotten out
of hand!

Behavior now matches Windows better (can use alt windows when in fullscreen
mode), and QTPlayer better (e.g. menubar happiness).

Note new files fullscreen_window.h, fullscreen_window.mm,
fullscreen_window_unittest.mm, and a very different -[BrowserWindowController
setFullscreen], where I implement this by hand instead of using the
helpful-but-not-really Cocoa calls.

http://codereview.chromium.org/126294/diff/1011/14
File chrome/browser/cocoa/browser_window_cocoa.mm (right):

http://codereview.chromium.org/126294/diff/1011/14#newcode129
Line 129: return !![controller_ isFullscreen];
On 2009/06/24 17:47:47, pink wrote:
> !!? huh?

BOOL --> bool, defensively

http://codereview.chromium.org/126294/diff/1011/17
File chrome/browser/cocoa/tab_strip_controller.mm (right):

http://codereview.chromium.org/126294/diff/1011/17#newcode81
Line 81: 
On 2009/06/24 17:47:47, pink wrote:
> omit blank line

This file has been removed from the CL

http://codereview.chromium.org/126294/diff/4030/5037
File chrome/browser/cocoa/bookmark_bar_controller.mm (right):

http://codereview.chromium.org/126294/diff/4030/5037#newcode49
Line 49: [self showBookmarkBar:YES immediately:NO];
On 2009/06/24 17:47:47, pink wrote:
> shouldn't this be YES? We don't want the animation when the window first
opens.
> It looks janky (several people have complained).

With this CL I am preserving existing behavior.
I will certainly work on bookmark bar issues but I'd like to keep that distinct.

http://codereview.chromium.org/126294/diff/4030/5031
File chrome/browser/cocoa/browser_window_cocoa.mm (right):

http://codereview.chromium.org/126294/diff/4030/5031#newcode130
Line 130: return !![controller_ isFullscreen];
On 2009/06/24 17:47:47, pink wrote:
> Use ternary operator here or |!= NO|. General consensus on irc is !! is way
too
> cryptic.

Funny; pkasting specifically requested the !! instead of ternary.

A quick scan found ~60 !!'s in Chrome.  I'm happy to match the consensus but I'd
rather not ping-pong depending on who is the reviewer.

Is there any other reference for this decision?

http://codereview.chromium.org/126294/diff/4030/5033
File chrome/browser/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/126294/diff/4030/5033#newcode348
Line 348: // Disable window commands if in fullscreen mode.
On 2009/06/24 17:47:47, pink wrote:
> Here's what quicktime does on leopard: "new" is still enabled when playing a
> movie in fullscreen. The menubar appears and the window shows like normal, the
> previous window is still full-screen behind it. Clicking the full window
> re-hides the menubar. Looks like we should probably find a way to duplicate
that
> behavior.

Done.  Note I am no longer using the "convenient" (?) [NSView
enterFullscreenMode], I am doing things manually.   See new files
fullscreen_window* on this CL.

http://codereview.chromium.org/126294/diff/4030/5033#newcode593
Line 593: [bookmarkBarController_ toggleBookmarkBarNow];
On 2009/06/24 17:47:47, pink wrote:
> does doing this affect the persisted state of the bookmark bar? shouldn't we
be
> able to later ask the bar controller (or prefs) for whether or not it should
be
> displayed, rather than caching it? Esp if we allow multiple windows to be open
> while in full screen, another window could change this setting.

This does not change global state.  Yes, I could ask the pref instead.
I have updated the CL to handle multiple windows (by implementing the fullscreen
mode explicitly instead of using Cocoa).

http://codereview.chromium.org/126294/diff/4030/5033#newcode690
Line 690: 
On 2009/06/24 17:47:47, pink wrote:
> omit extra line

removed

[pink (ping after 24hrs), 2009-06-29 18:19:05]

LGTM

http://codereview.chromium.org/126294/diff/5070/5078
File chrome/browser/cocoa/bookmark_bar_controller.h (right):

http://codereview.chromium.org/126294/diff/5070/5078#newcode80
Line 80: // Turn on or off the bookmark bar and prevent or reallow it's
s/it's/its

http://codereview.chromium.org/126294/diff/5070/5073
File chrome/browser/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/126294/diff/5070/5073#newcode596
Line 596: // bubble describing how to exit fullscreen mode.
how would the user know where to look in the menubar to find how to get out of
fullscreen mode? I think we probably still need a bubble.

http://codereview.chromium.org/126294/diff/5070/5073#newcode618
Line 618: [content removeFromSuperview];
does anything have the window cached? are we using it as an opaque handle for
any cross-platform IPC nonsense? just making sure.

[John Grabowski, 2009-06-29 19:45:20]

http://codereview.chromium.org/126294/diff/5070/5073
File chrome/browser/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/126294/diff/5070/5073#newcode596
Line 596: // bubble describing how to exit fullscreen mode.
On 2009/06/29 18:19:09, pink wrote:
> how would the user know where to look in the menubar to find how to get out of
> fullscreen mode? I think we probably still need a bubble.

I'd think the View menu would be clear.  Note QTPlayer does not have an info
bubble.  But point taken; I've changed the comment to a question and will ask UI
dudes.

http://codereview.chromium.org/126294/diff/5070/5073#newcode618
Line 618: [content removeFromSuperview];
On 2009/06/29 18:19:09, pink wrote:
> does anything have the window cached? are we using it as an opaque handle for
> any cross-platform IPC nonsense? just making sure.

BrowserWindowCocoa caches it, but experimentation (e.g. switching apps with
cmd-tab) doesn't show any problems.