Revert of [Mac] Refactor bookmark bar controller (patchset #10 id:180001 of...

Revert of [Mac] Refactor bookmark bar controller (patchset #10 id:180001 of https://codereview.chromium.org/2751573002/ )

Reason for revert:
Reverting as likely caused msan failurs. See https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Fchromium.memory%2FMac_ASan_64_Tests__1_%2F29660%2F%2B%2Frecipes%2Fsteps%2Fbrowser_tests%2F0%2Flogs%2FBookmarkFolderAppleScriptTest.DeleteBookmarkItems%2F0 for example:

==77706==ERROR: AddressSanitizer: heap-use-after-free on address 0x603000445b40 at pc 0x0001156ab4fe bp 0x7fff5c399fd0 sp 0x7fff5c399fc8
READ of size 8 at 0x603000445b40 thread T0
    #0 0x1156ab4fd in -[BookmarkBarController applyLayout:animated:] ??:0:0
    #1 0x1156a9db1 in -[BookmarkBarController rebuildLayoutWithAnimated:] ??:0:0
    #2 0x1156acf25 in -[BookmarkBarController nodeRemoved:parent:index:] ??:0:0
    #3 0x1127774c7 in bookmarks::BookmarkModel::RemoveAndDeleteNode(bookmarks::BookmarkNode*) ??:0:0
    #4 0x112776d40 in bookmarks::BookmarkModel::Remove(bookmarks::BookmarkNode const*) ??:0:0
    #5 0x10607a64c in (anonymous namespace)::BookmarkFolderAppleScriptTest_DeleteBookmarkItems_Test::RunTestOnMainThread() ??:0:0
    #6 0x10d1f6ac3 in content::BrowserTestBase::ProxyRunTestOnMainThreadLoop() ??:0:0
    #7 0x10bb87dc3 in ChromeBrowserMainParts::PreMainMessageLoopRunImpl() ??:0:0
    #8 0x10bb851e6 in ChromeBrowserMainParts::PreMainMessageLoopRun() ??:0:0
    #9 0x1077b02dd in content::BrowserMainLoop::PreMainMessageLoopRun() ??:0:0
    #10 0x1084ba8a3 in content::StartupTaskRunner::RunAllTasksNow() ??:0:0
    #11 0x1077abc3a in content::BrowserMainLoop::CreateStartupTasks() ??:0:0
    #12 0x1077b94ec in content::BrowserMainRunnerImpl::Initialize(content::MainFunctionParams const&) ??:0:0
    #13 0x1077a40b5 in content::BrowserMain(content::MainFunctionParams const&) ??:0:0
    #14 0x10b6a9604 in content::RunNamedProcessTypeMain(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, content::MainFunctionParams const&, content::ContentMainDelegate*) ??:0:0
    #15 0x10b6ab3c7 in content::ContentMainRunnerImpl::Run() ??:0:0
    #16 0x111afc88b in service_manager::Main(service_manager::MainParams const&) ??:0:0
    #17 0x10b6a8ff4 in content::ContentMain(content::ContentMainParams const&) ??:0:0
    #18 0x10d1f5c4f in content::BrowserTestBase::SetUp() ??:0:0
    #19 0x10ba09201 in InProcessBrowserTest::SetUp() ??:0:0
    #20 0x10ed76b2f in testing::Test::Run() ??:0:0
    #21 0x10ed78b43 in testing::TestInfo::Run() ??:0:0
    #22 0x10ed79e86 in testing::TestCase::Run() ??:0:0
    #23 0x10ed8cfb6 in testing::internal::UnitTestImpl::RunAllTests() ??:0:0
    #24 0x10ed8c588 in testing::UnitTest::Run() ??:0:0
    #25 0x10ba51d3e in base::TestSuite::Run() ??:0:0
    #26 0x10b6fc3ac in ChromeTestSuiteRunner::RunTestSuite(int, char**) ??:0:0
    #27 0x10d2c7991 in content::LaunchTests(content::TestLauncherDelegate*, int, int, char**) ??:0:0
    #28 0x10b6fc203 in main ??:0:0
    #29 0x7fff8a0125fc in start ??:0:0

Original issue's description:
> Yes, with RTL thrown in, since this is specifically designed to make it almost free.
>
> Two major things here:
> - The bar is no longer relaid-out directly in response to changes in view size, bookmark model etc. Instead, a new UI-direction-agnostic view model (BookmarkBarLayout) is created from the current state, and if it's different from before, it's applied to the view.
> - Removed a bunch of layout-related code that's no longer necessary
>
> BUG=648560
>
> Review-Url: https://codereview.chromium.org/2751573002
> Cr-Commit-Position: refs/heads/master@{#468364}
> Committed: https://chromium.googlesource.com/chromium/src/+/2644729cb7722a702a76cc2758d0ce372e1e6f92

TBR=ellyjones@chromium.org,avi@chromium.org,lgrey@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=648560

Review-Url: https://codereview.chromium.org/2853123002
Cr-Commit-Position: refs/heads/master@{#468398}

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

@@ -113,10 +113,9 @@ TEST_F(BookmarkBarBridgeTest, TestRedirect) {
   bridge->BookmarkNodeFaviconChanged(NULL, NULL);
   bridge->BookmarkNodeChildrenReordered(NULL, NULL);
   bridge->BookmarkNodeRemoved(NULL, NULL, 0, NULL, std::set<GURL>());
-  // 7 calls above plus two Loaded() in init routing makes 9.
-  EXPECT_EQ(controller.get()->called_selectors_.size(), 9U);
+  // 7 calls above plus an initial Loaded() in init routine makes 8.
+  EXPECT_EQ(controller.get()->called_selectors_.size(), 8U);
   std::vector<SEL> expected_selectors = {
-    @selector(loaded:),  // initial from init
     @selector(loaded:),  // initial from init
     @selector(loaded:),
     @selector(nodeMoved:oldParent:oldIndex:newParent:newIndex:),

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

@@ -10,7 +10,6 @@
 
 #include <map>
 #include <memory>
-#include <unordered_map>
 
 #import "base/mac/cocoa_protocols.h"
 #include "base/mac/scoped_nsobject.h"
@@ -134,67 +133,6 @@ const NSTimeInterval kDragHoverOpenDelay = 0.7;
 // no opportunity for overlap.
 const NSTimeInterval kDragHoverCloseDelay = 0.4;
 
-enum BookmarkBarVisibleElementsMask {
-  kVisibleElementsMaskNone = 0,
-  kVisibleElementsMaskAppsButton = 1 << 0,
-  kVisibleElementsMaskManagedBookmarksButton = 1 << 1,
-  kVisibleElementsMaskSupervisedBookmarksButton = 1 << 2,
-  kVisibleElementsMaskOffTheSideButton = 1 << 3,
-  kVisibleElementsMaskOtherBookmarksButton = 1 << 4,
-  kVisibleElementsMaskNoItemTextField = 1 << 5,
-  kVisibleElementsMaskImportBookmarksButton = 1 << 6,
-};
-
-// Specifies the location and visibility of the various sub-elements
-// of the bookmark bar. Allows calculating the layout and actually
-// applying it to views to be decoupled. For example, applying
-// the layout in an RTL context transforms all horizontal offsets
-// transparently.
-struct BookmarkBarLayout {
- public:
-  BookmarkBarLayout();
-  ~BookmarkBarLayout();
-  BookmarkBarLayout(BookmarkBarLayout&& other);
-  BookmarkBarLayout& operator=(BookmarkBarLayout&& other);
-
-  bool IsAppsButtonVisible() const {
-    return visible_elements & kVisibleElementsMaskAppsButton;
-  }
-  bool IsManagedBookmarksButtonVisible() const {
-    return visible_elements & kVisibleElementsMaskManagedBookmarksButton;
-  }
-  bool IsSupervisedBookmarksButtonVisible() const {
-    return visible_elements & kVisibleElementsMaskSupervisedBookmarksButton;
-  }
-  bool IsOffTheSideButtonVisible() const {
-    return visible_elements & kVisibleElementsMaskOffTheSideButton;
-  }
-  bool IsOtherBookmarksButtonVisible() const {
-    return visible_elements & kVisibleElementsMaskOtherBookmarksButton;
-  }
-  bool IsNoItemTextFieldVisible() const {
-    return visible_elements & kVisibleElementsMaskNoItemTextField;
-  }
-  bool IsImportBookmarksButtonVisible() const {
-    return visible_elements & kVisibleElementsMaskImportBookmarksButton;
-  }
-  size_t VisibleButtonCount() const { return button_offsets.size(); }
-
-  unsigned int visible_elements;
-  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;
-};
-
 }  // namespace bookmarks
 
 // The interface for the bookmark bar controller's delegate. Currently, the
@@ -288,6 +226,9 @@ willAnimateFromState:(BookmarkBar::State)oldState
       buttonView_;  // Contains 'no items' text fields.
   base::scoped_nsobject<BookmarkButton> offTheSideButton_;  // aka the chevron.
 
+  NSRect originalNoItemsRect_;  // Original, pre-resized field rect.
+  NSRect originalImportBookmarksRect_;  // Original, pre-resized field rect.
+
   // "Apps" button on the left side.
   base::scoped_nsobject<BookmarkButton> appsPageShortcutButton_;
 
@@ -312,6 +253,15 @@ willAnimateFromState:(BookmarkBar::State)oldState
   // initial build.
   CGFloat savedFrameWidth_;
 
+  // The number of buttons we display in the bookmark bar.  This does
+  // not include the "off the side" chevron or the "Other Bookmarks"
+  // button.  We use this number to determine if we need to display
+  // the chevron, and to know what to place in the chevron's menu.
+  // Since we create everything before doing layout we can't be sure
+  // that all bookmark buttons we create will be visible.  Thus,
+  // [buttons_ count] isn't a definitive check.
+  int displayedButtonCount_;
+
   // A state flag which tracks when the bar's folder menus should be shown.
   // An initial click in any of the folder buttons turns this on and
   // one of the following will turn it off: another click in the button,
@@ -477,29 +427,28 @@ willAnimateFromState:(BookmarkBar::State)oldState
 
 // These APIs should only be used by unit tests (or used internally).
 @interface BookmarkBarController(InternalOrTestingAPI)
-- (bookmarks::BookmarkBarLayout)layoutFromCurrentState;
-- (void)applyLayout:(const bookmarks::BookmarkBarLayout&)layout
-           animated:(BOOL)animated;
 - (void)openBookmarkFolder:(id)sender;
 - (void)openOrCloseBookmarkFolderForOffTheSideButton;
 - (BookmarkBarView*)buttonView;
 - (NSMutableArray*)buttons;
-- (BookmarkButton*)otherBookmarksButton;
-- (BookmarkButton*)managedBookmarksButton;
-- (BookmarkButton*)supervisedBookmarksButton;
+- (BOOL)offTheSideButtonIsHidden;
+- (BOOL)appsPageShortcutButtonIsHidden;
 - (BookmarkButton*)otherBookmarksButton;
 - (BookmarkBarFolderController*)folderController;
 - (id)folderTarget;
+- (int)displayedButtonCount;
 - (void)openURL:(GURL)url disposition:(WindowOpenDisposition)disposition;
 - (void)clearBookmarkBar;
 - (BookmarkButtonCell*)cellForBookmarkNode:(const bookmarks::BookmarkNode*)node;
 - (BookmarkButtonCell*)cellForCustomButtonWithText:(NSString*)text
                                              image:(NSImage*)image;
+- (NSRect)frameForBookmarkButtonFromCell:(NSCell*)cell xOffset:(int*)xOffset;
 - (void)checkForBookmarkButtonGrowth:(NSButton*)button;
 - (void)frameDidChange;
 - (void)updateTheme:(const ui::ThemeProvider*)themeProvider;
 - (BookmarkButton*)buttonForDroppingOnAtPoint:(NSPoint)point;
 - (BOOL)isEventAnExitEvent:(NSEvent*)event;
+- (BOOL)shrinkOrHideView:(NSView*)view forMaxX:(CGFloat)maxViewX;
 - (void)unhighlightBookmark:(const bookmarks::BookmarkNode*)node;
 
 @end

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

@@ -790,11 +790,8 @@ TEST_F(BookmarkBarFolderControllerMenuTest, DragMoveBarBookmarkToFolder) {
       "4f2f1b 4f2f2b 4f2f3b ] 4f3f:[ 4f3f1b 4f3f2b 4f3f3b ] ] 5b ");
   EXPECT_EQ(expected_string, bookmarks::test::ModelStringFromNode(root));
 
-  // Reopen the window.
-  [[toFolder target] performSelector:@selector(openBookmarkFolderFromButton:)
-                          withObject:toFolder];
+  // Verify the window still appears by looking for its controller.
   EXPECT_TRUE([bar_ folderController]);
-  folderController = [bar_ folderController];
 
   // Gather the new frames.
   NSRect newToFolderFrame = [toFolder frame];

chrome/browser/ui/cocoa/bookmarks/bookmark_bar_view_cocoa.h

@@ -11,10 +11,6 @@
 
 @class BookmarkBarController;
 
-namespace bookmarks {
-const CGFloat kInitialNoItemTextFieldXOrigin = 5;
-}
-
 // A simple custom NSView for the bookmark bar used to prevent clicking and
 // dragging from moving the browser window.
 @interface BookmarkBarView : NSView {
@@ -23,11 +19,13 @@ const CGFloat kInitialNoItemTextFieldXOrigin = 5;
   CGFloat dropIndicatorPosition_;  // x position
 
   BookmarkBarController* controller_;
-  base::scoped_nsobject<NSTextField> noItemTextField_;
+  base::scoped_nsobject<NSTextField> noItemTextfield_;
   base::scoped_nsobject<NSButton> importBookmarksButton_;
+  base::scoped_nsobject<NSView> noItemContainer_;
 }
-- (NSTextField*)noItemTextField;
+- (NSTextField*)noItemTextfield;
 - (NSButton*)importBookmarksButton;
+- (NSView*)noItemContainer;
 
 - (instancetype)initWithController:(BookmarkBarController*)controller
                              frame:(NSRect)frame;

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

@@ -28,10 +28,14 @@ using base::UserMetricsAction;
 using bookmarks::BookmarkModel;
 using bookmarks::BookmarkNode;
 
-static const CGFloat kInitialElementYOrigin = 7;
+static const CGFloat kInitialContainerWidth = 596;
+static const CGFloat kInitialContainerHeight = 41;
+static const CGFloat kInitialElementYOrigin = 20;
 static const CGFloat kInitialElementHeight = 14;
 static const CGFloat kInitialTextFieldXOrigin = 5;
+// static const CGFloat kInitialTextFieldWidth = 167;
 static const CGFloat kTextFieldTrailingPadding = 5;
+// static const CGFloat kInitialButtonWidth = 199;
 
 @interface BookmarkBarView (Private)
 - (void)themeDidChangeNotification:(NSNotification*)aNotification;
@@ -76,40 +80,53 @@ static const CGFloat kTextFieldTrailingPadding = 5;
 
     NSFont* smallSystemFont =
         [NSFont systemFontOfSize:[NSFont smallSystemFontSize]];
+    noItemContainer_.reset(
+        [[NSView alloc] initWithFrame:NSMakeRect(0, 0, kInitialContainerWidth,
+                                                 kInitialContainerHeight)]);
+    [noItemContainer_ setAutoresizingMask:NSViewMaxXMargin];
+    [noItemContainer_ setAutoresizingMask:NSViewWidthSizable];
 
-    noItemTextField_.reset([[NSTextField alloc]
+    noItemTextfield_.reset([[NSTextField alloc]
         initWithFrame:NSMakeRect(kInitialTextFieldXOrigin,
                                  kInitialElementYOrigin, CGFLOAT_MAX,
                                  kInitialElementHeight)]);
-    [noItemTextField_ setFont:smallSystemFont];
-    [noItemTextField_
+    [noItemTextfield_ setAutoresizingMask:NSViewWidthSizable];
+    [noItemTextfield_ setFont:smallSystemFont];
+    [noItemTextfield_
         setStringValue:l10n_util::GetNSString(IDS_BOOKMARKS_NO_ITEMS)];
-    [noItemTextField_ setEditable:NO];
+    [noItemTextfield_ setEditable:NO];
 
-    [noItemTextField_ setBordered:NO];
-    [[noItemTextField_ cell] setLineBreakMode:NSLineBreakByTruncatingTail];
+    [noItemTextfield_ setBordered:NO];
+    [[noItemTextfield_ cell] setLineBreakMode:NSLineBreakByTruncatingTail];
 
-    [noItemTextField_ setTextColor:[NSColor controlTextColor]];
-    [noItemTextField_ setBackgroundColor:[NSColor controlColor]];
+    [noItemTextfield_ setTextColor:[NSColor controlTextColor]];
+    [noItemTextfield_ setBackgroundColor:[NSColor controlColor]];
 
-    [noItemTextField_ setDrawsBackground:NO];
-    [noItemTextField_ setTextColor:[NSColor controlTextColor]];
-    [noItemTextField_ setBackgroundColor:[NSColor controlColor]];
-    [noItemTextField_ sizeToFit];
+    [noItemTextfield_ setDrawsBackground:NO];
+    [noItemTextfield_ setTextColor:[NSColor controlTextColor]];
+    [noItemTextfield_ setBackgroundColor:[NSColor controlColor]];
+    [noItemTextfield_ sizeToFit];
 
     NSButton* importButton = [HyperlinkButtonCell
         buttonWithString:l10n_util::GetNSString(IDS_BOOKMARK_BAR_IMPORT_LINK)];
     importBookmarksButton_.reset([importButton retain]);
     [importBookmarksButton_
-        setFrame:NSMakeRect(NSMaxX([noItemTextField_ frame]) +
+        setFrame:NSMakeRect(NSMaxX([noItemTextfield_ frame]) +
                                 kTextFieldTrailingPadding,
                             kInitialElementYOrigin, CGFLOAT_MAX,
                             kInitialElementHeight)];
+    [importBookmarksButton_ setAutoresizingMask:NSViewMaxXMargin];
     [importBookmarksButton_ setFont:smallSystemFont];
     [importBookmarksButton_ sizeToFit];
+    [noItemContainer_ addSubview:importBookmarksButton_];
 
-    [self addSubview:noItemTextField_];
-    [self addSubview:importBookmarksButton_];
+    [noItemContainer_ addSubview:noItemTextfield_];
+    NSRect containerFrame = [noItemContainer_ frame];
+    containerFrame.size.width = std::max(
+        NSWidth(containerFrame), NSMaxX([importBookmarksButton_ frame]));
+    [noItemContainer_ setFrame:containerFrame];
+
+    [self addSubview:noItemContainer_];
     [self registerForNotificationsAndDraggedTypes];
   }
   return self;
@@ -157,7 +174,7 @@ static const CGFloat kTextFieldTrailingPadding = 5;
 
   NSColor* color =
       themeProvider->GetNSColor(ThemeProperties::COLOR_BOOKMARK_TEXT);
-  [noItemTextField_ setTextColor:color];
+  [noItemTextfield_ setTextColor:color];
 }
 
 // Mouse down events on the bookmark bar should not allow dragging the parent
@@ -166,8 +183,12 @@ static const CGFloat kTextFieldTrailingPadding = 5;
   return NO;
 }
 
-- (NSTextField*)noItemTextField {
-  return noItemTextField_;
+- (NSTextField*)noItemTextfield {
+  return noItemTextfield_;
+}
+
+- (NSView*)noItemContainer {
+  return noItemContainer_;
 }
 
 - (NSButton*)importBookmarksButton {
@@ -236,6 +257,7 @@ static const CGFloat kTextFieldTrailingPadding = 5;
 - (void)draggingEnded:(id<NSDraggingInfo>)info {
   [controller_ draggingEnded:info];
 
+  [[BookmarkButton draggedButton] setHidden:NO];
   if (dropIndicatorShown_) {
     dropIndicatorShown_ = NO;
     [self dropIndicatorChanged];

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

@@ -81,6 +81,7 @@ const CGFloat kKernAmount = 0.2;
 - (NSDictionary*)titleTextAttributes;
 @end
 
+
 @implementation BookmarkButtonCell
 
 @synthesize startingChildIndex = startingChildIndex_;