Rewrites the Mac view resizing logic to have the BrowserWindowController...

Rewrites the Mac view resizing logic to have the BrowserWindowController
directly resize and relayout its children views.  Now when a view needs
to be resized, it asks its resize delegate (typically its controller's
parent) to perform the resize.

BUG=http://crbug.com/17619

TEST=Make sure that views are laid out correctly, even when they change size.
Open and close the bookmark bar.  Trigger an infobar and then close it.  Trigger
the download shelf and then close it.  Trigger a download shelf with the infobar
open, or with the bookmark bar open.  Switch to and from fullscreen with various
bars open.  Resize the browser window with various bars open.  Start the browser
with and without the bookmark bar open.  Try all of the above in a popup window.


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

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

http://codereview.chromium.org/159776/diff/21/1024
File chrome/browser/cocoa/download_shelf_mac.mm (right):

http://codereview.chromium.org/159776/diff/21/1024#newcode16
Line 16: //Show();
Deleted in my client.  Not going to bother uploading a new patch just for this.

[John Grabowski, 2009-08-04 07:55:23]

Overall I really like the direction this takes and how it cleans up resizing.

http://codereview.chromium.org/159776/diff/21/1026
File chrome/browser/cocoa/bookmark_bar_controller.h (right):

http://codereview.chromium.org/159776/diff/21/1026#newcode56
Line 56: BOOL barShouldBeShown_;
This variable means "do we show the bar?" (e.g. triggered on Cmd-B).
This would NOT override barIsEnabled_; it's the other way around.
Disabling the bar means "hide it and ignore show state changes".
barShouldBeShown_, this one, is unrelated to fullscreen.

http://codereview.chromium.org/159776/diff/21/1020
File chrome/browser/cocoa/bookmark_bar_controller.mm (left):

http://codereview.chromium.org/159776/diff/21/1020#oldcode35
Line 35: // "blue outline" of the omnibox.  Fix.
Did you really fix this?

