First cut at infobars on Mac. These are not expected to be...

First cut at infobars on Mac.  These are not expected to be
pretty.  Animations and aesthetic appeal will come in a
future CL.

BUG=http://crbug.com/14462
BUG=http://crbug.com/14937
BUG=http://crbug.com/15839
BUG=http://crbug.com/16487
TEST=Infobars should show up when expected.

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

[rohitrao (ping after 24h), 2009-07-14 20:43:28]

I think this is ready for a look.  They aren't pretty, but they work.  I'll go
through and clean the UI up once this is in, or maybe I'll ask Cole for help.

One structural change: I may play around with putting the ok/cancel buttons into
the nib file and hiding them when they aren't needed.  This will probably save
me some of the creation and positioning code.  I'd like to get something checked
in first before I experiment with that, though.

[pink (ping after 24hrs), 2009-07-15 16:35:31]

LGTM with items below addressed.

http://codereview.chromium.org/155494/diff/52/1043
File chrome/browser/cocoa/infobar.h (right):

http://codereview.chromium.org/155494/diff/52/1043#newcode14
Line 14: // InfoBarDelegate::CreateInfoBar().  Callers should delete it
this makes little sense to me. Is this how other platforms work? Better comments
about *why* we do this needed.

http://codereview.chromium.org/155494/diff/52/1042
File chrome/browser/cocoa/infobar_container.h (right):

http://codereview.chromium.org/155494/diff/52/1042#newcode35
Line 35: int DesiredHeight();
should this be a float? that's what sizes are in cocoa

http://codereview.chromium.org/155494/diff/52/1046
File chrome/browser/cocoa/infobar_container.mm (right):

http://codereview.chromium.org/155494/diff/52/1046#newcode18
Line 18: infobar_controllers_.reset([[NSMutableArray alloc]
initWithCapacity:3]);
why 3?

http://codereview.chromium.org/155494/diff/52/1046#newcode36
Line 36: int height = 0;
should probably be a float

