Switch HTML5 Fullscreen Over to AppKit

Made changes so that the transition to HTML5 fullscreen uses AppKit
instead of Immersive in 10.10+. A flag |blockLayoutSubviews_| is
implemented to prevent spurious resize messages from being sent
during the transition.

BUG=519763
Committed: https://crrev.com/2a09e4bff1e12bd7ea8d572709878f5de7c406f8
Cr-Commit-Position: refs/heads/master@{#353166}

[spqchan, 2015-10-07 20:41:46]

spqchan@chromium.org changed reviewers:
+ erikchen@chromium.org

[spqchan, 2015-10-07 20:41:46]

PTAL

[erikchen, 2015-10-07 21:51:03]

https://codereview.chromium.org/1387473002/diff/220001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/browser_window_controller.mm (right):

https://codereview.chromium.org/1387473002/diff/220001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_controller.mm:1970: [self
exitAppKitFullscreen];
I'm confused - should this be enterAppKitFullscreen?

Also, is this going to have the write sliding style?

https://codereview.chromium.org/1387473002/diff/220001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/browser_window_controller_private.mm (right):

https://codereview.chromium.org/1387473002/diff/220001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:1112: // if
(!enterFullScreen)
Huh? What are you trying to do here? If you want the function to always return
"NO", remove the commented out lines, and update the CL description.

https://codereview.chromium.org/1387473002/diff/220001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:1178:
blockLayoutSubviews_ = NO;
This code assumes that blockLayoutSubviews_ is YES. Instead, you should use
base/auto_reset.h.

[spqchan, 2015-10-07 22:54:01]

PTAL

https://codereview.chromium.org/1387473002/diff/220001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/browser_window_controller.mm (right):

https://codereview.chromium.org/1387473002/diff/220001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_controller.mm:1970: [self
exitAppKitFullscreen];
On 2015/10/07 21:51:03, erikchen wrote:
> I'm confused - should this be enterAppKitFullscreen?
> 
> Also, is this going to have the write sliding style?

