[Mac] Fullscreen Toolbar Edge Cases

[Mac] Fullscreen Toolbar Bar Visibility Edge Cases

Fixed edge case for the toolbar visibility locks.

BUG=641751, 641749
Committed: https://crrev.com/a431d992f7c15f070be18e6a51419c11445a785a
Cr-Commit-Position: refs/heads/master@{#417799}

[spqchan, 2016-08-30 19:27:55]

Description was changed from

==========
lol more fullscreen edge cases

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

to

==========
lol more fullscreen edge cases

BUG=642446
==========

[spqchan, 2016-08-30 23:17:57]

Description was changed from

==========
lol more fullscreen edge cases

BUG=642446
==========

to

==========
[Mac] Fullscreen Toolbar Edge Cases

Fixed conflicts between fullscreen toolbar tracking area, visibility locks
and animation.

BUG=641751, 641749, 642446
==========

[spqchan, 2016-08-30 23:17:57]

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

[spqchan, 2016-08-30 23:18:07]

Description was changed from

==========
[Mac] Fullscreen Toolbar Edge Cases

Fixed conflicts between fullscreen toolbar tracking area, visibility locks
and animation.

BUG=641751, 641749, 642446
==========

to

==========
[Mac] Fullscreen Toolbar Edge Cases

Fixed conflicts between fullscreen toolbar tracking area,
visibility locks and animation.

BUG=641751, 641749, 642446
==========

[spqchan, 2016-08-30 23:21:15]

More edge cases to fix. There were conflicting states between the visibility
locks, animation and tracking area.

Let me know if this getting too confusing/complex. I'm trying to keep things
simple but it's a bit hard to accommodate for everything

[erikchen, 2016-08-30 23:46:48]

My response became kind of huge. Let me know you want to talk in person.
Alternatively, I could write a CL with more details showing (rather than
describing) what I mean.

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

https://codereview.chromium.org/2296903002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm:21: const
NSTimeInterval kDropdownAnimationDuration = 0.17;
Should this vary by OS version?

https://codereview.chromium.org/2296903002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm:480:
hiddenToolbarState_ = fullscreen_mac::TOOLBAR_SHOWN;
This logic confuses me. Can we please move more logic into -toolbarFraction, and
less logic in the callbacks from AppKit?

It seems like we want something along the lines of:

1) If an NSMenuBar animation is in progress, use a state based on the progress
of the menubar.
2) Otherwise, if a toolbar animation is in progress, use a state based on the
progress of the animation.
3) Otherwise, use shouldShowToolbarWithoutMenu_.

Ditto for shouldShowToolbarWithoutMenu_. It's really not clear to me what the
ivar means. From the logic, it looks like it means:

If the NSMenuBar is in animation, and there is no toolbar animation in progress,
should we show the full height of the toolbar, or just part of the height.

But this can be simplified:
Let's say that we have the state: enum NSMenuBarState { HIDDEN, SHOWING,
TRANSITIONING}. And if state = TRANSITIONING, there are two further states:
CGFloat menubarFraction_
enum NSMenuBarAnimationDirection { IN, OUT}

Then 
"""
return shouldShowToolbarWithoutMenu_ ? kShowFraction : menubarFraction_;
"""

becomes:
"""
return direction == OUT ? kShowFraction : menubarFraction_
"""

AFAICT, that will have the same behavior, but without needing this intermediate
variable of shouldShowToolbarWithoutMenu_. 

Looking at your code, right now the question I'm having trouble answering is:
What happens when there's a Chrome animation in progress, and the NSMenuBar is
animating? hiddenToolbarState_ is changed by both of those processes.

[spqchan, 2016-08-30 23:49:16]

