mac: Major fullscreen refactor.

mac: Major fullscreen refactor.

The previous code was incorrectly documentated, and difficult to follow.  I
went through and renamed/redocumented all the methods to reflect their actual
behavior. In the process, I determined that the behavior of the code had
diverged from what the developers thought the code did.

Full a full explanation, see http://crbug.com/406656

Functionally, this fixed several bugs:
- Attempting to fullscreen HTML5 content with --enable-simplified-fullscreen
caused UI problems.
- Omnibox/toolbar did not slide in "system fullscreen"

From a refactor standpoint:
- Deleted FullscreenModeController.
  - May want to consider replacing logic in PresentationModeController to use
  Carbon sliding menu bar callbacks.
- External (outside of BWC) invocations of fullscreen mode are much simpler.
- Significant code reduction in BWC. There was an intense amount of duplicate
logic, checking for "presentation_mode" (which had different meanings in
different contexts), checking for the simplified fullscreen flag (tightly
coupled, fragile logic) which was removed.
- Code in BWC_Private got moved around a bit, but there is minimal new logic.

I was not able to make this refactor any smaller, since the previous logic was
very tightly coupled.

BUG=406656, 405561

[erikchen, 2014-08-23 01:13:31]

rohitrao, rsesek: Since neither of you work on chrome mac, I don't know which
person is less appropriate to ask for a review.

Read the documentation I added in this CL's changes to
chrome/browser/ui/cocoa/browser_window_controller.h before beginning the review.

[Robert Sesek, 2014-08-25 15:28:54]

I wanted to patch this in to just get a sense of the behavior before reviewing,
but it doesn't compile?

../../chrome/browser/ui/cocoa/browser_window_controller_private.mm:381:17:
error: instance method '-isFullscreen' not found (return type defaults to 'id')
[-Werror,-Wobjc-method-access]
      if ([self isFullscreen])
                ^~~~~~~~~~~~
../../chrome/browser/ui/cocoa/browser_window_controller.h:68:12: note: receiver
is instance of class declared here
@interface BrowserWindowController :
           ^

[erikchen, 2014-08-25 18:05:54]

rsesek: I've rebased the change against top of tree. (a CL landed this morning
which was not compatible with my CL). I've indicated the specific git revision
you should build against if you want to experience the same build that I am
using.

[erikchen, 2014-08-26 17:19:06]

rsesek: ping

[Robert Sesek, 2014-08-27 22:04:30]

I definitely want Rohit to take a look at this, but these are some initial
notes. Sorry this took so long; it's a large CL touching complicated code.

High level, some notes:
- The renaming seems pretty inconsistent right now. I'm not sure it's effective.
I do like that you've separated in the comment UI mode from the involved API,
though.
- After patching in and testing, the behavior seems pretty good. Bug: Entering
system fullscreen, going HTML5 fullscreen, and then exiting fullscreen took me
out of HTML5 FS too. (FYI, these cross-mode transitions are the hardest part to
get right.)
- There's no reason to keep simplified fullscreen around, AFAIK. If you want to
keep it as an option to demo for UI review, note that. Otherwise, just remove
it. I can put together a CL to do that instead, if you'd like.

https://codereview.chromium.org/493143004/diff/120001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/browser_window_controller.h (right):

https://codereview.chromium.org/493143004/diff/120001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller.h:63: OMNIBOX_PRESENT,        
  // Tab strip hidden.
When is this mode used?

https://codereview.chromium.org/493143004/diff/120001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller.h:454: // I've tried to remove
all references to it. It either refers to the
Please only use the third person in comments.

https://codereview.chromium.org/493143004/diff/120001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller.h:459: // + Canonical
Fullscreen: When a user clicks on the fullscreen button, they
This is as system fullscreen.

https://codereview.chromium.org/493143004/diff/120001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller.h:477: // + Simplified
fullscreen. Hidden by default. Some users have manually
Just kill this.

