[Mac] Fullscreen Toolbar Edge Cases

chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm

Index: chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm
diff --git a/chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm b/chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm
index e34cb4cb1cd6855e6bfc1b57bef253d304ea6508..85b76633bdb57dbbf3cf6381444d84441e53973c 100644
--- a/chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm
+++ b/chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm
@@ -267,6 +267,18 @@ OSStatus MenuBarRevealHandler(EventHandlerCallRef handler,
   return kFloatingBarVerticalOffset;
 }
 
+- (void)lockBarVisibilityWithAnimation:(BOOL)animate {
+  isLockingBarVisibility_ = YES;
+  [self ensureOverlayShownWithAnimation:animate];
+  isLockingBarVisibility_ = NO;
+}
+
+- (void)releaseBarVisibilityWithAnimation:(BOOL)animate {
+  isReleasingBarVisibility_ = YES;
+  [self ensureOverlayHiddenWithAnimation:animate];
+  isReleasingBarVisibility_ = NO;
+}
+
 - (void)ensureOverlayShownWithAnimation:(BOOL)animate {

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

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?

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

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

   if (!inFullscreenMode_)
     return;
@@ -288,6 +300,14 @@ OSStatus MenuBarRevealHandler(EventHandlerCallRef handler,
   if (self.slidingStyle != fullscreen_mac::OMNIBOX_TABS_HIDDEN)
     return;
 
+  if ([browserController_ isBarVisibilityLockedForOwner:nil] &&

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

I don't see how this condition could ever return true.
-isBarVisibilityLockedForOwner: should always return false.

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

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

+      !isReleasingBarVisibility_) {
+    return;
+  }
+
+  if ([self mouseInsideTrackingArea] || menubarFraction_ == kShowFraction)
+    return;
+
   [self cancelHideTimer];
   [self animateToolbarVisibility:NO];
 }
@@ -368,9 +388,6 @@ OSStatus MenuBarRevealHandler(EventHandlerCallRef handler,
 }
 
 - (CGFloat)toolbarFraction {
-  if ([browserController_ isBarVisibilityLockedForOwner:nil])
-    return kShowFraction;
-
   if (base::CommandLine::ForCurrentProcess()->HasSwitch(switches::kKioskMode))
     return kHideFraction;
 
@@ -380,12 +397,22 @@ OSStatus MenuBarRevealHandler(EventHandlerCallRef handler,
     case fullscreen_mac::OMNIBOX_TABS_NONE:
       return kHideFraction;
     case fullscreen_mac::OMNIBOX_TABS_HIDDEN:
+      if (IsCGFloatEqual(menubarFraction_, kShowFraction))
+        return kShowFraction;
+
       if (currentAnimation_.get())
         return [currentAnimation_ toolbarFraction];
 
       if (hideTimer_.get() || shouldAnimateToolbarOut_)
         return kShowFraction;
 
+      if (isLockingBarVisibility_)
+        return kHideFraction;

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

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]

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

+      else if (isReleasingBarVisibility_)
+        return kShowFraction;
+      else if ([browserController_ isBarVisibilityLockedForOwner:nil])
+        return kShowFraction;
+
       return toolbarFractionFromMenuProgress_;
   }
 }