[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..d15d1584f6792ca11b78664f8a5263748ea373a7 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;
+
 // Height of the toolbar in pixels when the bookmark bar is closed.
 const CGFloat kBaseToolbarHeightNormal = 35.0;
 
@@ -129,6 +132,7 @@ CGFloat BrowserActionsContainerDelegate::GetMaxAllowedWidth() {
 - (void)initCommandStatus:(CommandUpdater*)commands;
 - (void)prefChanged:(const std::string&)prefName;
 - (BackgroundGradientView*)backgroundGradientView;
+- (AnimatableView*)animatableView;
 - (void)toolbarFrameChanged;
 - (void)pinLocationBarToLeftOfBrowserActionsContainerAndAnimate:(BOOL)animate;
 - (void)maintainMinimumLocationBarWidth;
@@ -237,13 +241,18 @@ class NotificationBridge : public WrenchMenuBadgeController::Delegate {
 
 - (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
                         nibFileNamed:@"Toolbar"])) {

[tapted, 2015/10/16 00:06:16]

shouldn't a resizeDelegate argument be required for the nibFileNamed overload as
well, to support the header change?

[dominickn, 2015/10/20 04:40:21]

Done.

     // Start global error services now so we badge the menu correctly.
     SyncGlobalErrorFactory::GetForProfile(profile);
+
+    // Set the resize delegate (BWC) so that resizing triggers a layout redraw

[tapted, 2015/10/16 00:06:17]

nit: this comment doesn't add much, and can be dropped

[dominickn, 2015/10/20 04:40:22]

Done.

+    // automatically.
+    [[self animatableView] setResizeDelegate:resizeDelegate];
   }
   return self;
 }
@@ -349,15 +358,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 +395,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 animatableView] setResizeDelegate:nil];
+  [[self animatableView] stopAnimation];
+
   // Pass this call onto other reference counted objects.
   [backMenuController_ browserWillBeDestroyed];
   [forwardMenuController_ browserWillBeDestroyed];
@@ -589,19 +594,22 @@ class NotificationBridge : public WrenchMenuBadgeController::Delegate {
 
   // Make location bar not editable when in a pop-up.

[tapted, 2015/10/16 00:06:16]

is this consistent with what views does? (then, update comment to mention the
new window style?)

[dominickn, 2015/10/20 04:40:22]

Done.

   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.
+  [self shouldHideToolbarButtons:(!toolbar && locBar)];

[tapted, 2015/10/16 00:06:16]

nit: no parens required

[dominickn, 2015/10/20 04:40:22]

Done.

 }
 
