Fixes up bookmark bubbles and the browser window so that they shut down corre...

Fixes up bookmark bubbles and the browser window so that they shut down correctly.
BookmarkBubbleController has been made an NSWindowController instead of the
view controller that it used to be, and now loads its window from the nib instead
of creating it on the fly. Also cleans up fullscreen mode so that the window
referenced from browser_window_controller stays constant instead of having
[self window] and window_ potentially pointing at two different windows.

BookmarkBubble.xib has been modified so that it instantiates a window containing
a bubble view instead of just instantiating a view.

BUG=25054, 24734
TEST=Try going in and out of full screen mode. Try bringing up a bookmark bubble by clicking on the star. Try creating a pile of windows and then quitting,

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

[dmac, 2009-10-23 23:03:24]

I need to get this in before I can submit the rest of my unittest changes
because bookmark bubbles and statusbubbles were often leaving windows hanging
about.

jrg I included you on this review because I think you were quite involved in
putting together bookmark_bubbles and I didn't want to screw up anything you had
done. Any suggestions for testing would be appreciated as well.
http://codereview.chromium.org/327010 may be useful for some context.

[Mark Mentovai, 2009-10-24 00:30:28]

LGTM with these changes.  I'm not incredibly familiar with these classes, so you
should wait until someone else weighs in too.

http://codereview.chromium.org/333017/diff/3001/3010
File chrome/browser/cocoa/bookmark_bubble_controller.h (right):

http://codereview.chromium.org/333017/diff/3001/3010#newcode61
Line 61: - (IBAction)close:(id)sender;
I find it a little confusing that you've got close (from NSWindowController) and
also close:.

http://codereview.chromium.org/333017/diff/3001/3011
File chrome/browser/cocoa/bookmark_bubble_controller.mm (right):

http://codereview.chromium.org/333017/diff/3001/3011#newcode22
Line 22: NSString* nibpath = [mac_util::MainAppBundle()
nibPath?

http://codereview.chromium.org/333017/diff/3001/3011#newcode26
Line 26: // All these are weak...
This is really a comment that belongs in the header.

http://codereview.chromium.org/333017/diff/3001/3011#newcode118
Line 118: DCHECK([notification object] == [self window]);
DCHECK_EQ

http://codereview.chromium.org/333017/diff/3001/3006
File chrome/browser/cocoa/bookmark_bubble_view_unittest.mm (right):

http://codereview.chromium.org/333017/diff/3001/3006#newcode24
Line 24: TEST_VIEW(BookmarkBubbleViewTest, view_);
I initially thought that you were trying to do this CL before the one that
introduced test_window() and TEST_VIEW, so I got a little scared when I saw
this.  Nice to see that you got the other one checked in.

http://codereview.chromium.org/333017/diff/3001/3007
File chrome/browser/cocoa/bookmark_bubble_window_unittest.mm (right):

http://codereview.chromium.org/333017/diff/3001/3007#newcode13
Line 13: BookmarkBubbleWindow* window_ =
I should not be named with a trailing underscore!

(This confused the hell out of me.  "Wait a minute, that member should be
private.  It IS!  Wha?!")

http://codereview.chromium.org/333017/diff/3001/3007#newcode16
Line 16: styleMask:NSBorderlessWindowMask
I'd have lined up the colons, but whatever you like...

Actually, it looks like initContentsWithRect: would fit on the same line as the
alloc and then you could still line up the colons.

http://codereview.chromium.org/333017/diff/3001/3009
File chrome/browser/cocoa/browser_window_controller.h (right):

http://codereview.chromium.org/333017/diff/3001/3009#newcode55
Line 55: NSWindow *saved_regular_window_;
Style nit: put your *s on the type or the variable, but be consistent (and do
whatever the local code does).  You put it on the variable here, but on the type
for statusBubble_ and bookmarkBubbleController_.

Then again, I see you flip-flopping back and forth between* this and *this a
bunch.  Maybe you're just trying to match already-inconsistent style around you?
 I dunno.  I only bring it up because it's jarring to me during a review, and I
start to wonder how well something was planned if it bounces back and forth
style-wise.

Also, for this member: giveMeAnObjCStyleName_ and_not_one_of_these_.

http://codereview.chromium.org/333017/diff/3001/3009#newcode66
Line 66: // Strong. StatusBubble is a special case of a strong reference that
Blank line before so that it's clear that it applies to statusBubble_ and it
doesn't run together with all of the junk above.  Same for the comment attached
to bookmarkBubbleController_.  This is something else I've noticed a couple
other times in this change.  I think it improves readability when your eyes can
jump to the top of a comment easily and know whether it's attached to the thing
before or the thing after when horizontal whitespace is used judiciously.

http://codereview.chromium.org/333017/diff/3001/3004
File chrome/browser/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/333017/diff/3001/3004#newcode256
Line 256: DCHECK([notification object] == [self window]);
DCHECK_EQ

http://codereview.chromium.org/333017/diff/3001/3004#newcode262
Line 262: delete statusBubble_;
Set it to NULL here too so that you don't keep a bad pointer from now until you
dealloc?

http://codereview.chromium.org/333017/diff/3001/3004#newcode420
Line 420: [[self window] makeKeyAndOrderFront:NSApp];
NSApp, really?

http://codereview.chromium.org/333017/diff/3001/3004#newcode1066
Line 1066: node:node
align colon

http://codereview.chromium.org/333017/diff/3001/3005
File chrome/browser/cocoa/bubble_view_unittest.mm (right):

http://codereview.chromium.org/333017/diff/3001/3005#newcode25
Line 25: TEST_VIEW(BubbleViewTest, view_)
Hooray!

[Scott Hess - ex-Googler, 2009-10-24 01:13:44]

I am so glad someone other than me is doing this.

http://codereview.chromium.org/333017/diff/3001/3011
File chrome/browser/cocoa/bookmark_bubble_controller.mm (right):

http://codereview.chromium.org/333017/diff/3001/3011#newcode24
Line 24: ofType:@"nib"];
Formatting.  I think probably 4 additional spaces on path, line up :.

http://codereview.chromium.org/333017/diff/3001/3011#newcode63
Line 63: object:self];
Um... what?  I bet the reason is good, too!

