[Mac] Refactor bookmark bar controller

chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm

Index: chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm
diff --git a/chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm b/chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm
index b00e907c297242652b1d613d7685d6f0b2d35094..8920a6b3b11192590369c951dc84cd0cdc435733 100644
--- a/chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm
+++ b/chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm
@@ -42,6 +42,7 @@
 #import "chrome/browser/ui/cocoa/bookmarks/bookmark_model_observer_for_cocoa.h"
 #import "chrome/browser/ui/cocoa/bookmarks/bookmark_name_folder_controller.h"
 #import "chrome/browser/ui/cocoa/browser_window_controller.h"
+#import "chrome/browser/ui/cocoa/l10n_util.h"
 #import "chrome/browser/ui/cocoa/menu_button.h"
 #import "chrome/browser/ui/cocoa/themed_window.h"
 #import "chrome/browser/ui/cocoa/toolbar/toolbar_controller.h"
@@ -151,6 +152,11 @@ namespace {
 const NSTimeInterval kBookmarkBarAnimationDuration = 0.12;
 const NSTimeInterval kDragAndDropAnimationDuration = 0.25;
 
+const int kMaxReusePoolSize = 10;
+
+// Min width for no item text field and import bookmarks button

[Avi (use Gerrit), 2017/03/23 15:44:52]

full sentences; end with a .

[lgrey, 2017/04/10 19:14:32]

Done.

+const CGFloat kNoItemElementMinWidth = 30;
+
 void RecordAppLaunch(Profile* profile, GURL url) {
   const extensions::Extension* extension =
       extensions::ExtensionRegistry::Get(profile)->
@@ -162,95 +168,105 @@ void RecordAppLaunch(Profile* profile, GURL url) {
                                   extension->GetType());
 }
 
-}  // namespace
-
-@interface BookmarkBarController ()
-
-// Updates the sizes and positions of the subviews.
-- (void)layoutSubviews;
-
-// Moves to the given next state (from the current state), possibly animating.
-// If |animate| is NO, it will stop any running animation and jump to the given
-// state. If YES, it may either (depending on implementation) jump to the end of
-// the current animation and begin the next one, or stop the current animation
-// mid-flight and animate to the next state.
-- (void)moveToState:(BookmarkBar::State)nextState
-      withAnimation:(BOOL)animate;
-
-// Create buttons for all items in the given bookmark node tree.
-// Modifies self->buttons_.  Do not add more buttons than will fit on the view.
-- (void)addNodesToButtonList:(const BookmarkNode*)node;
-
-// Create an autoreleased button appropriate for insertion into the bookmark
-// bar. Update |xOffset| with the offset appropriate for the subsequent button.
-- (BookmarkButton*)buttonForNode:(const BookmarkNode*)node
-                         xOffset:(int*)xOffset;
-
-// Find a parent whose button is visible on the bookmark bar.
-- (BookmarkButton*)bookmarkButtonToPulseForNode:(const BookmarkNode*)node;
-
-// Puts stuff into the final state without animating, stopping a running
-// animation if necessary.
-- (void)finalizeState;
+enum VisibleElementsMask {
+  kVisibleElementsNone = 0,
+  kVisibleElementsAppsButton = 1 << 0,
+  kVisibleElementsManagedBookmarksButton = 1 << 1,
+  kVisibleElementsSupervisedBookmarksButton = 1 << 2,
+  kVisibleElementsOffTheSideButton = 1 << 3,
+  kVisibleElementsOtherBookmarksButton = 1 << 4,
+  kVisibleElementsNoItemContainer = 1 << 5,
+  kVisibleElementsImportBookmarksButton = 1 << 6,

[Avi (use Gerrit), 2017/03/23 15:44:53]

You should have "Mask" in these names (i.e.
kVisibleElementsMaskOffTheSideButton; see NSEventMaskXxxx). Otherwise from
context it will seem like these are enumerated objects rather than things to AND
against a value.

[lgrey, 2017/04/10 19:14:32]

Done.

+};
+
+typedef NSUInteger VisibleElements;

[Elly Fong-Jones, 2017/03/23 15:40:22]

I don't know that this typedef is adding any extra clarity - "unsigned int
visible_elements;" is easier to understand for me than "VisibleElements
visible_elements", which I assumed was a container type until I went and
actually looked up the typedef.

[lgrey, 2017/04/10 19:14:32]

Done.

+
+struct BookmarkBarLayout {

[Elly Fong-Jones, 2017/03/23 15:40:21]

I really like this approach of bundling the state up into a struct.

+  bool IsAppsButtonVisible() const {
+    return visible_elements & kVisibleElementsAppsButton;
+  }
+  bool IsManagedBookmarksButtonVisible() const {
+    return visible_elements & kVisibleElementsManagedBookmarksButton;
+  }
+  bool IsSupervisedBookmarksButtonVisible() const {
+    return visible_elements & kVisibleElementsSupervisedBookmarksButton;
+  }
+  bool IsOffTheSideButtonVisible() const {
+    return visible_elements & kVisibleElementsOffTheSideButton;
+  }
+  bool IsOtherBookmarksButtonVisible() const {
+    return visible_elements & kVisibleElementsOtherBookmarksButton;
+  }
+  bool IsNoItemContainerVisible() const {
+    return visible_elements & kVisibleElementsNoItemContainer;
+  }
+  bool IsImportBookmarksButtonVisible() const {
+    return visible_elements & kVisibleElementsImportBookmarksButton;
+  }
+  int VisibleButtonCount() const { return button_offsets.size(); }
+  VisibleElements visible_elements;

[Avi (use Gerrit), 2017/03/23 15:44:53]

blank line to separate the accessors from the data?

[lgrey, 2017/04/10 19:14:31]

Done.

+  CGFloat apps_button_offset;
+  CGFloat managed_bookmarks_button_offset;
+  CGFloat supervised_bookmarks_button_offset;
+  CGFloat off_the_side_button_offset;
+  CGFloat other_bookmarks_button_offset;
+  CGFloat no_item_textfield_offset;
+  CGFloat no_item_textfield_width;
+  CGFloat import_bookmarks_button_offset;
+  CGFloat import_bookmarks_button_width;
+  CGFloat max_x;
+
+  std::unordered_map<int64_t, CGFloat> button_offsets;
+};
+
+bool operator==(const BookmarkBarLayout& lhs, const BookmarkBarLayout& rhs) {

[Avi (use Gerrit), 2017/03/23 15:44:52]

C++11 style is now to use std::tie for building these operators.

[lgrey, 2017/04/10 19:14:31]

Done.

+  if (lhs.visible_elements != rhs.visible_elements)
+    return false;
+  if (lhs.apps_button_offset != rhs.apps_button_offset)
+    return false;
+  if (lhs.managed_bookmarks_button_offset !=
+      rhs.managed_bookmarks_button_offset)
+    return false;
+  if (lhs.supervised_bookmarks_button_offset !=
+      rhs.supervised_bookmarks_button_offset)
+    return false;
+  if (lhs.off_the_side_button_offset != rhs.supervised_bookmarks_button_offset)
+    return false;
+  if (lhs.other_bookmarks_button_offset != rhs.other_bookmarks_button_offset)
+    return false;
+  if (lhs.no_item_textfield_offset != rhs.no_item_textfield_offset)
+    return false;
+  if (lhs.import_bookmarks_button_offset != rhs.import_bookmarks_button_offset)
+    return false;
+  if (lhs.no_item_textfield_width != rhs.no_item_textfield_width)
+    return false;
+  if (lhs.import_bookmarks_button_width != rhs.import_bookmarks_button_width)
+    return false;
+  if (lhs.max_x != rhs.max_x)
+    return false;
+  return lhs.button_offsets == rhs.button_offsets;
+}
+
+bool operator!=(const BookmarkBarLayout& lhs, const BookmarkBarLayout& rhs) {
+  return !(lhs == rhs);
+}
+
+const CGFloat kBookmarkButtonHeightMinusPadding =
+    bookmarks::kBookmarkButtonHeight - bookmarks::kBookmarkVerticalPadding * 2;
 
-// Stops any current animation in its tracks (midway).
-- (void)stopCurrentAnimation;
-
-// Show/hide the bookmark bar.
-// if |animate| is YES, the changes are made using the animator; otherwise they
-// are made immediately.
-- (void)showBookmarkBarWithAnimation:(BOOL)animate;
-
-// Handles animating the resize of the content view. Returns YES if it handled
-// the animation, NO if not (and hence it should be done instantly).
-- (BOOL)doBookmarkBarAnimation;
-
-// |point| is in the base coordinate system of the destination window;
-// it comes from an id<NSDraggingInfo>. |copy| is YES if a copy is to be
-// made and inserted into the new location while leaving the bookmark in
-// the old location, otherwise move the bookmark by removing from its old
-// location and inserting into the new location.
-- (BOOL)dragBookmark:(const BookmarkNode*)sourceNode
-                  to:(NSPoint)point
-                copy:(BOOL)copy;
-
-// Returns the index in the model for a drag to the location given by
-// |point|. This is determined by finding the first button before the center
-// of which |point| falls, scanning left to right. Note that, currently, only
-// the x-coordinate of |point| is considered. Though not currently implemented,
-// we may check for errors, in which case this would return negative value;
-// callers should check for this.
-- (int)indexForDragToPoint:(NSPoint)point;
-
-// Add or remove buttons to/from the bar until it is filled but not overflowed.
-- (void)redistributeButtonsOnBarAsNeeded;
-
-// Determine the nature of the bookmark bar contents based on the number of
-// buttons showing. If too many then show the off-the-side list, if none
-// then show the no items label.
-- (void)reconfigureBookmarkBar;
-
-- (void)clearMenuTagMap;
-- (int)preferredHeight;
-- (void)addButtonsToView;
-- (BOOL)setManagedBookmarksButtonVisibility;
-- (BOOL)setSupervisedBookmarksButtonVisibility;
-- (BOOL)setOtherBookmarksButtonVisibility;
-- (BOOL)setAppsPageShortcutButtonVisibility;
-- (BookmarkButton*)createCustomBookmarkButtonForCell:(NSCell*)cell;
-- (void)createManagedBookmarksButton;
-- (void)createSupervisedBookmarksButton;
-- (void)createOtherBookmarksButton;
-- (void)createAppsPageShortcutButton;
-- (void)openAppsPage:(id)sender;
-- (void)centerNoItemsLabel;
-- (void)positionRightSideButtons;
-- (void)watchForExitEvent:(BOOL)watch;
-- (void)resetAllButtonPositionsWithAnimation:(BOOL)animate;
-
-@end
+}  // namespace
 
-@implementation BookmarkBarController
+@implementation BookmarkBarController {

[Avi (use Gerrit), 2017/03/23 15:44:52]

Excellent!

You're moving the variables one-by-one, as you figure them out?

+  BookmarkBarLayout layout_;
+  CGFloat originalNoItemTextFieldWidth_;
+  CGFloat originalImportBookmarksButtonWidth_;
+  CGFloat originalNoItemInterelementPadding_;
+  std::unordered_map<int64_t, base::scoped_nsobject<BookmarkButton>>
+      nodeIdToButtonMap_;

[Avi (use Gerrit), 2017/03/23 15:44:53]

Can you add a comment for this object? The buttons in this map seem
dually-owned: once by the map, once by the view. True?

[lgrey, 2017/04/10 19:14:32]

Done.

Yes that's correct re: ownership. My read is that they won't leak because:
1) In applyLayout:, any button that's not visible gets removed from here
2) On dealloc, they'll get removed from superview by NSView, then will get
released when the map is destructed

(But I'm a C++ noob, so maybe that's wrong?)

+  // A place to stash bookmark buttons that have been removed from the bar

[Avi (use Gerrit), 2017/03/23 15:44:53]

Have an extra line before comments like this to visually group them with the
variable they describe.

[lgrey, 2017/04/10 19:14:32]

Done.

+  // so that they can be reused instead of creating new ones.
+  base::scoped_nsobject<NSMutableArray> unusedButtonPool_;
+}
 
 @synthesize currentState = currentState_;
 @synthesize lastState = lastState_;