Whoops, yes this should be enterAppKitFullscreen 
(Fyi, enterAppKitFullscreen and exitAppKitFullscreen have the same
implementation so there's no need to make any further changes)

Also, yes this will have the right sliding style. See
adjustUIForEnteringFullscreen

https://codereview.chromium.org/1387473002/diff/220001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/browser_window_controller_private.mm (right):

https://codereview.chromium.org/1387473002/diff/220001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:1112: // if
(!enterFullScreen)
On 2015/10/07 21:51:03, erikchen wrote:
> Huh? What are you trying to do here? If you want the function to always return
> "NO", remove the commented out lines, and update the CL description.

This change was accidentally added to this CL when I was testing things on 10.11
Removing it.

https://codereview.chromium.org/1387473002/diff/220001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:1178:
blockLayoutSubviews_ = NO;
On 2015/10/07 21:51:03, erikchen wrote:
> This code assumes that blockLayoutSubviews_ is YES. Instead, you should use
> base/auto_reset.h.

Done.

[erikchen, 2015-10-07 23:17:53]

https://codereview.chromium.org/1387473002/diff/220001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/browser_window_controller.mm (right):

https://codereview.chromium.org/1387473002/diff/220001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_controller.mm:1970: [self
exitAppKitFullscreen];
On 2015/10/07 22:54:01, spqchan wrote:
> On 2015/10/07 21:51:03, erikchen wrote:
> > I'm confused - should this be enterAppKitFullscreen?
> > 
> > Also, is this going to have the write sliding style?
> 
> Whoops, yes this should be enterAppKitFullscreen 
> (Fyi, enterAppKitFullscreen and exitAppKitFullscreen have the same
> implementation so there's no need to make any further changes)
> 
> Also, yes this will have the right sliding style. See
> adjustUIForEnteringFullscreen

adjustUIForEnteringFullscreen relies on enteringPresentationMode_ to get the
right style, but that variable isn't set to YES here.

https://codereview.chromium.org/1387473002/diff/260001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/browser_window_controller_private.mm (right):

https://codereview.chromium.org/1387473002/diff/260001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:804: -
(void)adjustUIForEnteringFullscreen {
Add declarations for this method and the one above it, with comments on their
functionality.

[spqchan, 2015-10-07 23:53:26]

https://codereview.chromium.org/1387473002/diff/220001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/browser_window_controller.mm (right):

https://codereview.chromium.org/1387473002/diff/220001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_controller.mm:1970: [self
exitAppKitFullscreen];
On 2015/10/07 23:17:53, erikchen wrote:
> On 2015/10/07 22:54:01, spqchan wrote:
> > On 2015/10/07 21:51:03, erikchen wrote:
> > > I'm confused - should this be enterAppKitFullscreen?
> > > 
> > > Also, is this going to have the write sliding style?
> > 
> > Whoops, yes this should be enterAppKitFullscreen 
> > (Fyi, enterAppKitFullscreen and exitAppKitFullscreen have the same
> > implementation so there's no need to make any further changes)
> > 
> > Also, yes this will have the right sliding style. See
> > adjustUIForEnteringFullscreen
> 
> adjustUIForEnteringFullscreen relies on enteringPresentationMode_ to get the
> right style, but that variable isn't set to YES here. 

In adjustUIForEnteringFullscreen, we check if the fullscreen is for tab content
(which includes HTML5):
if (browser_->exclusive_access_manager()
          ->fullscreen_controller()
          ->IsWindowFullscreenForTabOrPending()) {
    style = fullscreen_mac::OMNIBOX_TABS_NONE;
}

https://codereview.chromium.org/1387473002/diff/260001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/browser_window_controller_private.mm (right):

https://codereview.chromium.org/1387473002/diff/260001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:804: -
(void)adjustUIForEnteringFullscreen {
On 2015/10/07 23:17:53, erikchen wrote:
> Add declarations for this method and the one above it, with comments on their
> functionality.

Done.

[erikchen, 2015-10-07 23:57:41]

lgtm

[spqchan, 2015-10-08 00:01:50]

spqchan@chromium.org changed reviewers:
+ rsesek@chromium.org

[spqchan, 2015-10-08 00:01:50]

+rsesek

[spqchan, 2015-10-08 00:01:53]

+rsesek

[Robert Sesek, 2015-10-08 15:21:22]

https://codereview.chromium.org/1387473002/diff/300001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/browser_window_controller.mm (right):

https://codereview.chromium.org/1387473002/diff/300001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_controller.mm:1971: if
(chrome::mac::SupportsSystemFullscreen() && base::mac::IsOSLionOrLater())
Is the condition after && necessary?

https://codereview.chromium.org/1387473002/diff/300001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/browser_window_controller_private.mm (right):

https://codereview.chromium.org/1387473002/diff/300001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:1168:
base::AutoReset<BOOL> auto_reset_in_run(&blockLayoutSubviews_, NO);
naming: autoReset (what does |in_run| mean ?)

https://codereview.chromium.org/1387473002/diff/300001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:1178:
base::AutoReset<BOOL> auto_reset_in_run(&blockLayoutSubviews_, NO);
naming: same

[spqchan, 2015-10-08 16:54:28]

PTAL

https://codereview.chromium.org/1387473002/diff/300001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/browser_window_controller.mm (right):

https://codereview.chromium.org/1387473002/diff/300001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_controller.mm:1971: if
(chrome::mac::SupportsSystemFullscreen() && base::mac::IsOSLionOrLater())
On 2015/10/08 15:21:22, Robert Sesek wrote:
> Is the condition after && necessary?

It's not. I removed it

https://codereview.chromium.org/1387473002/diff/300001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/browser_window_controller_private.mm (right):

https://codereview.chromium.org/1387473002/diff/300001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:1168:
base::AutoReset<BOOL> auto_reset_in_run(&blockLayoutSubviews_, NO);
On 2015/10/08 15:21:22, Robert Sesek wrote:
> naming: autoReset (what does |in_run| mean ?)

At the end of the variable's run in this scope, it will auto reset.
I named this according to some uses of AutoReset I found in .mm files
Would it be more appropriate if I name it auto_reset_block_layout_subviews?

https://codereview.chromium.org/1387473002/diff/300001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:1178:
base::AutoReset<BOOL> auto_reset_in_run(&blockLayoutSubviews_, NO);
On 2015/10/08 15:21:22, Robert Sesek wrote:
> naming: same

Same

[Robert Sesek, 2015-10-08 17:02:42]

https://codereview.chromium.org/1387473002/diff/300001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/browser_window_controller_private.mm (right):

https://codereview.chromium.org/1387473002/diff/300001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:1168:
base::AutoReset<BOOL> auto_reset_in_run(&blockLayoutSubviews_, NO);
On 2015/10/08 16:54:28, spqchan wrote:
> On 2015/10/08 15:21:22, Robert Sesek wrote:
> > naming: autoReset (what does |in_run| mean ?)
> 
> At the end of the variable's run in this scope, it will auto reset.
> I named this according to some uses of AutoReset I found in .mm files
> Would it be more appropriate if I name it auto_reset_block_layout_subviews?

I think the type and the pointer make this obvious. I'd just use |autoReset| (no
underscores in ObjC methods).

[spqchan, 2015-10-08 17:11:43]

PTAL

[Robert Sesek, 2015-10-08 17:17:11]

https://codereview.chromium.org/1387473002/diff/340001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/browser_window_controller_private.mm (right):

https://codereview.chromium.org/1387473002/diff/340001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:1168:
base::AutoReset<BOOL> auto_reset(&blockLayoutSubviews_, NO);
naming: autoReset. No underscores in ObjC methods.

[spqchan, 2015-10-08 17:23:26]

PTAL

https://codereview.chromium.org/1387473002/diff/340001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/browser_window_controller_private.mm (right):

https://codereview.chromium.org/1387473002/diff/340001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:1168:
base::AutoReset<BOOL> auto_reset(&blockLayoutSubviews_, NO);
On 2015/10/08 17:17:10, Robert Sesek wrote:
> naming: autoReset. No underscores in ObjC methods.

Done.

[Robert Sesek, 2015-10-08 17:28:43]

LGTM

[spqchan, 2015-10-08 17:29:56]

The CQ bit was checked by spqchan@chromium.org

[spqchan, 2015-10-08 17:29:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
erikchen@chromium.org
Link to the patchset: https://codereview.chromium.org/1387473002/#ps360001
(title: "Remove underscores")

[commit-bot: I haz the power, 2015-10-08 17:30:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1387473002/360001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1387473002/360001

[commit-bot: I haz the power, 2015-10-08 18:37:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-08 18:37:10]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[spqchan, 2015-10-08 21:42:45]

The CQ bit was checked by spqchan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-10-08 21:46:05]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1387473002/380001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1387473002/380001

[commit-bot: I haz the power, 2015-10-08 22:43:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-08 22:43:40]

Dry run: This issue passed the CQ dry run.

[spqchan, 2015-10-08 22:55:46]

The CQ bit was checked by spqchan@chromium.org

[spqchan, 2015-10-08 22:55:46]

The patchset sent to the CQ was uploaded after l-g-t-m from rsesek@chromium.org,
erikchen@chromium.org
Link to the patchset: https://codereview.chromium.org/1387473002/#ps380001
(title: "Add check for 10.10+")

[commit-bot: I haz the power, 2015-10-08 22:58:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1387473002/380001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1387473002/380001

[commit-bot: I haz the power, 2015-10-08 23:08:23]

Committed patchset #20 (id:380001)

[commit-bot: I haz the power, 2015-10-08 23:09:08]

Patchset 20 (id:??) landed as
https://crrev.com/2a09e4bff1e12bd7ea8d572709878f5de7c406f8
Cr-Commit-Position: refs/heads/master@{#353166}