Rework somewhat how the folder menu window and main view sizes and positions ...

Rework somewhat how the folder menu window and main view sizes and positions are calculated. Handle edge case where the top scroll arrow is not showing. Proposed window height was not taking into account the one additional vertical separation.

BUG=46101
TEST=Setup: Create a bookmark folder on the bookmark bar with plenty of bookmarks, more than enough to fill the screen, and so that it is scrollable. 1) Bring up the bookmark folder menu but do not cause it to scroll. 2) Using the contextual menu delete a bookmark midway in the menu. 3) Verify that the top of the menu window and contents do not move relative to the bookmark bar folder button. 4) Bring up the bookmark folder menu again. 5) Cause the menu to scroll up to fill the screen and then scroll it down so that the top scroll arrow is not showing. 6) Using the contextual menu delete a bookmark midway in the menu. 7) Verify that the top of the menu window and contents have not moved in relation to the screen top. 8) Bring up the bookmark folder menu and cause it to scroll such that both the top and bottom scroll arrows are showing. 9) Using the contextual menu, delete a bookmark midway in the menu. 10) Verify that the top scroll arrow, the top-most item showing in the menu, and the menu window have not changed position relative to the top of the window. Note that the contents of the folder menu below the bookmark item being delete should adjust upward to fill the slot originally occupied by the deleted bookmark.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=54187

[mrossetti, 2010-07-27 23:13:57]

Note that there is a pre-existing problem (i.e. this fix did not introduce this
problem) where the contents of the folder menu shift upward by 2px every time a
bookmark is cut/deletes/pasted. I've logged http://crbug.com/50437 for this
issue.

[John Grabowski, 2010-07-28 04:01:49]

http://codereview.chromium.org/3050021/diff/1/2
File chrome/browser/cocoa/bookmark_bar_folder_controller.mm (right):

http://codereview.chromium.org/3050021/diff/1/2#newcode373
chrome/browser/cocoa/bookmark_bar_folder_controller.mm:373: -
(void)adjustWindowForHeight:(int)windowHeight {
function very long.  Can you break up?

http://codereview.chromium.org/3050021/diff/1/2#newcode385
chrome/browser/cocoa/bookmark_bar_folder_controller.mm:385: if
(windowAlreadyShowing) {

Comment: "If the window is already visible then do not alter the top (y).. "
Logic:
  if (windowAlreadyShowing) {  .. alter... }

Seems to not match?

http://codereview.chromium.org/3050021/diff/1/2#newcode427
chrome/browser/cocoa/bookmark_bar_folder_controller.mm:427: NSView* superView =
[mainView_ superview];
Not to nitpick but does this warrant a unit test?
Would hate to lose this change by accident.

[mrossetti, 2010-07-28 17:18:43]

Unit test added.

http://codereview.chromium.org/3050021/diff/1/2
File chrome/browser/cocoa/bookmark_bar_folder_controller.mm (right):

http://codereview.chromium.org/3050021/diff/1/2#newcode373
chrome/browser/cocoa/bookmark_bar_folder_controller.mm:373: -
(void)adjustWindowForHeight:(int)windowHeight {
On 2010/07/28 04:01:49, John Grabowski wrote:
> function very long.  Can you break up?

It's 63 lines and fits on a screen without scrolling with room to spare, dude!
And, IMO, breaking it up would reduce the visibility of the logic. So I
rewrapped a couple of lines and removed a couple of blanks lines and got it down
to 59 lines. (And thought about adding a 'using
bookmarks::kScrollViewContentWidthMargin' but haven't seen many 'usings' in .mm
so forwent.)

http://codereview.chromium.org/3050021/diff/1/2#newcode385
chrome/browser/cocoa/bookmark_bar_folder_controller.mm:385: if
(windowAlreadyShowing) {
On 2010/07/28 04:01:49, John Grabowski wrote:
> 
> Comment: "If the window is already visible then do not alter the top (y).. "
> Logic:
>   if (windowAlreadyShowing) {  .. alter... }
> 
> Seems to not match?
> 

Revised comment.

http://codereview.chromium.org/3050021/diff/1/2#newcode427
chrome/browser/cocoa/bookmark_bar_folder_controller.mm:427: NSView* superView =
[mainView_ superview];
On 2010/07/28 04:01:49, John Grabowski wrote:
> Not to nitpick but does this warrant a unit test?
> Would hate to lose this change by accident.
> 

Done.

[John Grabowski, 2010-07-28 18:48:13]

LGTM++

http://codereview.chromium.org/3050021/diff/5001/6002
File chrome/browser/cocoa/bookmark_bar_folder_controller_unittest.mm (right):

http://codereview.chromium.org/3050021/diff/5001/6002#newcode506
chrome/browser/cocoa/bookmark_bar_folder_controller_unittest.mm:506:
TEST_F(BookmarkBarFolderControllerTest, MenuPlacementWhileScrollingDeleting) {
excellent