[Mac] Implement LocationBarViewMac::UpdateLocationBarVisibility()

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

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #import "chrome/browser/ui/cocoa/toolbar/toolbar_controller.h"
 
 #include <algorithm>
 
 #include "base/mac/bundle_locations.h"
 #include "base/mac/foundation_util.h"
@@ skipping 52 matching lines @@
 #include "ui/base/l10n/l10n_util_mac.h"
 #include "ui/gfx/geometry/rect.h"
 #include "ui/gfx/image/image.h"
 
 using content::OpenURLParams;
 using content::Referrer;
 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;
 
 // The padding above the toolbar elements. This is calculated from the values
 // in Toolbar.xib: the height of the toolbar (35) minus the height of the child
 // elements (29) minus the y-origin of the elements (4).
 const CGFloat kToolbarElementTopPadding = 2.0;
 
 // The minimum width of the location bar in pixels.
 const CGFloat kMinimumLocationBarWidth = 100.0;
@@ skipping 39 matching lines @@
 
 }  // namespace
 
 @interface ToolbarController()
 @property(assign, nonatomic) Browser* browser;
 - (void)cleanUp;
 - (void)addAccessibilityDescriptions;
 - (void)initCommandStatus:(CommandUpdater*)commands;
 - (void)prefChanged:(const std::string&)prefName;
 - (BackgroundGradientView*)backgroundGradientView;
+- (AnimatableView*)animatableView;
 - (void)toolbarFrameChanged;
 - (void)pinLocationBarToLeftOfBrowserActionsContainerAndAnimate:(BOOL)animate;
 - (void)maintainMinimumLocationBarWidth;
 - (void)adjustBrowserActionsContainerForNewWindow:(NSNotification*)notification;
 - (void)browserActionsContainerDragged:(NSNotification*)notification;
 - (void)browserActionsVisibilityChanged:(NSNotification*)notification;
 - (void)browserActionsContainerWillAnimate:(NSNotification*)notification;
 - (void)adjustLocationSizeBy:(CGFloat)dX animate:(BOOL)animate;
 - (void)updateWrenchButtonSeverity:(WrenchIconPainter::Severity)severity
                            animate:(BOOL)animate;
@@ skipping 88 matching lines @@
     commands->AddCommandObserver(IDC_BOOKMARK_PAGE, commandObserver_.get());
     // 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.
   }
   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
                         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;
 }
 
 // Called after the view is done loading and the outlets have been hooked up.
 // Now we can hook up bridges that rely on UI objects such as the location bar
 // and button state. -viewDidLoad is the recommended way to do this in 10.10
 // SDK. When running on 10.10 or above -awakeFromNib still works but for some
 // reason is not guaranteed to be called (http://crbug.com/526276), so implement
 // both.
@@ skipping 85 matching lines @@
   // Create the controllers for the back/forward menus.
   backMenuController_.reset([[BackForwardMenuController alloc]
           initWithBrowser:browser_
                 modelType:BACK_FORWARD_MENU_TYPE_BACK
                    button:backButton_]);
   forwardMenuController_.reset([[BackForwardMenuController alloc]
           initWithBrowser:browser_
                 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 |
                                            NSTrackingInVisibleRect |
                                            NSTrackingMouseEnteredAndExited |
                                            NSTrackingActiveAlways
                                      owner:self
                                   userInfo:nil]);
   NSView* toolbarView = [self view];
   [toolbarView addTrackingArea:trackingArea_.get()];
@@ skipping 17 matching lines @@
 
   [self addAccessibilityDescriptions];
 }
 
 - (void)dealloc {
   [self cleanUp];
   [super dealloc];
 }
 
 - (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];
   [browserActionsController_ browserWillBeDestroyed];
   [wrenchMenuController_ browserWillBeDestroyed];
 
   [self cleanUp];
 }
 
 - (void)cleanUp {
@@ skipping 172 matching lines @@
 
   hasToolbar_ = toolbar;
 
   // If there's a toolbar, there must be a location bar.
   DCHECK((toolbar && locBar) || !toolbar);
   hasLocationBar_ = toolbar ? YES : locBar;
 
   // 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.

[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);
+
+  // 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.

 }
 
-- (NSView*)view {
-  if (hasToolbar_)
-    return [super view];
-  return locationBar_;
+// (Private) Returns the backdrop to the toolbar as a BackgroundGradientView.
+- (BackgroundGradientView*)backgroundGradientView {
+  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.
-- (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 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 {
   if (obj == locationBar_) {
     // Lazilly construct Field editor, Cocoa UI code always runs on the
     // same thread, so there shoudn't be a race condition here.
     if (autocompleteTextFieldEditor_.get() == nil) {
       autocompleteTextFieldEditor_.reset(
           [[AutocompleteTextFieldEditor alloc] init]);
     }
@@ skipping 104 matching lines @@
     [[NSNotificationCenter defaultCenter]
         addObserver:self
            selector:@selector(adjustBrowserActionsContainerForNewWindow:)
                name:NSWindowDidBecomeKeyNotification
              object:[[self view] window]];
   }
   if (![browserActionsContainerView_ isHidden])
     [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];
   [[NSNotificationCenter defaultCenter]
       removeObserver:self
                 name:NSWindowDidBecomeKeyNotification
               object:[[self view] window]];
 }
 
 - (void)browserActionsContainerDragged:(NSNotification*)notification {
@@ skipping 128 matching lines @@
   if (!hasToolbar_)
     return NSHeight([locationBar_ frame]);
 
   return kBaseToolbarHeightNormal - compressByHeight;
 }
 
 - (void)setDividerOpacity:(CGFloat)opacity {
   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];
 }
 
 - (BrowserActionsController*)browserActionsController {
   return browserActionsController_.get();
 }
 
 - (NSView*)wrenchButton {
   return wrenchButton_;
@@ skipping 57 matching lines @@
 - (void)hideDropURLsIndicatorInView:(NSView*)view {
   // Do nothing.
 }
 
 // (URLDropTargetController protocol)
 - (BOOL)isUnsupportedDropData:(id<NSDraggingInfo>)info {
   return drag_util::IsUnsupportedDropData(profile_, info);
 }
 
 @end