On 2016/08/30 23:46:48, erikchen wrote:
> My response became kind of huge. Let me know you want to talk in person.
> Alternatively, I could write a CL with more details showing (rather than
> describing) what I mean.
> 
>
https://codereview.chromium.org/2296903002/diff/40001/chrome/browser/ui/cocoa...
> File chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm (right):
> 
>
https://codereview.chromium.org/2296903002/diff/40001/chrome/browser/ui/cocoa...
> chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm:21: const
> NSTimeInterval kDropdownAnimationDuration = 0.17;
> Should this vary by OS version?
> 
>
https://codereview.chromium.org/2296903002/diff/40001/chrome/browser/ui/cocoa...
> chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm:480:
> hiddenToolbarState_ = fullscreen_mac::TOOLBAR_SHOWN;
> This logic confuses me. Can we please move more logic into -toolbarFraction,
and
> less logic in the callbacks from AppKit?
> 
> It seems like we want something along the lines of:
> 
> 1) If an NSMenuBar animation is in progress, use a state based on the progress
> of the menubar.
> 2) Otherwise, if a toolbar animation is in progress, use a state based on the
> progress of the animation.
> 3) Otherwise, use shouldShowToolbarWithoutMenu_.
> 
> Ditto for shouldShowToolbarWithoutMenu_. It's really not clear to me what the
> ivar means. From the logic, it looks like it means:
> 
> If the NSMenuBar is in animation, and there is no toolbar animation in
progress,
> should we show the full height of the toolbar, or just part of the height.
> 
> But this can be simplified:
> Let's say that we have the state: enum NSMenuBarState { HIDDEN, SHOWING,
> TRANSITIONING}. And if state = TRANSITIONING, there are two further states:
> CGFloat menubarFraction_
> enum NSMenuBarAnimationDirection { IN, OUT}
> 
> Then 
> """
> return shouldShowToolbarWithoutMenu_ ? kShowFraction : menubarFraction_;
> """
> 
> becomes:
> """
> return direction == OUT ? kShowFraction : menubarFraction_
> """
> 
> AFAICT, that will have the same behavior, but without needing this
intermediate
> variable of shouldShowToolbarWithoutMenu_. 
> 
> Looking at your code, right now the question I'm having trouble answering is:
> What happens when there's a Chrome animation in progress, and the NSMenuBar is
> animating? hiddenToolbarState_ is changed by both of those processes.

I'm leaving soon so I'll get back to you on the CL tomorrow. I think it might be
better to do it in person, just let me collect my thoughts on all of the edge
cases

[erikchen, 2016-08-31 00:27:17]

On 2016/08/30 23:49:16, spqchan wrote:
> On 2016/08/30 23:46:48, erikchen wrote:
> > My response became kind of huge. Let me know you want to talk in person.
> > Alternatively, I could write a CL with more details showing (rather than
> > describing) what I mean.
> > 
> >
>
https://codereview.chromium.org/2296903002/diff/40001/chrome/browser/ui/cocoa...
> > File chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm (right):
> > 
> >
>
https://codereview.chromium.org/2296903002/diff/40001/chrome/browser/ui/cocoa...
> > chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm:21: const
> > NSTimeInterval kDropdownAnimationDuration = 0.17;
> > Should this vary by OS version?
> > 
> >
>
https://codereview.chromium.org/2296903002/diff/40001/chrome/browser/ui/cocoa...
> > chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm:480:
> > hiddenToolbarState_ = fullscreen_mac::TOOLBAR_SHOWN;
> > This logic confuses me. Can we please move more logic into -toolbarFraction,
> and
> > less logic in the callbacks from AppKit?
> > 
> > It seems like we want something along the lines of:
> > 
> > 1) If an NSMenuBar animation is in progress, use a state based on the
progress
> > of the menubar.
> > 2) Otherwise, if a toolbar animation is in progress, use a state based on
the
> > progress of the animation.
> > 3) Otherwise, use shouldShowToolbarWithoutMenu_.
> > 
> > Ditto for shouldShowToolbarWithoutMenu_. It's really not clear to me what
the
> > ivar means. From the logic, it looks like it means:
> > 
> > If the NSMenuBar is in animation, and there is no toolbar animation in
> progress,
> > should we show the full height of the toolbar, or just part of the height.
> > 
> > But this can be simplified:
> > Let's say that we have the state: enum NSMenuBarState { HIDDEN, SHOWING,
> > TRANSITIONING}. And if state = TRANSITIONING, there are two further states:
> > CGFloat menubarFraction_
> > enum NSMenuBarAnimationDirection { IN, OUT}
> > 
> > Then 
> > """
> > return shouldShowToolbarWithoutMenu_ ? kShowFraction : menubarFraction_;
> > """
> > 
> > becomes:
> > """
> > return direction == OUT ? kShowFraction : menubarFraction_
> > """
> > 
> > AFAICT, that will have the same behavior, but without needing this
> intermediate
> > variable of shouldShowToolbarWithoutMenu_. 
> > 
> > Looking at your code, right now the question I'm having trouble answering
is:
> > What happens when there's a Chrome animation in progress, and the NSMenuBar
is
> > animating? hiddenToolbarState_ is changed by both of those processes.
> 
> I'm leaving soon so I'll get back to you on the CL tomorrow. I think it might
be
> better to do it in person, just let me collect my thoughts on all of the edge
> cases

