[Mac] Refactor the fullscreen menubar

[Mac] Refactor the fullscreen menubar

Move the menubar logic from FullscreenToolbarController into a new class.

BUG=643418
Committed: https://crrev.com/1b5163a06e58d790add38bac69f94b37e69ef03e
Cr-Commit-Position: refs/heads/master@{#426330}

[spqchan, 2016-10-19 00:24:15]

Description was changed from

==========
[Mac] Refactor fullscreen menubar logic

Move the menubar logic from FullscreenToolbarController into a new class

BUG=
==========

to

==========
[Mac] Refactor the fullscreen menubar

Move the menubar logic from FullscreenToolbarController into a new class.

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

[spqchan, 2016-10-19 00:24:15]

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

[spqchan, 2016-10-19 00:48:46]

PTAL

[erikchen, 2016-10-19 01:43:23]

lgtm with question.

https://codereview.chromium.org/2430863002/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm (right):

https://codereview.chromium.org/2430863002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm:312: [self
updateTrackingArea];
in the code path that you're replacing, lines 311 and 312 are called in a
different order. Is this relevant?

[spqchan, 2016-10-19 01:51:23]

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

[spqchan, 2016-10-19 01:51:23]

thanks!

+ rsesek for owner

FYI, this CL is separated from https://codereview.chromium.org/2355413007/

https://codereview.chromium.org/2430863002/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm (right):

https://codereview.chromium.org/2430863002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm:312: [self
updateTrackingArea];
On 2016/10/19 01:43:23, erikchen wrote:
> in the code path that you're replacing, lines 311 and 312 are called in a
> different order. Is this relevant?

Good point. Right now the order doesn't matter but after I move mouse tracking
to a separate class, it will.

[spqchan, 2016-10-19 02:22:06]

+thakis for base/mac ownership

[spqchan, 2016-10-19 02:23:04]

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

[spqchan, 2016-10-19 02:23:05]

+mark for base/mac ownership

[Mark Mentovai, 2016-10-19 02:36:10]

LGTM otherwise in base

https://codereview.chromium.org/2430863002/diff/40001/base/mac/mac_util.mm
File base/mac/mac_util.mm (right):

https://codereview.chromium.org/2430863002/diff/40001/base/mac/mac_util.mm#ne...
base/mac/mac_util.mm:177: return fabs(a - b) <=
std::numeric_limits<CGFloat>::epsilon();
#include <cmath> and use std::fabs() or std::abs() instead of this.

fabs always operates on and returns doubles. The namespaced variants operate on
the type of their argument, so they’ll be correct regardless of what CGFloat
turns out to be.

[spqchan, 2016-10-19 03:46:13]

thanks!

https://codereview.chromium.org/2430863002/diff/40001/base/mac/mac_util.mm
File base/mac/mac_util.mm (right):

https://codereview.chromium.org/2430863002/diff/40001/base/mac/mac_util.mm#ne...
base/mac/mac_util.mm:177: return fabs(a - b) <=
std::numeric_limits<CGFloat>::epsilon();
On 2016/10/19 02:36:09, Mark Mentovai wrote:
> #include <cmath> and use std::fabs() or std::abs() instead of this.
> 
> fabs always operates on and returns doubles. The namespaced variants operate
on
> the type of their argument, so they’ll be correct regardless of what CGFloat
> turns out to be.

Done.

[Robert Sesek, 2016-10-19 18:49:22]

Nice cleanup.

https://codereview.chromium.org/2430863002/diff/60001/base/mac/mac_util.h
File base/mac/mac_util.h (right):

https://codereview.chromium.org/2430863002/diff/60001/base/mac/mac_util.h#new...
base/mac/mac_util.h:49: BASE_EXPORT bool IsCGFloatEqual(CGFloat a, CGFloat b);
Could this go in ui/base/cocoa instead?

https://codereview.chromium.org/2430863002/diff/60001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/fullscreen/fullscreen_menubar_tracker.h (right):

https://codereview.chromium.org/2430863002/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/fullscreen/fullscreen_menubar_tracker.h:8: #include
<Carbon/Carbon.h>
Carbon unused in the header. Move into the impl.

https://codereview.chromium.org/2430863002/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/fullscreen/fullscreen_menubar_tracker.h:31: -
(id)initWithFullscreenToolbarController:(FullscreenToolbarController*)owner;
instancetype in new code

https://codereview.chromium.org/2430863002/diff/60001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/fullscreen/fullscreen_menubar_tracker.mm (right):

https://codereview.chromium.org/2430863002/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/fullscreen/fullscreen_menubar_tracker.mm:69:
browserController_ = [owner_ browserWindowController];
naming: browserWindowController_ instead

https://codereview.chromium.org/2430863002/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/fullscreen/fullscreen_menubar_tracker.mm:73: // Install
the Carbon event handler for the menubar show, hide and
Move this code into the if block as well?

