a bunch of bookmark bar changes

- Fix janklist issue #1: "there is a pixel line below the main
  toolbar. The main toolbar should blend in with the bookmark bar when
  it's open"
- Fix janklist issue #2: "It's way too tall - the distance from the
  bottom of the bar to a bookmark button bottom edge should be the
  same as the distance from the omnibox to the bookmark button top
  edge (this will probably mean that the bar has to overlap the
  toolbar)."
- Fix janklist issue #4 (first part): "the bookmark bar bookmark buttons
  have a frame around them ... "
- Fix janklist issue #9: "the show/hide animation is very janky... I see
  a dark gray area behind".  Even with animators the grey is gone, but
  animators are disabled for now due to races.
- Fix unlisted jank related to 9: don't use animator when opening bar on
  launch.
- Also chipped away on unit tests.

TEST=Launch with bookmark bar both open and closed.  Make sure OK on launch.
In each case open and close a few times fast.
Repeat with multiple windows open.
Sanity check jank descriptions listed above are fixed.

BUG=crbug.com/14139, crbug.com/8381, crbug.com/14724


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

[John Grabowski, 2009-07-08 14:18:05]

Adding more reviewers since Avi is away?

[Avi (use Gerrit), 2009-07-08 15:16:17]

LG

http://codereview.chromium.org/149308/diff/1/9
File chrome/browser/cocoa/bookmark_bar_controller.h (right):

http://codereview.chromium.org/149308/diff/1/9#newcode48
Line 48: // Owned by the toolbar view, it's parent view.
its

http://codereview.chromium.org/149308/diff/1/3
File chrome/browser/cocoa/bookmark_bar_controller.mm (right):

http://codereview.chromium.org/149308/diff/1/3#newcode64
Line 64: // [contentView_ addSubview:bookmarkBarView_];
don't check in commented-out code

http://codereview.chromium.org/149308/diff/1/13
File chrome/browser/cocoa/bookmark_bar_controller_unittest.mm (right):

http://codereview.chromium.org/149308/diff/1/13#newcode42
Line 42: view:(BookmarkBarView*)bar_view_.get()
80col

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

http://codereview.chromium.org/149308/diff/1/7#newcode179
Line 179: starButton_, goButton_, locationBar_, nil];
Thought we wrapped to the colon.

[pink (ping after 24hrs), 2009-07-08 15:25:27]

http://codereview.chromium.org/149308/diff/1/9
File chrome/browser/cocoa/bookmark_bar_controller.h (right):

http://codereview.chromium.org/149308/diff/1/9#newcode48
Line 48: // Owned by the toolbar view, it's parent view.
s/it's/its

http://codereview.chromium.org/149308/diff/1/9#newcode62
Line 62: // content view.  We use |content| as the view for the bookmark
s/|content|/|webContentView|

http://codereview.chromium.org/149308/diff/1/3
File chrome/browser/cocoa/bookmark_bar_controller.mm (right):

http://codereview.chromium.org/149308/diff/1/3#newcode26
Line 26: const int kBookmarkBarHeight = 30;
rather than hardcoding a height, should we base it off the height of the view?

http://codereview.chromium.org/149308/diff/1/3#newcode47
Line 47: // We default to NOT open, which means height=0.
is it worth noting that the default is hidden in the comment for init in the
header?

http://codereview.chromium.org/149308/diff/1/3#newcode63
Line 63: // [[[contentView_ window] contentView] addSubview:bookmarkBarView_];
remove these lines?

http://codereview.chromium.org/149308/diff/1/3#newcode112
Line 112: NSRect superframe = [superview frame];
is it a member of the hall of justice? sorry, i couldn't resist ;)

http://codereview.chromium.org/149308/diff/1/13
File chrome/browser/cocoa/bookmark_bar_controller_unittest.mm (right):

http://codereview.chromium.org/149308/diff/1/13#newcode42
Line 42: view:(BookmarkBarView*)bar_view_.get()
80cols

