The frame for the button for a bookmark which is dropped at the top of a folder was never being recalculated and was left with the placeholder value. The placeholder value has been replaced with a default frame. Unit tests were enhanced to check for proper button vertical alignment. Comments were added to metric constants. A convenience constant was added for a frequently needed spacing from the base of one menu button to the base of the next.

BUG=58962
TEST=Drop a bookmark at the top of a folder and verify that it is drawn in the top button position.

Review URL: http://codereview.chromium.org/4168003

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@64185 0039d316-1c4b-4281-b951-d872f2087c98

4 files changed, 87 insertions, 26 deletions

diff --git a/chrome/browser/cocoa/bookmarks/bookmark_bar_controller.h b/chrome/browser/cocoa/bookmarks/bookmark_bar_controller.h
index 70f2ea6..6d0a0f6 100644
--- a/chrome/browser/cocoa/bookmarks/bookmark_bar_controller.h
+++ b/chrome/browser/cocoa/bookmarks/bookmark_bar_controller.h
@@ -13,6 +13,7 @@
 #include "base/scoped_nsobject.h"
 #include "base/scoped_ptr.h"
 #include "chrome/browser/cocoa/bookmarks/bookmark_bar_bridge.h"
+#import "chrome/browser/cocoa/bookmarks/bookmark_bar_constants.h"
 #import "chrome/browser/cocoa/bookmarks/bookmark_bar_state.h"
 #import "chrome/browser/cocoa/bookmarks/bookmark_bar_toolbar_view.h"
 #import "chrome/browser/cocoa/bookmarks/bookmark_button.h"
@@ -43,14 +44,33 @@ namespace bookmarks {
 // Used as a maximum width for buttons on the bar.
 const CGFloat kDefaultBookmarkWidth = 150.0;
 
-// TODO(jrg): http://crbug.com/36276 to get final sizes.
+// Horizontal frame inset for buttons in the bookmark bar.
+const CGFloat kBookmarkHorizontalPadding = 1.0;
+
+// Vertical frame inset for buttons in the bookmark bar.
+const CGFloat kBookmarkVerticalPadding = 2.0;
+
 // Used as a min/max width for buttons on menus (not on the bar).
 const CGFloat kBookmarkMenuButtonMinimumWidth = 100.0;
 const CGFloat kBookmarkMenuButtonMaximumWidth = 485.0;
 
-const CGFloat kBookmarkVerticalPadding = 2.0;
-const CGFloat kBookmarkHorizontalPadding = 1.0;
+// Horizontal separation between a menu button and both edges of its menu.
 const CGFloat kBookmarkSubMenuHorizontalPadding = 5.0;
+
+// TODO(mrossetti): Add constant (kBookmarkVerticalSeparation) for the gap
+// between buttons in a folder menu. Right now we're using
+// kBookmarkVerticalPadding, which is dual purpose and wrong.
+// http://crbug.com/59057
+
+// Convenience constant giving the vertical distance from the top extent of one
+// folder button to the next button.
+const CGFloat kBookmarkButtonVerticalSpan =
+    kBookmarkButtonHeight + kBookmarkVerticalPadding;
+
+// The minimum separation between a folder menu and the edge of the screen.
+// If the menu gets closer to the edge of the screen (either right or left)
+// then it is pops up in the opposite direction.
+// (See -[BookmarkBarFolderController childFolderWindowLeftForWidth:]).
 const CGFloat kBookmarkHorizontalScreenPadding = 8.0;
 
 // Our NSScrollView is supposed to be just barely big enough to fit its
diff --git a/chrome/browser/cocoa/bookmarks/bookmark_bar_controller.mm b/chrome/browser/cocoa/bookmarks/bookmark_bar_controller.mm
index 63cc171..03e867e 100644
--- a/chrome/browser/cocoa/bookmarks/bookmark_bar_controller.mm
+++ b/chrome/browser/cocoa/bookmarks/bookmark_bar_controller.mm
@@ -15,7 +15,6 @@
 #include "chrome/browser/browser_list.h"
 #import "chrome/browser/cocoa/background_gradient_view.h"
 #import "chrome/browser/cocoa/bookmarks/bookmark_bar_bridge.h"
-#import "chrome/browser/cocoa/bookmarks/bookmark_bar_constants.h"
 #import "chrome/browser/cocoa/bookmarks/bookmark_bar_folder_controller.h"
 #import "chrome/browser/cocoa/bookmarks/bookmark_bar_folder_window.h"
 #import "chrome/browser/cocoa/bookmarks/bookmark_bar_toolbar_view.h"
diff --git a/chrome/browser/cocoa/bookmarks/bookmark_bar_folder_controller.mm b/chrome/browser/cocoa/bookmarks/bookmark_bar_folder_controller.mm
index ca1514c..6eaca1a 100644
--- a/chrome/browser/cocoa/bookmarks/bookmark_bar_folder_controller.mm
+++ b/chrome/browser/cocoa/bookmarks/bookmark_bar_folder_controller.mm
@@ -27,13 +27,11 @@ const NSTimeInterval kBookmarkBarFolderScrollInterval = 0.1;
 // Amount to scroll by per timer fire.  We scroll rather slowly; to
 // accomodate we do several at a time.
 const CGFloat kBookmarkBarFolderScrollAmount =
-    3 * (bookmarks::kBookmarkButtonHeight +
-         bookmarks::kBookmarkVerticalPadding);
+    3 * bookmarks::kBookmarkButtonVerticalSpan;
 
 // Amount to scroll for each scroll wheel delta.
 const CGFloat kBookmarkBarFolderScrollWheelAmount =
-    1 * (bookmarks::kBookmarkButtonHeight +
-         bookmarks::kBookmarkVerticalPadding);
+    1 * bookmarks::kBookmarkButtonVerticalSpan;
 
 // When constraining a scrolling bookmark bar folder window to the
 // screen, shrink the "constrain" by this much vertically.  Currently
@@ -387,8 +385,7 @@ const CGFloat kScrollWindowVerticalMargin = 0.0;
 }
 
 - (int)windowHeightForButtonCount:(int)buttonCount {
-  return (buttonCount * (bookmarks::kBookmarkButtonHeight +
-                         bookmarks::kBookmarkVerticalPadding)) +
+  return (buttonCount * bookmarks::kBookmarkButtonVerticalSpan) +
       bookmarks::kBookmarkVerticalPadding;
 }
 