https://codereview.chromium.org/493143004/diff/120001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller.h:499: -
(BOOL)isInOrEnteringAnyFullscreenMode;
Not sure how helpful the "OrEntering" naming is and I think it was covered well
enough by comments. You also left -isInImmersive alone, so it's not quite
consistent.

https://codereview.chromium.org/493143004/diff/120001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller.h:518: -
(void)enterGenericFullscreenForURL:(const GURL&)url
Why is this generic? Fullscreen for a URL is a specific mode.

https://codereview.chromium.org/493143004/diff/120001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/browser_window_controller_private.h (right):

https://codereview.chromium.org/493143004/diff/120001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller_private.h:146: -
(BOOL)commonAppKitFullscreenSetup;
This method is not named appropriately for returning a BOOL.

https://codereview.chromium.org/493143004/diff/120001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller_private.h:158: // Toggles the
AppKit Fullscreen API. By default, doing so enters System
Why reference system fullscreen if that's a deprecated term?

https://codereview.chromium.org/493143004/diff/120001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/browser_window_controller_private.mm (right):

https://codereview.chromium.org/493143004/diff/120001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:672: -
(BOOL)commonAppKitFullscreenSetup {
nit: method indention is off

https://codereview.chromium.org/493143004/diff/120001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:1118: if
(siblingBelow)
nit: needs braces since body is multi-line

[erikchen, 2014-08-28 00:29:35]

Patchset #4 (id:140001) has been deleted

[erikchen, 2014-08-28 00:50:11]

On 2014/08/27 22:04:30, rsesek wrote:
> I definitely want Rohit to take a look at this, but these are some initial
> notes. Sorry this took so long; it's a large CL touching complicated code.
> 
> High level, some notes:
> - The renaming seems pretty inconsistent right now. I'm not sure it's
effective.
> I do like that you've separated in the comment UI mode from the involved API,
> though.
I agree that it is not as effective as it could be. I still prefer it over the
previous ambiguity in "system fullscreen" and "presentation mode".

> - After patching in and testing, the behavior seems pretty good. Bug: Entering
> system fullscreen, going HTML5 fullscreen, and then exiting fullscreen took me
> out of HTML5 FS too. (FYI, these cross-mode transitions are the hardest part
to
> get right.)
The specific behavior you referenced is working as intended.
+ It is the current behavior of Chrome.
+ As a user who is watching a fullscreen video, if I exit "system fullscreen",
but the video content is still fullscreen, I would be surprised.

> - There's no reason to keep simplified fullscreen around, AFAIK. If you want
to
> keep it as an option to demo for UI review, note that. Otherwise, just remove
> it. I can put together a CL to do that instead, if you'd like.
I'm happy to remove simplified fullscreen in a future CL. It might be worth
keeping it around as demo material for a bit. Having recently discovered it, I
actually prefer it over system fullscreen.

> 
>
https://codereview.chromium.org/493143004/diff/120001/chrome/browser/ui/cocoa...
> File chrome/browser/ui/cocoa/browser_window_controller.h (right):
> 
>
https://codereview.chromium.org/493143004/diff/120001/chrome/browser/ui/cocoa...
> chrome/browser/ui/cocoa/browser_window_controller.h:63: OMNIBOX_PRESENT,      
 
>   // Tab strip hidden.
> When is this mode used?
> 
>
https://codereview.chromium.org/493143004/diff/120001/chrome/browser/ui/cocoa...
> chrome/browser/ui/cocoa/browser_window_controller.h:454: // I've tried to
remove
> all references to it. It either refers to the
> Please only use the third person in comments.
> 
>
https://codereview.chromium.org/493143004/diff/120001/chrome/browser/ui/cocoa...
> chrome/browser/ui/cocoa/browser_window_controller.h:459: // + Canonical
> Fullscreen: When a user clicks on the fullscreen button, they
> This is as system fullscreen.
> 
>
https://codereview.chromium.org/493143004/diff/120001/chrome/browser/ui/cocoa...
> chrome/browser/ui/cocoa/browser_window_controller.h:477: // + Simplified
> fullscreen. Hidden by default. Some users have manually
> Just kill this.
> 
>
https://codereview.chromium.org/493143004/diff/120001/chrome/browser/ui/cocoa...
> chrome/browser/ui/cocoa/browser_window_controller.h:499: -
> (BOOL)isInOrEnteringAnyFullscreenMode;
> Not sure how helpful the "OrEntering" naming is and I think it was covered
well
> enough by comments. You also left -isInImmersive alone, so it's not quite
> consistent.
> 
>
https://codereview.chromium.org/493143004/diff/120001/chrome/browser/ui/cocoa...
> chrome/browser/ui/cocoa/browser_window_controller.h:518: -
> (void)enterGenericFullscreenForURL:(const GURL&)url
> Why is this generic? Fullscreen for a URL is a specific mode.
> 
>
https://codereview.chromium.org/493143004/diff/120001/chrome/browser/ui/cocoa...
> File chrome/browser/ui/cocoa/browser_window_controller_private.h (right):
> 
>
https://codereview.chromium.org/493143004/diff/120001/chrome/browser/ui/cocoa...
> chrome/browser/ui/cocoa/browser_window_controller_private.h:146: -
> (BOOL)commonAppKitFullscreenSetup;
> This method is not named appropriately for returning a BOOL.
> 
>
https://codereview.chromium.org/493143004/diff/120001/chrome/browser/ui/cocoa...
> chrome/browser/ui/cocoa/browser_window_controller_private.h:158: // Toggles
the
> AppKit Fullscreen API. By default, doing so enters System
> Why reference system fullscreen if that's a deprecated term?
> 
>
https://codereview.chromium.org/493143004/diff/120001/chrome/browser/ui/cocoa...
> File chrome/browser/ui/cocoa/browser_window_controller_private.mm (right):
> 
>
https://codereview.chromium.org/493143004/diff/120001/chrome/browser/ui/cocoa...
> chrome/browser/ui/cocoa/browser_window_controller_private.mm:672: -
> (BOOL)commonAppKitFullscreenSetup {
> nit: method indention is off
> 
>
https://codereview.chromium.org/493143004/diff/120001/chrome/browser/ui/cocoa...
> chrome/browser/ui/cocoa/browser_window_controller_private.mm:1118: if
> (siblingBelow)
> nit: needs braces since body is multi-line

[erikchen, 2014-08-28 00:50:28]

rohitrao: Please review.

rsesek: PTAL

https://codereview.chromium.org/493143004/diff/120001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/browser_window_controller.h (right):

https://codereview.chromium.org/493143004/diff/120001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller.h:63: OMNIBOX_PRESENT,        
  // Tab strip hidden.
On 2014/08/27 22:04:30, rsesek wrote:
> When is this mode used?

Only for simplified fullscreen.

https://codereview.chromium.org/493143004/diff/120001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller.h:454: // I've tried to remove
all references to it. It either refers to the
On 2014/08/27 22:04:30, rsesek wrote:
> Please only use the third person in comments.

Done. I went through the whole CL and removed all first/second person comments.

https://codereview.chromium.org/493143004/diff/120001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller.h:459: // + Canonical
Fullscreen: When a user clicks on the fullscreen button, they
On 2014/08/27 22:04:30, rsesek wrote:
> This is as system fullscreen.

What do you mean by your comment? Is that a question, or something you want me
to add to my comment?

https://codereview.chromium.org/493143004/diff/120001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller.h:477: // + Simplified
fullscreen. Hidden by default. Some users have manually
On 2014/08/27 22:04:30, rsesek wrote:
> Just kill this.

Okay. I will do so in a separate CL.

https://codereview.chromium.org/493143004/diff/120001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller.h:499: -
(BOOL)isInOrEnteringAnyFullscreenMode;
On 2014/08/27 22:04:30, rsesek wrote:
> Not sure how helpful the "OrEntering" naming is and I think it was covered
well
> enough by comments. You also left -isInImmersive alone, so it's not quite
> consistent.

Updated method names to remove "OrEntering".

https://codereview.chromium.org/493143004/diff/120001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller.h:518: -
(void)enterGenericFullscreenForURL:(const GURL&)url
On 2014/08/27 22:04:30, rsesek wrote:
> Why is this generic? Fullscreen for a URL is a specific mode.

Renamed the method to enterHTML5FullscreenForURL

https://codereview.chromium.org/493143004/diff/120001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/browser_window_controller_private.h (right):

https://codereview.chromium.org/493143004/diff/120001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller_private.h:146: -
(BOOL)commonAppKitFullscreenSetup;
On 2014/08/27 22:04:30, rsesek wrote:
> This method is not named appropriately for returning a BOOL.

I moved a small amount of code around to give this method a concise name.

https://codereview.chromium.org/493143004/diff/120001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller_private.h:158: // Toggles the
AppKit Fullscreen API. By default, doing so enters System
On 2014/08/27 22:04:30, rsesek wrote:
> Why reference system fullscreen if that's a deprecated term?

Oops. That was a mistake.

https://codereview.chromium.org/493143004/diff/120001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/browser_window_controller_private.mm (right):

https://codereview.chromium.org/493143004/diff/120001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:672: -
(BOOL)commonAppKitFullscreenSetup {
On 2014/08/27 22:04:30, rsesek wrote:
> nit: method indention is off

You're right. Wonder why clang-format didn't catch that.

https://codereview.chromium.org/493143004/diff/120001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:1118: if
(siblingBelow)
On 2014/08/27 22:04:30, rsesek wrote:
> nit: needs braces since body is multi-line

Done.

[erikchen, 2014-08-28 17:33:58]

https://codereview.chromium.org/493143004/diff/160001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/browser_window_controller.mm (right):

https://codereview.chromium.org/493143004/diff/160001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller.mm:2083:
//[presentationModeController_ inPresentationMode]);
comment to self: oops, remove this.

[rohitrao (ping after 24h), 2014-08-29 19:34:13]

This looks ok to me.  I agree that the naming is confusing.  The comment helps,
but we also have too many fullscreen modes.  I am in favor of removing both
simplified fullscreen and presentation mode, leaving just appkit fullscreen and
immersive fullscreen.

Let's get this into a canary build and see how it feels.

[erikchen, 2014-08-29 19:54:46]

rsesek: Please review. :D

[Robert Sesek, 2014-08-29 20:22:42]

I don't really want to review this again, so LGTM :-D.

https://codereview.chromium.org/493143004/diff/160001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/browser_window_controller.h (right):

https://codereview.chromium.org/493143004/diff/160001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller.h:480: //  - OMNIBOX_PRESENT.
Can be with either fullscreen API.
TODO(erikchen): Remove this.

https://codereview.chromium.org/493143004/diff/160001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller.h:482: // + Flash fullscreen.
Isn't this the same as HTML5?

https://codereview.chromium.org/493143004/diff/160001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller.h:499: // Returns YES if the
code is in or entering any fullscreen mode.
nit: Code doesn't enter. Window does.

https://codereview.chromium.org/493143004/diff/160001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller.h:523: -
(void)exitGenericFullscreen;
-exitAnyFullscreen or -exitCurrentFullscreen ? Generic sounds like a mode.

https://codereview.chromium.org/493143004/diff/160001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/browser_window_controller.mm (right):

https://codereview.chromium.org/493143004/diff/160001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller.mm:2118: if ([self
isInAppKitFullscreen])
This is where I think the bug I described is. Fullscreens should "stack" s.t.
you can exit HTML5 and still be in AppKit. Leave a TODO about this, not worth
fixing now:

TODO(erikchen): Don't exit AppKit fullscreen if the user was also in HTML5
fullscreen.

https://codereview.chromium.org/493143004/diff/160001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/browser_window_controller_private.mm (right):

https://codereview.chromium.org/493143004/diff/160001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:1133: -
(void)enterAppKitFullscreen {
Maybe DCHECK(base::mac::IsOSLionOrLater());

https://codereview.chromium.org/493143004/diff/160001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:1140: -
(void)exitAppKitFullscreen {
Same.

[Robert Sesek, 2014-08-29 20:23:35]

On 2014/08/29 19:34:13, rohitrao wrote:
> This looks ok to me.  I agree that the naming is confusing.  The comment
helps,
> but we also have too many fullscreen modes.  I am in favor of removing both
> simplified fullscreen and presentation mode, leaving just appkit fullscreen
and
> immersive fullscreen.
> 
> Let's get this into a canary build and see how it feels.

+1 this. I don't really like the name "canonical fullscreen" but I definitely
understand the present need to differentiate what we have from what we want. We
can rename things once things are better UI and behavior wise.

[erikchen, 2014-08-29 20:50:38]

https://codereview.chromium.org/493143004/diff/160001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/browser_window_controller.h (right):

https://codereview.chromium.org/493143004/diff/160001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller.h:480: //  - OMNIBOX_PRESENT.
Can be with either fullscreen API.
On 2014/08/29 20:22:42, rsesek wrote:
> TODO(erikchen): Remove this.

Done.

https://codereview.chromium.org/493143004/diff/160001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller.h:482: // + Flash fullscreen.
On 2014/08/29 20:22:42, rsesek wrote:
> Isn't this the same as HTML5?

Yeah, removed it.

https://codereview.chromium.org/493143004/diff/160001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller.h:499: // Returns YES if the
code is in or entering any fullscreen mode.
On 2014/08/29 20:22:42, rsesek wrote:
> nit: Code doesn't enter. Window does.

Done.

https://codereview.chromium.org/493143004/diff/160001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller.h:523: -
(void)exitGenericFullscreen;
On 2014/08/29 20:22:42, rsesek wrote:
> -exitAnyFullscreen or -exitCurrentFullscreen ? Generic sounds like a mode.

Went with exitAnyFullscreen.

https://codereview.chromium.org/493143004/diff/160001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/browser_window_controller.mm (right):

https://codereview.chromium.org/493143004/diff/160001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller.mm:2083:
//[presentationModeController_ inPresentationMode]);
On 2014/08/28 17:33:57, erikchen wrote:
> comment to self: oops, remove this.

Done.

https://codereview.chromium.org/493143004/diff/160001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller.mm:2118: if ([self
isInAppKitFullscreen])
On 2014/08/29 20:22:42, rsesek wrote:
> This is where I think the bug I described is. Fullscreens should "stack" s.t.
> you can exit HTML5 and still be in AppKit. Leave a TODO about this, not worth
> fixing now:
> 
> TODO(erikchen): Don't exit AppKit fullscreen if the user was also in HTML5
> fullscreen.

Added comment, with slightly more detailed wording.

https://codereview.chromium.org/493143004/diff/160001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/browser_window_controller_private.mm (right):

https://codereview.chromium.org/493143004/diff/160001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:1133: -
(void)enterAppKitFullscreen {
On 2014/08/29 20:22:42, rsesek wrote:
> Maybe DCHECK(base::mac::IsOSLionOrLater());

Done.

https://codereview.chromium.org/493143004/diff/160001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:1140: -
(void)exitAppKitFullscreen {
On 2014/08/29 20:22:42, rsesek wrote:
> Same.

Done.

[erikchen, 2014-09-02 20:33:57]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2014-09-02 20:36:28]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/erikchen@chromium.org/493143004/220001

[erikchen, 2014-09-02 20:37:42]

The CQ bit was unchecked by erikchen@chromium.org

[erikchen, 2014-09-04 17:12:37]

This CL was merged into
https://codereview.chromium.org/533383002/