[Mac] Refactor Immersive Fullscreen Logic

[Mac] Refactor Immersive Fullscreen Logic

- Move immersive fullscreen logic from FullscreenToolbarController to its
own class.
- Fixed a bug where the menubar doesn't appear properly for immersive fullscreen

BUG=643418
Committed: https://crrev.com/69c6fdec22b2e94128f6ad016ff7b496b2e6704a
Cr-Commit-Position: refs/heads/master@{#431099}

[spqchan, 2016-11-08 22:02:44]

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

[commit-bot: I haz the power, 2016-11-08 22:03:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-09 00:10:53]

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

[commit-bot: I haz the power, 2016-11-09 00:10:53]

Dry run: This issue passed the CQ dry run.

[spqchan, 2016-11-09 00:24:05]

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

[spqchan, 2016-11-09 00:24:05]

PTAL

[erikchen, 2016-11-09 00:39:43]

lgtm

https://codereview.chromium.org/2484973004/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/fullscreen/immersive_fullscreen_controller.mm
(right):

https://codereview.chromium.org/2484973004/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/fullscreen/immersive_fullscreen_controller.mm:113:
return [self doesScreenHaveMenubar] && shouldShowMenubar_;
This method nomenclature is very confusing. I think shouldShowMenubar_ should be
renamed, maybe to desiredMenubarVisiblit_? Make sure to update the comment for
the ivar.

https://codereview.chromium.org/2484973004/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/fullscreen/immersive_fullscreen_controller.mm:158: [self
removeTrackingArea];
removeTrackingArea already does the conditional check.

https://codereview.chromium.org/2484973004/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/fullscreen/immersive_fullscreen_controller.mm:186:
DCHECK(contentView_);  // |contentView_| better be valid.
Rather than having checks here and line 180, maybe just where contentView_ gets
set?

[erikchen, 2016-11-09 00:40:00]

On 2016/11/09 00:39:43, erikchen wrote:
> lgtm
> 
>
https://codereview.chromium.org/2484973004/diff/20001/chrome/browser/ui/cocoa...
> File chrome/browser/ui/cocoa/fullscreen/immersive_fullscreen_controller.mm
> (right):
> 
>
https://codereview.chromium.org/2484973004/diff/20001/chrome/browser/ui/cocoa...
> chrome/browser/ui/cocoa/fullscreen/immersive_fullscreen_controller.mm:113:
> return [self doesScreenHaveMenubar] && shouldShowMenubar_;
> This method nomenclature is very confusing. I think shouldShowMenubar_ should
be
> renamed, maybe to desiredMenubarVisiblit_? Make sure to update the comment for
> the ivar.
> 
>
https://codereview.chromium.org/2484973004/diff/20001/chrome/browser/ui/cocoa...
> chrome/browser/ui/cocoa/fullscreen/immersive_fullscreen_controller.mm:158:
[self
> removeTrackingArea];
> removeTrackingArea already does the conditional check.
> 
>
https://codereview.chromium.org/2484973004/diff/20001/chrome/browser/ui/cocoa...
> chrome/browser/ui/cocoa/fullscreen/immersive_fullscreen_controller.mm:186:
> DCHECK(contentView_);  // |contentView_| better be valid.
> Rather than having checks here and line 180, maybe just where contentView_
gets
> set?

Please add "No intended behavior change" ot the CL description.

[spqchan, 2016-11-09 01:06:12]

Description was changed from

==========
[Mac] Refactor Immersive Fullscreen Logic

Move immersive fullscreen logic from FullscreenToolbarController to its
own class

BUG=643418
==========

to

==========
[Mac] Refactor Immersive Fullscreen Logic

- Move immersive fullscreen logic from FullscreenToolbarController to its
own class.
- Fixed a bug where the menubar doesn't appear properly for immersive fullscreen

BUG=643418
==========

[spqchan, 2016-11-09 01:06:12]

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

[spqchan, 2016-11-09 01:06:23]

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

[commit-bot: I haz the power, 2016-11-09 01:07:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-09 02:11:11]

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

[commit-bot: I haz the power, 2016-11-09 02:11:12]

Dry run: This issue passed the CQ dry run.

[spqchan, 2016-11-09 17:26:06]

thanks!

+rsesek

https://codereview.chromium.org/2484973004/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/fullscreen/immersive_fullscreen_controller.mm
(right):