@@ -475,8 +472,7 @@ const CGFloat kScrollWindowVerticalMargin = 0.0;
   // http://crbug.com/35966
   NSRect buttonsOuterFrame = NSMakeRect(
     bookmarks::kBookmarkSubMenuHorizontalPadding,
-    (height -
-     (bookmarks::kBookmarkButtonHeight + bookmarks::kBookmarkVerticalPadding)),
+    (height - bookmarks::kBookmarkButtonVerticalSpan),
     bookmarks::kDefaultBookmarkWidth,
     bookmarks::kBookmarkButtonHeight);
 
@@ -498,8 +494,7 @@ const CGFloat kScrollWindowVerticalMargin = 0.0;
                                                  frame:buttonsOuterFrame];
       [buttons_ addObject:button];
       [mainView_ addSubview:button];
-      buttonsOuterFrame.origin.y -= (bookmarks::kBookmarkButtonHeight +
-                                     bookmarks::kBookmarkVerticalPadding);
+      buttonsOuterFrame.origin.y -= bookmarks::kBookmarkButtonVerticalSpan;
     }
   }
 
@@ -886,8 +881,6 @@ static BOOL ValueInRangeInclusive(CGFloat low, CGFloat value, CGFloat high) {
 // bookmark_bar_controller.mm, but vertical instead of horizontal.
 // Generalize to be axis independent then share code.
 // http://crbug.com/35966
-// Get UI review on "middle half" ness.
-// http://crbug.com/36276
 - (BookmarkButton*)buttonForDroppingOnAtPoint:(NSPoint)point {
   for (BookmarkButton* button in buttons_.get()) {
     // No early break -- makes no assumption about button ordering.
@@ -1252,8 +1245,11 @@ static BOOL ValueInRangeInclusive(CGFloat low, CGFloat value, CGFloat high) {
 
 - (void)addButtonForNode:(const BookmarkNode*)node
                  atIndex:(NSInteger)buttonIndex {
-  // Propose the frame for the new button.
-  NSRect newButtonFrame = NSMakeRect(0, 0, 500, 500);  // Placeholder values.
+  // Propose the frame for the new button. By default, this will be set to the
+  // topmost button's frame (and there will always be one) offset upward in
+  // anticipation of insertion.
+  NSRect newButtonFrame = [[buttons_ objectAtIndex:0] frame];
+  newButtonFrame.origin.y += bookmarks::kBookmarkButtonVerticalSpan;
   // When adding a button to an empty folder we must remove the 'empty'
   // placeholder button. This can be detected by checking for a parent
   // child count of 1.
@@ -1277,8 +1273,7 @@ static BOOL ValueInRangeInclusive(CGFloat low, CGFloat value, CGFloat high) {
     // which is where the new button will be located.
     newButtonFrame = [button frame];
     NSRect buttonFrame = [button frame];
-    buttonFrame.origin.y += bookmarks::kBookmarkBarHeight +
-        bookmarks::kBookmarkVerticalPadding;
+    buttonFrame.origin.y += bookmarks::kBookmarkButtonVerticalSpan;
     [button setFrame:buttonFrame];
   }
   [[button cell] mouseExited:nil];  // De-highlight.
@@ -1352,8 +1347,7 @@ static BOOL ValueInRangeInclusive(CGFloat low, CGFloat value, CGFloat high) {
       for (NSInteger i = fromIndex; i < toIndex; ++i) {
         BookmarkButton* button = [buttons_ objectAtIndex:i];
         NSRect frame = [button frame];
-        frame.origin.y += bookmarks::kBookmarkBarHeight +
-            bookmarks::kBookmarkVerticalPadding;
+        frame.origin.y += bookmarks::kBookmarkButtonVerticalSpan;
         [button setFrameOrigin:frame.origin];
       }
     } else {
@@ -1362,8 +1356,7 @@ static BOOL ValueInRangeInclusive(CGFloat low, CGFloat value, CGFloat high) {
       for (NSInteger i = fromIndex - 1; i >= toIndex; --i) {
         BookmarkButton* button = [buttons_ objectAtIndex:i];
         NSRect buttonFrame = [button frame];
-        buttonFrame.origin.y -= bookmarks::kBookmarkBarHeight +
-            bookmarks::kBookmarkVerticalPadding;
+        buttonFrame.origin.y -= bookmarks::kBookmarkButtonVerticalSpan;
         [button setFrameOrigin:buttonFrame.origin];
       }
     }
@@ -1408,8 +1401,7 @@ static BOOL ValueInRangeInclusive(CGFloat low, CGFloat value, CGFloat high) {
   for (NSInteger i = 0; i < buttonIndex; ++i) {
     BookmarkButton* button = [buttons_ objectAtIndex:i];
     NSRect buttonFrame = [button frame];
-    buttonFrame.origin.y -= bookmarks::kBookmarkButtonHeight +
-      bookmarks::kBookmarkVerticalPadding;
+    buttonFrame.origin.y -= bookmarks::kBookmarkButtonVerticalSpan;
     [button setFrame:buttonFrame];
   }
   // Search for and adjust submenus, if necessary.
diff --git a/chrome/browser/cocoa/bookmarks/bookmark_bar_folder_controller_unittest.mm b/chrome/browser/cocoa/bookmarks/bookmark_bar_folder_controller_unittest.mm
index 293aff1..ee9ccb8 100644
--- a/chrome/browser/cocoa/bookmarks/bookmark_bar_folder_controller_unittest.mm
+++ b/chrome/browser/cocoa/bookmarks/bookmark_bar_folder_controller_unittest.mm
@@ -24,12 +24,32 @@
 // Add a redirect to make testing easier.
 @interface BookmarkBarFolderController(MakeTestingEasier)
 - (IBAction)openBookmarkFolderFromButton:(id)sender;
+- (void)validateMenuSpacing;
 @end
 
 @implementation BookmarkBarFolderController(MakeTestingEasier)
 - (IBAction)openBookmarkFolderFromButton:(id)sender {
   [[self folderTarget] openBookmarkFolderFromButton:sender];
 }
+
+// Utility function to verify that the buttons in this folder are all
+// evenly spaced in a progressive manner.
+- (void)validateMenuSpacing {
+  BOOL firstButton = YES;
+  CGFloat lastVerticalOffset = 0.0;
+  for (BookmarkButton* button in [self buttons]) {
+    if (firstButton) {
+      firstButton = NO;
+      lastVerticalOffset = [button frame].origin.y;
+    } else {
+      CGFloat nextVerticalOffset = [button frame].origin.y;
+      EXPECT_CGFLOAT_EQ(lastVerticalOffset -
+                            bookmarks::kBookmarkButtonVerticalSpan,
+                        nextVerticalOffset);
+      lastVerticalOffset = nextVerticalOffset;
+    }
+  }
+}
 @end
 
 // Don't use a high window level when running unit tests -- it'll
@@ -709,6 +729,9 @@ TEST_F(BookmarkBarFolderControllerMenuTest, DragMoveBarBookmarkToFolder) {
   expectedToWindowFrame.size.height += diff;
   EXPECT_NSRECT_EQ(expectedToWindowFrame, newToWindowFrame);
 
+  // Check button spacing.
+  [folderController validateMenuSpacing];
+
   // Move the button back to the bar at the beginning.
   draggedButton = [folderController buttonWithTitleEqualTo:@"1b"];
   ASSERT_TRUE(draggedButton);
@@ -836,6 +859,10 @@ TEST_F(BookmarkBarFolderControllerMenuTest, DragMoveBarBookmarkToSubfolder) {
       "4f2f1b 4f2f2b 4f2f3b 5b ] 4f3f:[ 4f3f1b 4f3f2b 4f3f3b ] ] ");
   EXPECT_EQ(expected_string, model_test_utils::ModelStringFromNode(root));
 
+  // Check button spacing.
+  [folderController validateMenuSpacing];
+  [subfolderController validateMenuSpacing];
+
   // Check the window layouts. The folder window should not have changed,
   // but the subfolder window should have shifted vertically and grown.
   NSRect newToWindowFrame = [toWindow frame];
@@ -893,6 +920,9 @@ TEST_F(BookmarkBarFolderControllerMenuTest, DragMoveWithinFolder) {
   // The folder window should not have changed.
   NSRect newToWindowFrame = [toWindow frame];
   EXPECT_NSRECT_EQ(oldToWindowFrame, newToWindowFrame);
+
+  // Check button spacing.
+  [folderController validateMenuSpacing];
 }
 
 TEST_F(BookmarkBarFolderControllerMenuTest, DragParentOntoChild) {
@@ -927,6 +957,9 @@ TEST_F(BookmarkBarFolderControllerMenuTest, DragParentOntoChild) {
                           copy:NO];
   // The model should not have changed.
   EXPECT_EQ(model_string, model_test_utils::ModelStringFromNode(root));
+
+  // Check button spacing.
+  [folderController validateMenuSpacing];
 }
 
 TEST_F(BookmarkBarFolderControllerMenuTest, DragMoveChildToParent) {
@@ -973,6 +1006,8 @@ TEST_F(BookmarkBarFolderControllerMenuTest, DragMoveChildToParent) {
       "4f2f1b 4f2f2b ] 4f3f:[ 4f3f1b 4f3f2b 4f3f3b ] ] 5b ");
   EXPECT_EQ(expected_string, model_test_utils::ModelStringFromNode(root));
 
+  // Check button spacing.
+  [folderController validateMenuSpacing];
   // The window should not have gone away.
   EXPECT_TRUE([bar_ folderController]);
   // The subfolder should have gone away.
@@ -1077,6 +1112,9 @@ TEST_F(BookmarkBarFolderControllerMenuTest, MoveRemoveAddButtons) {
   EXPECT_NSEQ(@"2f2b", [[buttons objectAtIndex:1] title]);
   EXPECT_NSEQ(@"2f3b", [[buttons objectAtIndex:2] title]);
   EXPECT_EQ(oldDisplayedButtons, [buttons count]);
+
+  // Check button spacing.
+  [folder validateMenuSpacing];
 }
 
 TEST_F(BookmarkBarFolderControllerMenuTest, ControllerForNode) {
@@ -1199,6 +1237,9 @@ TEST_F(BookmarkBarFolderControllerMenuTest, MenuSizingAndScrollArrows) {
   // Check the size. It should have reduced.
   EXPECT_GT(menuWidth, NSWidth([folderMenu frame]));
   EXPECT_GT(buttonWidth, NSWidth([button frame]));
+
+  // Check button spacing.
+  [folderController validateMenuSpacing];
 }
 
 // See http://crbug.com/46101
@@ -1324,6 +1365,9 @@ TEST_F(BookmarkBarFolderControllerMenuTest, DragBookmarkData) {
                                "2f3b ] 3b 4b ");
   actual = model_test_utils::ModelStringFromNode(root);
   EXPECT_EQ(expectedA, actual);
+
+  // Check button spacing.
+  [folderController validateMenuSpacing];
 }
 
 TEST_F(BookmarkBarFolderControllerMenuTest, DragBookmarkDataToTrash) {
@@ -1363,6 +1407,9 @@ TEST_F(BookmarkBarFolderControllerMenuTest, DragBookmarkDataToTrash) {
                              "2f3b ] 3b 4b ");
   actual = model_test_utils::ModelStringFromNode(root);
   EXPECT_EQ(expected, actual);
+
+  // Check button spacing.
+  [folderController validateMenuSpacing];
 }
 
 TEST_F(BookmarkBarFolderControllerMenuTest, AddURLs) {
@@ -1405,6 +1452,9 @@ TEST_F(BookmarkBarFolderControllerMenuTest, AddURLs) {
                              "2f2f3b ] 2f3b ] 3b 4b ");
   actual = model_test_utils::ModelStringFromNode(root);
   EXPECT_EQ(expected, actual);
+
+  // Check button spacing.
+  [folderController validateMenuSpacing];
 }
 
 TEST_F(BookmarkBarFolderControllerMenuTest, DropPositionIndicator) {