Vertical scrolling arrows in bookmark bar folder windows when needed....

Vertical scrolling arrows in bookmark bar folder windows when needed.

pdfs from Cole.
BUG=42026
TEST=\
1) Small folders --> no arrows.
2) Big folders --> arrow at bottom initially.
3) Move browser to bottom of screen so a small folder falls off the bottom and has an arrow.
Open it and gently use scroll wheel to scroll.  Make sure transition to "no arrow" looks good.
Close and reopen.  Scroll super-fast.  Make sure it ends up in the same nice place.
4) Open big big folder.  Scroll so top goes offscreen so you now have 2 scroll arrows.
Use scroll wheel to gently go up and down (arrow hides and shows).  Make sure transitions OK.
Scroll all the way so bottom arrow disappears.  Gently up and down; watch for transitions.
Now FAST up and down.  Make sure destination looks OK.

BookmarkBarFolderWindow.xib change: BookmarkBarFolderWindowScrollView border turned off.


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

[mrossetti, 2010-04-30 17:17:49]

LGTM

Coupla nits.

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

http://codereview.chromium.org/1813003/diff/1/9#newcode52
chrome/browser/cocoa/bookmark_bar_folder_controller.mm:52: CGFloat
kVerticalScrollArrowOffset = 20.0;
How do you know this is the amount of room required? Wouldn't it be better if
this was determined from the image size, esp. since the drawing of the image
doesn't adhere to this 20.0 rule? Alternatively, limit the drawing to this
constant.

http://codereview.chromium.org/1813003/diff/1/9#newcode379
chrome/browser/cocoa/bookmark_bar_folder_controller.mm:379: CGFloat
arrowAdjustment = 0.0;
Why not just use a  conditional expression here? The += on an initial 0.0 is
funky.

http://codereview.chromium.org/1813003/diff/1/9#newcode400
chrome/browser/cocoa/bookmark_bar_folder_controller.mm:400: arrowAdjustment +=
kVerticalScrollArrowOffset;
Ditto on the conditional expression.

http://codereview.chromium.org/1813003/diff/1/9#newcode402
chrome/browser/cocoa/bookmark_bar_folder_controller.mm:402: NSPoint
scrollPosition = [scrollView_ documentVisibleRect].origin;
You don't use scrollPosition until line 411 so this line should be moved down.

http://codereview.chromium.org/1813003/diff/1/9#newcode408
chrome/browser/cocoa/bookmark_bar_folder_controller.mm:408: if (NSHeight([[self
window] frame]) == NSHeight(documentRect))
I had to think about this for a while but it finally made sense.

http://codereview.chromium.org/1813003/diff/1/9#newcode671
chrome/browser/cocoa/bookmark_bar_folder_controller.mm:671: // If we can't
scroll either up or down are are completely done.
s/are are/we are/.

http://codereview.chromium.org/1813003/diff/1/7
File chrome/browser/cocoa/bookmark_bar_folder_controller_unittest.mm (right):

http://codereview.chromium.org/1813003/diff/1/7#newcode376
chrome/browser/cocoa/bookmark_bar_folder_controller_unittest.mm:376: // Now
we'll go the other way.
Comment could be better. "Once all nodes have been scrolled up, only the down
scroller should be showing." or somesuch.

http://codereview.chromium.org/1813003/diff/1/6
File chrome/browser/cocoa/bookmark_bar_folder_window.h (right):

http://codereview.chromium.org/1813003/diff/1/6#newcode9
chrome/browser/cocoa/bookmark_bar_folder_window.h:9: #import
"base/cocoa_protocols_mac.h"
Needed for the NSWindowDelegate?

[dhollowa, 2010-04-30 18:00:20]

LGTM.

http://codereview.chromium.org/1813003/diff/1/7
File chrome/browser/cocoa/bookmark_bar_folder_controller_unittest.mm (right):

http://codereview.chromium.org/1813003/diff/1/7#newcode135
chrome/browser/cocoa/bookmark_bar_folder_controller_unittest.mm:135: int
nodecount = 150;
const int kNodeCount = 150?  (Not to be overly nitty).  Same below.

http://codereview.chromium.org/1813003/diff/1/10
File chrome/browser/cocoa/bookmark_bar_folder_window.mm (right):

http://codereview.chromium.org/1813003/diff/1/10#newcode43
chrome/browser/cocoa/bookmark_bar_folder_window.mm:43:
arrowUpImage_.reset([nsimage_cache::ImageNamed(@"menu_overflow_up.pdf")
nit: indent 2 spaces.

[John Grabowski, 2010-04-30 20:31:01]

Thanks guys.  All feedback applied except...

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

http://codereview.chromium.org/1813003/diff/1/9#newcode400
chrome/browser/cocoa/bookmark_bar_folder_controller.mm:400: arrowAdjustment +=
kVerticalScrollArrowOffset;
On 2010/04/30 17:17:50, mrossetti wrote:
> Ditto on the conditional expression.

left this one in for clarity

http://codereview.chromium.org/1813003/diff/1/9#newcode402
chrome/browser/cocoa/bookmark_bar_folder_controller.mm:402: NSPoint
scrollPosition = [scrollView_ documentVisibleRect].origin;
On 2010/04/30 17:17:50, mrossetti wrote:
> You don't use scrollPosition until line 411 so this line should be moved down.

I don't agree; there are times it makes sense to say "get all data" then 'act on
it'.

http://codereview.chromium.org/1813003/diff/1/9#newcode408
chrome/browser/cocoa/bookmark_bar_folder_controller.mm:408: if (NSHeight([[self
window] frame]) == NSHeight(documentRect))
On 2010/04/30 17:17:50, mrossetti wrote:
> I had to think about this for a while but it finally made sense.

same here

http://codereview.chromium.org/1813003/diff/1/6
File chrome/browser/cocoa/bookmark_bar_folder_window.h (right):

http://codereview.chromium.org/1813003/diff/1/6#newcode9
chrome/browser/cocoa/bookmark_bar_folder_window.h:9: #import
"base/cocoa_protocols_mac.h"
On 2010/04/30 17:17:50, mrossetti wrote:
> Needed for the NSWindowDelegate?

yes