https://codereview.chromium.org/2484973004/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/fullscreen/immersive_fullscreen_controller.mm:113:
return [self doesScreenHaveMenubar] && shouldShowMenubar_;
On 2016/11/09 00:39:43, erikchen wrote:
> This method nomenclature is very confusing. I think shouldShowMenubar_ should
be
> renamed, maybe to desiredMenubarVisiblit_? Make sure to update the comment for
> the ivar.

Done.

https://codereview.chromium.org/2484973004/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/fullscreen/immersive_fullscreen_controller.mm:158: [self
removeTrackingArea];
On 2016/11/09 00:39:43, erikchen wrote:
> removeTrackingArea already does the conditional check.

Done.

https://codereview.chromium.org/2484973004/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/fullscreen/immersive_fullscreen_controller.mm:186:
DCHECK(contentView_);  // |contentView_| better be valid.
On 2016/11/09 00:39:43, erikchen wrote:
> Rather than having checks here and line 180, maybe just where contentView_
gets
> set?

Done.

[Robert Sesek, 2016-11-09 19:23:03]

LGTM

https://codereview.chromium.org/2484973004/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/fullscreen/immersive_fullscreen_controller.h
(right):

https://codereview.chromium.org/2484973004/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/fullscreen/immersive_fullscreen_controller.h:18: -
(id)initWithBrowserController:(BrowserWindowController*)bwc;
instancetype

https://codereview.chromium.org/2484973004/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/fullscreen/immersive_fullscreen_controller.mm
(right):

https://codereview.chromium.org/2484973004/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/fullscreen/immersive_fullscreen_controller.mm:130: BOOL
isWindowOnPrimaryScreen = screen == primaryScreen;
Should this use isEqual: instead?

[spqchan, 2016-11-09 22:34:40]

thanks!

https://codereview.chromium.org/2484973004/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/fullscreen/immersive_fullscreen_controller.h
(right):

https://codereview.chromium.org/2484973004/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/fullscreen/immersive_fullscreen_controller.h:18: -
(id)initWithBrowserController:(BrowserWindowController*)bwc;
On 2016/11/09 19:23:03, Robert Sesek wrote:
> instancetype

Done.

https://codereview.chromium.org/2484973004/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/fullscreen/immersive_fullscreen_controller.mm
(right):

https://codereview.chromium.org/2484973004/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/fullscreen/immersive_fullscreen_controller.mm:130: BOOL
isWindowOnPrimaryScreen = screen == primaryScreen;
On 2016/11/09 19:23:03, Robert Sesek wrote:
> Should this use isEqual: instead?

Done.

[spqchan, 2016-11-09 22:34:45]

The CQ bit was checked by spqchan@chromium.org

[spqchan, 2016-11-09 22:34:45]

The patchset sent to the CQ was uploaded after l-g-t-m from
erikchen@chromium.org, rsesek@chromium.org
Link to the patchset: https://codereview.chromium.org/2484973004/#ps60001
(title: "fix for rsesek")

[commit-bot: I haz the power, 2016-11-09 22:35:19]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-10 00:33:32]

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

[commit-bot: I haz the power, 2016-11-10 00:33:37]

Failed to apply the patch.
On branch working_branch
Your branch is up-to-date with 'origin/refs/pending/heads/master'.
nothing to commit, working tree clean

[spqchan, 2016-11-10 00:37:02]

The CQ bit was checked by spqchan@chromium.org

[commit-bot: I haz the power, 2016-11-10 00:38:05]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-10 00:38:21]

Description was changed from

==========
[Mac] Refactor Immersive Fullscreen Logic

- Move immersive fullscreen logic from FullscreenToolbarController to its
own class.
- Fixed a bug where the menubar doesn't appear properly for immersive fullscreen

BUG=643418
==========

to

==========
[Mac] Refactor Immersive Fullscreen Logic

- Move immersive fullscreen logic from FullscreenToolbarController to its
own class.
- Fixed a bug where the menubar doesn't appear properly for immersive fullscreen

BUG=643418

Committed: https://crrev.com/69c6fdec22b2e94128f6ad016ff7b496b2e6704a
Cr-Commit-Position: refs/heads/master@{#431099}
==========

[commit-bot: I haz the power, 2016-11-10 00:38:23]

Patchset 4 (id:??) landed as
https://crrev.com/69c6fdec22b2e94128f6ad016ff7b496b2e6704a
Cr-Commit-Position: refs/heads/master@{#431099}