http://codereview.chromium.org/333017/diff/3001/3011#newcode119
Line 119: [self performSelector:@selector(close:) withObject:self afterDelay:0];
I bet this connects to the +cancelPrevious* call.  Why?  Presumably this is like
a homegrown -autorelease.  But without the comments, I won't know what's up six
months down the road.

http://codereview.chromium.org/333017/diff/3001/3006
File chrome/browser/cocoa/bookmark_bubble_view_unittest.mm (right):

http://codereview.chromium.org/333017/diff/3001/3006#newcode16
Line 16: initWithFrame:frame]);
If this is going to be two lines anyhow, I'd prefer everything between [] to be
together on one of them.

http://codereview.chromium.org/333017/diff/3001/3006#newcode18
Line 18: [[test_window() contentView] addSubview:view_];
I'm starting to think this is going to happen pretty often.  Do you think it's
worth a helper function, something like CocoaTest::SetupView()?  ["addSubview"
is pretty literal, and it might do other things beyond just adding the view to
the window.  Such as making sure the view is going to be visible on the window.]

http://codereview.chromium.org/333017/diff/3001/3006#newcode24
Line 24: TEST_VIEW(BookmarkBubbleViewTest, view_);
I seem to recall the other occurance of this wasn't with the semi-colon.  The
semi-colon is probably the right thing to have, so make sure that your macro is
setup so the semi-colon works-works, and doesn't just work accidentally when all
the previous code in the file is well-formed.

http://codereview.chromium.org/333017/diff/3001/3012
File chrome/browser/cocoa/bookmark_bubble_window.mm (right):

http://codereview.chromium.org/333017/diff/3001/3012#newcode16
Line 16: defer:flag])) {
Win!

http://codereview.chromium.org/333017/diff/3001/3007
File chrome/browser/cocoa/bookmark_bubble_window_unittest.mm (right):

http://codereview.chromium.org/333017/diff/3001/3007#newcode18
Line 18: defer:NO];
I think indentation needs work, here.

http://codereview.chromium.org/333017/diff/3001/3007#newcode24
Line 24: [window_ close];
Since nobody else has the reference, should it be scoped_nsobject<>, and this
line like:
   [window_.release() close];