I put together a quick CL demoing what I meant when I asked you to put more
logic in -toolbarFraction. 
https://codereview.chromium.org/2296663003/

Let's discuss this more tomorrow.

[spqchan, 2016-08-31 17:36:41]

On 2016/08/31 00:27:17, erikchen wrote:
> On 2016/08/30 23:49:16, spqchan wrote:
> > On 2016/08/30 23:46:48, erikchen wrote:
> > > My response became kind of huge. Let me know you want to talk in person.
> > > Alternatively, I could write a CL with more details showing (rather than
> > > describing) what I mean.
> > > 
> > >
> >
>
https://codereview.chromium.org/2296903002/diff/40001/chrome/browser/ui/cocoa...
> > > File chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2296903002/diff/40001/chrome/browser/ui/cocoa...
> > > chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm:21: const
> > > NSTimeInterval kDropdownAnimationDuration = 0.17;
> > > Should this vary by OS version?
> > > 
> > >
> >
>
https://codereview.chromium.org/2296903002/diff/40001/chrome/browser/ui/cocoa...
> > > chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm:480:
> > > hiddenToolbarState_ = fullscreen_mac::TOOLBAR_SHOWN;
> > > This logic confuses me. Can we please move more logic into
-toolbarFraction,
> > and
> > > less logic in the callbacks from AppKit?
> > > 
> > > It seems like we want something along the lines of:
> > > 
> > > 1) If an NSMenuBar animation is in progress, use a state based on the
> progress
> > > of the menubar.
> > > 2) Otherwise, if a toolbar animation is in progress, use a state based on
> the
> > > progress of the animation.
> > > 3) Otherwise, use shouldShowToolbarWithoutMenu_.
> > > 
> > > Ditto for shouldShowToolbarWithoutMenu_. It's really not clear to me what
> the
> > > ivar means. From the logic, it looks like it means:
> > > 
> > > If the NSMenuBar is in animation, and there is no toolbar animation in
> > progress,
> > > should we show the full height of the toolbar, or just part of the height.
> > > 
> > > But this can be simplified:
> > > Let's say that we have the state: enum NSMenuBarState { HIDDEN, SHOWING,
> > > TRANSITIONING}. And if state = TRANSITIONING, there are two further
states:
> > > CGFloat menubarFraction_
> > > enum NSMenuBarAnimationDirection { IN, OUT}
> > > 
> > > Then 
> > > """
> > > return shouldShowToolbarWithoutMenu_ ? kShowFraction : menubarFraction_;
> > > """
> > > 
> > > becomes:
> > > """
> > > return direction == OUT ? kShowFraction : menubarFraction_
> > > """
> > > 
> > > AFAICT, that will have the same behavior, but without needing this
> > intermediate
> > > variable of shouldShowToolbarWithoutMenu_. 
> > > 
> > > Looking at your code, right now the question I'm having trouble answering
> is:
> > > What happens when there's a Chrome animation in progress, and the
NSMenuBar
> is
> > > animating? hiddenToolbarState_ is changed by both of those processes.
> > 
> > I'm leaving soon so I'll get back to you on the CL tomorrow. I think it
might
> be
> > better to do it in person, just let me collect my thoughts on all of the
edge
> > cases
> 
> I put together a quick CL demoing what I meant when I asked you to put more
> logic in -toolbarFraction. 
> https://codereview.chromium.org/2296663003/
> 
> Let's discuss this more tomorrow.

I looked over this and it looks like I was trying to fit too many things at
once. 
I'm going to revert everything and just fix the edge cases.
After that is set, I'll refactor it

[spqchan, 2016-08-31 19:21:22]

Description was changed from

==========
[Mac] Fullscreen Toolbar Edge Cases