-// (Private) Returns the backdrop to the toolbar.
+// (Private) Returns the backdrop to the toolbar as a BackgroundGradientView.
 - (BackgroundGradientView*)backgroundGradientView {
-  // We really do mean |[super view]|; see our override of |-view|.
-  DCHECK([[super view] isKindOfClass:[BackgroundGradientView class]]);
-  return (BackgroundGradientView*)[super view];
+  DCHECK([[self view] isKindOfClass:[BackgroundGradientView class]]);
+  return (BackgroundGradientView*)[self view];

[tapted, 2015/10/16 00:06:17]

might as well change this to ObjCCastStrict if diff is picking it up anyway

[dominickn, 2015/10/20 04:40:22]

Done.

+}
+
+// (Private) Returns the backdrop to the toolbar as an AnimatableView.
+- (AnimatableView*)animatableView{
+  DCHECK([[self view] isKindOfClass:[AnimatableView class]]);

[tapted, 2015/10/16 00:06:16]

DCHECK not required (happens in ObjCCastStrict)

[dominickn, 2015/10/20 04:40:22]

Done.

+  return base::mac::ObjCCastStrict<AnimatableView>([self view]);
 }
 
 - (id)customFieldEditorForObject:(id)obj {
@@ -726,6 +734,86 @@ class NotificationBridge : public WrenchMenuBadgeController::Delegate {
     [self pinLocationBarToLeftOfBrowserActionsContainerAndAnimate:NO];
 }
 
+- (void)updateVisibility:(BOOL)visible withAnimation:(BOOL)animate {

[tapted, 2015/10/16 00:06:17]

setToolbarHidden?

[dominickn, 2015/10/20 04:40:22]

This method can set the toolbar to hidden or visible, so I didn't use that name.

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

Ah, I meant since Cocoa nomenclature is "[NSView isHidden]", and this is an
ObjectiveC function, we should use the same, and flip the logic in the bridge
instead. This would make the `if` below more readable and avoid the need for the
explainer comment above it.

But, `hasLocationBar` is the reverse already, and it was there before you, so I
don't feel strongly about this.

+  // |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.
+  AnimatableView* view = [self animatableView];
+  if ([view isHidden] == visible) {
+    // Send shouldHideToolbarButtons:YES if there is no toolbar and there is a
+    // location bar. This has the effect of displaying a toolbar with only the
+    // location bar inside it.
+    [self setHasToolbar:hasToolbar_ hasLocationBar:visible];

[tapted, 2015/10/16 00:06:17]

DCHECK(hasToolbar_)?  (and pass YES) here?

[dominickn, 2015/10/20 04:40:22]

hasToolbar_ isn't usually YES in this case. For bookmark apps, hasToolbar_ is NO
and hasLocationBar_ is YES

+
+    // 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 BWC to resize this view in

[tapted, 2015/10/16 00:06:17]

nit: (shouldn't abbreviate in comments): BWC -> BrowserWindowController

[dominickn, 2015/10/20 04:40:22]

Done.

+    // the browser layout as required.
+    if (animate) {
+      [view animateToNewHeight:newHeight
+                      duration:kToolBarAnimationDuration];
+    } else {
+      [view setHeight:newHeight];
+    }
+  }
+}
+
+- (void)shouldHideToolbarButtons:(BOOL)hide {
+  // Ignore this message if the toolbar is shown or the location bar is hidden.
+  if (hasToolbar_ || !hasLocationBar_)

[tapted, 2015/10/17 00:44:15]

I think the only caller does

[self shouldHideToolbarButtons:(!toolbar && locBar)];

arg = !toolbar && locBar
if = toolbar || !locBar

toolbar locBar  arg  if
YES        YES  YES  NO
YES        NO   NO   YES
NO         YES  NO   YES
NO         NO   NO   YES


So... is it correct that, after the return below, `hide` will always be true?


In which case I think this method can just be called showLocationBarOnly (or
permanentlyHideAllButtons), and document that there is no way to escape from
this state.

Then all the layout logic below can just boil down to 

locationFrame = [locationBar_ frame];
locationFrame.origin.x = 0;
locationFrame.size.width = NSWidth(<toolbar>) - padding?
[locationBar_ setFrame:locationFrame];

[button* setHidden:YES];

[dominickn, 2015/10/20 04:40:22]

I tried to keep this as general as possible so that it could be unwound. But I'm
happy to keep it as a one-way transform as that will make it much cleaner.

+    return;
+
+  // Work out if we need to show or hide the buttons. Assumes that the back,
+  // forward, reload, and wrench buttons are either shown or hidden together.
+  DCHECK([backButton_ isHidden] == [forwardButton_ isHidden] &&

[tapted, 2015/10/17 00:44:15]

perhaps

DCHECK_EQ(NO, [backButton_ isHidden] | [forwardButton_ isHidden] | ...)

[dominickn, 2015/10/20 04:40:22]

Done.

+             [forwardButton_ isHidden] == [reloadButton_ isHidden] &&
+             [reloadButton_ isHidden] == [wrenchButton_ isHidden]);
+  BOOL hidden = [backButton_ isHidden];

[tapted, 2015/10/17 00:44:15]

hidden isn't used after this, so I'd move this into the `if`

[dominickn, 2015/10/20 04:40:22]

Done.

+
+  // Don't do anything if the current state matches the desired state.
+  if (hidden == hide)
+    return;
+
+  // We need to X-shift the location bar (back, forward, reload buttons are left
+  // of the bar) and adjust its width (wrench button right of the bar).
+  // Calculate the necessary X-shift minus three pixels since the frame edges
+  // of each button are right on top of each other.
+  CGFloat moveX = [backButton_ frame].size.width +

[tapted, 2015/10/16 00:06:17]

I think this stuff should be part of the layout function (e.g. does calling
layout again clobber the changes here?)

i.e. set a bool - preferably something that can be set in the initialiser and
never changes..

[tapted, 2015/10/17 00:44:15]

Hm, since the homeButton hiding is here, I guess here is appropriate. But try to
keep this method together with the home button hiding.

[dominickn, 2015/10/20 04:40:22]

Done.

+                  [forwardButton_ frame].size.width +

[tapted, 2015/10/17 00:44:15]

It's more typical to do NSWidth([forwardButton_ frame]) than this

+                  [reloadButton_ frame].size.width - 3.0;

[tapted, 2015/10/17 00:44:15]

This "3" feels a bit magic. Is is 3 * kSomeNegativePaddingOrSomething. I don't
understand the comment about the frame edges being on top of each other (maybe
refer to a method that sets these?)

[tapted, 2015/10/17 00:44:15]

If these are laid out together already, you can just do

NSMaxX([reloadButton_ frame]) (which should be the sum of widths, plus the
padding on the left of the back button)

+
+  // Calculate the necessary width change minus one pixel for the frame edge.
+  CGFloat dX = [wrenchButton_ frame].size.width - 1.0;
+
+  // Add the home button width to the X-shift if it is currently visible.
+  BOOL homeButtonEnabled = showHomeButton_.GetValue() ? YES : NO;
+  if (homeButtonEnabled)
+    moveX += [homeButton_ frame].size.width - 1.0;
+
+  // If we're hiding, the direction of the shift must be reversed (leftwards).
+  // Otherwise, the direction of the resize must be reversed (leftwards).
+  if (hide)
+    moveX *= -1;
+  else
+    dX *= -1;
+
+  // Do the necessary moving and resizing, and update the visibility of the
+  // buttons appropriately.
+  [self adjustLocationSizeBy:dX animate:NO];

[tapted, 2015/10/17 00:44:15]

since this is being called on `self` and we pass `NO` to animate, I think this
will be simpler with something like

NSView* lastButton = showHomeButton_.GetValue() ? homeButton_ : homeButton_;
locationFrame = [locationBar_ frame];
locationFrame.origin.x = hide ? 0 : NSMaxX([lastButton frame]);
locationFrame.size.width = NSWidth(<toolbar>) -
    (hide ? NSMinX([browserActionsContainerView_ frame] : 0) -
    NSMinX(locationFrame);
[locationBar_ setFrame:locationFrame];

(plus any padding you need)

+  [locationBar_ setFrame:[self adjustRect:[locationBar_ frame]

[tapted, 2015/10/17 00:44:15]

this clobbers the [locationBar_ setFrame:locationFrame]; done in
adjustLocationSizeBy no?

[dominickn, 2015/10/20 04:40:22]

It's assumed that adjustLocationSizeBy won't be called in conjunction with this:
I'll document it.

+                                 byAmount:moveX]];
+  [backButton_ setHidden:hide];
+  [forwardButton_ setHidden:hide];
+  [reloadButton_ setHidden:hide];
+  [wrenchButton_ setHidden:hide];
+
+  if (homeButtonEnabled)
+    [homeButton_ setHidden:hide];
+}
+
 - (void)adjustBrowserActionsContainerForNewWindow:
     (NSNotification*)notification {
   [self toolbarFrameChanged];
@@ -874,12 +962,8 @@ class NotificationBridge : public WrenchMenuBadgeController::Delegate {
   BackgroundGradientView* view = [self backgroundGradientView];
   [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;

[tapted, 2015/10/16 00:06:16]

ObjCCastStrict


But! actually. Now I see this code, I think we should just make a toolbarView
accessor that does this, and just get rid of the backgroundGradieView and
animatableView accessors

[dominickn, 2015/10/20 04:40:21]

Done.

+  [toolbarView setDividerOpacity:opacity];
 
   [view setNeedsDisplay:YES];
 }