to handle the retains?

Evil thought ... scoped_nswindow<>, scoped_nswindowcontroller<>,
scoped_nsviewcontroller<> :-).

http://codereview.chromium.org/333017/diff/3001/3009
File chrome/browser/cocoa/browser_window_controller.h (right):

http://codereview.chromium.org/333017/diff/3001/3009#newcode73
Line 73: BookmarkBubbleController* bookmarkBubbleController_;
For these cases, another option would be to continue to use the
scoped_nsobject<>, but to call .reset() in the appropriate place.

Regardless, with all of these if you're going to make them bare pointers, then
you should NULL or nil them out as appropriate after you release or delete them.
 And, just to be safe, you might also want to throw a DCHECK in -dealloc to make
sure we aren't getting there inappropriately.

http://codereview.chromium.org/333017/diff/3001/3004
File chrome/browser/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/333017/diff/3001/3004#newcode245
Line 245: // to the main event loop.
This is the kind of comment I want in the other place!

http://codereview.chromium.org/333017/diff/3001/3004#newcode346
Line 346: ([NSApp keyWindow] == [self window]))
Can probably make this one line, now.  And drop the excess ().

http://codereview.chromium.org/333017/diff/3001/3004#newcode918
Line 918: // Retain the contentView while we remove it from its superview.
space before the comment.

http://codereview.chromium.org/333017/diff/3001/3004#newcode920
Line 920: // Disable autoresizing of subviews while we move views around.  This
Here too.

http://codereview.chromium.org/333017/diff/3001/3004#newcode932
Line 932: dstWindow = [saved_regular_window_ autorelease];
Could this be a -release?  If so, you could make saved_regular_window_ a
scoped_nsobject, and here dstWindow = saved_regular_window_.release().

[Yes, consistency is the hobgoblin of small minds ... working with what I've
got, here!]

http://codereview.chromium.org/333017/diff/3001/3004#newcode935
Line 935: // With this call, valgrind yells at me about "Conditional jump or
You get the picture on the comments.

I realize the previous code had things run together.  Great, we can win double!

http://codereview.chromium.org/333017/diff/3001/3004#newcode1074
Line 1074: [bookmarkBubbleWindow makeKeyAndOrderFront:self];
This, or [controller_ showWindow:self]?  I know they're the same thing in this
case, just thinking of being consistent WRT NSWindowController usage.  And then
you can drop the line snagging the window.

http://codereview.chromium.org/333017/diff/3001/3004#newcode1268
Line 1268: }
What happens when you're running fullscreen and use Expose?

http://codereview.chromium.org/333017/diff/3001/3003
File chrome/browser/cocoa/browser_window_controller_unittest.mm (right):

http://codereview.chromium.org/333017/diff/3001/3003#newcode74
Line 74: takeOwnership:NO];
4 spaces indent, then line up colon.

http://codereview.chromium.org/333017/diff/3001/3003#newcode121
Line 121: TEST_F(BrowserWindowControllerTest, TestFullscreen) {
This test and the next are almost entirely concerned with a controller which is
not THE controller_ but is some other similar controller.  Maybe these are
actually a separate set of tests masquerading as BrowserWindowControllerTests?

http://codereview.chromium.org/333017/diff/3001/3003#newcode126
Line 126: takeOwnership:NO];
+1 on removing scoped_nsobject, but need to retain the relative indent.

http://codereview.chromium.org/333017/diff/3001/3003#newcode175
Line 175: EXPECT_TRUE([controller isKindOfClass:[BrowserWindowController
class]]);
Probably ASSERT_TRUE here.  Though then you'll miss the -close.  But if it
doesn't respond to -isNormalWindow you'll crash in the next line anyhow.

http://codereview.chromium.org/333017/diff/3001/3005
File chrome/browser/cocoa/bubble_view_unittest.mm (right):

http://codereview.chromium.org/333017/diff/3001/3005#newcode16
Line 16: themeProvider:test_window()]);
indent init line 4, next line lines up with colon.

http://codereview.chromium.org/333017/diff/3001/3005#newcode31
Line 31: themeProvider:nil]);
As long as we have two lines, entire [] can be on the second.

[John Grabowski, 2009-10-26 04:05:53]

