Move bookmark's folder menu upwards only when its parent button is moved

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

Index: chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_controller.mm
diff --git a/chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_controller.mm b/chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_controller.mm
index 2eb0880c5f79ed46df5d53ad068412a2a6691471..b0f830ce51690b40fc682a70d19ffc31f44be743 100644
--- a/chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_controller.mm
+++ b/chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_controller.mm
@@ -350,15 +350,6 @@ NSRect GetFirstButtonFrameForHeight(CGFloat height) {
   return parentButton_.get();
 }
 
-- (void)offsetFolderMenuWindow:(NSSize)offset {
-  NSWindow* window = [self window];
-  NSRect windowFrame = [window frame];
-  windowFrame.origin.x -= offset.width;
-  windowFrame.origin.y += offset.height;  // Yes, in the opposite direction!
-  [window setFrame:windowFrame display:YES];
-  [folderController_ offsetFolderMenuWindow:offset];
-}
-
 - (void)reconfigureMenu {
   [NSObject cancelPreviousPerformRequestsWithTarget:self];
   for (BookmarkButton* button in buttons_.get()) {
@@ -1931,35 +1922,28 @@ static BOOL ValueInRangeInclusive(CGFloat low, CGFloat value, CGFloat high) {
     buttonFrame.origin.y -= bookmarks::kBookmarkFolderButtonHeight;
     [button setFrame:buttonFrame];
   }
-  // Search for and adjust submenus, if necessary.
+
   NSInteger buttonCount = [buttons_ count];
-  if (buttonCount) {
-    BookmarkButton* subButton = [folderController_ parentButton];
-    for (NSButton* aButton in buttons_.get()) {
-      // If this button is showing its menu then we need to move the menu, too.

[Alexei Svitkine (slow), 2014/09/26 15:14:37]

Are you sure we never need to do this?

What if you're moving folder2 to folder3 instead, such that folder3 is below and
thus needs to shift upward?

Is it possible to add some tests for all these cases?

[Gaja, 2014/09/27 05:56:33]

Oh, I did not test this case, I will check the behavior and update the same(on
monday :( ). Also I will add test cases if necessary. Thanks!

-      if (aButton == subButton)
-        [folderController_
-            offsetFolderMenuWindow:NSMakeSize(0.0, chrome::kBookmarkBarHeight)];

[Gaja, 2014/09/25 09:00:39]

@asvitkine,
This CL is to remove only the above 'if' block. Along with that I have
re-factored the below 'if else' code block to improve code readability. PTAL.

-    }
-  } else if (parentButton_ != [barController_ otherBookmarksButton]) {
-    // If all nodes have been removed from this folder then add in the
-    // 'empty' placeholder button except for "Other bookmarks" folder
-    // as we are going to hide it.
-    NSRect buttonFrame =
-        GetFirstButtonFrameForHeight([self menuHeightForButtonCount:1]);
-    BookmarkButton* button = [self makeButtonForNode:nil
+  if (buttonCount == 0) {
+    if (parentButton_ != [barController_ otherBookmarksButton]) {
+      // If all nodes have been removed from this folder then add in the
+      // 'empty' placeholder button except for "Other bookmarks" folder
+      // as we are going to hide it.
+      NSRect buttonFrame =
+          GetFirstButtonFrameForHeight([self menuHeightForButtonCount:1]);
+      BookmarkButton* button = [self makeButtonForNode:nil
                                                frame:buttonFrame];

[Alexei Svitkine (slow), 2014/09/26 15:14:37]

Nit: Align :'s or wrap after =.

[Gaja, 2014/09/27 05:56:33]

Acknowledged.

-    [buttons_ addObject:button];
-    [folderView_ addSubview:button];
-    buttonCount = 1;
-  }
-
-  // buttonCount will be 0 if "Other bookmarks" folder is empty, so close
-  // the folder before hiding it.
-  if (buttonCount == 0)
-    [barController_ closeBookmarkFolder:nil];
-  else if (buttonCount > 0)
-    [self adjustWindowForButtonCount:buttonCount];
+      [buttons_ addObject:button];
+      [folderView_ addSubview:button];
+      buttonCount = 1;

[Alexei Svitkine (slow), 2014/09/26 15:14:37]

Shouldn't this block call [self adjustWindowForButtonCount:buttonCount]; too
like the previous code?

Or the "if buttonCount > 0" block shouldn't be an else if below.

[Gaja, 2014/09/27 05:56:33]

I think we don't need this line. Here is my analysis,

FMPOV, if we are at this point, it means that we just removed last button and
adding (empty) placeholder button, so we don't need to call [self
adjustWindowForButtonCount:buttonCount];, that is we don't need to adjust the
window because we removed one button and added one button at the same
place(height). Since "[self adjustWindowForButtonCount:buttonCount];" is in
'else if' block, it is never called when we are removing last button.

In case of "Other bookmarks" button (else {} part), when we remove last button,
we do not add '(empty)' placeholder button, but we close the folder and hide it,
therefore, again we don't need to call "[self
adjustWindowForButtonCount:buttonCount];".

Calling [self adjustWindowForButtonCount:buttonCount]; is necessary only when
the removed button is not the last button in that folder.

Please correct me if I was wrong anywhere :).

+    }
+    else {

[Alexei Svitkine (slow), 2014/09/26 15:14:37]

Nit: Put on previous line.

[Gaja, 2014/09/27 05:56:33]

Acknowledged.

+      // buttonCount will be 0 if "Other bookmarks" folder is empty, so close
+      // the folder before hiding it.
+      [barController_ closeBookmarkFolder:nil];
+    }
+  } else if (buttonCount > 0)

[Alexei Svitkine (slow), 2014/09/26 15:14:37]

Nit: {}'s

[Gaja, 2014/09/27 05:56:33]

Acknowledged.

+      [self adjustWindowForButtonCount:buttonCount];
 
   if (animate && !ignoreAnimations_)
     NSShowAnimationEffect(NSAnimationEffectDisappearingItemDefault, poofPoint,