Refactor BrowserWindow fullscreen and presentation on Mac to be consistent with other platforms.

Refactor BrowserWindow fullscreen and presentation on Mac to be consistent with other platforms.

Part of a series of refactoring changes that will enable simpler code in FullscreenController as well as correcting behavior there. This change attempts to make the minimal modifications required in order to have BrowserWindow::EnterFullscreen consistently mean fullscreen with no browser chrome on all platforms.

BUG=169508
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=179399

[Robert Sesek, 2013-01-18 19:41:35]

https://codereview.chromium.org/12018007/diff/7001/chrome/browser/ui/cocoa/br...
File chrome/browser/ui/cocoa/browser_window_cocoa.mm (right):

https://codereview.chromium.org/12018007/diff/7001/chrome/browser/ui/cocoa/br...
chrome/browser/ui/cocoa/browser_window_cocoa.mm:339: [controller_
enterPresentationModeForURL:url
Why is this switching to enterPresentationMode? Shouldn't this map to
enterFullscreen so it does Lion fullscreen on 10.7+ and presentation mode/the
only FS mode on 10.6?

https://codereview.chromium.org/12018007/diff/7001/chrome/browser/ui/cocoa/br...
chrome/browser/ui/cocoa/browser_window_cocoa.mm:597: const GURL& url,
Can this URL ever be non-empty? I don't think a web page should be able to enter
the chromed fullscreen mode. Maybe worth a DCHECK or remove the param.

https://codereview.chromium.org/12018007/diff/7001/chrome/browser/ui/cocoa/br...
File chrome/browser/ui/cocoa/browser_window_cocoa_unittest.mm (right):

https://codereview.chromium.org/12018007/diff/7001/chrome/browser/ui/cocoa/br...
chrome/browser/ui/cocoa/browser_window_cocoa_unittest.mm:67: return false;
return NO

https://codereview.chromium.org/12018007/diff/7001/chrome/browser/ui/fullscre...
File chrome/browser/ui/fullscreen/fullscreen_controller.cc (right):

https://codereview.chromium.org/12018007/diff/7001/chrome/browser/ui/fullscre...
chrome/browser/ui/fullscreen/fullscreen_controller.cc:524:
window_->EnterFullscreenWithChrome(url, GetFullscreenExitBubbleType());
This should probably just be EnterFullscreen so that if the URL is empty, it
goes into the with-chrome mode, and if it is not, it goes into presentation
mode.

https://codereview.chromium.org/12018007/diff/7001/chrome/browser/ui/fullscre...
File
chrome/browser/ui/fullscreen/fullscreen_controller_interactive_browsertest.cc
(right):

https://codereview.chromium.org/12018007/diff/7001/chrome/browser/ui/fullscre...
chrome/browser/ui/fullscreen/fullscreen_controller_interactive_browsertest.cc:375:
ASSERT_TRUE(browser()->window()->IsFullscreen());
Why are these ASSERTs?

[scheib, 2013-01-18 22:12:33]

Thanks for review, I'll address the other comments, but wanted to iterate on
this before you're out to confirm intent of this change:

https://codereview.chromium.org/12018007/diff/7001/chrome/browser/ui/cocoa/br...
File chrome/browser/ui/cocoa/browser_window_cocoa.mm (right):

https://codereview.chromium.org/12018007/diff/7001/chrome/browser/ui/cocoa/br...
chrome/browser/ui/cocoa/browser_window_cocoa.mm:339: [controller_
enterPresentationModeForURL:url
On 2013/01/18 19:41:35, rsesek wrote:
> Why is this switching to enterPresentationMode? Shouldn't this map to
> enterFullscreen so it does Lion fullscreen on 10.7+ and presentation mode/the
> only FS mode on 10.6?

The goal is that 'EnterFullscreen' goes to a fullscreen without chrome on all
platforms. To enter Lion fullscreen in this change use
EnterFullscreenWithChrome. (on 10.6 and earlier EnterFullscreen and
EnterFullscreenWithChrome will both only enter a fullscreen state without
chrome. (I'll add more detail to the comments in browser_window.h))

[Robert Sesek, 2013-01-18 22:19:12]

https://codereview.chromium.org/12018007/diff/7001/chrome/browser/ui/cocoa/br...
File chrome/browser/ui/cocoa/browser_window_cocoa.mm (right):

https://codereview.chromium.org/12018007/diff/7001/chrome/browser/ui/cocoa/br...
chrome/browser/ui/cocoa/browser_window_cocoa.mm:339: [controller_
enterPresentationModeForURL:url
On 2013/01/18 22:12:33, scheib wrote:
> On 2013/01/18 19:41:35, rsesek wrote:
> > Why is this switching to enterPresentationMode? Shouldn't this map to
> > enterFullscreen so it does Lion fullscreen on 10.7+ and presentation
mode/the
> > only FS mode on 10.6?
> 
> The goal is that 'EnterFullscreen' goes to a fullscreen without chrome on all
> platforms. To enter Lion fullscreen in this change use
> EnterFullscreenWithChrome. (on 10.6 and earlier EnterFullscreen and
> EnterFullscreenWithChrome will both only enter a fullscreen state without
> chrome. (I'll add more detail to the comments in browser_window.h))

Got it and SGTM.

This may need a change in BrowserCommandController, since I believe the current
Lion fullscreen method hooks up through EnterFullscreen.

On 10.6 EnterFullscreen should call WithoutChrome and EnterFullscreenWithChrome
should assert.

[scheib, 2013-01-23 18:13:37]

https://codereview.chromium.org/12018007/diff/7001/chrome/browser/ui/cocoa/br...
File chrome/browser/ui/cocoa/browser_window_cocoa.mm (right):

https://codereview.chromium.org/12018007/diff/7001/chrome/browser/ui/cocoa/br...
chrome/browser/ui/cocoa/browser_window_cocoa.mm:597: const GURL& url,
On 2013/01/18 19:41:35, rsesek wrote:
> Can this URL ever be non-empty? I don't think a web page should be able to
enter
> the chromed fullscreen mode. Maybe worth a DCHECK or remove the param.

Done.

https://codereview.chromium.org/12018007/diff/7001/chrome/browser/ui/cocoa/br...
File chrome/browser/ui/cocoa/browser_window_cocoa_unittest.mm (right):

https://codereview.chromium.org/12018007/diff/7001/chrome/browser/ui/cocoa/br...
chrome/browser/ui/cocoa/browser_window_cocoa_unittest.mm:67: return false;
On 2013/01/18 19:41:35, rsesek wrote:
> return NO

Done.

https://codereview.chromium.org/12018007/diff/7001/chrome/browser/ui/fullscre...
File chrome/browser/ui/fullscreen/fullscreen_controller.cc (right):

https://codereview.chromium.org/12018007/diff/7001/chrome/browser/ui/fullscre...
chrome/browser/ui/fullscreen/fullscreen_controller.cc:524:
window_->EnterFullscreenWithChrome(url, GetFullscreenExitBubbleType());
On 2013/01/18 19:41:35, rsesek wrote:
> This should probably just be EnterFullscreen so that if the URL is empty, it
> goes into the with-chrome mode, and if it is not, it goes into presentation
> mode.

I'd like to keep it explicit here that we are going into Lion Fullscreen. The
URL must always be empty as well, else we're going into presentation mode and
the fullscreen controller knows that. I've added CHECKs to this effect.

https://codereview.chromium.org/12018007/diff/7001/chrome/browser/ui/fullscre...
File
chrome/browser/ui/fullscreen/fullscreen_controller_interactive_browsertest.cc
(right):

https://codereview.chromium.org/12018007/diff/7001/chrome/browser/ui/fullscre...
chrome/browser/ui/fullscreen/fullscreen_controller_interactive_browsertest.cc:375:
ASSERT_TRUE(browser()->window()->IsFullscreen());
On 2013/01/18 19:41:35, rsesek wrote:
> Why are these ASSERTs?

Done, changed to EXPECT.

[Robert Sesek, 2013-01-23 20:22:07]

ACKing receipt of the latest round. But I'm sheriff for the next two days and so
I may not have time to do a thorough review for you until after my stint is up.

[scheib, 2013-01-23 22:11:40]

Thanks for the ack, I'll pipeline up the next.

[Robert Sesek, 2013-01-28 18:36:10]

https://codereview.chromium.org/12018007/diff/16002/chrome/browser/ui/cocoa/b...
File chrome/browser/ui/cocoa/browser_window_cocoa_unittest.mm (right):

https://codereview.chromium.org/12018007/diff/16002/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/browser_window_cocoa_unittest.mm:52: enum { kNormal,
kFullscreen, kPresentation } window_state_;
naming: windowState_

https://codereview.chromium.org/12018007/diff/16002/chrome/browser/ui/fullscr...
File chrome/browser/ui/fullscreen/fullscreen_controller.cc (right):

https://codereview.chromium.org/12018007/diff/16002/chrome/browser/ui/fullscr...
chrome/browser/ui/fullscreen/fullscreen_controller.cc:279: else if
(IsFullscreenForBrowser())
nit: put braces around these bodies?

[scheib, 2013-01-28 19:32:19]

sky: owners please.

https://codereview.chromium.org/12018007/diff/16002/chrome/browser/ui/cocoa/b...
File chrome/browser/ui/cocoa/browser_window_cocoa_unittest.mm (right):

https://codereview.chromium.org/12018007/diff/16002/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/browser_window_cocoa_unittest.mm:52: enum { kNormal,
kFullscreen, kPresentation } window_state_;
On 2013/01/28 18:36:10, rsesek wrote:
> naming: windowState_

Done.

https://codereview.chromium.org/12018007/diff/16002/chrome/browser/ui/fullscr...
File chrome/browser/ui/fullscreen/fullscreen_controller.cc (right):

https://codereview.chromium.org/12018007/diff/16002/chrome/browser/ui/fullscr...
chrome/browser/ui/fullscreen/fullscreen_controller.cc:279: else if
(IsFullscreenForBrowser())
On 2013/01/28 18:36:10, rsesek wrote:
> nit: put braces around these bodies?

Done.

[scheib, 2013-01-28 19:32:19]

sky: owners please.

[Robert Sesek, 2013-01-28 19:33:06]

LGTM

[sky, 2013-01-28 22:20:27]

scheib, what specific files do you need me to review?

[scheib, 2013-01-28 23:06:03]

sky: owners review for 
chrome/browser/ui/browser_window.h
chrome/test/base/test_browser_window.h
chrome/test/base/test_browser_window.cc

[sky, 2013-01-29 15:36:31]

LGTM

[commit-bot: I haz the power, 2013-01-29 18:45:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/scheib@chromium.org/12018007/34002

[commit-bot: I haz the power, 2013-01-29 20:29:02]

Change committed as 179399