Fullscreen has been a bit fragile.  Please play with it a bit before landing
this change.

http://codereview.chromium.org/333017/diff/3001/3010
File chrome/browser/cocoa/bookmark_bubble_controller.h (right):

http://codereview.chromium.org/333017/diff/3001/3010#newcode25
Line 25: @interface BookmarkBubbleController :
NSWindowController<NSWindowDelegate> {
I didn't see a way in IB to create a borderless window in a nib.  Apparently
there is.  What did I miss?

http://codereview.chromium.org/333017/diff/3001/3011
File chrome/browser/cocoa/bookmark_bubble_controller.mm (right):

http://codereview.chromium.org/333017/diff/3001/3011#newcode61
Line 61: [NSObject cancelPreviousPerformRequestsWithTarget:self
Seems awkward.  Why is this needed?

http://codereview.chromium.org/333017/diff/3001/3012
File chrome/browser/cocoa/bookmark_bubble_window.mm (right):

http://codereview.chromium.org/333017/diff/3001/3012#newcode14
Line 14: styleMask:NSBorderlessWindowMask
Ah... here's the sneaky way of forcing borderless from a nib.

http://codereview.chromium.org/333017/diff/3001/3004
File chrome/browser/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/333017/diff/3001/3004#newcode115
Line 115: NSWindow *window = [self window];
A* b

http://codereview.chromium.org/333017/diff/3001/3004#newcode653
Line 653: NSWindow *window = [self window];
A* b

[Scott Hess - ex-Googler, 2009-10-26 16:49:31]

BTW - does this fix ISsue 25054?  If so, please edit the BUG=.

[dmac, 2009-10-26 18:46:25]

http://codereview.chromium.org/333017/diff/3001/3010
File chrome/browser/cocoa/bookmark_bubble_controller.h (right):

http://codereview.chromium.org/333017/diff/3001/3010#newcode25
Line 25: @interface BookmarkBubbleController :
NSWindowController<NSWindowDelegate> {
On 2009/10/26 04:05:53, John Grabowski wrote:
> I didn't see a way in IB to create a borderless window in a nib.  Apparently
> there is.  What did I miss?

Custom window class

http://codereview.chromium.org/333017/diff/3001/3010#newcode61
Line 61: - (IBAction)close:(id)sender;
On 2009/10/24 00:30:28, Mark Mentovai wrote:
> I find it a little confusing that you've got close (from NSWindowController)
and
> also close:.

Changed to ok:

http://codereview.chromium.org/333017/diff/3001/3011
File chrome/browser/cocoa/bookmark_bubble_controller.mm (right):

http://codereview.chromium.org/333017/diff/3001/3011#newcode22
Line 22: NSString* nibpath = [mac_util::MainAppBundle()
On 2009/10/24 00:30:28, Mark Mentovai wrote:
> nibPath?

Done.

http://codereview.chromium.org/333017/diff/3001/3011#newcode24
Line 24: ofType:@"nib"];
On 2009/10/24 01:13:44, shess wrote:
> Formatting.  I think probably 4 additional spaces on path, line up :.

Done.

http://codereview.chromium.org/333017/diff/3001/3011#newcode26
Line 26: // All these are weak...
On 2009/10/24 00:30:28, Mark Mentovai wrote:
> This is really a comment that belongs in the header.

Done.

http://codereview.chromium.org/333017/diff/3001/3011#newcode63
Line 63: object:self];
On 2009/10/24 01:13:44, shess wrote:
> Um... what?  I bet the reason is good, too!

No longer needed. Was required as a workaround for closing until I cleaned it
up. Good catch.

http://codereview.chromium.org/333017/diff/3001/3011#newcode118
Line 118: DCHECK([notification object] == [self window]);
On 2009/10/24 00:30:28, Mark Mentovai wrote:
> DCHECK_EQ

Done.

http://codereview.chromium.org/333017/diff/3001/3011#newcode119
Line 119: [self performSelector:@selector(close:) withObject:self afterDelay:0];
On 2009/10/24 01:13:44, shess wrote:
> I bet this connects to the +cancelPrevious* call.  Why?  Presumably this is
like
> a homegrown -autorelease.  But without the comments, I won't know what's up
six
> months down the road.

Done.

http://codereview.chromium.org/333017/diff/3001/3006
File chrome/browser/cocoa/bookmark_bubble_view_unittest.mm (right):

http://codereview.chromium.org/333017/diff/3001/3006#newcode16
Line 16: initWithFrame:frame]);
On 2009/10/24 01:13:44, shess wrote:
> If this is going to be two lines anyhow, I'd prefer everything between [] to
be
> together on one of them.