Fixed conflicts between fullscreen toolbar tracking area,
visibility locks and animation.

BUG=641751, 641749, 642446
==========

to

==========
[Mac] Fullscreen Toolbar Bar Visibility Edge Cases

Fixed edge case for the toolbar visibility locks.

BUG=641751, 641749
==========

[spqchan, 2016-08-31 19:47:19]

Alright, let me know what you think of this. If it looks fine, I'll clean up and
the code and add comments. Let me know if you have any questions.

My last attempt was to reduce the number of flags I use for each edge case.
Looks like it made things a bit more confusing instead

[erikchen, 2016-08-31 20:19:50]

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

https://codereview.chromium.org/2296903002/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm:282: -
(void)ensureOverlayShownWithAnimation:(BOOL)animate {
I notice that this is also called by browser_window_controller_private.mm.
Should that also become lockBarVisiblity, and should this become a private
method?

https://codereview.chromium.org/2296903002/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm:303: if
([browserController_ isBarVisibilityLockedForOwner:nil] &&
I don't see how this condition could ever return true.
-isBarVisibilityLockedForOwner: should always return false.

https://codereview.chromium.org/2296903002/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm:410: return
kHideFraction;
I see what you're doing here, but I think this will have the wrong behavior if
the browser tries to lock the bar visiblity while the NSMenuBar is in the middle
of an animation.

[spqchan, 2016-09-07 19:46:48]

Thanks for the discussion about redoing how the fullscreen layout works. I
talked to shrike about prioritizing the fullscreen refactor and he agrees that
doing it would be better in the long run, especially since I'm currently working
on a couple features for it anyway.

Anyway, I'm going to work on a design doc for reworking this and send it to you
when I'm done. In the meantime, can you review this CL, since we need to fix
this by M54?

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

https://codereview.chromium.org/2296903002/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm:282: -
(void)ensureOverlayShownWithAnimation:(BOOL)animate {
On 2016/08/31 20:19:50, erikchen wrote:
> I notice that this is also called by browser_window_controller_private.mm.
> Should that also become lockBarVisiblity, and should this become a private
> method?

No, lockBarVisibility and releaseBarVisibility should only be called when the
barVisibilityLocks_ are actually being changed. Added comments to reflect that.

https://codereview.chromium.org/2296903002/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm:303: if
([browserController_ isBarVisibilityLockedForOwner:nil] &&
On 2016/08/31 20:19:50, erikchen wrote:
> I don't see how this condition could ever return true.
> -isBarVisibilityLockedForOwner: should always return false.

ensureOverlayHiddenWithAnimation can be called when the bar visibility is
locked. !isReleasingBarVisibility_ is unnecessary though. Removed it

https://codereview.chromium.org/2296903002/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm:410: return
kHideFraction;
On 2016/08/31 20:19:50, erikchen wrote:
> I see what you're doing here, but I think this will have the wrong behavior if
> the browser tries to lock the bar visiblity while the NSMenuBar is in the
middle
> of an animation.

True. Updated the logic so that if the menubar is shown or in the progress of
showing itself, it will take priority over the animation

[spqchan, 2016-09-07 19:46:52]

Thanks for the discussion about redoing how the fullscreen layout works. I
talked to shrike about prioritizing the fullscreen refactor and he agrees that
doing it would be better in the long run, especially since I'm currently working
on a couple features for it anyway.

Anyway, I'm going to work on a design doc for reworking this and send it to you
when I'm done. In the meantime, can you review this CL, since we need to fix
this by M54?

[erikchen, 2016-09-07 21:58:49]

https://codereview.chromium.org/2296903002/diff/140001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm (right):

https://codereview.chromium.org/2296903002/diff/140001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm:422: return
toolbarFractionFromMenuProgress_;
I think that there is a logic error here. If the user moves the mouse to the top
of the screen, the menubar and toolbar show, toolbarFractionFromMenuProgress_
gets set to 1. The user moves the mouse onto the toolbar, which causes the
menubar to disappear, but toolbarFractionFromMenuProgress_ stays at 1. Now the
user moves the mouse off of the toolbar. During the disappearing animation, we
return an appropriate fraction. Once the animation finishes, the logic falls
through to this line, which returns 1, when we want it to return 0.

I think that if you add another condition:

if (tracking area && [self mouseInsideTrackingArea])
  return kShowFraction

Then you can get rid of toolbarFractionFromMenuProgress_ and use menuProgress_
everywhere.

[spqchan, 2016-09-08 01:49:41]

PTAL

https://codereview.chromium.org/2296903002/diff/140001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm (right):

https://codereview.chromium.org/2296903002/diff/140001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm:422: return
toolbarFractionFromMenuProgress_;
On 2016/09/07 21:58:49, erikchen wrote:
> I think that there is a logic error here. If the user moves the mouse to the
top
> of the screen, the menubar and toolbar show, toolbarFractionFromMenuProgress_
> gets set to 1. The user moves the mouse onto the toolbar, which causes the
> menubar to disappear, but toolbarFractionFromMenuProgress_ stays at 1. Now the
> user moves the mouse off of the toolbar. During the disappearing animation, we
> return an appropriate fraction. Once the animation finishes, the logic falls
> through to this line, which returns 1, when we want it to return 0.
> 
> I think that if you add another condition:
> 
> if (tracking area && [self mouseInsideTrackingArea])
>   return kShowFraction
> 
> Then you can get rid of toolbarFractionFromMenuProgress_ and use menuProgress_
> everywhere.

Done.

[erikchen, 2016-09-08 03:10:46]

lgtm

[spqchan, 2016-09-08 16:45:28]

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

[spqchan, 2016-09-08 16:45:29]

Thanks!

+rsesek for OWNERS

[Robert Sesek, 2016-09-08 18:37:43]

https://codereview.chromium.org/2296903002/diff/180001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/fullscreen_toolbar_controller.h (right):

https://codereview.chromium.org/2296903002/diff/180001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/fullscreen_toolbar_controller.h:27: enum MenubarState {
Follow-up cleanup idea: drop the namespace and make this |enum class
FullscreenMenubarState| and similarly above |enum class FullscreenSlidingStyle|.

https://codereview.chromium.org/2296903002/diff/180001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm (right):

https://codereview.chromium.org/2296903002/diff/180001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm:272:
isLockingBarVisibility_ = YES;
I think this is a good candidate for base::AutoReset, and elsewhere.

[spqchan, 2016-09-08 21:35:57]

PTAL

https://codereview.chromium.org/2296903002/diff/180001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/fullscreen_toolbar_controller.h (right):

https://codereview.chromium.org/2296903002/diff/180001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/fullscreen_toolbar_controller.h:27: enum MenubarState {
On 2016/09/08 18:37:42, Robert Sesek wrote:
> Follow-up cleanup idea: drop the namespace and make this |enum class
> FullscreenMenubarState| and similarly above |enum class
FullscreenSlidingStyle|.

Done.

https://codereview.chromium.org/2296903002/diff/180001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm (right):

https://codereview.chromium.org/2296903002/diff/180001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm:272:
isLockingBarVisibility_ = YES;
On 2016/09/08 18:37:42, Robert Sesek wrote:
> I think this is a good candidate for base::AutoReset, and elsewhere.

Done.

https://codereview.chromium.org/2296903002/diff/180001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm:272:
isLockingBarVisibility_ = YES;
On 2016/09/08 18:37:42, Robert Sesek wrote:
> I think this is a good candidate for base::AutoReset, and elsewhere.

Done.

[spqchan, 2016-09-08 21:35:57]

PTAL

https://codereview.chromium.org/2296903002/diff/180001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/fullscreen_toolbar_controller.h (right):

https://codereview.chromium.org/2296903002/diff/180001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/fullscreen_toolbar_controller.h:27: enum MenubarState {
On 2016/09/08 18:37:42, Robert Sesek wrote:
> Follow-up cleanup idea: drop the namespace and make this |enum class
> FullscreenMenubarState| and similarly above |enum class
FullscreenSlidingStyle|.

Done.

https://codereview.chromium.org/2296903002/diff/180001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm (right):

https://codereview.chromium.org/2296903002/diff/180001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm:272:
isLockingBarVisibility_ = YES;
On 2016/09/08 18:37:42, Robert Sesek wrote:
> I think this is a good candidate for base::AutoReset, and elsewhere.

Done.

https://codereview.chromium.org/2296903002/diff/180001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm:272:
isLockingBarVisibility_ = YES;
On 2016/09/08 18:37:42, Robert Sesek wrote:
> I think this is a good candidate for base::AutoReset, and elsewhere.

Done.

[Robert Sesek, 2016-09-08 22:40:02]

https://codereview.chromium.org/2296903002/diff/180001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/fullscreen_toolbar_controller.h (right):

https://codereview.chromium.org/2296903002/diff/180001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/fullscreen_toolbar_controller.h:27: enum MenubarState {
On 2016/09/08 21:35:57, spqchan wrote:
> On 2016/09/08 18:37:42, Robert Sesek wrote:
> > Follow-up cleanup idea: drop the namespace and make this |enum class
> > FullscreenMenubarState| and similarly above |enum class
> FullscreenSlidingStyle|.
> 
> Done.

I suggested making this an enum class, not just an enum.

[spqchan, 2016-09-09 01:26:52]

On 2016/09/08 22:40:02, Robert Sesek wrote:
>
https://codereview.chromium.org/2296903002/diff/180001/chrome/browser/ui/coco...
> File chrome/browser/ui/cocoa/fullscreen_toolbar_controller.h (right):
> 
>
https://codereview.chromium.org/2296903002/diff/180001/chrome/browser/ui/coco...
> chrome/browser/ui/cocoa/fullscreen_toolbar_controller.h:27: enum MenubarState
{
> On 2016/09/08 21:35:57, spqchan wrote:
> > On 2016/09/08 18:37:42, Robert Sesek wrote:
> > > Follow-up cleanup idea: drop the namespace and make this |enum class
> > > FullscreenMenubarState| and similarly above |enum class
> > FullscreenSlidingStyle|.
> > 
> > Done.
> 
> I suggested making this an enum class, not just an enum.

Whoops, sorry. PTAL

[Robert Sesek, 2016-09-09 18:27:04]

LGTM

https://codereview.chromium.org/2296903002/diff/240001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/fullscreen_toolbar_controller.h (right):

https://codereview.chromium.org/2296903002/diff/240001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/fullscreen_toolbar_controller.h:27: MENUBAR_SHOWN,    //
Menubar is fully shown.
optional: Drop "MENUBAR_" now that usage of these values is always qualified
with the enum class name.

[spqchan, 2016-09-10 00:23:04]

Thanks!

https://codereview.chromium.org/2296903002/diff/240001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/fullscreen_toolbar_controller.h (right):

https://codereview.chromium.org/2296903002/diff/240001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/fullscreen_toolbar_controller.h:27: MENUBAR_SHOWN,    //
Menubar is fully shown.
On 2016/09/09 18:27:04, Robert Sesek wrote:
> optional: Drop "MENUBAR_" now that usage of these values is always qualified
> with the enum class name.

Done.

[spqchan, 2016-09-10 00:23:09]

The CQ bit was checked by spqchan@chromium.org

[spqchan, 2016-09-10 00:23:10]

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/2296903002/#ps260001
(title: "menubar enum name change")

[commit-bot: I haz the power, 2016-09-10 00:23:43]

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-09-10 02:07:11]

Description was changed from

==========
[Mac] Fullscreen Toolbar Bar Visibility Edge Cases

Fixed edge case for the toolbar visibility locks.

BUG=641751, 641749
==========

to

==========
[Mac] Fullscreen Toolbar Bar Visibility Edge Cases

Fixed edge case for the toolbar visibility locks.

BUG=641751, 641749
==========

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

Committed patchset #14 (id:260001)

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

Description was changed from

==========
[Mac] Fullscreen Toolbar Bar Visibility Edge Cases

Fixed edge case for the toolbar visibility locks.

BUG=641751, 641749
==========

to

==========
[Mac] Fullscreen Toolbar Bar Visibility Edge Cases

Fixed edge case for the toolbar visibility locks.

BUG=641751, 641749

Committed: https://crrev.com/a431d992f7c15f070be18e6a51419c11445a785a
Cr-Commit-Position: refs/heads/master@{#417799}
==========

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

Patchset 14 (id:??) landed as
https://crrev.com/a431d992f7c15f070be18e6a51419c11445a785a
Cr-Commit-Position: refs/heads/master@{#417799}