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

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

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #import "chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_controller.h"
 
 #include "base/mac/bundle_locations.h"
 #include "base/mac/mac_util.h"
 #include "base/strings/sys_string_conversions.h"
 #import "chrome/browser/bookmarks/bookmark_model_factory.h"
@@ skipping 332 matching lines @@
 }
 
 - (int)buttonCount {
   return [[self buttons] count];
 }
 
 - (BookmarkButton*)parentButton {
   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()) {
     [button setDelegate:nil];
     [button removeFromSuperview];
   }
   [buttons_ removeAllObjects];
   [self configureWindow];
 }
 
@@ skipping 1552 matching lines @@
 
   [oldButton setDelegate:nil];
   [oldButton removeFromSuperview];
   [buttons_ removeObjectAtIndex:buttonIndex];
   for (NSInteger i = 0; i < buttonIndex; ++i) {
     BookmarkButton* button = [buttons_ objectAtIndex:i];
     NSRect buttonFrame = [button frame];
     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.

+  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;

[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 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];
-    [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];
+    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,
                           NSZeroSize, nil, nil, nil);
 }
 
 - (id<BookmarkButtonControllerProtocol>)controllerForNode:
     (const BookmarkNode*)node {
   // See if we are holding this node, otherwise see if it is in our
   // hierarchy of visible folder menus.
@@ skipping 30 matching lines @@
 
 - (void)setIgnoreAnimations:(BOOL)ignore {
   ignoreAnimations_ = ignore;
 }
 
 - (BookmarkButton*)buttonThatMouseIsIn {
   return buttonThatMouseIsIn_;
 }
 
 @end  // BookmarkBarFolderController