Done.

http://codereview.chromium.org/333017/diff/3001/3006#newcode24
Line 24: TEST_VIEW(BookmarkBubbleViewTest, view_);
On 2009/10/24 01:13:44, shess wrote:
> I seem to recall the other occurance of this wasn't with the semi-colon.  The
> semi-colon is probably the right thing to have, so make sure that your macro
is
> setup so the semi-colon works-works, and doesn't just work accidentally when
all
> the previous code in the file is well-formed.

It does work with the semi-colon (and without). I'll fix up the other location.

http://codereview.chromium.org/333017/diff/3001/3012
File chrome/browser/cocoa/bookmark_bubble_window.mm (right):

http://codereview.chromium.org/333017/diff/3001/3012#newcode14
Line 14: styleMask:NSBorderlessWindowMask
On 2009/10/26 04:05:53, John Grabowski wrote:
> Ah... here's the sneaky way of forcing borderless from a nib.
> 

One man's sneak is another man's style ;-)

http://codereview.chromium.org/333017/diff/3001/3012#newcode16
Line 16: defer:flag])) {
On 2009/10/24 01:13:44, shess wrote:
> Win!

Done.

http://codereview.chromium.org/333017/diff/3001/3007
File chrome/browser/cocoa/bookmark_bubble_window_unittest.mm (right):

http://codereview.chromium.org/333017/diff/3001/3007#newcode13
Line 13: BookmarkBubbleWindow* window_ =
On 2009/10/24 00:30:28, Mark Mentovai wrote:
> I should not be named with a trailing underscore!
> 
> (This confused the hell out of me.  "Wait a minute, that member should be
> private.  It IS!  Wha?!")

Sorry... it was a member of the class previously. Fixed.

http://codereview.chromium.org/333017/diff/3001/3007#newcode16
Line 16: styleMask:NSBorderlessWindowMask
On 2009/10/24 00:30:28, Mark Mentovai wrote:
> I'd have lined up the colons, but whatever you like...
> 
> Actually, it looks like initContentsWithRect: would fit on the same line as
the
> alloc and then you could still line up the colons.

Done.

http://codereview.chromium.org/333017/diff/3001/3007#newcode18
Line 18: defer:NO];
On 2009/10/24 01:13:44, shess wrote:
> I think indentation needs work, here.

Done.

http://codereview.chromium.org/333017/diff/3001/3007#newcode24
Line 24: [window_ close];
On 2009/10/24 01:13:44, shess wrote:
> Since nobody else has the reference, should it be scoped_nsobject<>, and this
> line like:
>    [window_.release() close];
> to handle the retains?
> 
> Evil thought ... scoped_nswindow<>, scoped_nswindowcontroller<>,
> scoped_nsviewcontroller<> :-).

I thought about this and didn't see the point of the scoped_nsobject if we were
calling release on it explicitly at the end of the method anyways. In general we
would never retain a window, we would let the window controller do it, so this
is a contrived unit test case. Hence the reason I didn't create scoped_nswindow,
and scoped_nswindowcontroller. scoped_nsviewcontroller is a different matter,
and I'm considering that one.

http://codereview.chromium.org/333017/diff/3001/3009
File chrome/browser/cocoa/browser_window_controller.h (right):

http://codereview.chromium.org/333017/diff/3001/3009#newcode55
Line 55: NSWindow *saved_regular_window_;
On 2009/10/24 00:30:28, Mark Mentovai wrote:
> Style nit: put your *s on the type or the variable, but be consistent (and do
> whatever the local code does).  You put it on the variable here, but on the
type
> for statusBubble_ and bookmarkBubbleController_.
> 
> Then again, I see you flip-flopping back and forth between* this and *this a
> bunch.  Maybe you're just trying to match already-inconsistent style around
you?
>  I dunno.  I only bring it up because it's jarring to me during a review, and
I
> start to wonder how well something was planned if it bounces back and forth
> style-wise.

