[Mac] Implement LocationBarViewMac::UpdateLocationBarVisibility()

chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm

Index: chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm
diff --git a/chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm b/chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm
index bee28087eb69274f25b34dbd33ce242df1b79605..f1a5f779bf09120797243027b20bf4cca8ab3473 100644
--- a/chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm
+++ b/chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm
@@ -70,6 +70,9 @@ using content::WebContents;
 
 namespace {
 
+// Duration of the toolbar animation.
+const NSTimeInterval kToolBarAnimationDuration = 0.12;

[tapted, 2015/10/20 06:31:09]

Can this be shared with something?  (just asking... not sure if that's a good
change)

[dominickn, 2015/10/26 02:53:04]

I set this value to be the same as the bookmark bar animation, but I don't know
if it's worth sharing the value between the two as they both currently use an
anonymous constant for it.

[tapted, 2015/10/26 04:40:55]

So probably it should move to animatable_view.mm and the `duration` argument
removed, since everything passes 0.12. Something for another CL though.

[dominickn, 2015/10/26 06:42:42]

Acknowledged.

+
 // Height of the toolbar in pixels when the bookmark bar is closed.
 const CGFloat kBaseToolbarHeightNormal = 35.0;
 
@@ -81,6 +84,10 @@ const CGFloat kToolbarElementTopPadding = 2.0;
 // The minimum width of the location bar in pixels.
 const CGFloat kMinimumLocationBarWidth = 100.0;
 
+// The padding to the left and right of the location bar when it is shown
+// by itself in the toolbar.
+const CGFloat kLocationBarOnlyHorizontalPadding = 3.0;
+
 // The amount of left padding that the wrench menu should have.
 const CGFloat kWrenchMenuLeftPadding = 3.0;
 
@@ -128,7 +135,7 @@ CGFloat BrowserActionsContainerDelegate::GetMaxAllowedWidth() {
 - (void)addAccessibilityDescriptions;
 - (void)initCommandStatus:(CommandUpdater*)commands;
 - (void)prefChanged:(const std::string&)prefName;
-- (BackgroundGradientView*)backgroundGradientView;
+- (ToolbarView*)toolbarView;
 - (void)toolbarFrameChanged;
 - (void)pinLocationBarToLeftOfBrowserActionsContainerAndAnimate:(BOOL)animate;
 - (void)maintainMinimumLocationBarWidth;
@@ -209,6 +216,7 @@ class NotificationBridge : public WrenchMenuBadgeController::Delegate {
 - (id)initWithCommands:(CommandUpdater*)commands
                profile:(Profile*)profile
                browser:(Browser*)browser
+        resizeDelegate:(id<ViewResizer>)resizeDelegate
           nibFileNamed:(NSString*)nibName {
   DCHECK(commands && profile && [nibName length]);
   if ((self = [super initWithNibName:nibName
@@ -231,16 +239,20 @@ class NotificationBridge : public WrenchMenuBadgeController::Delegate {
     // NOTE: Don't remove the command observers. ToolbarController is
     // autoreleased at about the same time as the CommandUpdater (owned by the
     // Browser), so |commands_| may not be valid any more.
+
+    [[self toolbarView] setResizeDelegate:resizeDelegate];
   }
   return self;
 }
 
 - (id)initWithCommands:(CommandUpdater*)commands
                profile:(Profile*)profile
-               browser:(Browser*)browser {
+               browser:(Browser*)browser
+        resizeDelegate:(id<ViewResizer>)resizeDelegate {
   if ((self = [self initWithCommands:commands
                              profile:profile
                              browser:browser
+                      resizeDelegate:resizeDelegate
                         nibFileNamed:@"Toolbar"])) {
     // Start global error services now so we badge the menu correctly.
     SyncGlobalErrorFactory::GetForProfile(profile);
@@ -349,15 +361,6 @@ class NotificationBridge : public WrenchMenuBadgeController::Delegate {
                 modelType:BACK_FORWARD_MENU_TYPE_FORWARD
                    button:forwardButton_]);
 
-  // For a popup window, the toolbar is really just a location bar
-  // (see override for [ToolbarController view], below).  When going
-  // fullscreen, we remove the toolbar controller's view from the view
-  // hierarchy.  Calling [locationBar_ removeFromSuperview] when going
-  // fullscreen causes it to get released, making us unhappy
-  // (http://crbug.com/18551).  We avoid the problem by incrementing
-  // the retain count of the location bar; use of the scoped object
-  // helps us remember to release it.
-  locationBarRetainer_.reset([locationBar_ retain]);
   trackingArea_.reset(
       [[CrTrackingArea alloc] initWithRect:NSZeroRect // Ignored
                                    options:NSTrackingMouseMoved |
@@ -395,6 +398,11 @@ class NotificationBridge : public WrenchMenuBadgeController::Delegate {
 }
 
 - (void)browserWillBeDestroyed {
+  // Clear resize delegate so it doesn't get called during stopAnimation, and
+  // stop any in-flight animation.
+  [[self toolbarView] setResizeDelegate:nil];
+  [[self toolbarView] stopAnimation];
+
   // Pass this call onto other reference counted objects.
   [backMenuController_ browserWillBeDestroyed];
   [forwardMenuController_ browserWillBeDestroyed];
@@ -587,21 +595,18 @@ class NotificationBridge : public WrenchMenuBadgeController::Delegate {
   // Decide whether to hide/show based on whether there's a location bar.
   [[self view] setHidden:!hasLocationBar_];
 
-  // Make location bar not editable when in a pop-up.
+  // Make location bar not editable when in a pop-up or an app window.
   locationBarView_->SetEditable(toolbar);
-}
 
-- (NSView*)view {
-  if (hasToolbar_)
-    return [super view];
-  return locationBar_;
+  // Hide the toolbar buttons and only display the location bar in the view
+  // if we do not have a toolbar but have a location bar.
+  if (!toolbar && locBar)
+    [self showLocationBarOnly];
 }
 
-// (Private) Returns the backdrop to the toolbar.
-- (BackgroundGradientView*)backgroundGradientView {
-  // We really do mean |[super view]|; see our override of |-view|.
-  DCHECK([[super view] isKindOfClass:[BackgroundGradientView class]]);
-  return (BackgroundGradientView*)[super view];
+// (Private) Returns the backdrop to the toolbar as a ToolbarView.
+- (ToolbarView*)toolbarView{
+  return base::mac::ObjCCastStrict<ToolbarView>([self view]);
 }
 
 - (id)customFieldEditorForObject:(id)obj {
@@ -726,6 +731,62 @@ class NotificationBridge : public WrenchMenuBadgeController::Delegate {
     [self pinLocationBarToLeftOfBrowserActionsContainerAndAnimate:NO];
 }
 
+- (void)updateVisibility:(BOOL)visible withAnimation:(BOOL)animate {
+  // |visible| being YES is equivalent to [[toolbarController_ view] isHidden]
+  // being NO, and vice-versa. That is, when |visible| and |isHidden| have the
+  // same value, a state change is needed.
+  ToolbarView* view = [self toolbarView];
+  if ([view isHidden] == visible) {
+    // Send showLocationBarOnly if there is no toolbar and there is a location
+    // bar. This displays the toolbar with only the location bar inside it.
+    [self setHasToolbar:hasToolbar_ hasLocationBar:visible];

[tapted, 2015/10/20 06:31:09]

This is in the wrong place. But.. I'm wondering if it can simply be removed...

The constructor of a Browser calls
HostedAppBrowserController::UpdateLocationBarVisibility(false).

Presumably that's because something has already called setHasToolbar:NO
hasLocationBar:YES -- maybe this is only (effectively) happening on views at the
moment, and there's a missing piece for Cocoa where that should be happening
during construction.

[dominickn, 2015/10/26 02:53:04]

HostedAppBrowserController::UpdateLocationBarVisibility(false) eventually calls
this function via the LocationBarViewMac, and that call is the same as when the
visibility is changed. I've changed how showLocationOnly is called and moved it
explicitly here to resolve the pop up window visual differences.

[tapted, 2015/10/26 04:40:55]

The point is that, currently, browser_window_controller.mm is the only place
that calls setHasToolbar:hasLocationBar and it does it exactly once, immediately
after the ToolbarController is initialized.

What breaks if this line is removed?

And (assuming something breaks), can it be resolved by having the current call
site always pass YES for hasLocationBar? (perhaps, only when there is a
hosted_app_controller_). That is, the case where there *is* a
hosted_app_controller but the location bar is hidden should be _always_ handled
by setting the height to 0. If hasLocationBar starts off as `NO` then the
toolbar is in a strange state until the first time it's shown (sure, it's
hidden, so you can't _see_ the strange state, but it's there..)

[dominickn, 2015/10/26 06:42:42]

Removing the call breaks the animation for bookmark apps because it's using the
isHidden property of the location bar view to toggle the visible/not visible
state. hasLocationBar is always YES for bookmark apps anyway, so that doesn't
fix the issue. I've added a call to [view setHidden:!visible] to fix this.

+
+    // updateVisibility is only called when in a hosted app. The bookmark bar is
+    // never visible in these apps, so the desired height will always be
+    // kBaseToolbarHeightNormal.
+    CGFloat newHeight = visible ? kBaseToolbarHeightNormal : 0;
+
+    // Perform the animation, which will cause the BrowserWindowController to
+    // resize this view in the browser layout as required.
+    if (animate) {
+      [view animateToNewHeight:newHeight
+                      duration:kToolBarAnimationDuration];
+    } else {
+      [view setHeight:newHeight];
+    }
+  }
+}
+
+- (void)showLocationBarOnly {
+  // Ignore this message if the toolbar is shown or the location bar is hidden.
+  if (hasToolbar_ || !hasLocationBar_)

[tapted, 2015/10/20 06:31:09]

Can this be a DCHECK now? (with the reverse logic)

[dominickn, 2015/10/26 02:53:04]

Now that the callsite for this method has changed, this can only be a DCHECK if
the callsite is guarded by an if (!hasToolbar_ && hasLocationBar_) (since
setHasToolbar will update the location bar visibility).

+    return;
+
+  // If the toolbar buttons are already hidden, return. Assumes that the back,
+  // forward, reload, and wrench buttons are either shown or hidden together.
+  if ([backButton_ isHidden]) {
+    DCHECK_EQ(YES, [backButton_ isHidden] && [forwardButton_ isHidden] &&
+                       [reloadButton_ isHidden] && [wrenchButton_ isHidden]);
+    return;
+  }
+
+  // Adjust the location bar frame to stretch the full width of the toolbar,
+  // with padding on either side.
+  NSRect locationFrame = [locationBar_ frame];
+  locationFrame.origin.x = kLocationBarOnlyHorizontalPadding;
+  locationFrame.size.width =
+      NSWidth([[self view] frame]) - 2 * kLocationBarOnlyHorizontalPadding;
+  [locationBar_ setFrame:locationFrame];
+
+  [backButton_ setHidden:YES];
+  [forwardButton_ setHidden:YES];
+  [reloadButton_ setHidden:YES];
+  [wrenchButton_ setHidden:YES];
+
+  if (!showHomeButton_.GetValue())

[tapted, 2015/10/20 06:31:09]

This check isn't needed

[dominickn, 2015/10/26 02:53:04]

Done.

+    [homeButton_ setHidden:YES];
+}
+
 - (void)adjustBrowserActionsContainerForNewWindow:
     (NSNotification*)notification {
   [self toolbarFrameChanged];
@@ -871,15 +932,11 @@ class NotificationBridge : public WrenchMenuBadgeController::Delegate {
 }
 
 - (void)setDividerOpacity:(CGFloat)opacity {
-  BackgroundGradientView* view = [self backgroundGradientView];
+  ToolbarView* view = [self toolbarView];

[tapted, 2015/10/20 06:31:09]

view -> toolbarView

[dominickn, 2015/10/26 02:53:04]

Done.

   [view setShowsDivider:(opacity > 0 ? YES : NO)];
 
-  // We may not have a toolbar view (e.g., popup windows only have a location
-  // bar).
-  if ([view isKindOfClass:[ToolbarView class]]) {
-    ToolbarView* toolbarView = (ToolbarView*)view;
-    [toolbarView setDividerOpacity:opacity];
-  }
+  ToolbarView* toolbarView = (ToolbarView*)view;
+  [toolbarView setDividerOpacity:opacity];
 
   [view setNeedsDisplay:YES];
 }