http://codereview.chromium.org/149308/diff/1/13#newcode83
Line 83: BookmarkURLOpenerPong* pong = [[[BookmarkURLOpenerPong alloc] init]
there's no explicit autorelease pool, so i don't know when |pong| actually gets
cleaned up (probably at the very end of all the tests). Seems a bit fragile,
given the weak reference as a delegate. Worth fixing?

http://codereview.chromium.org/149308/diff/1/13#newcode87
Line 87: NSButtonCell *cell = [[[NSButtonCell alloc] init] autorelease];
worth using scoped_nsobject here since they don't technically need to be
autoreleased?

http://codereview.chromium.org/149308/diff/1/8
File chrome/browser/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/149308/diff/1/8#newcode162
Line 162: webContentView:[self tabContentArea]
line up :

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

http://codereview.chromium.org/149308/diff/1/10#newcode15
Line 15: 
omit blank line

http://codereview.chromium.org/149308/diff/1/10#newcode16
Line 16: if ([self isBordered]) {
I already have a patch that does something similar checked in. you might want to
check it out as you still want to draw the selection gradient if you're not
bordered and the cell is highlighted.

http://codereview.chromium.org/149308/diff/1/11
File chrome/browser/cocoa/toolbar_controller.h (right):

http://codereview.chromium.org/149308/diff/1/11#newcode33
Line 33: // A controller for the toolbar in the browser window. Manages updating
the
add to the comment that it's also responsible for the bookmark bar and its
position in the window relative to the web content view

http://codereview.chromium.org/149308/diff/1/11#newcode57
Line 57: IBOutlet BookmarkBarView* bookmarkBarView_;
i checked in a ton of code here yesterday for the home button and page/wrench
buttons, make sure it merges correctly and the unit tests pass.

[John Grabowski, 2009-07-08 22:03:01]

Replies to comments.
I still need to sync/merge/hit_the_try_servers on this.

http://codereview.chromium.org/149308/diff/1/9
File chrome/browser/cocoa/bookmark_bar_controller.h (right):

http://codereview.chromium.org/149308/diff/1/9#newcode48
Line 48: // Owned by the toolbar view, it's parent view.
On 2009/07/08 15:16:17, Avi wrote:
> its

fixed

http://codereview.chromium.org/149308/diff/1/9#newcode62
Line 62: // content view.  We use |content| as the view for the bookmark
On 2009/07/08 15:25:27, pink wrote:
> s/|content|/|webContentView|

fixed

http://codereview.chromium.org/149308/diff/1/3
File chrome/browser/cocoa/bookmark_bar_controller.mm (right):

http://codereview.chromium.org/149308/diff/1/3#newcode26
Line 26: const int kBookmarkBarHeight = 30;
On 2009/07/08 15:25:27, pink wrote:
> rather than hardcoding a height, should we base it off the height of the view?

Toolbar height is 38; omnibox height is 27.  Not sure a "toolbarHeight - 8" is a
good idea.  Part of the problem is that the BMB needs to overlap the toolbar by
just a few pixels to even things up, and it isn't clear that "tbh - 8" will
still look even if the tbh changes.  I agree this seems awkward.

http://codereview.chromium.org/149308/diff/1/3#newcode47
Line 47: // We default to NOT open, which means height=0.
On 2009/07/08 15:25:27, pink wrote:
> is it worth noting that the default is hidden in the comment for init in the
> header?

Yes; added.

http://codereview.chromium.org/149308/diff/1/3#newcode63
Line 63: // [[[contentView_ window] contentView] addSubview:bookmarkBarView_];
On 2009/07/08 15:25:27, pink wrote:
> remove these lines?

killed

http://codereview.chromium.org/149308/diff/1/3#newcode64
Line 64: // [contentView_ addSubview:bookmarkBarView_];
On 2009/07/08 15:16:17, Avi wrote:
> don't check in commented-out code

removed

http://codereview.chromium.org/149308/diff/1/3#newcode112
Line 112: NSRect superframe = [superview frame];
On 2009/07/08 15:25:27, pink wrote:
> is it a member of the hall of justice? sorry, i couldn't resist ;)

Clarified with a comment.

http://codereview.chromium.org/149308/diff/1/13
File chrome/browser/cocoa/bookmark_bar_controller_unittest.mm (right):

http://codereview.chromium.org/149308/diff/1/13#newcode42
Line 42: view:(BookmarkBarView*)bar_view_.get()
On 2009/07/08 15:16:17, Avi wrote:
> 80col

shrunk

http://codereview.chromium.org/149308/diff/1/13#newcode42
Line 42: view:(BookmarkBarView*)bar_view_.get()
On 2009/07/08 15:25:27, pink wrote:
> 80cols

shrunk

http://codereview.chromium.org/149308/diff/1/13#newcode83
Line 83: BookmarkURLOpenerPong* pong = [[[BookmarkURLOpenerPong alloc] init]
On 2009/07/08 15:25:27, pink wrote:
> there's no explicit autorelease pool, so i don't know when |pong| actually
gets
> cleaned up (probably at the very end of all the tests). Seems a bit fragile,
> given the weak reference as a delegate. Worth fixing?

scoped_nsobjectified.

http://codereview.chromium.org/149308/diff/1/13#newcode87
Line 87: NSButtonCell *cell = [[[NSButtonCell alloc] init] autorelease];
On 2009/07/08 15:25:27, pink wrote:
> worth using scoped_nsobject here since they don't technically need to be
> autoreleased?

scoped_nsobjectified.

http://codereview.chromium.org/149308/diff/1/8
File chrome/browser/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/149308/diff/1/8#newcode162
Line 162: webContentView:[self tabContentArea]
On 2009/07/08 15:25:27, pink wrote:
> line up :

lined up

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

http://codereview.chromium.org/149308/diff/1/10#newcode15
Line 15: 
On 2009/07/08 15:25:27, pink wrote:
> omit blank line

it's gone

http://codereview.chromium.org/149308/diff/1/10#newcode16
Line 16: if ([self isBordered]) {
On 2009/07/08 15:25:27, pink wrote:
> I already have a patch that does something similar checked in. you might want
to
> check it out as you still want to draw the selection gradient if you're not
> bordered and the cell is highlighted.

I will be sure to merge and test before landing.

http://codereview.chromium.org/149308/diff/1/11
File chrome/browser/cocoa/toolbar_controller.h (right):

http://codereview.chromium.org/149308/diff/1/11#newcode33
Line 33: // A controller for the toolbar in the browser window. Manages updating
the
On 2009/07/08 15:25:27, pink wrote:
> add to the comment that it's also responsible for the bookmark bar and its
> position in the window relative to the web content view

added

http://codereview.chromium.org/149308/diff/1/11#newcode57
Line 57: IBOutlet BookmarkBarView* bookmarkBarView_;
On 2009/07/08 15:25:27, pink wrote:
> i checked in a ton of code here yesterday for the home button and page/wrench
> buttons, make sure it merges correctly and the unit tests pass.

I will be sure to sync/merge/test before landing.

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

http://codereview.chromium.org/149308/diff/1/7#newcode179
Line 179: starButton_, goButton_, locationBar_, nil];
On 2009/07/08 15:16:17, Avi wrote:
> Thought we wrapped to the colon.

I checked and didn't see any rule specific to this.  It did not wrap to the
colon before my change if that matters.  This is where Emacs
ObjC-mode-with-Google-extensions placed it, if that matters either.

[pink (ping after 24hrs), 2009-07-09 14:19:42]

http://codereview.chromium.org/149308/diff/63/1034
File chrome/browser/cocoa/bookmark_bar_controller_unittest.mm (right):

http://codereview.chromium.org/149308/diff/63/1034#newcode83
Line 83: scoped_nsobject<BookmarkURLOpenerPong> pong([[BookmarkURLOpenerPong
alloc]
the problem with converting this to a scoped object is now the delegate will be
garbage after this method returns until bar_ is deallocated. In practice, we're
safe, since we know that nothing is going to use it, but maybe better safe than
sorry by clearing the delegate before going out of scope?

[John Grabowski, 2009-07-09 22:07:26]

http://codereview.chromium.org/149308/diff/63/1034
File chrome/browser/cocoa/bookmark_bar_controller_unittest.mm (right):

http://codereview.chromium.org/149308/diff/63/1034#newcode83
Line 83: scoped_nsobject<BookmarkURLOpenerPong> pong([[BookmarkURLOpenerPong
alloc]
On 2009/07/09 14:19:42, pink wrote:
> the problem with converting this to a scoped object is now the delegate will
be
> garbage after this method returns until bar_ is deallocated. In practice,
we're
> safe, since we know that nothing is going to use it, but maybe better safe
than
> sorry by clearing the delegate before going out of scope?

It's a good idea; I will follow-up in a future CL.