No, it's just a style I'm not used to and as I flip flop between QSB and Chrome
I screw it up. Having the * attached to the type seems just silly to me.

What is "b" in char* a, b; How do I make it a pointer and follow the rules?

char *a, *b seems much cleaner... but back to the review.

Fixed.

> Also, for this member: giveMeAnObjCStyleName_ and_not_one_of_these_.

Done.

http://codereview.chromium.org/333017/diff/3001/3009#newcode66
Line 66: // Strong. StatusBubble is a special case of a strong reference that
On 2009/10/24 00:30:28, Mark Mentovai wrote:
> Blank line before so that it's clear that it applies to statusBubble_ and it
> doesn't run together with all of the junk above.  Same for the comment
attached
> to bookmarkBubbleController_.  This is something else I've noticed a couple
> other times in this change.  I think it improves readability when your eyes
can
> jump to the top of a comment easily and know whether it's attached to the
thing
> before or the thing after when horizontal whitespace is used judiciously.

Done.

http://codereview.chromium.org/333017/diff/3001/3009#newcode73
Line 73: BookmarkBubbleController* bookmarkBubbleController_;
On 2009/10/24 01:13:44, shess wrote:
> For these cases, another option would be to continue to use the
> scoped_nsobject<>, but to call .reset() in the appropriate place.
> 
> Regardless, with all of these if you're going to make them bare pointers, then
> you should NULL or nil them out as appropriate after you release or delete
them.
>  And, just to be safe, you might also want to throw a DCHECK in -dealloc to
make
> sure we aren't getting there inappropriately.

Unfortunately I don't want to call reset on them. I want to call [close] on
them. Also if we are going to be explicitly calling "reset" on an
scoped_nsobject is there any point in wrapping it in the scoped_nsobject in the
first place?

Good call on nilling things out though.

http://codereview.chromium.org/333017/diff/3001/3004
File chrome/browser/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/333017/diff/3001/3004#newcode115
Line 115: NSWindow *window = [self window];
On 2009/10/26 04:05:53, John Grabowski wrote:
> A* b
> 

Done.

http://codereview.chromium.org/333017/diff/3001/3004#newcode245
Line 245: // to the main event loop.
On 2009/10/24 01:13:44, shess wrote:
> This is the kind of comment I want in the other place!

Done.

http://codereview.chromium.org/333017/diff/3001/3004#newcode256
Line 256: DCHECK([notification object] == [self window]);
On 2009/10/24 00:30:28, Mark Mentovai wrote:
> DCHECK_EQ

Done.

http://codereview.chromium.org/333017/diff/3001/3004#newcode262
Line 262: delete statusBubble_;
On 2009/10/24 00:30:28, Mark Mentovai wrote:
> Set it to NULL here too so that you don't keep a bad pointer from now until
you
> dealloc?

Done.

http://codereview.chromium.org/333017/diff/3001/3004#newcode346
Line 346: ([NSApp keyWindow] == [self window]))
On 2009/10/24 01:13:44, shess wrote:
> Can probably make this one line, now.  And drop the excess ().

Made it one line, but left the () just to be clear as to order of ops.

http://codereview.chromium.org/333017/diff/3001/3004#newcode420
Line 420: [[self window] makeKeyAndOrderFront:NSApp];
On 2009/10/24 00:30:28, Mark Mentovai wrote:
> NSApp, really?

Didn't hurt, but self works just as well ;-)

http://codereview.chromium.org/333017/diff/3001/3004#newcode653
Line 653: NSWindow *window = [self window];
On 2009/10/26 04:05:53, John Grabowski wrote:
> A* b
> 

Done.

http://codereview.chromium.org/333017/diff/3001/3004#newcode918
Line 918: // Retain the contentView while we remove it from its superview.
On 2009/10/24 01:13:44, shess wrote:
> space before the comment.

Done.

http://codereview.chromium.org/333017/diff/3001/3004#newcode920
Line 920: // Disable autoresizing of subviews while we move views around.  This
On 2009/10/24 01:13:44, shess wrote:
> Here too.

Done.