http://codereview.chromium.org/155494/diff/52/1046#newcode109
Line 109: for (int i = [infobar_controllers_ count] - 1; i >= 0; --i) {
can you use a reverse iterator here, or fast enumeration?

http://codereview.chromium.org/155494/diff/52/1046#newcode113
Line 113: [[controller retain] autorelease];
why do this for every single controller? Also, it seems a little odd. Why won't
normal releasing work? is there controller code on the stack?

http://codereview.chromium.org/155494/diff/52/1046#newcode123
Line 123: for (int i = [infobar_controllers_ count] - 1; i >= 0; --i) {
can you use a reverse iterator here, or fast enumeration?

http://codereview.chromium.org/155494/diff/52/1046#newcode127
Line 127: // clean up after itself before it is dealloc'ed.
as above, i don't understand why this is necessary.

http://codereview.chromium.org/155494/diff/52/1044
File chrome/browser/cocoa/infobar_controller.h (right):

http://codereview.chromium.org/155494/diff/52/1044#newcode33
Line 33: @property(readonly) NSImageView* image;
any reason for these to be accessible via the public api? testing? You could
make them @protected if you wanted them accessible from subclasses (which is
probably the reason here).

[rohitrao (ping after 24h), 2009-07-15 19:28:24]

I wasn't correctly handling bookmark bar toggling before, so I added code to
handle that.

http://codereview.chromium.org/155494/diff/52/1043
File chrome/browser/cocoa/infobar.h (right):

http://codereview.chromium.org/155494/diff/52/1043#newcode14
Line 14: // InfoBarDelegate::CreateInfoBar().  Callers should delete it
Since there is no defined InfoBar interface, it was easier to just toss it and
deal with the controller directly rather than turn it into a bridge class. 
Comment added.

http://codereview.chromium.org/155494/diff/52/1042
File chrome/browser/cocoa/infobar_container.h (right):

http://codereview.chromium.org/155494/diff/52/1042#newcode35
Line 35: int DesiredHeight();
On 2009/07/15 16:35:31, pink wrote:
> should this be a float? that's what sizes are in cocoa

Yup, changed.

http://codereview.chromium.org/155494/diff/52/1046
File chrome/browser/cocoa/infobar_container.mm (right):

http://codereview.chromium.org/155494/diff/52/1046#newcode18
Line 18: infobar_controllers_.reset([[NSMutableArray alloc]
initWithCapacity:3]);
On 2009/07/15 16:35:31, pink wrote:
> why 3?

Lowered it to 2, since I've never seen more than that many infobars open at
once.  It's a somewhat arbitrary choice.  Comment added.

http://codereview.chromium.org/155494/diff/52/1046#newcode36
Line 36: int height = 0;
On 2009/07/15 16:35:31, pink wrote:
> should probably be a float

Done.

http://codereview.chromium.org/155494/diff/52/1046#newcode109
Line 109: for (int i = [infobar_controllers_ count] - 1; i >= 0; --i) {
On 2009/07/15 16:35:31, pink wrote:
> can you use a reverse iterator here, or fast enumeration?

Per IRC, replacing with fast enumeration over a shallow copy, plus deleting from
the original list.

http://codereview.chromium.org/155494/diff/52/1046#newcode113
Line 113: [[controller retain] autorelease];
On 2009/07/15 16:35:31, pink wrote:
> why do this for every single controller? Also, it seems a little odd. Why
won't
> normal releasing work? is there controller code on the stack?

Oops, that should be inside the if.  This code can be executed while
-[InfoBarController closeInfoBar] is still on the stack, so we retain and
autorelease the controller to prevent it from being dealloc'ed too early.

http://codereview.chromium.org/155494/diff/52/1046#newcode123
Line 123: for (int i = [infobar_controllers_ count] - 1; i >= 0; --i) {
On 2009/07/15 16:35:31, pink wrote:
> can you use a reverse iterator here, or fast enumeration?

Replaced with a fast enumerator and removeAllObjects.

http://codereview.chromium.org/155494/diff/52/1046#newcode127
Line 127: // clean up after itself before it is dealloc'ed.
On 2009/07/15 16:35:31, pink wrote:
> as above, i don't understand why this is necessary.

I don't think this function ever gets hit with controller code on the stack, so
I'm removing the retain/autorelease.

http://codereview.chromium.org/155494/diff/52/1046#newcode142
Line 142: for (int i = [infobar_controllers_ count] - 1; i >= 0; --i) {
Replaced with a reverse fast enumerator.

[pink (ping after 24hrs), 2009-07-15 19:58:02]

still lgtm, will defer to jrg on resizing stuff.

http://codereview.chromium.org/155494/diff/1059/1062
File chrome/browser/cocoa/infobar.h (right):

http://codereview.chromium.org/155494/diff/1059/1062#newcode27
Line 27: // // deleted later.  InfoBar will be deleted automatically.
s/InfoBar/|infobar| maybe? am I misreading?

http://codereview.chromium.org/155494/diff/1059/1065
File chrome/browser/cocoa/infobar_container.mm (right):

http://codereview.chromium.org/155494/diff/1059/1065#newcode120
Line 120: [infobar_controllers_ removeObject:controller];
do you want to |break| at this point, since there's no point continuing the
enumeration?

[John Grabowski, 2009-07-15 20:13:13]

Quite a lot of work here!

I'm disappointed to have initiated awkwardness with the bookmark bar adjusting
views it shouldn't... which has degraded even more with this CL.  For that I am
sorry.  Once this lands let's you and I clean that up in a future CL as you
proposed.

http://codereview.chromium.org/155494/diff/1059/1072
File chrome/browser/cocoa/bookmark_bar_controller.h (right):

http://codereview.chromium.org/155494/diff/1059/1072#newcode67
Line 67: infoBarView:(NSView*)infoBarView
Don't like the idea of bookmark bars knowing about info bars.
This makes a bad idea (bookmark bar modifying the view of a sibling of it's
parent, the content view, when the toolbar grows) worse.

http://codereview.chromium.org/155494/diff/1059/1070
File chrome/browser/cocoa/bookmark_bar_controller.mm (right):

http://codereview.chromium.org/155494/diff/1059/1070#newcode105
Line 105: // TODO(jrg): it is awkward we change the sizes of views for our
This is my fault for providing an awkward example. :-(

http://codereview.chromium.org/155494/diff/1059/1070#newcode111
Line 111: - (void)applyContentAreaOffset:(BOOL)apply
immediately:(BOOL)immediately {
We need to move all this logic up one level.
E.g. "applyContentAreaOffset" first calls parent view's controller's "grow by
X", which grows it and shrinks web area (webContentView_).  Parent controller,
at that time, moves info bar.  Then bookmark bar grows itself.

http://codereview.chromium.org/155494/diff/1059/1074
File chrome/browser/cocoa/browser_window_controller.h (right):

http://codereview.chromium.org/155494/diff/1059/1074#newcode57
Line 57: scoped_ptr<InfoBarContainer> infobarContainer_;
The model we've tended to use is NSWindowController owns the bridge (e.g.
BrowserWindowController owns a TabStripModelObserverBridge;
BookmarkBarController owns the bookmark bar bridge, etc).  In this case you've
reversed the ownership, where the C++ Observer class owns the NSViewController. 
Is there a reason for this switcheroo?

http://codereview.chromium.org/155494/diff/1059/1066
File chrome/browser/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/155494/diff/1059/1066#newcode792
Line 792: - (void)resizeInfoBar:(int)newHeight {
Perhaps some of this logic should go in the InfoBarController?  E.g. all of it
except [[self tabConteltArea] setFrame:]?

http://codereview.chromium.org/155494/diff/1059/1066#newcode815
Line 815: - (void)positionInfoBar {
Perhaps some of this logic should go in the InfoBarController?  E.g. all of it
except [[[self window] contentView] addSubView:]?

http://codereview.chromium.org/155494/diff/1059/1061
File chrome/browser/cocoa/infobar_container.h (right):

http://codereview.chromium.org/155494/diff/1059/1061#newcode18
Line 18: // C++ wrapper around an NSView that contains the infobars.  There is
Reverse logic of ownership for other objc/C++ cousin classes (see other comment
in review)

http://codereview.chromium.org/155494/diff/1059/1061#newcode74
Line 74: // PositionInfoBarsAndRedraw() after calling this method.
Why can't the exposed API call PositionInfoBarsAndRedraw(), preventing the need
for this comment?

http://codereview.chromium.org/155494/diff/1059/1061#newcode83
Line 83: scoped_nsobject<NSView> container_view_;
When first looking at the header I guessed container_view_ was [window
contentView].  But that's not true; there is apparently a special "owns all info
bars" NSView.  Seems like something to document here.

http://codereview.chromium.org/155494/diff/1059/1065
File chrome/browser/cocoa/infobar_container.mm (right):

http://codereview.chromium.org/155494/diff/1059/1065#newcode14
Line 14: InfoBarContainer::InfoBarContainer(TabStripModel* model,
Class has no unit test.

http://codereview.chromium.org/155494/diff/1059/1065#newcode120
Line 120: [infobar_controllers_ removeObject:controller];
On 2009/07/15 19:58:02, pink wrote:
> do you want to |break| at this point, since there's no point continuing the
> enumeration?

And DCHECK() if we never hit the break.

http://codereview.chromium.org/155494/diff/1059/1063
File chrome/browser/cocoa/infobar_controller.h (right):

http://codereview.chromium.org/155494/diff/1059/1063#newcode13
Line 13: // override awakeFromNib to add additional controls.
// Subclasses are expected to call [super awakeFromNib] if they override it.

(or perhaps add a new method called from awakeFromNib, such as
addAdditionalControls, which is a noop in the base class, to make sure we can't
hit a bug where [super awakeFromNib] is never called.

http://codereview.chromium.org/155494/diff/1059/1063#newcode21
Line 21: InfoBarContainer* container_;  // weak
// weak, owns me

http://codereview.chromium.org/155494/diff/1059/1064
File chrome/browser/cocoa/infobar_controller_unittest.mm (right):

http://codereview.chromium.org/155494/diff/1059/1064#newcode218
Line 218: // Tests
Excellent

http://codereview.chromium.org/155494/diff/1059/1068
File chrome/browser/cocoa/infobar_text_field.mm (right):

http://codereview.chromium.org/155494/diff/1059/1068#newcode11
Line 11: - (BOOL)textView:(NSTextView*)aTextView
unit test please

[John Grabowski, 2009-07-16 00:16:41]

Did a quick scan of your CL (mostly headers); new files and "v7" changes.  I
like the new class abstractions much better.  When ready (e.g. unit tests) ping
me and I'll do a full review.

[rohitrao (ping after 24h), 2009-07-16 03:56:46]

This is ready for a real look.  I've punted on the resizing/positioning TODOs,
but I reorganized the classes and added more unit tests.

http://codereview.chromium.org/155494/diff/1059/1072
File chrome/browser/cocoa/bookmark_bar_controller.h (right):

http://codereview.chromium.org/155494/diff/1059/1072#newcode67
Line 67: infoBarView:(NSView*)infoBarView
On 2009/07/15 20:13:13, John Grabowski wrote:
> Don't like the idea of bookmark bars knowing about info bars.
> This makes a bad idea (bookmark bar modifying the view of a sibling of it's
> parent, the content view, when the toolbar grows) worse.

TODO added.

http://codereview.chromium.org/155494/diff/1059/1074
File chrome/browser/cocoa/browser_window_controller.h (right):

http://codereview.chromium.org/155494/diff/1059/1074#newcode57
Line 57: scoped_ptr<InfoBarContainer> infobarContainer_;
On 2009/07/15 20:13:13, John Grabowski wrote:
> The model we've tended to use is NSWindowController owns the bridge (e.g.
> BrowserWindowController owns a TabStripModelObserverBridge;
> BookmarkBarController owns the bookmark bar bridge, etc).  In this case you've
> reversed the ownership, where the C++ Observer class owns the
NSViewController. 
> Is there a reason for this switcheroo?

InfoBarContainer changed to InfoBarContainerController.  This should be more in
line with the other classes now.

http://codereview.chromium.org/155494/diff/1059/1066
File chrome/browser/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/155494/diff/1059/1066#newcode792
Line 792: - (void)resizeInfoBar:(int)newHeight {
On 2009/07/15 20:13:13, John Grabowski wrote:
> Perhaps some of this logic should go in the InfoBarController?  E.g. all of it
> except [[self tabConteltArea] setFrame:]?

TODO added.

http://codereview.chromium.org/155494/diff/1059/1066#newcode815
Line 815: - (void)positionInfoBar {
On 2009/07/15 20:13:13, John Grabowski wrote:
> Perhaps some of this logic should go in the InfoBarController?  E.g. all of it
> except [[[self window] contentView] addSubView:]?
> 

TODO added.

http://codereview.chromium.org/155494/diff/1059/1062
File chrome/browser/cocoa/infobar.h (right):

http://codereview.chromium.org/155494/diff/1059/1062#newcode27
Line 27: // // deleted later.  InfoBar will be deleted automatically.
On 2009/07/15 19:58:02, pink wrote:
> s/InfoBar/|infobar| maybe? am I misreading?

Done.

http://codereview.chromium.org/155494/diff/1059/1061
File chrome/browser/cocoa/infobar_container.h (right):

http://codereview.chromium.org/155494/diff/1059/1061#newcode18
Line 18: // C++ wrapper around an NSView that contains the infobars.  There is
On 2009/07/15 20:13:13, John Grabowski wrote:
> Reverse logic of ownership for other objc/C++ cousin classes (see other
comment
> in review)

Done.

http://codereview.chromium.org/155494/diff/1059/1061#newcode74
Line 74: // PositionInfoBarsAndRedraw() after calling this method.
On 2009/07/15 20:13:13, John Grabowski wrote:
> Why can't the exposed API call PositionInfoBarsAndRedraw(), preventing the
need
> for this comment?

The common use case is to call these functions a few times in a row (on tab
switch, remove all infobars, then add a couple new ones).  Making the caller
call Position() separately lets us do the resize once per batch.

http://codereview.chromium.org/155494/diff/1059/1061#newcode83
Line 83: scoped_nsobject<NSView> container_view_;
On 2009/07/15 20:13:13, John Grabowski wrote:
> When first looking at the header I guessed container_view_ was [window
> contentView].  But that's not true; there is apparently a special "owns all
info
> bars" NSView.  Seems like something to document here.

Somewhat obsolete, as now it's [InfoBarContainerController view].

http://codereview.chromium.org/155494/diff/1059/1065
File chrome/browser/cocoa/infobar_container.mm (right):

http://codereview.chromium.org/155494/diff/1059/1065#newcode14
Line 14: InfoBarContainer::InfoBarContainer(TabStripModel* model,
On 2009/07/15 20:13:13, John Grabowski wrote:
> Class has no unit test.

Done.

http://codereview.chromium.org/155494/diff/1059/1065#newcode120
Line 120: [infobar_controllers_ removeObject:controller];
On 2009/07/15 20:13:13, John Grabowski wrote:
> On 2009/07/15 19:58:02, pink wrote:
> > do you want to |break| at this point, since there's no point continuing the
> > enumeration?
> 
> And DCHECK() if we never hit the break.
> 

The other two platforms are written as though there could be multiple infobars
for a single delegate, although I don't know how that could happen.  Leaving the
break out seems safer.  (Really though, we're talking about a < 2 element array
in most cases, so it doesn't matter too much.)

http://codereview.chromium.org/155494/diff/1059/1063
File chrome/browser/cocoa/infobar_controller.h (right):

http://codereview.chromium.org/155494/diff/1059/1063#newcode13
Line 13: // override awakeFromNib to add additional controls.
On 2009/07/15 20:13:13, John Grabowski wrote:
> // Subclasses are expected to call [super awakeFromNib] if they override it.
> 
> (or perhaps add a new method called from awakeFromNib, such as
> addAdditionalControls, which is a noop in the base class, to make sure we
can't
> hit a bug where [super awakeFromNib] is never called.

Created addAdditionalControls.

http://codereview.chromium.org/155494/diff/1059/1063#newcode21
Line 21: InfoBarContainer* container_;  // weak
On 2009/07/15 20:13:13, John Grabowski wrote:
> // weak, owns me

Done.

http://codereview.chromium.org/155494/diff/1059/1068
File chrome/browser/cocoa/infobar_text_field.mm (right):

http://codereview.chromium.org/155494/diff/1059/1068#newcode11
Line 11: - (BOOL)textView:(NSTextView*)aTextView
On 2009/07/15 20:13:13, John Grabowski wrote:
> unit test please

No idea how to test this class.  I can put linkified text into the text field,
but I couldn't figure out how to simulate a click, which is the only thing that
will trigger this method.  Suggestions welcome.

[John Grabowski, 2009-07-17 01:07:43]

LGTM
Great job Rohit; this is a monster CL.

http://codereview.chromium.org/155494/diff/1059/1068
File chrome/browser/cocoa/infobar_text_field.mm (right):

http://codereview.chromium.org/155494/diff/1059/1068#newcode11
Line 11: - (BOOL)textView:(NSTextView*)aTextView
On 2009/07/16 03:56:46, rohitrao wrote:
> On 2009/07/15 20:13:13, John Grabowski wrote:
> > unit test please
> 
> No idea how to test this class.  I can put linkified text into the text field,
> but I couldn't figure out how to simulate a click, which is the only thing
that
> will trigger this method.  Suggestions welcome.

Call this method explicitly and confirm return value.
Add mock delegate which does respond to selector and make sure it gets called.

http://codereview.chromium.org/155494/diff/1164/134
File chrome/browser/cocoa/browser_window_controller.h (right):

http://codereview.chromium.org/155494/diff/1164/134#newcode129
Line 129: - (void)resizeInfoBarView:(float)newHeight;
nit: the method is named like a command; "resizeInfoBarView" implies "do it". 
Perhaps a gentler name, like infoBarViewResized:, might be more clear.  See the
method below this (userChangedTheme) for an example.
Note you added a method to browser_window_controller called positionInfoBar
which IS a command (and sounds like one).

http://codereview.chromium.org/155494/diff/1164/126
File chrome/browser/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/155494/diff/1164/126#newcode192
Line 192: [[NSNotificationCenter defaultCenter]
Not your fault, I know, but it looks like we need a removeObserver:self call in
our dealloc.

http://codereview.chromium.org/155494/diff/1164/135
File chrome/browser/cocoa/infobar_container_controller.mm (right):

http://codereview.chromium.org/155494/diff/1164/135#newcode83
Line 83: DCHECK([infobarControllers_ count] == 0);
Do we need a line like this here:
registrar_.RemoveAll();
to be sure we're not observing notifications anymore (e.g. nobody has a pointer
to us?)

http://codereview.chromium.org/155494/diff/1164/135#newcode184
Line 184: [view setFrame:frame];
Add TODO(rohit,jrg): consider animators once coordinated

http://codereview.chromium.org/155494/diff/1164/140
File chrome/browser/cocoa/infobar_container_controller_unittest.mm (right):

http://codereview.chromium.org/155494/diff/1164/140#newcode17
Line 17: @interface BrowserWindowControllerPong : BrowserWindowController {
pongtastic!

http://codereview.chromium.org/155494/diff/1164/124
File chrome/browser/cocoa/infobar_controller.h (right):

http://codereview.chromium.org/155494/diff/1164/124#newcode16
Line 16: InfoBarContainerController* containerController_;  // weak, owns us
Couldn't find where this gets assigned.  And it's private so it must be in
infobar_controller.mm.

http://codereview.chromium.org/155494/diff/1164/127
File chrome/browser/cocoa/infobar_controller.mm (right):

http://codereview.chromium.org/155494/diff/1164/127#newcode67
Line 67: [containerController_ removeDelegate:delegate_];
Always a noop since containerController_ never set

http://codereview.chromium.org/155494/diff/1164/133
File chrome/browser/cocoa/toolbar_controller.h (right):

http://codereview.chromium.org/155494/diff/1164/133#newcode45
Line 45: NSView* infoBarView_;     // weak; where the infobars go
Perhaps named infoBarContainerView_?  Or infoBarsView?

[rohitrao (ping after 24h), 2009-07-17 03:19:36]

http://codereview.chromium.org/155494/diff/1164/134
File chrome/browser/cocoa/browser_window_controller.h (right):

http://codereview.chromium.org/155494/diff/1164/134#newcode129
Line 129: - (void)resizeInfoBarView:(float)newHeight;
On 2009/07/17 01:07:43, John Grabowski wrote:
> nit: the method is named like a command; "resizeInfoBarView" implies "do it". 
> Perhaps a gentler name, like infoBarViewResized:, might be more clear.  See
the
> method below this (userChangedTheme) for an example.
> Note you added a method to browser_window_controller called positionInfoBar
> which IS a command (and sounds like one).

Done.

http://codereview.chromium.org/155494/diff/1164/126
File chrome/browser/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/155494/diff/1164/126#newcode192
Line 192: [[NSNotificationCenter defaultCenter]
On 2009/07/17 01:07:43, John Grabowski wrote:
> Not your fault, I know, but it looks like we need a removeObserver:self call
in
> our dealloc.
> 

I think I put this code in, so it probably is my fault =)
Done.

http://codereview.chromium.org/155494/diff/1164/135
File chrome/browser/cocoa/infobar_container_controller.mm (right):

http://codereview.chromium.org/155494/diff/1164/135#newcode83
Line 83: DCHECK([infobarControllers_ count] == 0);
On 2009/07/17 01:07:43, John Grabowski wrote:
> Do we need a line like this here:
> registrar_.RemoveAll();
> to be sure we're not observing notifications anymore (e.g. nobody has a
pointer
> to us?)

The NotificationRegistrar calls RemoveAll() for us when it's destroyed.

http://codereview.chromium.org/155494/diff/1164/135#newcode184
Line 184: [view setFrame:frame];
On 2009/07/17 01:07:43, John Grabowski wrote:
> Add TODO(rohit,jrg): consider animators once coordinated

Done.

http://codereview.chromium.org/155494/diff/1164/124
File chrome/browser/cocoa/infobar_controller.h (right):

http://codereview.chromium.org/155494/diff/1164/124#newcode16
Line 16: InfoBarContainerController* containerController_;  // weak, owns us
On 2009/07/17 01:07:43, John Grabowski wrote:
> Couldn't find where this gets assigned.  And it's private so it must be in
> infobar_controller.mm.

It's in [InfoBarContainerController addInfoBar], where we call CreateInfoBar()
and then set its container.