https://codereview.chromium.org/2430863002/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/fullscreen/fullscreen_menubar_tracker.mm:86:
InstallApplicationEventHandler(NewEventHandlerUPP(&MenuBarRevealHandler), 3,
arraysize() instead of 3

[spqchan, 2016-10-19 19:28:42]

PTAL

https://codereview.chromium.org/2430863002/diff/60001/base/mac/mac_util.h
File base/mac/mac_util.h (right):

https://codereview.chromium.org/2430863002/diff/60001/base/mac/mac_util.h#new...
base/mac/mac_util.h:49: BASE_EXPORT bool IsCGFloatEqual(CGFloat a, CGFloat b);
On 2016/10/19 18:49:21, Robert Sesek wrote:
> Could this go in ui/base/cocoa instead?

Sure thing, I put it in appkit_utils.

https://codereview.chromium.org/2430863002/diff/60001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/fullscreen/fullscreen_menubar_tracker.h (right):

https://codereview.chromium.org/2430863002/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/fullscreen/fullscreen_menubar_tracker.h:8: #include
<Carbon/Carbon.h>
On 2016/10/19 18:49:21, Robert Sesek wrote:
> Carbon unused in the header. Move into the impl.

Done.

https://codereview.chromium.org/2430863002/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/fullscreen/fullscreen_menubar_tracker.h:31: -
(id)initWithFullscreenToolbarController:(FullscreenToolbarController*)owner;
On 2016/10/19 18:49:21, Robert Sesek wrote:
> instancetype in new code

Done.

https://codereview.chromium.org/2430863002/diff/60001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/fullscreen/fullscreen_menubar_tracker.mm (right):

https://codereview.chromium.org/2430863002/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/fullscreen/fullscreen_menubar_tracker.mm:69:
browserController_ = [owner_ browserWindowController];
On 2016/10/19 18:49:21, Robert Sesek wrote:
> naming: browserWindowController_ instead

Done.

https://codereview.chromium.org/2430863002/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/fullscreen/fullscreen_menubar_tracker.mm:73: // Install
the Carbon event handler for the menubar show, hide and
On 2016/10/19 18:49:21, Robert Sesek wrote:
> Move this code into the if block as well?

Done.

https://codereview.chromium.org/2430863002/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/fullscreen/fullscreen_menubar_tracker.mm:86:
InstallApplicationEventHandler(NewEventHandlerUPP(&MenuBarRevealHandler), 3,
On 2016/10/19 18:49:21, Robert Sesek wrote:
> arraysize() instead of 3

Done.

[Robert Sesek, 2016-10-19 21:06:52]

LGTM w/ IWYU

https://codereview.chromium.org/2430863002/diff/80001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/fullscreen/fullscreen_menubar_tracker.mm (right):

https://codereview.chromium.org/2430863002/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/fullscreen/fullscreen_menubar_tracker.mm:8: 
#include "base/macros.h"

https://codereview.chromium.org/2430863002/diff/80001/ui/base/cocoa/appkit_ut...
File ui/base/cocoa/appkit_utils.mm (right):

https://codereview.chromium.org/2430863002/diff/80001/ui/base/cocoa/appkit_ut...
ui/base/cocoa/appkit_utils.mm:6: 
#include <math>

[spqchan, 2016-10-19 21:44:16]

thanks!

https://codereview.chromium.org/2430863002/diff/80001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/fullscreen/fullscreen_menubar_tracker.mm (right):

https://codereview.chromium.org/2430863002/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/fullscreen/fullscreen_menubar_tracker.mm:8: 
On 2016/10/19 21:06:52, Robert Sesek wrote:
> #include "base/macros.h"

Done.

https://codereview.chromium.org/2430863002/diff/80001/ui/base/cocoa/appkit_ut...
File ui/base/cocoa/appkit_utils.mm (right):

https://codereview.chromium.org/2430863002/diff/80001/ui/base/cocoa/appkit_ut...
ui/base/cocoa/appkit_utils.mm:6: 
On 2016/10/19 21:06:52, Robert Sesek wrote:
> #include <math>

Done.

[spqchan, 2016-10-19 21:44:26]

The CQ bit was checked by spqchan@chromium.org

[spqchan, 2016-10-19 21:44:26]

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

[commit-bot: I haz the power, 2016-10-19 21:45:22]

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-10-19 23:22:41]

Description was changed from

==========
[Mac] Refactor the fullscreen menubar

Move the menubar logic from FullscreenToolbarController into a new class.

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

to

==========
[Mac] Refactor the fullscreen menubar

Move the menubar logic from FullscreenToolbarController into a new class.

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

[commit-bot: I haz the power, 2016-10-19 23:22:42]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-10-21 13:13:01]

Description was changed from

==========
[Mac] Refactor the fullscreen menubar

Move the menubar logic from FullscreenToolbarController into a new class.

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

to

==========
[Mac] Refactor the fullscreen menubar

Move the menubar logic from FullscreenToolbarController into a new class.

BUG=643418

Committed: https://crrev.com/1b5163a06e58d790add38bac69f94b37e69ef03e
Cr-Commit-Position: refs/heads/master@{#426330}
==========

[commit-bot: I haz the power, 2016-10-21 13:13:02]

Patchset 6 (id:??) landed as
https://crrev.com/1b5163a06e58d790add38bac69f94b37e69ef03e
Cr-Commit-Position: refs/heads/master@{#426330}