http://codereview.chromium.org/333017/diff/3001/3004#newcode932
Line 932: dstWindow = [saved_regular_window_ autorelease];
On 2009/10/24 01:13:44, shess wrote:
> Could this be a -release?  If so, you could make saved_regular_window_ a
> scoped_nsobject, and here dstWindow = saved_regular_window_.release().
> 
> [Yes, consistency is the hobgoblin of small minds ... working with what I've
> got, here!]

Can't release it until I'm done with it at the end of this method. If I released
it here we may lose it before I need it on line 944+

http://codereview.chromium.org/333017/diff/3001/3004#newcode935
Line 935: // With this call, valgrind yells at me about "Conditional jump or
On 2009/10/24 01:13:44, shess wrote:
> You get the picture on the comments.
> 
> I realize the previous code had things run together.  Great, we can win
double!

Done.

http://codereview.chromium.org/333017/diff/3001/3004#newcode1066
Line 1066: node:node
On 2009/10/24 00:30:28, Mark Mentovai wrote:
> align colon

Done.

http://codereview.chromium.org/333017/diff/3001/3004#newcode1074
Line 1074: [bookmarkBubbleWindow makeKeyAndOrderFront:self];
On 2009/10/24 01:13:44, shess wrote:
> This, or [controller_ showWindow:self]?  I know they're the same thing in this
> case, just thinking of being consistent WRT NSWindowController usage.  And
then
> you can drop the line snagging the window.

Done.

http://codereview.chromium.org/333017/diff/3001/3004#newcode1268
Line 1268: }
On 2009/10/24 01:13:44, shess wrote:
> What happens when you're running fullscreen and use Expose?

Works fine.

http://codereview.chromium.org/333017/diff/3001/3003
File chrome/browser/cocoa/browser_window_controller_unittest.mm (right):

http://codereview.chromium.org/333017/diff/3001/3003#newcode74
Line 74: takeOwnership:NO];
On 2009/10/24 01:13:44, shess wrote:
> 4 spaces indent, then line up colon.

Done.

http://codereview.chromium.org/333017/diff/3001/3003#newcode121
Line 121: TEST_F(BrowserWindowControllerTest, TestFullscreen) {
On 2009/10/24 01:13:44, shess wrote:
> This test and the next are almost entirely concerned with a controller which
is
> not THE controller_ but is some other similar controller.  Maybe these are
> actually a separate set of tests masquerading as BrowserWindowControllerTests?

Done.

http://codereview.chromium.org/333017/diff/3001/3003#newcode126
Line 126: takeOwnership:NO];
On 2009/10/24 01:13:44, shess wrote:
> +1 on removing scoped_nsobject, but need to retain the relative indent.

Changed

http://codereview.chromium.org/333017/diff/3001/3003#newcode175
Line 175: EXPECT_TRUE([controller isKindOfClass:[BrowserWindowController
class]]);
On 2009/10/24 01:13:44, shess wrote:
> Probably ASSERT_TRUE here.  Though then you'll miss the -close.  But if it
> doesn't respond to -isNormalWindow you'll crash in the next line anyhow.

Done.

http://codereview.chromium.org/333017/diff/3001/3005
File chrome/browser/cocoa/bubble_view_unittest.mm (right):

http://codereview.chromium.org/333017/diff/3001/3005#newcode16
Line 16: themeProvider:test_window()]);
On 2009/10/24 01:13:44, shess wrote:
> indent init line 4, next line lines up with colon.

Done.

http://codereview.chromium.org/333017/diff/3001/3005#newcode25
Line 25: TEST_VIEW(BubbleViewTest, view_)
On 2009/10/24 00:30:28, Mark Mentovai wrote:
> Hooray!

Semi-colon added.

http://codereview.chromium.org/333017/diff/3001/3005#newcode31
Line 31: themeProvider:nil]);
On 2009/10/24 01:13:44, shess wrote:
> As long as we have two lines, entire [] can be on the second.

Done.

[Scott Hess - ex-Googler, 2009-10-26 19:45:21]

I'm starting to feel past my depth, so ... LGTM!

The one pertinent comment is the last one, about whether you're turning
autoresize back on too early.  I don't know, but from the previous code to the
new code, it's not entirely clear that it's still doing what the comment
suggests it's supposed to do.