http://codereview.chromium.org/159776/diff/21/1020#oldcode84
Line 84: [[self view] setAutoresizingMask:(NSViewWidthSizable |
Where did this logic go?  In the nib?

http://codereview.chromium.org/159776/diff/21/1020
File chrome/browser/cocoa/bookmark_bar_controller.mm (right):

http://codereview.chromium.org/159776/diff/21/1020#newcode128
Line 128: [[self view] setHidden:show ? NO : YES];
Perhaps unhide before resizing?  Else we may get unnecessary painting of the
"behind this view" if the parent resizes.

http://codereview.chromium.org/159776/diff/21/1022
File chrome/browser/cocoa/browser_window_controller.mm (left):

http://codereview.chromium.org/159776/diff/21/1022#oldcode198
Line 198: // Register ourselves for frame changed notifications from the
Why do we no longer need this?

http://codereview.chromium.org/159776/diff/21/1022
File chrome/browser/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/159776/diff/21/1022#newcode7
Line 7: #include "base/mac_util.h"
For this whole file: be sure to add generous amounts of regression testing in
the bug.  E.g.
- fullscreen and back, with info bar open and closed, and bookmark bar open and
closed
- pop-up windows

http://codereview.chromium.org/159776/diff/21/1022#newcode365
Line 365: frame.size.height = height;
Doesn't this cause the view to grow up due to Cocoa coordinate flipping?
Wouldn't we want to "grow down"?  E.g.
 frame.origin.y -= (height - frame.size.height);
 frame.size.height = height;
It looks like resizeSubviews takes care of this issue; best to doc otherwise
behavior is mysterious.

http://codereview.chromium.org/159776/diff/21/1022#newcode732
Line 732: [[toolbarController_ bookmarkBarController] setBookmarkBarEnabled:NO];
This is wrong?  We don't show the toolbar in fullscreen so no need to turn off
the bookmark bar.

http://codereview.chromium.org/159776/diff/21/1022#newcode742
Line 742: [[toolbarController_ bookmarkBarController]
setBookmarkBarEnabled:YES];
Like above, this line is probably unnecessary.

http://codereview.chromium.org/159776/diff/21/1022#newcode764
Line 764: [self adjustUIForFullscreen:fullscreen];
Why move this down?  I suppose it doesn't matter visually but I am curious why
it moved.

http://codereview.chromium.org/159776/diff/21/1022#newcode1016
Line 1016: - (void)layoutSubviews {
Unit test candidate

http://codereview.chromium.org/159776/diff/21/1032
File chrome/browser/cocoa/find_bar_cocoa_controller.h (right):

http://codereview.chromium.org/159776/diff/21/1032#newcode44
Line 44: - (void)positionFindBarView:(NSView*)infoBarContainerView;
Does this assume the info bar container view exists (and is height=0) even when
there is no info bar?  Seems a little awkward.

http://codereview.chromium.org/159776/diff/21/1033
File chrome/browser/cocoa/find_bar_cocoa_controller.mm (right):

http://codereview.chromium.org/159776/diff/21/1033#newcode61
Line 61: - (void)positionFindBarView:(NSView*)infoBarContainerView {
Perhaps, like the resizing, this is a question best asked of our parent view's
controller?  I like the idea of the main window controller (tab window
controller) telling this guy where the origin is, so this guy doesn't need to
know or care about details of fullscreen and a missing toolbar.

http://codereview.chromium.org/159776/diff/21/1021
File chrome/browser/cocoa/toolbar_controller.mm (right):

http://codereview.chromium.org/159776/diff/21/1021#newcode126
Line 126: // bookmark bar as a subview, so its width is never set correctly. 
Reset the
Perhaps pass a value in for the bmb controller initializer to make it set
correctly at awakeFromNib time?  Better to have no option for confusion.

http://codereview.chromium.org/159776/diff/21/1021#newcode133
Line 133: - (void)resizeView:(NSView*)view newHeight:(float)height {
unit test

http://codereview.chromium.org/159776/diff/21/1030
File chrome/browser/cocoa/view_resizer.h (right):

http://codereview.chromium.org/159776/diff/21/1030#newcode18
Line 18: - (void)resizeView:(NSView*)view newHeight:(float)height;
Can we assume everything grows the same way?
If so, doc it as such.  Note slight confusion in terms (visually growing down
means moving to a lower origin.y while keeping NSMaxY fixed).

Also, when growing the bookmark bar, we grow it more than the parent view,
overlapping (on purpose) other elements so it looks proportional (Cole mentions
this in a comment in some proto code).  Thus, we may need to enrichen the API to
reflect our needs.  For example,  from the original bookmark_bar_controller.m:
const int kBookmarkBarHeight = 30;
// How much to adjust our parent view.
const int kBookmarkBarSuperviewHeightAdjustment = 25;

http://codereview.chromium.org/159776/diff/21/1035
File chrome/browser/cocoa/view_resizer_pong.h (right):

http://codereview.chromium.org/159776/diff/21/1035#newcode12
Line 12: @interface ViewResizerPong : NSObject<ViewResizer> {
Pongtastic!

[rohitrao (ping after 24h), 2009-08-04 19:46:02]

http://codereview.chromium.org/159776/diff/21/1026
File chrome/browser/cocoa/bookmark_bar_controller.h (right):

http://codereview.chromium.org/159776/diff/21/1026#newcode56
Line 56: BOOL barShouldBeShown_;
On 2009/08/04 07:55:23, John Grabowski wrote:
> This variable means "do we show the bar?" (e.g. triggered on Cmd-B).

Ah, ok.  Comment changed.

http://codereview.chromium.org/159776/diff/21/1020
File chrome/browser/cocoa/bookmark_bar_controller.mm (left):

http://codereview.chromium.org/159776/diff/21/1020#oldcode35
Line 35: // "blue outline" of the omnibox.  Fix.
On 2009/08/04 07:55:23, John Grabowski wrote:
> Did you really fix this?

Scott/Cole pushed through a change to the way we draw the focus ring, so it no
longer escapes the bounds of the locationbar.  If I'm reading this TODO
correctly, then it's no longer possible for the focus ring to overlap with
anything.

http://codereview.chromium.org/159776/diff/21/1020#oldcode84
Line 84: [[self view] setAutoresizingMask:(NSViewWidthSizable |
On 2009/08/04 07:55:23, John Grabowski wrote:
> Where did this logic go?  In the nib?  

These flags were already set correctly in the nib.

http://codereview.chromium.org/159776/diff/21/1020
File chrome/browser/cocoa/bookmark_bar_controller.mm (right):

http://codereview.chromium.org/159776/diff/21/1020#newcode128
Line 128: [[self view] setHidden:show ? NO : YES];
On 2009/08/04 07:55:23, John Grabowski wrote:
> Perhaps unhide before resizing?  Else we may get unnecessary painting of the
> "behind this view" if the parent resizes.

Done.  I'm going to go back through in a few days and take a look at how many
times we repaint and relayout, to try and get rid of the unnecessary ones.

http://codereview.chromium.org/159776/diff/21/1022
File chrome/browser/cocoa/browser_window_controller.mm (left):

http://codereview.chromium.org/159776/diff/21/1022#oldcode198
Line 198: // Register ourselves for frame changed notifications from the
On 2009/08/04 07:55:23, John Grabowski wrote:
> Why do we no longer need this?

This notification is leftover from an earlier hack to make animation work. 
Instead of animating the bookmarkbar and download shelf open, we used to animate
the tabContentArea's frame and resize the bookmarkbar/downloadshelf whenever the
frame changed.  I got rid of all that code, so we no longer need to do any work
when we get a frame changed notification.

http://codereview.chromium.org/159776/diff/21/1022
File chrome/browser/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/159776/diff/21/1022#newcode365
Line 365: frame.size.height = height;
On 2009/08/04 07:55:23, John Grabowski wrote:
> Doesn't this cause the view to grow up due to Cocoa coordinate flipping?
> Wouldn't we want to "grow down"?  E.g.
>  frame.origin.y -= (height - frame.size.height);
>  frame.size.height = height;
> It looks like resizeSubviews takes care of this issue; best to doc otherwise
> behavior is mysterious.

Comment added.  I'm setting the frame height indiscriminately, because
layoutSubviews will put each view where it needs to be.

This is one place that could use optimization: we call setFrame: twice for the
resized view, once here and once more in layoutSubviews.  I've added a TODO to
look into this.

http://codereview.chromium.org/159776/diff/21/1022#newcode732
Line 732: [[toolbarController_ bookmarkBarController] setBookmarkBarEnabled:NO];
On 2009/08/04 07:55:23, John Grabowski wrote:
> This is wrong?  We don't show the toolbar in fullscreen so no need to turn off
> the bookmark bar.
> 

Don't we need this call to properly handle enabling/disabling Cmd-B?

http://codereview.chromium.org/159776/diff/21/1022#newcode764
Line 764: [self adjustUIForFullscreen:fullscreen];
On 2009/08/04 07:55:23, John Grabowski wrote:
> Why move this down?  I suppose it doesn't matter visually but I am curious why
> it moved.

I put the layoutSubviews call into adjustUIForFullscreen, and the relayout needs
to happen after we move the content into the correct window.  It seemed simplest
to just move this call down a bit.  Comments added.

http://codereview.chromium.org/159776/diff/21/1022#newcode1016
Line 1016: - (void)layoutSubviews {
On 2009/08/04 07:55:23, John Grabowski wrote:
> Unit test candidate

Test added for resizeView:newHeight:, because that was an easier entry point. 
The test has some crazy setup because of the way layout works.  Let me know what
you think.

http://codereview.chromium.org/159776/diff/21/1032
File chrome/browser/cocoa/find_bar_cocoa_controller.h (right):

http://codereview.chromium.org/159776/diff/21/1032#newcode44
Line 44: - (void)positionFindBarView:(NSView*)infoBarContainerView;
On 2009/08/04 07:55:23, John Grabowski wrote:
> Does this assume the info bar container view exists (and is height=0) even
when
> there is no info bar?  Seems a little awkward.

Yes, that's the assumption, although it turns out to always be true.  I've added
a TODO to move this logic into BrowserWindowController.

http://codereview.chromium.org/159776/diff/21/1033
File chrome/browser/cocoa/find_bar_cocoa_controller.mm (right):

http://codereview.chromium.org/159776/diff/21/1033#newcode61
Line 61: - (void)positionFindBarView:(NSView*)infoBarContainerView {
On 2009/08/04 07:55:23, John Grabowski wrote:
> Perhaps, like the resizing, this is a question best asked of our parent view's
> controller?  I like the idea of the main window controller (tab window
> controller) telling this guy where the origin is, so this guy doesn't need to
> know or care about details of fullscreen and a missing toolbar.

The logic here was complicated enough that I wasn't sure how to move it.  Should
the BWC controller be responsible for knowing that the findbar should be
positioned 25px from the right edge of the window?  I've added a TODO to revisit
this in a later CL.

http://codereview.chromium.org/159776/diff/21/1021
File chrome/browser/cocoa/toolbar_controller.mm (right):

http://codereview.chromium.org/159776/diff/21/1021#newcode126
Line 126: // bookmark bar as a subview, so its width is never set correctly. 
Reset the
On 2009/08/04 07:55:23, John Grabowski wrote:
> Perhaps pass a value in for the bmb controller initializer to make it set
> correctly at awakeFromNib time?  Better to have no option for confusion.

Done, although it feels a bit weird to pass in an initialWidth.

http://codereview.chromium.org/159776/diff/21/1021#newcode133
Line 133: - (void)resizeView:(NSView*)view newHeight:(float)height {
On 2009/08/04 07:55:23, John Grabowski wrote:
> unit test

Done.

http://codereview.chromium.org/159776/diff/21/1030
File chrome/browser/cocoa/view_resizer.h (right):

http://codereview.chromium.org/159776/diff/21/1030#newcode18
Line 18: - (void)resizeView:(NSView*)view newHeight:(float)height;
On 2009/08/04 07:55:23, John Grabowski wrote:
> Can we assume everything grows the same way?
> If so, doc it as such.  Note slight confusion in terms (visually growing down
> means moving to a lower origin.y while keeping NSMaxY fixed).

The protocol revolves around changing a view's height only, since we never
really care about widths (views are almost always as wide as their parent).  It
doesn't specify anything about growing up or growing down, because the parent
takes care of that.  Essentially, the child view is saying "I need to be this
tall, put me wherever you want to accomplish that."

> 
> Also, when growing the bookmark bar, we grow it more than the parent view,
> overlapping (on purpose) other elements so it looks proportional (Cole
mentions
> this in a comment in some proto code).  Thus, we may need to enrichen the API
to
> reflect our needs.  For example,  from the original bookmark_bar_controller.m:
> const int kBookmarkBarHeight = 30;
> // How much to adjust our parent view.
> const int kBookmarkBarSuperviewHeightAdjustment = 25;

The toolbar presents a special case.  I've gotten around it for now by coding a
5px "overlap" into ToolbarController, which seems to work well enough.  If this
turns out to be a problem, we can revisit it.

[Nico, 2009-08-04 20:20:57]

Download shelf changes look good. The other platforms call show() on shelf
construction, but since AddDownload() calls show as well, that should be fine.
Would be awesome if you could see if you can do a similar change on the other
platforms, so that the code stays similar.

[John Grabowski, 2009-08-04 20:25:36]

LGTM

Thanks for taking this on Rohit.
I'll bounce some bugs to you fixed by this change.

http://codereview.chromium.org/159776/diff/21/1022
File chrome/browser/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/159776/diff/21/1022#newcode732
Line 732: [[toolbarController_ bookmarkBarController] setBookmarkBarEnabled:NO];
On 2009/08/04 19:46:02, rohitrao wrote:
> On 2009/08/04 07:55:23, John Grabowski wrote:
> > This is wrong?  We don't show the toolbar in fullscreen so no need to turn
off
> > the bookmark bar.
> > 
> 
> Don't we need this call to properly handle enabling/disabling Cmd-B?

I'm not sure.  Right now, the bookmark bar is a subview of the toolbar view.
No toolbar view on the fullscreen window means, no matter what, you'll never see
the bookmark bar.  I suspect this was added when the bookmark bar was not a
subview of the toolbar.

But don't worry about it; just add a TODO(jrg) here for me.

http://codereview.chromium.org/159776/diff/21/1033
File chrome/browser/cocoa/find_bar_cocoa_controller.mm (right):

http://codereview.chromium.org/159776/diff/21/1033#newcode61
Line 61: - (void)positionFindBarView:(NSView*)infoBarContainerView {
On 2009/08/04 19:46:02, rohitrao wrote:
> On 2009/08/04 07:55:23, John Grabowski wrote:
> > Perhaps, like the resizing, this is a question best asked of our parent
view's
> > controller?  I like the idea of the main window controller (tab window
> > controller) telling this guy where the origin is, so this guy doesn't need
to
> > know or care about details of fullscreen and a missing toolbar.
> 
> The logic here was complicated enough that I wasn't sure how to move it. 
Should
> the BWC controller be responsible for knowing that the findbar should be
> positioned 25px from the right edge of the window?  I've added a TODO to
revisit
> this in a later CL.

Perhaps you are right; follow your gut if you think this is the right spot.

http://codereview.chromium.org/159776/diff/21/1021
File chrome/browser/cocoa/toolbar_controller.mm (right):

http://codereview.chromium.org/159776/diff/21/1021#newcode126
Line 126: // bookmark bar as a subview, so its width is never set correctly. 
Reset the
On 2009/08/04 19:46:02, rohitrao wrote:
> On 2009/08/04 07:55:23, John Grabowski wrote:
> > Perhaps pass a value in for the bmb controller initializer to make it set
> > correctly at awakeFromNib time?  Better to have no option for confusion.
> 
> Done, although it feels a bit weird to pass in an initialWidth.

Perhaps pass in a parent view where the initial width can be pulled from?
Weird is bad.  The point of an initializer is to make sure no variables are
uninitialized, yes?  Can you think of a better way to accomplish this?

http://codereview.chromium.org/159776/diff/28/1063
File chrome/browser/cocoa/browser_window_controller_unittest.mm (right):

http://codereview.chromium.org/159776/diff/28/1063#newcode161
Line 161: TEST_F(BrowserWindowControllerTest, TestResizeViews) {
Excellent.

http://codereview.chromium.org/159776/diff/28/1078
File chrome/browser/cocoa/toolbar_controller_unittest.mm (right):

http://codereview.chromium.org/159776/diff/28/1078#newcode195
Line 195: // resize to 69px.
If possible use the kBaseToolbarHeight constant in case it changes.

[Nico, 2009-08-04 20:34:53]

For the records, rohitrao on IRC: " the show call into [BWC downloadShelf], so
the shelf is explicitly shown when it is created.". So the download shelf change
LG.

Thanks a ton for tackling this!

http://codereview.chromium.org/159776/diff/28/1062
File chrome/browser/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/159776/diff/28/1062#newcode358
Line 358: view == [downloadShelfController_ view]);
Perhaps you also want to check that view != nil.

[rohitrao (ping after 24h), 2009-08-04 20:54:04]

http://codereview.chromium.org/159776/diff/21/1022
File chrome/browser/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/159776/diff/21/1022#newcode7
Line 7: #include "base/mac_util.h"
On 2009/08/04 07:55:23, John Grabowski wrote:
> For this whole file: be sure to add generous amounts of regression testing in
> the bug.  E.g.
> - fullscreen and back, with info bar open and closed, and bookmark bar open
and
> closed
> - pop-up windows
> 
> 

Done.

http://codereview.chromium.org/159776/diff/21/1022#newcode732
Line 732: [[toolbarController_ bookmarkBarController] setBookmarkBarEnabled:NO];
On 2009/08/04 20:25:36, John Grabowski wrote:
> On 2009/08/04 19:46:02, rohitrao wrote:
> > On 2009/08/04 07:55:23, John Grabowski wrote:
> > > This is wrong?  We don't show the toolbar in fullscreen so no need to turn
> off
> > > the bookmark bar.
> > > 
> > 
> > Don't we need this call to properly handle enabling/disabling Cmd-B?
> 
> I'm not sure.  Right now, the bookmark bar is a subview of the toolbar view.
> No toolbar view on the fullscreen window means, no matter what, you'll never
see
> the bookmark bar.  I suspect this was added when the bookmark bar was not a
> subview of the toolbar.
> 
> But don't worry about it; just add a TODO(jrg) here for me.

Done.

http://codereview.chromium.org/159776/diff/21/1021
File chrome/browser/cocoa/toolbar_controller.mm (right):

http://codereview.chromium.org/159776/diff/21/1021#newcode126
Line 126: // bookmark bar as a subview, so its width is never set correctly. 
Reset the
On 2009/08/04 20:25:36, John Grabowski wrote:
> Can you think of a better way to accomplish this?   
> 

I think my main objection is to storing a variable just so we can use it once in
awakeFromNib and never again, but it's not a very big objection.  This solution
seems as good as any to me, so I'll leave it as is.

http://codereview.chromium.org/159776/diff/28/1078
File chrome/browser/cocoa/toolbar_controller_unittest.mm (right):

http://codereview.chromium.org/159776/diff/28/1078#newcode195
Line 195: // resize to 69px.
On 2009/08/04 20:25:36, John Grabowski wrote:
> If possible use the kBaseToolbarHeight constant in case it changes.

The constant doesn't help me directly, since the only way to derive the correct
value from the constant is to copy/paste the math from the code =)