@@ -262,8 +278,7 @@ void RecordAppLaunch(Profile* profile, GURL url) {
 - (id)initWithBrowser:(Browser*)browser
          initialWidth:(CGFloat)initialWidth
              delegate:(id<BookmarkBarControllerDelegate>)delegate {
-  if ((self = [super initWithNibName:@"BookmarkBar"
-                              bundle:base::mac::FrameworkBundle()])) {
+  if ((self = [super initWithNibName:nil bundle:nil])) {

[Avi (use Gerrit), 2017/03/23 15:44:53]

Yay!

Can you remove the nib file too in this change?

[lgrey, 2017/04/10 19:14:32]

Already done in crrev.com/2729603008 !

     currentState_ = BookmarkBar::HIDDEN;
     lastState_ = BookmarkBar::HIDDEN;
 
@@ -274,6 +289,7 @@ void RecordAppLaunch(Profile* profile, GURL url) {
     managedBookmarkService_ =
         ManagedBookmarkServiceFactory::GetForProfile(browser_->profile());
     buttons_.reset([[NSMutableArray alloc] init]);
+    unusedButtonPool_.reset([[NSMutableArray alloc] init]);
     delegate_ = delegate;
     folderTarget_.reset(
         [[BookmarkFolderTarget alloc] initWithController:self
@@ -340,14 +356,17 @@ void RecordAppLaunch(Profile* profile, GURL url) {
   [[buttonView_ importBookmarksButton] setAction:@selector(importBookmarks:)];
 
   [self createOffTheSideButton];
+  [self createAppsPageShortcutButton];

[Avi (use Gerrit), 2017/03/23 15:44:52]

Adding this line between the other two just brings up questions. Those two are
1. create a button 2. add it.

This interrupts that pattern, but also doesn't add it. If it doesn't need to be
added, can you say why? e.g.

  [self createOffTheSideButton];
  [buttonView_ addSubview:offTheSideButton_];

  [self createAppsPageShortcutButton];
  // Don't (yet) add the apps page shortcut button because _reasons_.

[lgrey, 2017/04/10 19:14:32]

Factored the "extra" button creation stuff out

   [buttonView_ addSubview:offTheSideButton_];
 
   [self.view addSubview:buttonView_];
+
   // viewDidLoad became part of the API in 10.10

[Avi (use Gerrit), 2017/03/23 15:44:53]

. for sentences; might as well add them as you go.

[lgrey, 2017/04/10 19:14:32]

Done.

   if (!base::mac::IsAtLeastOS10_10())
     [self viewDidLoad];
 }
 
+// Find a parent whose button is visible on the bookmark bar.
 - (BookmarkButton*)bookmarkButtonToPulseForNode:(const BookmarkNode*)node {

[Elly Fong-Jones, 2017/03/23 15:40:21]

this comment only needs to exist because this function name is very opaque, so
perhaps we can change the function name to something more descriptive?
findAncestorButtonOnBarForNode or something :)

[lgrey, 2017/04/10 19:14:32]

Done.

   // Find the closest parent that is visible on the bar.
   while (node) {
@@ -457,20 +476,13 @@ void RecordAppLaunch(Profile* profile, GURL url) {
 
   // Remember the original sizes of the 'no items' and 'import bookmarks'
   // fields to aid in resizing when the window frame changes.
-  originalNoItemsRect_ = [[buttonView_ noItemTextfield] frame];
-  originalImportBookmarksRect_ = [[buttonView_ importBookmarksButton] frame];
-
-  // Bookmark buttons start farther from the bookmark bar's left edge so
-  // adjust the positions of the noItems and importBookmarks textfields.
-  const CGFloat kBookmarksTextfieldOffsetX = 14;
-  originalNoItemsRect_.origin.x += kBookmarksTextfieldOffsetX;
-  [[buttonView_ noItemTextfield] setFrame:originalNoItemsRect_];
-
-  originalImportBookmarksRect_.origin.x += kBookmarksTextfieldOffsetX;
-  [[buttonView_ importBookmarksButton] setFrame:originalImportBookmarksRect_];
+  NSRect noItemTextfieldFrame = [[buttonView_ noItemTextField] frame];
+  NSRect noItemButtonFrame = [[buttonView_ importBookmarksButton] frame];
+  originalNoItemTextFieldWidth_ = NSWidth(noItemTextfieldFrame);
+  originalImportBookmarksButtonWidth_ = NSWidth(noItemButtonFrame);
+  originalNoItemInterelementPadding_ =
+      NSMinX(noItemButtonFrame) - NSMaxX(noItemTextfieldFrame);
 
-  // When resized we may need to add new buttons, or remove them (if
-  // no longer visible), or add/remove the "off the side" menu.
   [[self view] setPostsFrameChangedNotifications:YES];
   [[NSNotificationCenter defaultCenter]
     addObserver:self
@@ -489,7 +501,7 @@ void RecordAppLaunch(Profile* profile, GURL url) {
          selector:@selector(willEnterOrLeaveFullscreen:)
              name:NSWindowWillExitFullScreenNotification
            object:nil];
-
+  [self layoutSubviews];
   // Don't pass ourself along (as 'self') until our init is completely

[Avi (use Gerrit), 2017/03/23 15:44:53]

This is another situation where we should have a blank line before this comment
section, to visually group it with the code under it that it talks about.

[lgrey, 2017/04/10 19:14:31]

Done.

   // done.  Thus, this call is (almost) last.
   bridge_.reset(new BookmarkBarBridge(browser_->profile(), self,
@@ -554,6 +566,7 @@ void RecordAppLaunch(Profile* profile, GURL url) {
             [[self view] autoresizingMask]);
   [[self view] setFrame:frame];
   [self layoutSubviews];
+  [self frameDidChange];
 }
 
 // Change the layout of the bookmark bar's subviews in response to a visibility
@@ -569,7 +582,6 @@ void RecordAppLaunch(Profile* profile, GURL url) {
   padding = bookmarks::kNTPBookmarkBarPadding;
   buttonViewFrame =
       NSInsetRect(buttonViewFrame, morph * padding, morph * padding);
-
   [buttonView_ setFrame:buttonViewFrame];
 
   // Update bookmark button backgrounds.
@@ -592,19 +604,6 @@ void RecordAppLaunch(Profile* profile, GURL url) {
   [self showBookmarkBarWithAnimation:NO];
 }
 
-- (void)updateExtraButtonsVisibility {
-  if (!appsPageShortcutButton_.get() ||
-      !managedBookmarksButton_.get() ||
-      !supervisedBookmarksButton_.get()) {
-    return;
-  }
-  [self setAppsPageShortcutButtonVisibility];
-  [self setManagedBookmarksButtonVisibility];
-  [self setSupervisedBookmarksButtonVisibility];
-  [self resetAllButtonPositionsWithAnimation:NO];
-  [self reconfigureBookmarkBar];
-}
-
 - (void)updateHiddenState {
   BOOL oldHidden = [[self view] isHidden];
   BOOL newHidden = ![self isVisible];
@@ -829,7 +828,7 @@ void RecordAppLaunch(Profile* profile, GURL url) {
   // Middle click on chevron should not open bookmarks under it, instead just
   // open its folder menu.
   if (sender == offTheSideButton_.get()) {
-    [[sender cell] setStartingChildIndex:displayedButtonCount_];
+    [[sender cell] setStartingChildIndex:layout_.VisibleButtonCount()];
     NSEvent* event = [NSApp currentEvent];
     if ([event type] == NSOtherMouseUp) {
       [self openOrCloseBookmarkFolderForOffTheSideButton];
@@ -892,55 +891,6 @@ void RecordAppLaunch(Profile* profile, GURL url) {
       BOOKMARK_LAUNCH_LOCATION_ATTACHED_BAR;
 }
 
-// Position the right-side buttons including the off-the-side chevron.
-- (void)positionRightSideButtons {
-  int maxX = NSMaxX([[self buttonView] bounds]) -
-      bookmarks::kBookmarkHorizontalPadding;
-  int right = maxX;
-
-  int ignored = 0;
-  NSRect frame = [self frameForBookmarkButtonFromCell:
-      [otherBookmarksButton_ cell] xOffset:&ignored];
-  if (![otherBookmarksButton_ isHidden]) {
-    right -= NSWidth(frame);
-    frame.origin.x = right;
-  } else {
-    frame.origin.x = maxX - NSWidth(frame);
-  }
-  [otherBookmarksButton_ setFrame:frame];
-
-  frame = [offTheSideButton_ frame];
-  frame.size.height = bookmarks::kBookmarkFolderButtonHeight;
-  right -= frame.size.width;
-  frame.origin.x = right;
-  [offTheSideButton_ setFrame:frame];
-}
-
-// Configure the off-the-side button (e.g. specify the node range,
-// check if we should enable or disable it, etc).
-- (void)configureOffTheSideButtonContentsAndVisibility {
-  [[offTheSideButton_ cell] setStartingChildIndex:displayedButtonCount_];
-  [[offTheSideButton_ cell]
-   setBookmarkNode:bookmarkModel_->bookmark_bar_node()];
-  int bookmarkChildren = bookmarkModel_->bookmark_bar_node()->child_count();
-  if (bookmarkChildren > displayedButtonCount_) {
-    [offTheSideButton_ setHidden:NO];
-    // Set the off the side button as needing re-display. This is needed to
-    // avoid the button being shown with a black background the first time
-    // it's displayed. See https://codereview.chromium.org/1630453002/ for
-    // more context.
-    [offTheSideButton_ setNeedsDisplay:YES];
-  } else {
-    // If we just deleted the last item in an off-the-side menu so the
-    // button will be going away, make sure the menu goes away.
-    if (folderController_ &&
-        ([folderController_ parentButton] == offTheSideButton_))
-      [self closeAllBookmarkFolders];
-    // (And hide the button, too.)
-    [offTheSideButton_ setHidden:YES];
-  }
-}
-
 // Main menubar observation code, so we can know to close our fake menus if the
 // user clicks on the actual menubar, as multiple unconnected menus sharing
 // the screen looks weird.
@@ -997,19 +947,30 @@ void RecordAppLaunch(Profile* profile, GURL url) {
   }
 }
 
-// Keep the "no items" label centered in response to a frame size change.
+// Keep the "no items" label vertically centered in response to a frame size
+// change.
 - (void)centerNoItemsLabel {
   // Note that this computation is done in the parent's coordinate system,
   // which is unflipped. Also, we want the label to be a fixed distance from
   // the bottom, so that it slides up properly (on animating to hidden).
   // The textfield sits in the itemcontainer, so to center it we maintain
   // equal vertical padding on the top and bottom.
-  int yoffset = (NSHeight([[buttonView_ noItemTextfield] frame]) -
-                 NSHeight([[buttonView_ noItemContainer] frame])) / 2;
-  [[buttonView_ noItemContainer] setFrameOrigin:NSMakePoint(0, yoffset)];
+  NSView* textField = [buttonView_ noItemTextField];
+  NSView* button = [buttonView_ importBookmarksButton];
+  NSRect textFieldFrame = [textField frame];
+  NSRect buttonFrame = [button frame];
+  // +1 is a fudge factor to make it line up

[Avi (use Gerrit), 2017/03/23 15:44:52]

period at the end of sentences

Side question re lining up: tested on both retina and non-retina screens? This
is a bug fix?

[lgrey, 2017/04/10 19:14:31]

Turns out this whole thing isn't necessary and a static y origin is fine. I'm
changing the view hierarchy to be shallower, so the fudge factor was to
compensate for a superview that's not there anymore.

+  int yOffset =
+      (NSHeight([buttonView_ frame]) - NSHeight(textFieldFrame)) / 2 + 1;
+  textFieldFrame.origin.y = yOffset;
+  buttonFrame.origin.y = yOffset;
+  [textField setFrame:textFieldFrame];
+  [button setFrame:buttonFrame];
 }
 
-// (Private)
+// Show/hide the bookmark bar.
+// if |animate| is YES, the changes are made using the animator; otherwise they

[Avi (use Gerrit), 2017/03/23 15:44:53]

Full sentences; capitalize the first letter.

[lgrey, 2017/04/10 19:14:32]

Done.

+// are made immediately.
 - (void)showBookmarkBarWithAnimation:(BOOL)animate {
   if (animate && stateAnimationsEnabled_) {
     // If |-doBookmarkBarAnimation| does the animation, we're done.
@@ -1036,7 +997,8 @@ void RecordAppLaunch(Profile* profile, GURL url) {
   [self frameDidChange];
 }
 
-// (Private)
+// Handles animating the resize of the content view. Returns YES if it handled
+// the animation, NO if not (and hence it should be done instantly).
 - (BOOL)doBookmarkBarAnimation {
   BookmarkBarToolbarView* view = [self controlledView];
   if ([self isAnimatingFromState:BookmarkBar::HIDDEN
@@ -1112,7 +1074,7 @@ void RecordAppLaunch(Profile* profile, GURL url) {
   return std::min([cell cellSize].width, bookmarks::kDefaultBookmarkWidth);
 }
 
-- (IBAction)openBookmarkMenuItem:(id)sender {
+- (void)openBookmarkMenuItem:(id)sender {

[Avi (use Gerrit), 2017/03/23 15:44:53]

Getting rid of IBAction makes me happier than it reasonably should.

   int64_t tag = [self nodeIdFromMenuTag:[sender tag]];
   const BookmarkNode* node =
       bookmarks::GetBookmarkNodeByID(bookmarkModel_, tag);
@@ -1121,189 +1083,66 @@ void RecordAppLaunch(Profile* profile, GURL url) {
   [self openURL:node->url() disposition:disposition];
 }
 
-// For the given root node of the bookmark bar, show or hide (as
-// appropriate) the "no items" container (text which says "bookmarks
-// go here").
-- (void)showOrHideNoItemContainerForNode:(const BookmarkNode*)node {
-  BOOL hideNoItemWarning = !node->empty();
-  [[buttonView_ noItemContainer] setHidden:hideNoItemWarning];
-}
-
-// TODO(jrg): write a "build bar" so there is a nice spot for things
-// like the contextual menu which is invoked when not over a
-// bookmark.  On Safari that menu has a "new folder" option.
-- (void)addNodesToButtonList:(const BookmarkNode*)node {
-  [self showOrHideNoItemContainerForNode:node];
-
-  CGFloat maxViewX = NSMaxX([[self view] bounds]);
-  int xOffset =
-      bookmarks::kBookmarkLeftMargin - bookmarks::kBookmarkHorizontalPadding;
-
-  // Draw the apps bookmark if needed.
-  if (![appsPageShortcutButton_ isHidden]) {
-    NSRect frame =
-        [self frameForBookmarkButtonFromCell:[appsPageShortcutButton_ cell]
-                                     xOffset:&xOffset];
-    [appsPageShortcutButton_ setFrame:frame];
-  }
-
-  // Draw the managed bookmark folder if needed.
-  if (![managedBookmarksButton_ isHidden]) {
-    xOffset += bookmarks::kBookmarkHorizontalPadding;
-    NSRect frame =
-        [self frameForBookmarkButtonFromCell:[managedBookmarksButton_ cell]
-                                     xOffset:&xOffset];
-    [managedBookmarksButton_ setFrame:frame];
-  }
-
-  // Draw the supervised bookmark folder if needed.
-  if (![supervisedBookmarksButton_ isHidden]) {
-    xOffset += bookmarks::kBookmarkHorizontalPadding;
-    NSRect frame =
-        [self frameForBookmarkButtonFromCell:[supervisedBookmarksButton_ cell]
-                                     xOffset:&xOffset];
-    [supervisedBookmarksButton_ setFrame:frame];
-  }
-
-  for (int i = 0; i < node->child_count(); i++) {
-    const BookmarkNode* child = node->GetChild(i);
-    BookmarkButton* button = [self buttonForNode:child xOffset:&xOffset];
-    if (NSMinX([button frame]) >= maxViewX) {
-      [button setDelegate:nil];
-      break;
-    }
-    [buttons_ addObject:button];
-  }
-}
-
-- (BookmarkButton*)buttonForNode:(const BookmarkNode*)node
-                         xOffset:(int*)xOffset {
-  BookmarkButtonCell* cell = [self cellForBookmarkNode:node];
-  NSRect frame = [self frameForBookmarkButtonFromCell:cell xOffset:xOffset];
-
-  base::scoped_nsobject<BookmarkButton> button(
-      [[BookmarkButton alloc] initWithFrame:frame]);
-  DCHECK(button.get());
-
-  // [NSButton setCell:] warns to NOT use setCell: other than in the
-  // initializer of a control.  However, we are using a basic
-  // NSButton whose initializer does not take an NSCell as an
-  // object.  To honor the assumed semantics, we do nothing with
-  // NSButton between alloc/init and setCell:.
-  [button setCell:cell];
-  [button setDelegate:self];
-
-  // We cannot set the button cell's text color until it is placed in
-  // the button (e.g. the [button setCell:cell] call right above).  We
-  // also cannot set the cell's text color until the view is added to
-  // the hierarchy.  If that second part is now true, set the color.
-  // (If not we'll set the color on the 1st themeChanged:
-  // notification.)
-  const ui::ThemeProvider* themeProvider = [[[self view] window] themeProvider];
-  if (themeProvider) {
-    NSColor* color =
-        themeProvider->GetNSColor(ThemeProperties::COLOR_BOOKMARK_TEXT);
-    [cell setTextColor:color];
+- (BookmarkButton*)buttonForNode:(const BookmarkNode*)node {

[Elly Fong-Jones, 2017/03/23 15:40:21]

the name of this reads like a simple accessor to me. Since it can actually
allocate a new button, I would call it "getOrCreateButtonForNode:", which is
longer but I think clearer.

+  BookmarkButton* button = nil;
+  int64_t nodeId = node->id();
+  if (nodeIdToButtonMap_.find(nodeId) != nodeIdToButtonMap_.end()) {
+    button = nodeIdToButtonMap_.at(node->id()).get();

[Avi (use Gerrit), 2017/03/23 15:44:52]

auto buttonIt = nodeIdToButtonMap_.find(nodeId);
if (buttonIt != nodeIdToButtonMap_.end() {
  button = *buttonIt;

lets you reuse the iterator.

[lgrey, 2017/04/10 19:14:32]

Done.

+    [self updateTitleAndTooltipForButton:button];
+  } else if ([unusedButtonPool_ count] > 0) {
+    button = [[[unusedButtonPool_ firstObject] retain] autorelease];
+    [unusedButtonPool_ removeObjectAtIndex:0];
+    BOOL darkTheme = [[[self view] window] hasDarkTheme];
+    [[button cell]
+        setBookmarkNode:node
+                  image:[self faviconForNode:node forADarkTheme:darkTheme]];
+  } else {
+    BookmarkButtonCell* cell = [self cellForBookmarkNode:node];
+    NSRect frame = NSMakeRect(0, bookmarks::kBookmarkVerticalPadding, 0,
+                              kBookmarkButtonHeightMinusPadding);
+    button = [[[BookmarkButton alloc] initWithFrame:frame] autorelease];
+    [button setCell:cell];
+    [buttonView_ addSubview:button];
+    [button setDelegate:self];
   }
-
+  DCHECK(button);
+  // Do setup

[Avi (use Gerrit), 2017/03/23 15:44:52]

blank line above, period at end

[lgrey, 2017/04/10 19:14:32]

Done.

+  nodeIdToButtonMap_.insert(

[Elly Fong-Jones, 2017/03/23 15:40:22]

why does this happen even if we found the button in nodeIdToButtonMap_ earlier?

[lgrey, 2017/04/10 19:14:32]

A certain someone got sloppy rearranging while iterating on this. Fixed.

+      {nodeId, base::scoped_nsobject<BookmarkButton>([button retain])});
   if (node->is_folder()) {
     [button setTarget:self];
     [button setAction:@selector(openBookmarkFolderFromButton:)];
     [[button draggableButton] setActsOnMouseDown:YES];
-    // If it has a title, and it will be truncated, show full title in
-    // tooltip.
-    NSString* title = base::SysUTF16ToNSString(node->GetTitle());
-    if ([title length] &&
-        [[button cell] cellSize].width > bookmarks::kDefaultBookmarkWidth) {
-      [button setToolTip:title];
-    }
   } else {
     // Make the button do something
     [button setTarget:self];
     [button setAction:@selector(openBookmark:)];
-    if (node->is_url())
-      [button setToolTip:[BookmarkMenuCocoaController tooltipForNode:node]];
-  }
-  return [[button.get() retain] autorelease];
-}
-
-// Add bookmark buttons to the view only if they are completely
-// visible and don't overlap the "other bookmarks".  Remove buttons
-// which are clipped.  Called when building the bookmark bar the first time.
-- (void)addButtonsToView {
-  displayedButtonCount_ = 0;
-  NSMutableArray* buttons = [self buttons];
-  for (NSButton* button in buttons) {
-    if (NSMaxX([button frame]) > (NSMinX([offTheSideButton_ frame]) -
-                                  bookmarks::kBookmarkHorizontalPadding))
-      break;
-    [buttonView_ addSubview:button];
-    ++displayedButtonCount_;
-  }
-  NSUInteger removalCount =
-      [buttons count] - (NSUInteger)displayedButtonCount_;
-  if (removalCount > 0) {
-    NSRange removalRange = NSMakeRange(displayedButtonCount_, removalCount);
-    [buttons removeObjectsInRange:removalRange];
+    [[button draggableButton] setActsOnMouseDown:NO];
   }
+  [self updateTitleAndTooltipForButton:button];
+  return button;
 }
 
-// Shows or hides the Managed, Supervised, or Other Bookmarks button as
-// appropriate, and returns whether it ended up visible.
-- (BOOL)setBookmarkButtonVisibility:(BookmarkButton*)button
-                            canShow:(BOOL)show
-            resetAllButtonPositions:(BOOL)resetButtons {
-  if (!button)
-    return NO;
-
-  BOOL visible = ![button bookmarkNode]->empty() && show;
-  BOOL currentVisibility = ![button isHidden];
-  if (currentVisibility != visible) {
-    [button setHidden:!visible];
-    if (resetButtons)
-      [self resetAllButtonPositionsWithAnimation:NO];
+- (void)updateTitleAndTooltipForButton:(BookmarkButton*)button {
+  const BookmarkNode* node = [button bookmarkNode];
+  CGFloat buttonWidth = [self widthOfButtonForNode:node];
+  NSString* buttonTitle = base::SysUTF16ToNSString(node->GetTitle());
+  if (NSWidth([button frame]) == buttonWidth &&
+      [[button title] isEqualToString:buttonTitle])
+    return;
+  CGRect frame = [button frame];
+  frame.size.width = buttonWidth;
+  [button setFrame:frame];
+  [[button cell] setTitle:buttonTitle];
+  NSString* tooltip = nil;
+  // Folders show a tooltip iff the title is truncated.

[Avi (use Gerrit), 2017/03/23 15:44:52]

blank line above this comment

this function needs more blank lines to clarify flow

[lgrey, 2017/04/10 19:14:33]

Done.

+  if (node->is_folder() && [buttonTitle length] > 0 &&
+      [[button cell] cellSize].width < buttonWidth) {
+    tooltip = buttonTitle;
+  } else if (node->is_url()) {
+    tooltip = [BookmarkMenuCocoaController tooltipForNode:node];
   }
-  return visible;
-}
-
-// Shows or hides the Managed Bookmarks button as appropriate, and returns
-// whether it ended up visible.
-- (BOOL)setManagedBookmarksButtonVisibility {
-  PrefService* prefs = browser_->profile()->GetPrefs();
-  BOOL prefIsSet =
-      prefs->GetBoolean(bookmarks::prefs::kShowManagedBookmarksInBookmarkBar);
-  return [self setBookmarkButtonVisibility:managedBookmarksButton_.get()
-                                   canShow:prefIsSet
-                   resetAllButtonPositions:YES];
-}
-
-// Shows or hides the Supervised Bookmarks button as appropriate, and returns
-// whether it ended up visible.
-- (BOOL)setSupervisedBookmarksButtonVisibility {
-  return [self setBookmarkButtonVisibility:supervisedBookmarksButton_.get()
-                                   canShow:YES
-                   resetAllButtonPositions:YES];
-}
-
-// Shows or hides the Other Bookmarks button as appropriate, and returns
-// whether it ended up visible.
-- (BOOL)setOtherBookmarksButtonVisibility {
-  return [self setBookmarkButtonVisibility:otherBookmarksButton_.get()
-                                   canShow:YES
-                   resetAllButtonPositions:NO];
-}
-
-// Shows or hides the Apps button as appropriate, and returns whether it ended
-// up visible.
-- (BOOL)setAppsPageShortcutButtonVisibility {
-  if (!appsPageShortcutButton_.get())
-    return NO;
-
-  BOOL visible =
-      bookmarkModel_->loaded() &&
-      chrome::ShouldShowAppsShortcutInBookmarkBar(browser_->profile());
-  [appsPageShortcutButton_ setHidden:!visible];
-  return visible;
+  [button setToolTip:tooltip];
 }
 
 // Creates a bookmark bar button that does not correspond to a regular bookmark
@@ -1331,9 +1170,6 @@ void RecordAppLaunch(Profile* profile, GURL url) {
     NSCell* cell = [managedBookmarksButton_ cell];
     [cell setTitle:title];
 
-    // Its visibility may have changed too.
-    [self setManagedBookmarksButtonVisibility];
-
     return;
   }
 
@@ -1342,16 +1178,17 @@ void RecordAppLaunch(Profile* profile, GURL url) {
   managedBookmarksButton_.reset([self createCustomBookmarkButtonForCell:cell]);
   [managedBookmarksButton_ setAction:@selector(openBookmarkFolderFromButton:)];
   view_id_util::SetID(managedBookmarksButton_.get(), VIEW_ID_MANAGED_BOOKMARKS);
+  NSRect frame = NSMakeRect(0, bookmarks::kBookmarkVerticalPadding,
+                            [self widthForBookmarkButtonCell:cell],
+                            kBookmarkButtonHeightMinusPadding);
+  [managedBookmarksButton_ setFrame:frame];
   [buttonView_ addSubview:managedBookmarksButton_.get()];
 
-  [self setManagedBookmarksButtonVisibility];
 }
 
 // Creates the button for "Supervised Bookmarks", but does not position it.
 - (void)createSupervisedBookmarksButton {
   if (supervisedBookmarksButton_.get()) {
-    // The button's already there, but its visibility may have changed.
-    [self setSupervisedBookmarksButtonVisibility];
     return;
   }
 
@@ -1363,9 +1200,11 @@ void RecordAppLaunch(Profile* profile, GURL url) {
       setAction:@selector(openBookmarkFolderFromButton:)];
   view_id_util::SetID(supervisedBookmarksButton_.get(),
                       VIEW_ID_SUPERVISED_BOOKMARKS);
+  NSRect frame = NSMakeRect(0, bookmarks::kBookmarkVerticalPadding,
+                            [self widthForBookmarkButtonCell:cell],
+                            kBookmarkButtonHeightMinusPadding);
+  [supervisedBookmarksButton_ setFrame:frame];
   [buttonView_ addSubview:supervisedBookmarksButton_.get()];
-
-  [self setSupervisedBookmarksButtonVisibility];
 }
 
 // Creates the button for "Other Bookmarks", but does not position it.
@@ -1373,19 +1212,18 @@ void RecordAppLaunch(Profile* profile, GURL url) {
   // Can't create this until the model is loaded, but only need to
   // create it once.
   if (otherBookmarksButton_.get()) {
-    [self setOtherBookmarksButtonVisibility];
     return;
   }
 
   NSCell* cell = [self cellForBookmarkNode:bookmarkModel_->other_node()];
   otherBookmarksButton_.reset([self createCustomBookmarkButtonForCell:cell]);
-  // Peg at right; keep same height as bar.
-  [otherBookmarksButton_ setAutoresizingMask:(NSViewMinXMargin)];
   [otherBookmarksButton_ setAction:@selector(openBookmarkFolderFromButton:)];
+  NSRect frame = NSMakeRect(0, bookmarks::kBookmarkVerticalPadding,
+                            [self widthForBookmarkButtonCell:cell],
+                            kBookmarkButtonHeightMinusPadding);
+  [otherBookmarksButton_ setFrame:frame];
   view_id_util::SetID(otherBookmarksButton_.get(), VIEW_ID_OTHER_BOOKMARKS);
   [buttonView_ addSubview:otherBookmarksButton_.get()];
-
-  [self setOtherBookmarksButtonVisibility];
 }
 
 // Creates the button for "Apps", but does not position it.
@@ -1393,7 +1231,6 @@ void RecordAppLaunch(Profile* profile, GURL url) {
   // Can't create this until the model is loaded, but only need to
   // create it once.
   if (appsPageShortcutButton_.get()) {
-    [self setAppsPageShortcutButtonVisibility];
     return;
   }
 
@@ -1403,7 +1240,12 @@ void RecordAppLaunch(Profile* profile, GURL url) {
       IDR_BOOKMARK_BAR_APPS_SHORTCUT).ToNSImage();
   NSCell* cell = [self cellForCustomButtonWithText:text
                                              image:image];
+  NSRect frame;
+  frame.origin.y = bookmarks::kBookmarkVerticalPadding;
+  frame.size = NSMakeSize([self widthForBookmarkButtonCell:cell],
+                          kBookmarkButtonHeightMinusPadding);
   appsPageShortcutButton_.reset([self createCustomBookmarkButtonForCell:cell]);
+  [appsPageShortcutButton_ setFrame:frame];
   [[appsPageShortcutButton_ draggableButton] setActsOnMouseDown:NO];
   [appsPageShortcutButton_ setAction:@selector(openAppsPage:)];
   NSString* tooltip =
@@ -1411,7 +1253,6 @@ void RecordAppLaunch(Profile* profile, GURL url) {
   [appsPageShortcutButton_ setToolTip:tooltip];
   [buttonView_ addSubview:appsPageShortcutButton_.get()];
 
-  [self setAppsPageShortcutButtonVisibility];
 }
 
 - (void)createOffTheSideButton {
@@ -1433,6 +1274,7 @@ void RecordAppLaunch(Profile* profile, GURL url) {
   [offTheSideButton_ setDelegate:self];
   [[offTheSideButton_ draggableButton] setDraggable:NO];

[Elly Fong-Jones, 2017/03/23 15:40:21]

this may be a silly question, but if the offTheSideButton_ is never draggable,
why does it have a draggableButton?

[lgrey, 2017/04/10 19:14:32]

BookmarkButton is a subclass of DraggableButton. If the question is why is this
a BookmarkButton vs. something else, I'm not totally sure (maybe the
|bookmarkNode| property?) I can look into this in a follow-up CL, does that
sound good?

   [[offTheSideButton_ draggableButton] setActsOnMouseDown:YES];
+  [offTheSideButton_ setHidden:YES];
 }
 
 - (void)openAppsPage:(id)sender {
@@ -1449,6 +1291,11 @@ void RecordAppLaunch(Profile* profile, GURL url) {
   [contextMenuController_ cancelTracking];
 }
 
+// Moves to the given next state (from the current state), possibly animating.
+// If |animate| is NO, it will stop any running animation and jump to the given
+// state. If YES, it may either (depending on implementation) jump to the end of
+// the current animation and begin the next one, or stop the current animation
+// mid-flight and animate to the next state.
 - (void)moveToState:(BookmarkBar::State)nextState
       withAnimation:(BOOL)animate {
   BOOL isAnimationRunning = [self isAnimationRunning];
@@ -1515,7 +1362,8 @@ void RecordAppLaunch(Profile* profile, GURL url) {
   [self moveToState:newState withAnimation:animate];
 }
 
-// (Private)
+// Puts stuff into the final state without animating, stopping a running

[Elly Fong-Jones, 2017/03/23 15:40:21]

Can you expand on what "stuff" is? What state specifically is being finalized
here?

[lgrey, 2017/04/10 19:14:32]

Done.

+// animation if necessary.
 - (void)finalizeState {
   // We promise that our delegate that the variables will be finalized before
   // the call to |-bookmarkBar:didChangeFromState:toState:|.
@@ -1532,7 +1380,7 @@ void RecordAppLaunch(Profile* profile, GURL url) {
   [self updateVisibility];
 }
 
-// (Private)
+// Stops any current animation in its tracks (midway).
 - (void)stopCurrentAnimation {
   [[self controlledView] stopAnimation];
 }
@@ -1543,36 +1391,6 @@ void RecordAppLaunch(Profile* profile, GURL url) {
   [self finalizeState];
 }
 
-- (void)reconfigureBookmarkBar {
-  [self setManagedBookmarksButtonVisibility];
-  [self setSupervisedBookmarksButtonVisibility];
-  [self redistributeButtonsOnBarAsNeeded];
-  [self positionRightSideButtons];
-  [self configureOffTheSideButtonContentsAndVisibility];
-  [self centerNoItemsLabel];
-}
-
-// Determine if the given |view| can completely fit within the constraint of
-// maximum x, given by |maxViewX|, and, if not, narrow the view up to a minimum
-// width. If the minimum width is not achievable then hide the view. Return YES
-// if the view was hidden.
-- (BOOL)shrinkOrHideView:(NSView*)view forMaxX:(CGFloat)maxViewX {
-  BOOL wasHidden = NO;
-  // See if the view needs to be narrowed.
-  NSRect frame = [view frame];
-  if (NSMaxX(frame) > maxViewX) {
-    // Resize if more than 30 pixels are showing, otherwise hide.
-    if (NSMinX(frame) + 30.0 < maxViewX) {
-      frame.size.width = maxViewX - NSMinX(frame);
-      [view setFrame:frame];
-    } else {
-      [view setHidden:YES];
-      wasHidden = YES;
-    }
-  }
-  return wasHidden;
-}
-
 // Bookmark button menu items that open a new window (e.g., open in new window,
 // open in incognito, edit, etc.) cause us to lose a mouse-exited event
 // on the button, which leaves it in a hover state.
@@ -1598,173 +1416,9 @@ void RecordAppLaunch(Profile* profile, GURL url) {
   }
 }
 
-
-// Adjust the horizontal width, x position and the visibility of the "For quick

[Elly Fong-Jones, 2017/03/23 15:40:22]

it warms my heart to see all this code being removed :)

-// access" text field and "Import bookmarks..." button based on the current
-// width of the containing |buttonView_| (which is affected by window width).
-- (void)adjustNoItemContainerForMaxX:(CGFloat)maxViewX {
-  if (![[buttonView_ noItemContainer] isHidden]) {
-    // Reset initial frames for the two items, then adjust as necessary.
-    NSTextField* noItemTextfield = [buttonView_ noItemTextfield];
-    NSRect noItemsRect = originalNoItemsRect_;
-    NSRect importBookmarksRect = originalImportBookmarksRect_;
-    if (![appsPageShortcutButton_ isHidden]) {
-      float width = NSWidth([appsPageShortcutButton_ frame]);
-      noItemsRect.origin.x += width;
-      importBookmarksRect.origin.x += width;
-    }
-    if (![managedBookmarksButton_ isHidden]) {
-      float width = NSWidth([managedBookmarksButton_ frame]);
-      noItemsRect.origin.x += width;
-      importBookmarksRect.origin.x += width;
-    }
-    if (![supervisedBookmarksButton_ isHidden]) {
-      float width = NSWidth([supervisedBookmarksButton_ frame]);
-      noItemsRect.origin.x += width;
-      importBookmarksRect.origin.x += width;
-    }
-    [noItemTextfield setFrame:noItemsRect];
-    [noItemTextfield setHidden:NO];
-    NSButton* importBookmarksButton = [buttonView_ importBookmarksButton];
-    [importBookmarksButton setFrame:importBookmarksRect];
-    [importBookmarksButton setHidden:NO];
-    // Check each to see if they need to be shrunk or hidden.
-    if ([self shrinkOrHideView:importBookmarksButton forMaxX:maxViewX])
-      [self shrinkOrHideView:noItemTextfield forMaxX:maxViewX];
-  }
-}
-
-// Scans through all buttons from left to right, calculating from scratch where
-// they should be based on the preceding widths, until it finds the one
-// requested.
-// Returns NSZeroRect if there is no such button in the bookmark bar.
-// Enables you to work out where a button will end up when it is done animating.
-- (NSRect)finalRectOfButton:(BookmarkButton*)wantedButton {
-  CGFloat left = bookmarks::kBookmarkLeftMargin;
-  NSRect buttonFrame = NSZeroRect;
-
-  // Draw the apps bookmark if needed.
-  if (![appsPageShortcutButton_ isHidden]) {
-    left = NSMaxX([appsPageShortcutButton_ frame]) +
-        bookmarks::kBookmarkHorizontalPadding;
-  }
-
-  // Draw the managed bookmarks folder if needed.
-  if (![managedBookmarksButton_ isHidden]) {
-    left = NSMaxX([managedBookmarksButton_ frame]) +
-        bookmarks::kBookmarkHorizontalPadding;
-  }
-
-  // Draw the supervised bookmarks folder if needed.
-  if (![supervisedBookmarksButton_ isHidden]) {
-    left = NSMaxX([supervisedBookmarksButton_ frame]) +
-        bookmarks::kBookmarkHorizontalPadding;
-  }
-
-  for (NSButton* button in buttons_.get()) {
-    // Hidden buttons get no space.
-    if ([button isHidden])
-      continue;
-    buttonFrame = [button frame];
-    buttonFrame.origin.x = left;
-    left += buttonFrame.size.width + bookmarks::kBookmarkHorizontalPadding;
-    if (button == wantedButton)
-      return buttonFrame;
-  }
-  return NSZeroRect;
-}
-
-// Calculates the final position of the last button in the bar.
-// We can't just use [[self buttons] lastObject] frame] because the button
-// may be animating currently.
-- (NSRect)finalRectOfLastButton {
-  return [self finalRectOfButton:[[self buttons] lastObject]];
-}
-
-- (CGFloat)buttonViewMaxXWithOffTheSideButtonIsVisible:(BOOL)visible {
-  CGFloat maxViewX = NSMaxX([buttonView_ bounds]);
-  // If necessary, pull in the width to account for the Other Bookmarks button.
-  if ([self setOtherBookmarksButtonVisibility]) {
-    maxViewX = [otherBookmarksButton_ frame].origin.x -
-        bookmarks::kBookmarkRightMargin;
-  }
-
-  [self positionRightSideButtons];
-  // If we're already overflowing, then we need to account for the chevron.
-  if (visible) {
-    maxViewX =
-        [offTheSideButton_ frame].origin.x - bookmarks::kBookmarkRightMargin;
-  }
-
-  return maxViewX;
-}
-
-- (void)redistributeButtonsOnBarAsNeeded {
-  const BookmarkNode* node = bookmarkModel_->bookmark_bar_node();
-  NSInteger barCount = node->child_count();
-
-  // Determine the current maximum extent of the visible buttons.
-  [self positionRightSideButtons];
-  BOOL offTheSideButtonVisible = (barCount > displayedButtonCount_);
-  CGFloat maxViewX = [self buttonViewMaxXWithOffTheSideButtonIsVisible:
-      offTheSideButtonVisible];
-
-  // As a result of pasting or dragging, the bar may now have more buttons
-  // than will fit so remove any which overflow.  They will be shown in
-  // the off-the-side folder.
-  while (displayedButtonCount_ > 0) {
-    BookmarkButton* button = [buttons_ lastObject];
-    if (NSMaxX([self finalRectOfLastButton]) < maxViewX)
-      break;
-    [buttons_ removeLastObject];
-    [button setDelegate:nil];
-    [button removeFromSuperview];
-    --displayedButtonCount_;
-    // Account for the fact that the chevron might now be visible.
-    if (!offTheSideButtonVisible) {
-      offTheSideButtonVisible = YES;
-      maxViewX = [self buttonViewMaxXWithOffTheSideButtonIsVisible:YES];
-    }
-  }
-
-  // As a result of cutting, deleting and dragging, the bar may now have room
-  // for more buttons.
-  int xOffset;
-  if (displayedButtonCount_ > 0) {
-    xOffset = NSMaxX([self finalRectOfLastButton]);
-  } else if (![managedBookmarksButton_ isHidden]) {
-    xOffset = NSMaxX([managedBookmarksButton_ frame]) +
-        bookmarks::kBookmarkHorizontalPadding;
-  } else if (![supervisedBookmarksButton_ isHidden]) {
-    xOffset = NSMaxX([supervisedBookmarksButton_ frame]) +
-        bookmarks::kBookmarkHorizontalPadding;
-  } else if (![appsPageShortcutButton_ isHidden]) {
-    xOffset = NSMaxX([appsPageShortcutButton_ frame]) +
-        bookmarks::kBookmarkHorizontalPadding;
-  } else {
-    xOffset = bookmarks::kBookmarkLeftMargin -
-        bookmarks::kBookmarkHorizontalPadding;
-  }
-  for (int i = displayedButtonCount_; i < barCount; ++i) {
-    const BookmarkNode* child = node->GetChild(i);
-    BookmarkButton* button = [self buttonForNode:child xOffset:&xOffset];
-    // If we're testing against the last possible button then account
-    // for the chevron no longer needing to be shown.
-    if (i == barCount - 1)
-      maxViewX = [self buttonViewMaxXWithOffTheSideButtonIsVisible:NO];
-    if (NSMaxX([button frame]) > maxViewX) {
-      [button setDelegate:nil];
-      break;
-    }
-    ++displayedButtonCount_;
-    [buttons_ addObject:button];
-    [buttonView_ addSubview:button];
-  }
-
-  // While we're here, adjust the horizontal width and the visibility
-  // of the "For quick access" and "Import bookmarks..." text fields.
-  if (![buttons_ count])
-    [self adjustNoItemContainerForMaxX:maxViewX];
+- (void)addButtonForNode:(const bookmarks::BookmarkNode*)node
+                 atIndex:(NSInteger)buttonIndex {
+  [self rebuildLayout:NO];
 }
 
 #pragma mark Private Methods Exposed for Testing
@@ -1797,10 +1451,6 @@ void RecordAppLaunch(Profile* profile, GURL url) {
   return folderTarget_.get();
 }
 
-- (int)displayedButtonCount {
-  return displayedButtonCount_;
-}
-
 // Delete all buttons (bookmarks, chevron, "other bookmarks") from the
 // bookmark bar; reset knowledge of bookmarks.
 - (void)clearBookmarkBar {
@@ -1811,7 +1461,6 @@ void RecordAppLaunch(Profile* profile, GURL url) {
   }
   [buttons_ removeAllObjects];
   [self clearMenuTagMap];
-  displayedButtonCount_ = 0;
 
   // Make sure there are no stale pointers in the pasteboard.  This
   // can be important if a bookmark is deleted (via bookmark sync)
@@ -1858,59 +1507,10 @@ void RecordAppLaunch(Profile* profile, GURL url) {
   return cell;
 }
 
-// Returns a frame appropriate for the given bookmark cell, suitable
-// for creating an NSButton that will contain it.  |xOffset| is the X
-// offset for the frame; it is increased to be an appropriate X offset
-// for the next button.
-- (NSRect)frameForBookmarkButtonFromCell:(NSCell*)cell
-                                 xOffset:(int*)xOffset {
-  DCHECK(xOffset);
-  NSRect bounds = [buttonView_ bounds];
-  bounds.size.height = bookmarks::kBookmarkButtonHeight;
-
-  NSRect frame = NSInsetRect(bounds,
-                             bookmarks::kBookmarkHorizontalPadding,
-                             bookmarks::kBookmarkVerticalPadding);
-  frame.size.width = [self widthForBookmarkButtonCell:cell];
-
-  // Add an X offset based on what we've already done
-  frame.origin.x += *xOffset;
-
-  // And up the X offset for next time.
-  *xOffset = NSMaxX(frame);
-
-  return frame;
-}
-
-// A bookmark button's contents changed.  Check for growth
-// (e.g. increase the width up to the maximum).  If we grew, move
-// other bookmark buttons over.
-- (void)checkForBookmarkButtonGrowth:(NSButton*)changedButton {
-  NSRect frame = [changedButton frame];
-  CGFloat desiredSize = [self widthForBookmarkButtonCell:[changedButton cell]];
-  CGFloat delta = desiredSize - frame.size.width;
-  if (delta) {
-    frame.size.width = desiredSize;
-    [changedButton setFrame:frame];
-    for (NSButton* button in buttons_.get()) {
-      NSRect buttonFrame = [button frame];
-      if (buttonFrame.origin.x > frame.origin.x) {
-        buttonFrame.origin.x += delta;
-        [button setFrame:buttonFrame];
-      }
-    }
-  }
-  // We may have just crossed a threshold to enable the off-the-side
-  // button.
-  [self configureOffTheSideButtonContentsAndVisibility];
-}
-
 // Called when our controlled frame has changed size.
 - (void)frameDidChange {
-  if (!bookmarkModel_->loaded())
-    return;
-  [self updateTheme:[[[self view] window] themeProvider]];
-  [self reconfigureBookmarkBar];
+  [self centerNoItemsLabel];
+  [self rebuildLayout:NO];
 }
 
 // Given a NSMenuItem tag, return the appropriate bookmark node id.
@@ -2017,8 +1617,6 @@ static BOOL ValueInRangeInclusive(CGFloat low, CGFloat value, CGFloat high) {
 // given array, or nil if none.  We use this for distinguishing
 // between a hover-open candidate or drop-indicator draw.
 // Helper for buttonForDroppingOnAtPoint:.
-// Get UI review on "middle half" ness.
-// http://crbug.com/36276
 - (BookmarkButton*)buttonForDroppingOnAtPoint:(NSPoint)point
                                     fromArray:(NSArray*)array {
   for (BookmarkButton* button in array) {
@@ -2069,7 +1667,7 @@ static BOOL ValueInRangeInclusive(CGFloat low, CGFloat value, CGFloat high) {
   // the button's folder being closed if the mouse temporarily leaves the
   // middle half but is still within the button bounds.
   if (hoverButton_ && NSPointInRect(point, [hoverButton_ frame]))
-     return hoverButton_.get();
+    return hoverButton_.get();
 
   BookmarkButton* button = [self buttonForDroppingOnAtPoint:point
                                                   fromArray:buttons_.get()];
@@ -2087,32 +1685,35 @@ static BOOL ValueInRangeInclusive(CGFloat low, CGFloat value, CGFloat high) {
 }
 
 - (int)indexForDragToPoint:(NSPoint)point {
-  // TODO(jrg): revisit position info based on UI team feedback.
-  // dropLocation is in bar local coordinates.
   NSPoint dropLocation =
       [[self view] convertPoint:point
                        fromView:[[[self view] window] contentView]];
-  BookmarkButton* buttonToTheRightOfDraggedButton = nil;
+  BookmarkButton* buttonAfterDraggedButton = nil;
+  BOOL isRTL = cocoa_l10n_util::ShouldDoExperimentalRTLLayout();

[Avi (use Gerrit), 2017/03/23 15:44:52]

Mixing in RTL support at the same time? Good luck... 😬

   for (BookmarkButton* button in buttons_.get()) {
     CGFloat midpoint = NSMidX([button frame]);
-    if (dropLocation.x <= midpoint) {
-      buttonToTheRightOfDraggedButton = button;
+    if (isRTL ? dropLocation.x >= midpoint : dropLocation.x <= midpoint) {
+      buttonAfterDraggedButton = button;
       break;
     }
   }
-  if (buttonToTheRightOfDraggedButton) {
-    const BookmarkNode* afterNode =
-        [buttonToTheRightOfDraggedButton bookmarkNode];
+  if (buttonAfterDraggedButton) {
+    const BookmarkNode* afterNode = [buttonAfterDraggedButton bookmarkNode];
     DCHECK(afterNode);
     int index = afterNode->parent()->GetIndexOf(afterNode);
     // Make sure we don't get confused by buttons which aren't visible.
-    return std::min(index, displayedButtonCount_);
+    return std::min(index, layout_.VisibleButtonCount());
   }
-
   // If nothing is to my right I am at the end!
-  return displayedButtonCount_;
+  return layout_.VisibleButtonCount();
 }
 
+// Returns the index in the model for a drag to the location given by
+// |point| is in the base coordinate system of the destination window;

[Avi (use Gerrit), 2017/03/23 15:44:53]

Fix run-on sentence.

[lgrey, 2017/04/10 19:14:32]

No wonder -- I accidentally smashed two docstrings together while copying them
over! Thanks for the catch.

+// it comes from an id<NSDraggingInfo>. |copy| is YES if a copy is to be
+// made and inserted into the new location while leaving the bookmark in
+// the old location, otherwise move the bookmark by removing from its old
+// location and inserting into the new location.
 // TODO(mrossetti,jrg): Yet more duplicated code.

[Elly Fong-Jones, 2017/03/23 15:40:21]

is the mentioned duplicated code actually still here?

[lgrey, 2017/04/10 19:14:31]

I regret to say yes

https://cs.chromium.org/chromium/src/chrome/browser/ui/cocoa/bookmarks/bookma...

 // http://crbug.com/35966
 - (BOOL)dragBookmark:(const BookmarkNode*)sourceNode
@@ -2159,159 +1760,346 @@ static BOOL ValueInRangeInclusive(CGFloat low, CGFloat value, CGFloat high) {
 
 - (void)draggingEnded:(id<NSDraggingInfo>)info {
   [self closeFolderAndStopTrackingMenus];
-  [[BookmarkButton draggedButton] setHidden:NO];
-  [self resetAllButtonPositionsWithAnimation:YES];
+  layout_ = {};  // Force layout

[Avi (use Gerrit), 2017/03/23 15:44:53]

.

[lgrey, 2017/04/10 19:14:32]

Done.

+  [self rebuildLayout:YES];
 }
 
 // Set insertionPos_ and hasInsertionPos_, and make insertion space for a
 // hypothetical drop with the new button having a left edge of |where|.
-// Gets called only by our view.
+// Gets called only by the BookmarkBarView.

[Elly Fong-Jones, 2017/03/23 15:40:21]

is it important that this is the case?

[lgrey, 2017/04/10 19:14:32]

Done.

 - (void)setDropInsertionPos:(CGFloat)where {
-  if (!hasInsertionPos_ || where != insertionPos_) {
-    insertionPos_ = where;
-    hasInsertionPos_ = YES;
-    CGFloat left;
-    if (![supervisedBookmarksButton_ isHidden]) {
-      left = NSMaxX([supervisedBookmarksButton_ frame]) +
-             bookmarks::kBookmarkHorizontalPadding;
-    } else if (![managedBookmarksButton_ isHidden]) {
-      left = NSMaxX([managedBookmarksButton_ frame]) +
-             bookmarks::kBookmarkHorizontalPadding;
-    } else if (![appsPageShortcutButton_ isHidden]) {
-      left = NSMaxX([appsPageShortcutButton_ frame]) +
-             bookmarks::kBookmarkHorizontalPadding;
-    } else {
-      left = bookmarks::kBookmarkLeftMargin;
-    }
-    CGFloat paddingWidth = bookmarks::kDefaultBookmarkWidth;
-    BookmarkButton* draggedButton = [BookmarkButton draggedButton];
-    if (draggedButton) {
-      paddingWidth = std::min(bookmarks::kDefaultBookmarkWidth,
-                              NSWidth([draggedButton frame]));
+  if (hasInsertionPos_ && where == insertionPos_) {
+    return;
+  }
+  insertionPos_ = where;
+  hasInsertionPos_ = YES;
+  CGFloat paddingWidth = bookmarks::kDefaultBookmarkWidth;
+  BookmarkButton* draggedButton = [BookmarkButton draggedButton];
+  BOOL draggedButtonIsOnBar = NO;
+  int64_t draggedButtonNodeId;
+  CGFloat draggedButtonOffset;
+  if (draggedButton) {
+    paddingWidth = std::min(bookmarks::kDefaultBookmarkWidth,
+                            NSWidth([draggedButton frame]));
+    draggedButtonNodeId = [draggedButton bookmarkNode]->id();
+    if (layout_.button_offsets.find(draggedButtonNodeId) !=
+        layout_.button_offsets.end()) {
+      draggedButtonIsOnBar = YES;
+      draggedButtonOffset = layout_.button_offsets[draggedButtonNodeId];
     }
-    // Put all the buttons where they belong, with all buttons to the right
-    // of the insertion point shuffling right to make space for it.
-    [NSAnimationContext beginGrouping];
-    [[NSAnimationContext currentContext]
-        setDuration:kDragAndDropAnimationDuration];
-    for (NSButton* button in buttons_.get()) {
-      // Hidden buttons get no space.
-      if ([button isHidden])
+  }
+  paddingWidth += bookmarks::kBookmarkHorizontalPadding;
+  BOOL isRTL = cocoa_l10n_util::ShouldDoExperimentalRTLLayout();
+
+  /* If the button being dragged is not currently on the bar
+     (for example, a drag from the URL bar, a link on the desktop,

[Elly Fong-Jones, 2017/03/23 15:40:21]

We use the style with a leading // on every line. At least for me, since I don't
use syntax highlighting, I also find the leading-// style easier to read :)

[lgrey, 2017/04/10 19:14:32]

Done.

+     a button inside a menu, etc.), buttons in front of the drag position
+     (to the right in LTR, to the left in RTL), should make room for it.
+     Otherwise:
+     If a given button was to the left of the dragged button's original
+     position, but is to the right of the drag position, or
+     if it was to the right of the dragged button's original position and
+     is to the left of the drag position, make room. */

[Avi (use Gerrit), 2017/03/23 15:44:53]

Don't use /* comments. Use // ones for a block.

[lgrey, 2017/04/10 19:14:32]

Done.

+  [NSAnimationContext beginGrouping];
+  [[NSAnimationContext currentContext]
+      setDuration:kDragAndDropAnimationDuration];
+  for (BookmarkButton* button in buttons_.get()) {
+    CGRect buttonFrame = [button frame];
+    int64_t nodeId = [button bookmarkNode]->id();
+    CGFloat offset = layout_.button_offsets[nodeId];
+    CGFloat buttonEdge = isRTL ? NSWidth([buttonView_ frame]) - offset : offset;
+
+    if (draggedButtonIsOnBar) {
+      if (nodeId == draggedButtonNodeId)
         continue;
-      NSRect buttonFrame = [button frame];
-      buttonFrame.origin.x = left;
-      // Update "left" for next time around.
-      left += buttonFrame.size.width;
-      if (left > insertionPos_)
-        buttonFrame.origin.x += paddingWidth;
-      left += bookmarks::kBookmarkHorizontalPadding;
-      if (innerContentAnimationsEnabled_)
-        [[button animator] setFrame:buttonFrame];
-      else
-        [button setFrame:buttonFrame];
+      BOOL wasBefore = offset < draggedButtonOffset;
+      BOOL isBefore = isRTL ? buttonEdge > where : buttonEdge < where;
+      if (isBefore && !wasBefore) {
+        offset -= paddingWidth;
+        // slide button back
+      } else if (!isBefore && wasBefore) {
+        // slide button forward
+        offset += paddingWidth;
+      }
+    } else if ((isRTL && where > buttonEdge) ||
+               where < offset + NSWidth(buttonFrame)) {
+      offset += paddingWidth;
     }
-    [NSAnimationContext endGrouping];
+    [self applyXOffset:offset
+              toButton:button
+              animated:innerContentAnimationsEnabled_];
   }
+  [NSAnimationContext endGrouping];
 }
 
-// Put all visible bookmark bar buttons in their normal locations, either with
-// or without animation according to the |animate| flag.
-// This is generally useful, so is called from various places internally.
-- (void)resetAllButtonPositionsWithAnimation:(BOOL)animate {
+- (void)rebuildLayout:(BOOL)animated {

[Elly Fong-Jones, 2017/03/23 15:40:22]

okay, I really like this - it is much easier to follow and much more test-able
than what we had before.

+  if (!bookmarkModel_->loaded())
+    return;
+  BookmarkBarLayout layout = {};
+  layout.visible_elements = kVisibleElementsNone;
+  const BookmarkNode* node = bookmarkModel_->bookmark_bar_node();
+  CGFloat viewWidth = NSWidth([buttonView_ frame]);
+  CGFloat xOffset = bookmarks::kBookmarkLeftMargin;
+  CGFloat maxX = viewWidth - bookmarks::kBookmarkHorizontalPadding;
+  layout.max_x = maxX;
+  if (chrome::ShouldShowAppsShortcutInBookmarkBar(browser_->profile())) {
+    layout.visible_elements |= kVisibleElementsAppsButton;
+    layout.apps_button_offset = xOffset;
+    xOffset += NSWidth([appsPageShortcutButton_ frame]) +
+               bookmarks::kBookmarkHorizontalPadding;
+  }
+  if (!managedBookmarkService_->managed_node()->empty()) {
+    layout.visible_elements |= kVisibleElementsManagedBookmarksButton;
+    layout.managed_bookmarks_button_offset = xOffset;
+    xOffset += NSWidth([managedBookmarksButton_ frame]) +
+               bookmarks::kBookmarkHorizontalPadding;
+  }
+  if (!managedBookmarkService_->supervised_node()->empty()) {
+    layout.visible_elements |= kVisibleElementsSupervisedBookmarksButton;
+    layout.supervised_bookmarks_button_offset = xOffset;
+    xOffset += NSWidth([supervisedBookmarksButton_ frame]) +
+               bookmarks::kBookmarkHorizontalPadding;
+  }

[Avi (use Gerrit), 2017/03/23 15:44:53]

moar blank lines through this function

[lgrey, 2017/04/10 19:14:32]

Done.

+  if (node->empty()) {
+    if (maxX - xOffset >= kNoItemElementMinWidth) {
+      layout.visible_elements |= kVisibleElementsNoItemContainer;
+      layout.no_item_textfield_offset = xOffset;
+      layout.no_item_textfield_width =
+          std::min(maxX - xOffset, originalNoItemTextFieldWidth_);
+      xOffset +=
+          layout.no_item_textfield_width + originalNoItemInterelementPadding_;
+      // Does the "import bookmarks" button fit?
+      if (maxX - xOffset >= kNoItemElementMinWidth) {
+        layout.visible_elements |= kVisibleElementsImportBookmarksButton;
+        layout.import_bookmarks_button_offset = xOffset;
+        layout.import_bookmarks_button_width =
+            std::min(maxX - xOffset, originalImportBookmarksButtonWidth_);
+      }
+    }
+  } else {
+    if (!bookmarkModel_->other_node()->empty()) {
+      layout.visible_elements |= kVisibleElementsOtherBookmarksButton;
+      maxX -= NSWidth([otherBookmarksButton_ frame]);
+      layout.other_bookmarks_button_offset = maxX;
+    }
+    // Bookmark buttons and chevron
+    CGFloat offTheSideButtonWidth = NSWidth([offTheSideButton_ frame]);
+    int buttonCount = node->child_count();
+    for (int i = 0; i < buttonCount; i++) {
+      const BookmarkNode* buttonNode = node->GetChild(i);
+      CGFloat widthOfButton = [self widthOfButtonForNode:buttonNode];
+      // If it's the last button, we just need to ensure that it can fit.
+      // If not, we need to be able to fit both it and the chevron.
+      CGFloat widthToCheck = i == buttonCount - 1
+                                 ? widthOfButton
+                                 : widthOfButton + offTheSideButtonWidth;
+      if (xOffset + widthToCheck > maxX) {
+        layout.visible_elements |= kVisibleElementsOffTheSideButton;
+        layout.off_the_side_button_offset = maxX - offTheSideButtonWidth;
+        break;
+      }
+      layout.button_offsets.insert({buttonNode->id(), xOffset});
+      xOffset += widthOfButton + bookmarks::kBookmarkHorizontalPadding;
+    }
+  }
 
-  // Position the apps bookmark if needed.
-  CGFloat left = bookmarks::kBookmarkLeftMargin;
-  if (![appsPageShortcutButton_ isHidden]) {
-    int xOffset = bookmarks::kBookmarkLeftMargin -
-            bookmarks::kBookmarkHorizontalPadding;
-    NSRect frame =
-        [self frameForBookmarkButtonFromCell:[appsPageShortcutButton_ cell]
-                                     xOffset:&xOffset];
-    [appsPageShortcutButton_ setFrame:frame];
-    left = xOffset + bookmarks::kBookmarkHorizontalPadding;
+  if (layout_ != layout) {
+    layout_ = layout;
+    [self applyLayout:layout animated:animated];
+  }
+}
+
+- (void)applyLayout:(BookmarkBarLayout)layout animated:(BOOL)animated {
+  if (!bookmarkModel_->loaded())
+    return;
+  [self updateSpecialButton:appsPageShortcutButton_
+                withXOffset:layout.apps_button_offset
+                     hidden:!layout.IsAppsButtonVisible()];
+  [self updateSpecialButton:supervisedBookmarksButton_
+                withXOffset:layout.supervised_bookmarks_button_offset
+                     hidden:!layout.IsSupervisedBookmarksButtonVisible()];
+  [self updateSpecialButton:managedBookmarksButton_
+                withXOffset:layout.managed_bookmarks_button_offset
+                     hidden:!layout.IsManagedBookmarksButtonVisible()];
+  [self updateSpecialButton:offTheSideButton_
+                withXOffset:layout.off_the_side_button_offset
+                     hidden:!layout.IsOffTheSideButtonVisible()];
+  [self updateSpecialButton:otherBookmarksButton_
+                withXOffset:layout.other_bookmarks_button_offset
+                     hidden:!layout.IsOtherBookmarksButtonVisible()];
+  if (layout.IsOffTheSideButtonVisible())
+    [[offTheSideButton_ cell]
+        setStartingChildIndex:layout_.VisibleButtonCount()];
+  // 1) Hide all buttons initially

[Elly Fong-Jones, 2017/03/23 15:40:21]

It might be nice to actually break this function into three functions, each of
which does one of these steps, because then instead of this comment, the code
would say:

[self hideAllButtons];
[self showAllButtonsInLayout:layout];
[self removeAllHiddenButtons];

which I think would be quite nice.

+  // 2) Show all buttons in the layout
+  // 3) Remove hidden buttons from the node ID -> button map and
+  //    add them to the reuse pool.
+  for (BookmarkButton* button in buttons_.get()) {
+    [button setHidden:YES];
   }
+  [buttons_ removeAllObjects];
 
-  // Position the managed bookmarks folder if needed.
-  if (![managedBookmarksButton_ isHidden]) {
-    int xOffset = left;
-    NSRect frame =
-        [self frameForBookmarkButtonFromCell:[managedBookmarksButton_ cell]
-                                     xOffset:&xOffset];
-    [managedBookmarksButton_ setFrame:frame];
-    left = xOffset + bookmarks::kBookmarkHorizontalPadding;
+  // TODO(lgrey): View rename
+  [[buttonView_ noItemTextField] setHidden:!layout.IsNoItemContainerVisible()];
+  [[buttonView_ importBookmarksButton]
+      setHidden:!layout.IsImportBookmarksButtonVisible()];
+  if (layout.IsNoItemContainerVisible()) {
+    NSTextField* noItemTextField = [buttonView_ noItemTextField];
+    [noItemTextField setHidden:NO];
+    NSRect textFieldFrame = [noItemTextField frame];
+    textFieldFrame.size.width = layout.no_item_textfield_width;
+    if (cocoa_l10n_util::ShouldDoExperimentalRTLLayout()) {
+      textFieldFrame.origin.x = NSWidth([buttonView_ frame]) -
+                                layout.no_item_textfield_offset -
+                                layout.no_item_textfield_width;
+    } else {
+      textFieldFrame.origin.x = layout.no_item_textfield_offset;
+    }
+    [noItemTextField setFrame:textFieldFrame];
+    if (layout.IsImportBookmarksButtonVisible()) {
+      NSButton* importBookmarksButton = [buttonView_ importBookmarksButton];
+      NSRect buttonFrame = [importBookmarksButton frame];
+      buttonFrame.size.width = layout.import_bookmarks_button_width;
+      if (cocoa_l10n_util::ShouldDoExperimentalRTLLayout()) {
+        buttonFrame.origin.x = NSWidth([buttonView_ frame]) -
+                               layout.import_bookmarks_button_offset -
+                               layout.import_bookmarks_button_width;
+      } else {
+        buttonFrame.origin.x = layout.import_bookmarks_button_offset;
+      }
+      [importBookmarksButton setFrame:buttonFrame];
+    }
+  }
+  const BookmarkNode* parentNode = bookmarkModel_->bookmark_bar_node();
+  for (int i = 0; i < layout.VisibleButtonCount(); i++) {
+    const BookmarkNode* node = parentNode->GetChild(i);
+    DCHECK(node);
+    BookmarkButton* button = [self buttonForNode:node];
+    CGFloat offset = layout.button_offsets.at(node->id());
+    [self applyXOffset:offset
+              toButton:button
+              animated:animated && innerContentAnimationsEnabled_];
+    [buttons_ addObject:button];
+    [button setHidden:NO];
   }
+  for (auto& item : nodeIdToButtonMap_) {
+    if ([item.second isHidden]) {
+      if ([unusedButtonPool_ count] < kMaxReusePoolSize) {
+        [unusedButtonPool_ addObject:item.second.get()];
+      } else {
+        [item.second removeFromSuperview];
+      }
 
-  // Position the supervised bookmarks folder if needed.
-  if (![supervisedBookmarksButton_ isHidden]) {
-    int xOffset = left;
-    NSRect frame =
-        [self frameForBookmarkButtonFromCell:[supervisedBookmarksButton_ cell]
-                                     xOffset:&xOffset];
-    [supervisedBookmarksButton_ setFrame:frame];
-    left = xOffset + bookmarks::kBookmarkHorizontalPadding;
+      nodeIdToButtonMap_.erase(item.first);
+    }
   }
+  const ui::ThemeProvider* themeProvider = [[[self view] window] themeProvider];
+  [self updateTheme:themeProvider];
+}
 
-  animate &= innerContentAnimationsEnabled_;
+- (void)updateSpecialButton:(BookmarkButton*)button

[Elly Fong-Jones, 2017/03/23 15:40:21]

"update" is a bit generic - could this function name be more specific about what
it's doing? repositionSpecialButton:toXOffset:hidden: perhaps?

[lgrey, 2017/04/10 19:14:32]

Decided to just inline this without the conditional, since not applying an
offset when it's hidden isn't much of an optimization and probably isn't worth
the indirection cost.

+                withXOffset:(CGFloat)offset
+                     hidden:(bool)hidden {
+  if (hidden) {
+    [button setHidden:YES];
+  } else {
+    [button setHidden:NO];
+    [self applyXOffset:offset toButton:button animated:NO];
+  }
+}
 
-  for (NSButton* button in buttons_.get()) {
-    // Hidden buttons get no space.
-    if ([button isHidden])
-      continue;
-    NSRect buttonFrame = [button frame];
-    buttonFrame.origin.x = left;
-    left += buttonFrame.size.width + bookmarks::kBookmarkHorizontalPadding;
-    if (animate)
-      [[button animator] setFrame:buttonFrame];
-    else
-      [button setFrame:buttonFrame];
+- (void)applyXOffset:(CGFloat)offset

[Elly Fong-Jones, 2017/03/23 15:40:22]

the previous function has the button before the offset, so this one probably
should as well... perhaps this could be part of updateSpecialButton: itself?

[lgrey, 2017/04/10 19:14:32]

It's also used for the vanilla bookmark buttons.

+            toButton:(BookmarkButton*)button
+            animated:(BOOL)animated {
+  CGRect frame = [button frame];
+  if (cocoa_l10n_util::ShouldDoExperimentalRTLLayout()) {
+    frame.origin.x = NSWidth([buttonView_ frame]) - offset - NSWidth(frame);
+  } else {
+    frame.origin.x = offset;
+  }
+  //}
+  if (animated) {
+    [[button animator] setFrame:frame];
+  } else {
+    [button setFrame:frame];
   }
 }
 
+- (CGFloat)widthOfButtonForNode:(const BookmarkNode*)node {
+  // TODO(lgrey): Can we get this information without an actual image?
+  NSImage* image = [self faviconForNode:node forADarkTheme:NO];
+  CGFloat width = [BookmarkButtonCell cellWidthForNode:node image:image];
+  return std::min(width, bookmarks::kDefaultBookmarkWidth);
+}
+
 // Clear insertion flag, remove insertion space and put all visible bookmark
 // bar buttons in their normal locations.
 // Gets called only by our view.
+// TODO(lgrey): Is there a sane way to dedupe this with |setDropInsertionPos:|?
 - (void)clearDropInsertionPos {
-  if (hasInsertionPos_) {
-    hasInsertionPos_ = NO;
-    [self resetAllButtonPositionsWithAnimation:YES];
+  if (!hasInsertionPos_) {
+    return;
+  }
+  hasInsertionPos_ = NO;
+  BookmarkButton* draggedButton = [BookmarkButton draggedButton];
+  if (!draggedButton) {
+    [self rebuildLayout:YES];
+    return;
   }
+  int64_t draggedButtonNodeId = [draggedButton bookmarkNode]->id();
+  if (layout_.button_offsets.find(draggedButtonNodeId) ==
+      layout_.button_offsets.end()) {
+    [self rebuildLayout:YES];
+    return;
+  }
+  // The dragged button came from the bar, but is being dragged outside
+  // of it now, so the rest of the buttons should be laid out as if it
+  // were removed.
+  CGFloat draggedButtonOffset = layout_.button_offsets[draggedButtonNodeId];
+  CGFloat spaceForDraggedButton =
+      NSWidth([draggedButton frame]) + bookmarks::kBookmarkHorizontalPadding;
+  [NSAnimationContext beginGrouping];
+  [[NSAnimationContext currentContext]
+      setDuration:kDragAndDropAnimationDuration];
+
+  for (BookmarkButton* button in buttons_.get()) {
+    int64_t nodeId = [button bookmarkNode]->id();
+    CGFloat offset = layout_.button_offsets[nodeId];
+    if (nodeId == draggedButtonNodeId)
+      continue;
+    if (offset > draggedButtonOffset) {
+      offset -= spaceForDraggedButton;
+      [self applyXOffset:offset
+                toButton:button
+                animated:innerContentAnimationsEnabled_];
+    }
+  }
+  [NSAnimationContext endGrouping];
 }
 
 #pragma mark Bridge Notification Handlers
 
-// TODO(jrg): for now this is brute force.
 - (void)loaded:(BookmarkModel*)model {
   DCHECK(model == bookmarkModel_);
   if (!model->loaded())
     return;
+  [self createSupervisedBookmarksButton];
+  [self createManagedBookmarksButton];
+  [self createOtherBookmarksButton];
+  [[offTheSideButton_ cell]
+      setBookmarkNode:bookmarkModel_->bookmark_bar_node()];
 
   // If this is a rebuild request while we have a folder open, close it.
-  // TODO(mrossetti): Eliminate the need for this because it causes the folder
-  // menu to disappear after a cut/copy/paste/delete change.
-  // See: http://crbug.com/36614
   if (folderController_)
     [self closeAllBookmarkFolders];
-
-  // Brute force nuke and build.
-  savedFrameWidth_ = NSWidth([[self view] frame]);
-  const BookmarkNode* node = model->bookmark_bar_node();
-  [self clearBookmarkBar];
-  [self createAppsPageShortcutButton];
-  [self createManagedBookmarksButton];
-  [self createSupervisedBookmarksButton];
-  [self addNodesToButtonList:node];
-  [self createOtherBookmarksButton];
-  [self updateTheme:[[[self view] window] themeProvider]];
-  [self positionRightSideButtons];
-  [self addButtonsToView];
-  [self configureOffTheSideButtonContentsAndVisibility];
-  [self reconfigureBookmarkBar];
+  [self rebuildLayout:NO];
 }
 
 - (void)beingDeleted:(BookmarkModel*)model {
-  // The browser may be being torn down; little is safe to do.  As an
-  // example, it may not be safe to clear the pasteboard.
-  // http://crbug.com/38665
+}
+
+- (void)updateExtraButtonsVisibility {

[Elly Fong-Jones, 2017/03/23 15:40:21]

the code seems to alternate between "special" and "extra" as the word for these
buttons

[lgrey, 2017/04/10 19:14:32]

Stuck with extra since that terminology is used outside of Cocoa, though I'm
personally not a huge fan.

+  [self rebuildLayout:NO];
 }
 
 - (void)nodeAdded:(BookmarkModel*)model
@@ -2323,11 +2111,7 @@ static BOOL ValueInRangeInclusive(CGFloat low, CGFloat value, CGFloat high) {
   id<BookmarkButtonControllerProtocol> newController =
       [self controllerForNode:newParent];
   [newController addButtonForNode:newNode atIndex:newIndex];
-  // If we go from 0 --> 1 bookmarks we may need to hide the
-  // "bookmarks go here" text container.
-  [self showOrHideNoItemContainerForNode:model->bookmark_bar_node()];
-  // Cope with chevron or "Other Bookmarks" buttons possibly changing state.
-  [self reconfigureBookmarkBar];
+  [self rebuildLayout:NO];
 }
 
 // TODO(jrg): for now this is brute force.
@@ -2350,11 +2134,7 @@ static BOOL ValueInRangeInclusive(CGFloat low, CGFloat value, CGFloat high) {
     [oldController removeButton:oldIndex animate:NO];
     [newController addButtonForNode:movedNode atIndex:newIndex];
   }
-  // If the bar is one of the parents we may need to update the visibility
-  // of the "bookmarks go here" presentation.
-  [self showOrHideNoItemContainerForNode:model->bookmark_bar_node()];
-  // Cope with chevron or "Other Bookmarks" buttons possibly changing state.
-  [self reconfigureBookmarkBar];
+  [self rebuildLayout:NO];
 }
 
 - (void)nodeRemoved:(BookmarkModel*)model
@@ -2367,17 +2147,9 @@ static BOOL ValueInRangeInclusive(CGFloat low, CGFloat value, CGFloat high) {
   id<BookmarkButtonControllerProtocol> parentController =
       [self controllerForNode:oldParent];
   [parentController removeButton:index animate:YES];
-  // If we go from 1 --> 0 bookmarks we may need to show the
-  // "bookmarks go here" text container.
-  [self showOrHideNoItemContainerForNode:model->bookmark_bar_node()];
-  // If we deleted the only item on the "off the side" menu we no
-  // longer need to show it.
-  [self reconfigureBookmarkBar];
+  [self rebuildLayout:NO];
 }
 
-// TODO(jrg): linear searching is bad.
-// Need a BookmarkNode-->NSCell mapping.
-//
 // TODO(jrg): if the bookmark bar is open on launch, we see the
 // buttons all placed, then "scooted over" as the favicons load.  If
 // this looks bad I may need to change widthForBookmarkButtonCell to
@@ -2385,21 +2157,13 @@ static BOOL ValueInRangeInclusive(CGFloat low, CGFloat value, CGFloat high) {
 // favicons will eventually load.
 - (void)nodeFaviconLoaded:(BookmarkModel*)model
                      node:(const BookmarkNode*)node {
-  for (BookmarkButton* button in buttons_.get()) {
-    const BookmarkNode* cellnode = [button bookmarkNode];
-    if (cellnode == node) {
-      BOOL darkTheme = [[[self view] window] hasDarkTheme];
-      NSImage* theImage = [self faviconForNode:node forADarkTheme:darkTheme];
-      [[button cell] setBookmarkCellText:[button title]
-                                   image:theImage];
-      // Adding an image means we might need more room for the
-      // bookmark.  Test for it by growing the button (if needed)
-      // and shifting everything else over.
-      [self checkForBookmarkButtonGrowth:button];
-      return;
-    }
+  if (nodeIdToButtonMap_.find(node->id()) != nodeIdToButtonMap_.end()) {
+    BookmarkButton* button = nodeIdToButtonMap_[node->id()];

[Avi (use Gerrit), 2017/03/23 15:44:53]

do the find outside the if, then you can reuse the iterator here.

[lgrey, 2017/04/10 19:14:31]

Done.

+    BOOL darkTheme = [[[self view] window] hasDarkTheme];
+    NSImage* theImage = [self faviconForNode:node forADarkTheme:darkTheme];
+    [[button cell] setBookmarkCellText:[button title] image:theImage];
   }
-
+  [self rebuildLayout:NO];
   if (folderController_)
     [folderController_ faviconLoadedForNode:node];
 }
@@ -2543,8 +2307,7 @@ static BOOL ValueInRangeInclusive(CGFloat low, CGFloat value, CGFloat high) {
 
 - (void)bookmarkDragDidEnd:(BookmarkButton*)button
                  operation:(NSDragOperation)operation {
-  [button setHidden:NO];
-  [self resetAllButtonPositionsWithAnimation:YES];
+  [self rebuildLayout:YES];
 }
 
 
@@ -2704,50 +2467,50 @@ static BOOL ValueInRangeInclusive(CGFloat low, CGFloat value, CGFloat high) {
   CGFloat x = 0;
   CGFloat halfHorizontalPadding = 0.5 * bookmarks::kBookmarkHorizontalPadding;
   int destIndex = [self indexForDragToPoint:point];
-  int numButtons = displayedButtonCount_;
-
-  CGFloat leftmostX;
-  if (![supervisedBookmarksButton_ isHidden]) {
-    leftmostX =
-        NSMaxX([supervisedBookmarksButton_ frame]) + halfHorizontalPadding;
-  } else if (![managedBookmarksButton_ isHidden]) {
-    leftmostX = NSMaxX([managedBookmarksButton_ frame]) + halfHorizontalPadding;
-  } else if (![appsPageShortcutButton_ isHidden]) {
-    leftmostX = NSMaxX([appsPageShortcutButton_ frame]) + halfHorizontalPadding;
+  int numButtons = layout_.VisibleButtonCount();
+
+  CGFloat leadingOffset;
+  if (layout_.IsSupervisedBookmarksButtonVisible()) {
+    leadingOffset =
+        layout_.supervised_bookmarks_button_offset + halfHorizontalPadding;
+  } else if (layout_.IsManagedBookmarksButtonVisible()) {
+    leadingOffset =
+        layout_.managed_bookmarks_button_offset + halfHorizontalPadding;
+  } else if (layout_.IsAppsButtonVisible()) {
+    leadingOffset = layout_.apps_button_offset + halfHorizontalPadding;
   } else {
-    leftmostX = bookmarks::kBookmarkLeftMargin - halfHorizontalPadding;
+    leadingOffset = bookmarks::kBookmarkLeftMargin - halfHorizontalPadding;
   }
 
   // If it's a drop strictly between existing buttons ...
   if (destIndex == 0) {
-    x = leftmostX;
+    x = leadingOffset;
   } else if (destIndex > 0 && destIndex < numButtons) {
     // ... put the indicator right between the buttons.
-    BookmarkButton* button =
-        [buttons_ objectAtIndex:static_cast<NSUInteger>(destIndex-1)];
-    DCHECK(button);
-    NSRect buttonFrame = [button frame];
-    x = NSMaxX(buttonFrame) + halfHorizontalPadding;
-
+    int64_t nodeId =
+        bookmarkModel_->bookmark_bar_node()->GetChild(destIndex)->id();
+    x = layout_.button_offsets[nodeId];
     // If it's a drop at the end (past the last button, if there are any) ...
   } else if (destIndex == numButtons) {
     // and if it's past the last button ...
     if (numButtons > 0) {
-      // ... find the last button, and put the indicator to its right.
-      BookmarkButton* button =
-          [buttons_ objectAtIndex:static_cast<NSUInteger>(destIndex - 1)];
-      DCHECK(button);
-      x = NSMaxX([button frame]) + halfHorizontalPadding;
-
+      // ... find the last button, and put the indicator after it.
+      const BookmarkNode* node =
+          bookmarkModel_->bookmark_bar_node()->GetChild(destIndex - 1);
+      int64_t nodeId = node->id();
+      x = layout_.button_offsets[nodeId] + [self widthOfButtonForNode:node] +
+          halfHorizontalPadding;
       // Otherwise, put it right at the beginning.
     } else {
-      x = leftmostX;
+      x = leadingOffset;
     }
   } else {
     NOTREACHED();
   }
 
-  return x;
+  return cocoa_l10n_util::ShouldDoExperimentalRTLLayout()
+             ? NSWidth([buttonView_ frame]) - x
+             : x;
 }
 
 - (void)childFolderWillShow:(id<BookmarkButtonControllerProtocol>)child {
@@ -2807,34 +2570,6 @@ static BOOL ValueInRangeInclusive(CGFloat low, CGFloat value, CGFloat high) {
                   browser_->profile());
 }
 
-- (void)addButtonForNode:(const BookmarkNode*)node
-                 atIndex:(NSInteger)buttonIndex {
-  int newOffset =
-      bookmarks::kBookmarkLeftMargin - bookmarks::kBookmarkHorizontalPadding;
-  if (buttonIndex == -1)
-    buttonIndex = [buttons_ count];  // New button goes at the end.
-  if (buttonIndex <= (NSInteger)[buttons_ count]) {
-    if (buttonIndex) {
-      BookmarkButton* targetButton = [buttons_ objectAtIndex:buttonIndex - 1];
-      NSRect targetFrame = [targetButton frame];
-      newOffset = targetFrame.origin.x + NSWidth(targetFrame) +
-          bookmarks::kBookmarkHorizontalPadding;
-    }
-    BookmarkButton* newButton = [self buttonForNode:node xOffset:&newOffset];
-    ++displayedButtonCount_;
-    [buttons_ insertObject:newButton atIndex:buttonIndex];
-    [buttonView_ addSubview:newButton];
-    [self resetAllButtonPositionsWithAnimation:NO];
-    // See if any buttons need to be pushed off to or brought in from the side.
-    [self reconfigureBookmarkBar];
-  } else  {
-    // A button from somewhere else (not the bar) is being moved to the
-    // off-the-side so insure it gets redrawn if its showing.
-    [self reconfigureBookmarkBar];
-    [folderController_ reconfigureMenu];
-  }
-}
-
 // TODO(mrossetti): Duplicate code with BookmarkBarFolderController.
 // http://crbug.com/35966
 - (BOOL)addURLs:(NSArray*)urls withTitles:(NSArray*)titles at:(NSPoint)point {
@@ -2882,49 +2617,22 @@ static BOOL ValueInRangeInclusive(CGFloat low, CGFloat value, CGFloat high) {
 }
 
 - (void)moveButtonFromIndex:(NSInteger)fromIndex toIndex:(NSInteger)toIndex {
-  if (fromIndex != toIndex) {
-    NSInteger buttonCount = (NSInteger)[buttons_ count];
-    if (toIndex == -1)
-      toIndex = buttonCount;
-    // See if we have a simple move within the bar, which will be the case if
-    // both button indexes are in the visible space.
-    if (fromIndex < buttonCount && toIndex < buttonCount) {
-      BookmarkButton* movedButton = [buttons_ objectAtIndex:fromIndex];
-      [buttons_ removeObjectAtIndex:fromIndex];
-      [buttons_ insertObject:movedButton atIndex:toIndex];
-      [movedButton setHidden:NO];
-      [self resetAllButtonPositionsWithAnimation:NO];
-    } else if (fromIndex < buttonCount) {
-      // A button is being removed from the bar and added to off-the-side.
-      // By now the node has already been inserted into the model so the
-      // button to be added is represented by |toIndex|. Things get
-      // complicated because the off-the-side is showing and must be redrawn
-      // while possibly re-laying out the bookmark bar.
-      [self removeButton:fromIndex animate:NO];
-      [self reconfigureBookmarkBar];
-      [folderController_ reconfigureMenu];
-    } else if (toIndex < buttonCount) {
-      // A button is being added to the bar and removed from off-the-side.
-      // By now the node has already been inserted into the model so the
-      // button to be added is represented by |toIndex|.
-      const BookmarkNode* node = bookmarkModel_->bookmark_bar_node();
-      const BookmarkNode* movedNode = node->GetChild(toIndex);
-      DCHECK(movedNode);
-      [self addButtonForNode:movedNode atIndex:toIndex];
-      [self reconfigureBookmarkBar];
-    } else {
-      // A button is being moved within the off-the-side.
-      fromIndex -= buttonCount;
-      toIndex -= buttonCount;
-      [folderController_ moveButtonFromIndex:fromIndex toIndex:toIndex];
-    }
+  int buttonCount = layout_.VisibleButtonCount();
+  BOOL isMoveWithinOffTheSideMenu =

[Elly Fong-Jones, 2017/03/23 15:40:22]

I like that you have broken this out into a semantically-named boolean - it
makes the resulting if statement less cluttered & easier to read.

+      (toIndex >= buttonCount) && (fromIndex >= buttonCount);
+  if (isMoveWithinOffTheSideMenu) {
+    fromIndex -= buttonCount;
+    toIndex -= buttonCount;
+    [folderController_ moveButtonFromIndex:fromIndex toIndex:toIndex];
+  } else {
+    [self rebuildLayout:NO];
   }
 }
 
 - (void)removeButton:(NSInteger)buttonIndex animate:(BOOL)animate {
-  if (buttonIndex < (NSInteger)[buttons_ count]) {
+  if (buttonIndex < layout_.VisibleButtonCount()) {
     // The button being removed is showing in the bar.
-    BookmarkButton* oldButton = [buttons_ objectAtIndex:buttonIndex];
+    BookmarkButton* oldButton = buttons_[buttonIndex];
     if (oldButton == [folderController_ parentButton]) {
       // If we are deleting a button whose folder is currently open, close it!
       [self closeAllBookmarkFolders];
@@ -2935,17 +2643,12 @@ static BOOL ValueInRangeInclusive(CGFloat low, CGFloat value, CGFloat high) {
       NSShowAnimationEffect(NSAnimationEffectDisappearingItemDefault, poofPoint,
                             NSZeroSize, nil, nil, nil);
     }
-    [oldButton setDelegate:nil];
-    [oldButton removeFromSuperview];
-    [buttons_ removeObjectAtIndex:buttonIndex];
-    --displayedButtonCount_;
-    [self resetAllButtonPositionsWithAnimation:YES];
-    [self reconfigureBookmarkBar];
+    [self rebuildLayout:YES];
   } else if (folderController_ &&
              [folderController_ parentButton] == offTheSideButton_) {
     // The button being removed is in the OTS (off-the-side) and the OTS
     // menu is showing so we need to remove the button.
-    NSInteger index = buttonIndex - displayedButtonCount_;
+    NSInteger index = buttonIndex - layout_.VisibleButtonCount();
     [folderController_ removeButton:index animate:animate];
   }
 }