http://codereview.chromium.org/333017/diff/3001/3009
File chrome/browser/cocoa/browser_window_controller.h (right):

http://codereview.chromium.org/333017/diff/3001/3009#newcode73
Line 73: BookmarkBubbleController* bookmarkBubbleController_;
On 2009/10/26 18:46:25, dmac wrote:
> On 2009/10/24 01:13:44, shess wrote:
> > For these cases, another option would be to continue to use the
> > scoped_nsobject<>, but to call .reset() in the appropriate place.
> > 
> > Regardless, with all of these if you're going to make them bare pointers,
then
> > you should NULL or nil them out as appropriate after you release or delete
> them.
> >  And, just to be safe, you might also want to throw a DCHECK in -dealloc to
> make
> > sure we aren't getting there inappropriately.
> 
> Unfortunately I don't want to call reset on them. I want to call [close] on
> them. Also if we are going to be explicitly calling "reset" on an
> scoped_nsobject is there any point in wrapping it in the scoped_nsobject in
the
> first place?

Sorry, didn't mean to imply .reset() to clean them up.  Rather meant to imply
that manually arranging things doesn't mean you can't use scoped_*, just that
you have an option.  Saying [object.release() close]; handles making certain
that the object has a valid value all along, and it sometimes nice.

Guess I'm just debating our pattern going forward :-).

> Good call on nilling things out though.

I love clearing variables.  I for the longest time was upset that -release
didn't return nil.  Darn them!

http://codereview.chromium.org/333017/diff/3001/3004
File chrome/browser/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/333017/diff/3001/3004#newcode932
Line 932: dstWindow = [saved_regular_window_ autorelease];
On 2009/10/26 18:46:25, dmac wrote:
> On 2009/10/24 01:13:44, shess wrote:
> > Could this be a -release?  If so, you could make saved_regular_window_ a
> > scoped_nsobject, and here dstWindow = saved_regular_window_.release().
> > 
> > [Yes, consistency is the hobgoblin of small minds ... working with what I've
> > got, here!]
> 
> Can't release it until I'm done with it at the end of this method. If I
released
> it here we may lose it before I need it on line 944+

Darn, my comment doesn't make sense to me, now.  I think I was thinking in terms
of stealing the owning reference using .release().  Having to think it through
indicates it was probably a bad idea.

http://codereview.chromium.org/333017/diff/3001/3004#newcode1268
Line 1268: }
On 2009/10/26 18:46:25, dmac wrote:
> On 2009/10/24 01:13:44, shess wrote:
> > What happens when you're running fullscreen and use Expose?
> 
> Works fine.

Aha - because we aren't using a weirdo window in that case.  OK.

http://codereview.chromium.org/333017/diff/8016/8025
File chrome/browser/cocoa/bookmark_bubble_controller.h (right):

http://codereview.chromium.org/333017/diff/8016/8025#newcode29
Line 29: NSPoint topLeftForBubble_;  // weak
Probably don't need weak on NSPoint.

http://codereview.chromium.org/333017/diff/8016/8026
File chrome/browser/cocoa/bookmark_bubble_controller.mm (right):

http://codereview.chromium.org/333017/diff/8016/8026#newcode89
Line 89: [self ok:sender];
Having -cancel: call -close: made reasonable sense, but having it call -ok:
seems misleading.  If you click the star, edit on the bookmark, then hit ESC,
does that undo your editing?

http://codereview.chromium.org/333017/diff/8016/8026#newcode97
Line 97: [self ok:sender];
Same here, -ok: feels odd.

http://codereview.chromium.org/333017/diff/8016/8019
File chrome/browser/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/333017/diff/8016/8019#newcode942
Line 942: // Cocoa calls.  Stack added to suppressions_mac.txt.
Just checking - does this comment now apply to -setAutoresizesSubviews:?  Before
it was on -makeKeyAndOrderFront:.

http://codereview.chromium.org/333017/diff/8016/8019#newcode943
Line 943: [content setAutoresizesSubviews:YES];
Previously this line was way down the list.  The comment at disable makes it
sounds like some of these methods caused those extra resizes ... maybe
-adjustUIForFullScreen: could cause resizing?