Convert more members to scoped pointers. Move status bubble into browser window controller. Move extra window retain into BWC because it's needed for things in there, not the BrowserWindow impl. Be explicit about the ordering and nature of the ownership in BWC, which should be better since it's now all in one place. Clean up a lot of un-used cruft in tab strip and tab contents now that the toolbaris no longer there, preparing for the BookmarkBar to leave as well.

Review URL: http://codereview.chromium.org/66047

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@13589 0039d316-1c4b-4281-b951-d872f2087c98

11 files changed, 60 insertions, 72 deletions

diff --git a/chrome/browser/cocoa/browser_window_cocoa.h b/chrome/browser/cocoa/browser_window_cocoa.h
index 92b59b5..b1c5cc0 100644
--- a/chrome/browser/cocoa/browser_window_cocoa.h
+++ b/chrome/browser/cocoa/browser_window_cocoa.h
@@ -5,8 +5,6 @@
 #ifndef CHROME_BROWSER_COCOA_BROWSER_WINDOW_COCOA_H_
 #define CHROME_BROWSER_COCOA_BROWSER_WINDOW_COCOA_H_
 
-#include "base/scoped_ptr.h"
-#include "base/scoped_nsobject.h"
 #include "chrome/browser/browser_window.h"
 #include "chrome/browser/bookmarks/bookmark_model.h"
 
@@ -15,8 +13,6 @@ class Browser;
 @class NSWindow;
 @class NSMenu;
 
-class StatusBubbleMac;
-
 // An implementation of BrowserWindow for Cocoa. Bridges between C++ and
 // the Cocoa NSWindow. Cross-platform code will interact with this object when
 // it needs to manipulate the window.
@@ -74,15 +70,9 @@ class BrowserWindowCocoa : public BrowserWindow {
   virtual void DestroyBrowser();
 
  private:
-  // We hold a strong ref to the window so that it will live after the
-  // NSWindowController (the BWC) has run its dealloc. We won't do anything
-  // with it, just make sure it survives until C++ teardown (where we get
-  // destroyed).
-  scoped_nsobject<NSWindow> window_;
+  NSWindow* window_;  // weak, owned by controller
   Browser* browser_;  // weak, owned by controller
   BrowserWindowController* controller_;  // weak, owns us
-  // The status bubble manager.  Always non-NULL.
-  scoped_ptr<StatusBubbleMac> status_bubble_;
 };
 
 #endif  // CHROME_BROWSER_COCOA_BROWSER_WINDOW_COCOA_H_
diff --git a/chrome/browser/cocoa/browser_window_cocoa.mm b/chrome/browser/cocoa/browser_window_cocoa.mm
index 3b7f129..a2b2a9c 100644
--- a/chrome/browser/cocoa/browser_window_cocoa.mm
+++ b/chrome/browser/cocoa/browser_window_cocoa.mm
@@ -7,13 +7,11 @@
 #include "chrome/browser/cocoa/browser_window_cocoa.h"
 #include "chrome/browser/cocoa/browser_window_controller.h"
 #include "chrome/browser/browser.h"
-#include "chrome/browser/cocoa/status_bubble_mac.h"
 
 BrowserWindowCocoa::BrowserWindowCocoa(Browser* browser,
                                        BrowserWindowController* controller,
                                        NSWindow* window)
-  : window_([window retain]), browser_(browser), controller_(controller) {
-  status_bubble_.reset(new StatusBubbleMac(window_));
+  : window_(window), browser_(browser), controller_(controller) {
 }
 
 BrowserWindowCocoa::~BrowserWindowCocoa() {
@@ -64,7 +62,7 @@ BrowserWindowTesting* BrowserWindowCocoa::GetBrowserWindowTesting() {
 }
 
 StatusBubble* BrowserWindowCocoa::GetStatusBubble() {
-  return status_bubble_.get();
+  return [controller_ statusBubble];
 }
 
 void BrowserWindowCocoa::SelectedTabToolbarSizeChanged(bool is_animating) {
diff --git a/chrome/browser/cocoa/browser_window_controller.h b/chrome/browser/cocoa/browser_window_controller.h
index e795273..aab0345 100644
--- a/chrome/browser/cocoa/browser_window_controller.h
+++ b/chrome/browser/cocoa/browser_window_controller.h
@@ -15,12 +15,12 @@
 #include "base/scoped_nsobject.h"
 #include "base/scoped_ptr.h"
 #import "chrome/browser/cocoa/tab_window_controller.h"
-#import "chrome/browser/cocoa/toolbar_view.h"
 
 class Browser;
 class BrowserWindow;
 class BrowserWindowCocoa;
 class LocationBar;
+class StatusBubble;
 class TabContents;
 @class TabContentsController;
 @class TabStripController;
@@ -31,27 +31,40 @@ class TabStripModelObserverBridge;
 @interface BrowserWindowController :
     TabWindowController<NSUserInterfaceValidations> {
  @private
+  // The ordering of these members is important as it determines the order in
+  // which they are destroyed. |browser_| needs to be destroyed last as most of
+  // the other objects hold weak references to it or things it owns
+  // (tab/toolbar/bookmark models, profiles, etc). We hold a strong ref to the
+  // window so that it will live after the NSWindowController dealloc has run
+  // (which happens *before* these scoped pointers are torn down). Keeping it
+  // alive ensures that weak view or window pointers remain valid through
+  // their destruction sequence.
   scoped_ptr<Browser> browser_;
+  scoped_nsobject<NSWindow> window_;
   scoped_ptr<TabStripModelObserverBridge> tabObserver_;
   scoped_ptr<BrowserWindowCocoa> windowShim_;
   scoped_nsobject<ToolbarController> toolbarController_;
   scoped_nsobject<TabStripController> tabStripController_;
+  scoped_ptr<StatusBubble> statusBubble_;
 }
 
 // Load the browser window nib and do any Cocoa-specific initialization.
 // Takes ownership of |browser|.
 - (id)initWithBrowser:(Browser*)browser;
 
-// call to make the browser go away from other places in the cross-platform
+// Call to make the browser go away from other places in the cross-platform
 // code.
 - (void)destroyBrowser;
 
-// Access the C++ bridge between the NSWindow and the rest of Chromium
+// Access the C++ bridge between the NSWindow and the rest of Chromium.
 - (BrowserWindow*)browserWindow;
 
-// Get the C++ bridge object representing the location bar for the current tab.
+// Access the C++ bridge object representing the location bar.
 - (LocationBar*)locationBar;
 
+// Access the C++ bridge object representing the status bubble for the window.
+- (StatusBubble*)statusBubble;
+
 // Updates the toolbar (and transitively the location bar) with the states of
 // the specified |tab|.  If |shouldRestore| is true, we're switching
 // (back?) to this tab and should restore any previous location bar state
diff --git a/chrome/browser/cocoa/browser_window_controller.mm b/chrome/browser/cocoa/browser_window_controller.mm
index f076e26..b092e07 100644
--- a/chrome/browser/cocoa/browser_window_controller.mm
+++ b/chrome/browser/cocoa/browser_window_controller.mm
@@ -10,6 +10,7 @@
 #include "chrome/browser/tabs/tab_strip_model.h"
 #import "chrome/browser/cocoa/browser_window_cocoa.h"
 #import "chrome/browser/cocoa/browser_window_controller.h"
+#import "chrome/browser/cocoa/status_bubble_mac.h"
 #import "chrome/browser/cocoa/tab_strip_model_observer_bridge.h"
 #import "chrome/browser/cocoa/tab_strip_view.h"
 #import "chrome/browser/cocoa/tab_strip_controller.h"
@@ -36,6 +37,8 @@
     // The window is now fully realized and |-windowDidLoad:| has been
     // called. We shouldn't do much in wDL because |windowShim_| won't yet
     // be initialized (as it's called in response to |[self window]| above).
+    // Retain it per the comment in the header.
+    window_.reset([[self window] retain]);
 
     // Get the most appropriate size for the window. The window shim will handle
     // flipping the coordinates for us so we can use it to save some code.
@@ -58,6 +61,9 @@
                                initWithModel:browser->toolbar_model()
                                     commands:browser->command_updater()]);
     [self positionToolbar];
+
+    // Create the bridge for the status bubble.
+    statusBubble_.reset(new StatusBubbleMac([self window]));
   }
   return self;
 }
@@ -207,6 +213,10 @@
   return [toolbarController_ locationBar];
 }
 
+- (StatusBubble*)statusBubble {
+  return statusBubble_.get();
+}
+
 - (void)updateToolbarWithContents:(TabContents*)tab
                shouldRestoreState:(BOOL)shouldRestore {
   [toolbarController_ updateToolbarWithContents:shouldRestore ? tab : NULL];
diff --git a/chrome/browser/cocoa/tab_contents_controller.h b/chrome/browser/cocoa/tab_contents_controller.h
index ac8bd30..f4098ed 100644
--- a/chrome/browser/cocoa/tab_contents_controller.h
+++ b/chrome/browser/cocoa/tab_contents_controller.h
@@ -16,19 +16,8 @@ class TabContentsCommandObserver;
 class TabStripModel;
 @class ToolbarView;
 
-// A class that controls the contents of a tab, including the toolbar and
-// web area.
-
-// TODO(pinkerton): Cole and I went back and forth about whether or not each
-// tab should have its own copy of the toolbar. Right now, we decided to leave
-// it like this as he expects it will make it easier for him to implement
-// tab dragging and tear-off into new windows. It's also not very expensive.
-// As we hook things up, we'll see if this imposes other restrictions (such
-// as command-handling or dispatch) that will require us to change the view
-// layout.
-// TODO(jrg): Following on to pink's comments... each tab does in fact
-// have its own ToolbarView.  Similarly, each also has its own
-// BookmarkView.  That makes things marginally more expensive.
+// A class that controls the web contents of a tab. It manages displaying the
+// native view for a given TabContents in |contentsBox_|.
 
 @interface TabContentsController : NSViewController {
  @private
@@ -49,10 +38,8 @@ class TabStripModel;
 }
 
 // Create the contents of a tab represented by |contents| and loaded from the
-// nib given by |name|. |commands| allows tracking of what's enabled and
-// disabled. It may be nil if no updating is desired.
+// nib given by |name|.
 - (id)initWithNibName:(NSString*)name
-               bundle:(NSBundle*)bundle
              contents:(TabContents*)contents
         bookmarkModel:(BookmarkModel*)bookmarkModel;
 
diff --git a/chrome/browser/cocoa/tab_contents_controller.mm b/chrome/browser/cocoa/tab_contents_controller.mm
index ce00782..c72b97d 100644
--- a/chrome/browser/cocoa/tab_contents_controller.mm
+++ b/chrome/browser/cocoa/tab_contents_controller.mm
@@ -15,10 +15,9 @@
 @implementation TabContentsController
 
 - (id)initWithNibName:(NSString*)name
-               bundle:(NSBundle*)bundle
              contents:(TabContents*)contents
         bookmarkModel:(BookmarkModel*)bookmarkModel {
-  if ((self = [super initWithNibName:name bundle:bundle])) {
+  if ((self = [super initWithNibName:name bundle:nil])) {
     contents_ = contents;
     bookmarkModel_ = bookmarkModel;
   }
diff --git a/chrome/browser/cocoa/tab_strip_controller.h b/chrome/browser/cocoa/tab_strip_controller.h
index ab6b902..dae7218 100644
--- a/chrome/browser/cocoa/tab_strip_controller.h
+++ b/chrome/browser/cocoa/tab_strip_controller.h
@@ -7,6 +7,8 @@
 
 #import <Cocoa/Cocoa.h>
 
+#include "base/scoped_nsobject.h"
+#include "base/scoped_ptr.h"
 #import "chrome/browser/cocoa/tab_controller_target.h"
 
 @class TabStripView;
@@ -14,7 +16,6 @@
 
 class BookmarkModel;
 class Browser;
-class CommandUpdater;
 class LocationBar;
 class TabStripModelObserverBridge;
 class TabStripModel;
@@ -38,21 +39,20 @@ class ToolbarModel;
   TabStripView* tabView_;  // weak
   NSView* switchView_;  // weak
   NSButton* newTabButton_;  // weak, obtained from the nib.
-  TabStripModelObserverBridge* bridge_;
+  scoped_ptr<TabStripModelObserverBridge> bridge_;
   TabStripModel* tabModel_;  // weak
   BookmarkModel* bookmarkModel_;  // weak, one per profile (= one per Browser*)
-  CommandUpdater* commands_;  // weak, may be nil
   // access to the TabContentsControllers (which own the parent view
   // for the toolbar and associated tab contents) given an index. This needs
   // to be kept in the same order as the tab strip's model as we will be
   // using its index from the TabStripModelObserver calls.
-  NSMutableArray* tabContentsArray_;
+  scoped_nsobject<NSMutableArray> tabContentsArray_;
   // an array of TabControllers which manage the actual tab views. As above,
   // this is kept in the same order as the tab strip model.
-  NSMutableArray* tabArray_;
+  scoped_nsobject<NSMutableArray> tabArray_;
 
   // Controller for bookmark bar state, shared among all TabContents.
-  BookmarkBarStateController* bookmarkBarStateController_;
+  scoped_nsobject<BookmarkBarStateController> bookmarkBarStateController_;
 }
 
 // Initialize the controller with a view and browser that contains
diff --git a/chrome/browser/cocoa/tab_strip_controller.mm b/chrome/browser/cocoa/tab_strip_controller.mm
index e4d669a..0fca35f 100644
--- a/chrome/browser/cocoa/tab_strip_controller.mm
+++ b/chrome/browser/cocoa/tab_strip_controller.mm
@@ -28,12 +28,11 @@
     switchView_ = switchView;
     tabModel_ = browser->tabstrip_model();
     bookmarkModel_ = browser->profile()->GetBookmarkModel();
-    commands_ = browser->command_updater();
-    bridge_ = new TabStripModelObserverBridge(tabModel_, self);
-    tabContentsArray_ = [[NSMutableArray alloc] init];
-    tabArray_ = [[NSMutableArray alloc] init];
-    bookmarkBarStateController_ = [[BookmarkBarStateController alloc]
-                                    initWithBrowser:browser];
+    bridge_.reset(new TabStripModelObserverBridge(tabModel_, self));
+    tabContentsArray_.reset([[NSMutableArray alloc] init]);
+    tabArray_.reset([[NSMutableArray alloc] init]);
+    bookmarkBarStateController_.reset([[BookmarkBarStateController alloc]
+                                        initWithBrowser:browser]);
     // Take the only child view present in the nib as the new tab button. For
     // some reason, if the view is present in the nib apriori, it draws
     // correctly. If we create it in code and add it to the tab view, it draws
@@ -49,14 +48,6 @@
   return self;
 }
 
-- (void)dealloc {
-  delete bridge_;
-  [bookmarkBarStateController_ release];
-  [tabContentsArray_ release];
-  [tabArray_ release];
-  [super dealloc];
-}
-
 // Finds the associated TabContentsController at the given |index| and swaps
 // out the sole child of the contentArea to display its contents.
 - (void)swapInTabAtIndex:(NSInteger)index {
@@ -103,7 +94,7 @@
 // Returns the index of the subview |view|. Returns -1 if not present.
 - (NSInteger)indexForTabView:(NSView*)view {
   NSInteger index = 0;
-  for (TabController* current in tabArray_) {
+  for (TabController* current in tabArray_.get()) {
     if ([current view] == view)
       return index;
     ++index;
@@ -162,7 +153,7 @@
               kMaxTabWidth),
           kMinTabWidth);
 
-  for (TabController* tab in tabArray_) {
+  for (TabController* tab in tabArray_.get()) {
     // BOOL isPlaceholder = ![[[tab view] superview] isEqual:tabView_];
     BOOL isPlaceholder = NO;
     NSRect tabFrame = [[tab view] frame];
@@ -221,7 +212,6 @@
   // the new controller with |contents| so it can be looked up later.
   TabContentsController* contentsController =
       [[[TabContentsController alloc] initWithNibName:@"TabContents"
-                                               bundle:nil
                                              contents:contents
                                         bookmarkModel:bookmarkModel_]
           autorelease];
@@ -254,7 +244,7 @@
                   userGesture:(bool)wasUserGesture {
   // De-select all other tabs and select the new tab.
   int i = 0;
-  for (TabController* current in tabArray_) {
+  for (TabController* current in tabArray_.get()) {
     [current setSelected:(i == index) ? YES : NO];
     ++i;
   }
@@ -337,7 +327,7 @@
 - (void)toggleBookmarkBar {
   [bookmarkBarStateController_ toggleBookmarkBar];
   BOOL visible = [self isBookmarkBarVisible];
-  for (TabContentsController *controller in tabContentsArray_) {
+  for (TabContentsController *controller in tabContentsArray_.get()) {
     [controller toggleBookmarkBar:visible];
   }
 }
diff --git a/chrome/browser/cocoa/toolbar_controller.h b/chrome/browser/cocoa/toolbar_controller.h
index d2cc2c4..4388c15 100644
--- a/chrome/browser/cocoa/toolbar_controller.h
+++ b/chrome/browser/cocoa/toolbar_controller.h
@@ -7,6 +7,7 @@
 
 #import <Cocoa/Cocoa.h>
 
+#include "base/scoped_ptr.h"
 #import "chrome/browser/cocoa/command_observer_bridge.h"
 
 class CommandUpdater;
@@ -23,8 +24,8 @@ class ToolbarView;
  @private
   ToolbarModel* toolbarModel_;  // weak, one per window
   CommandUpdater* commands_;  // weak, one per window
-  CommandObserverBridge* commandObserver_;
-  LocationBarViewMac* locationBarView_;
+  scoped_ptr<CommandObserverBridge> commandObserver_;
+  scoped_ptr<LocationBarViewMac> locationBarView_;
 
   IBOutlet NSButton* backButton_;
   IBOutlet NSButton* forwardButton_;
diff --git a/chrome/browser/cocoa/toolbar_controller.mm b/chrome/browser/cocoa/toolbar_controller.mm
index 01a9c21..a44cccd 100644
--- a/chrome/browser/cocoa/toolbar_controller.mm
+++ b/chrome/browser/cocoa/toolbar_controller.mm
@@ -27,7 +27,7 @@ static NSString* const kStarredImageName = @"starred";
     commands_ = commands;
 
     // Register for notifications about state changes for the toolbar buttons
-    commandObserver_ = new CommandObserverBridge(self, commands);
+    commandObserver_.reset(new CommandObserverBridge(self, commands));
     commandObserver_->ObserveCommand(IDC_BACK);
     commandObserver_->ObserveCommand(IDC_FORWARD);
     commandObserver_->ObserveCommand(IDC_RELOAD);
@@ -42,24 +42,22 @@ static NSString* const kStarredImageName = @"starred";
 // bar and button state.
 - (void)awakeFromNib {
   [self initCommandStatus:commands_];
-  locationBarView_ = new LocationBarViewMac(locationBar_, commands_,
-                                            toolbarModel_);
+  locationBarView_.reset(new LocationBarViewMac(locationBar_, commands_,
+                                                toolbarModel_));
   locationBarView_->Init();
   [locationBar_ setStringValue:@"http://dev.chromium.org"];
 }
 
 - (void)dealloc {
-  delete locationBarView_;
-  delete commandObserver_;
   [super dealloc];
 }
 
 - (LocationBar*)locationBar {
-  return locationBarView_;
+  return locationBarView_.get();
 }
 
 - (void)focusLocationBar {
-  if (locationBarView_) {
+  if (locationBarView_.get()) {
     locationBarView_->FocusLocation();
   }
 }
@@ -107,7 +105,7 @@ static NSString* const kStarredImageName = @"starred";
   // pulldown.  It should also be the right thing to do to save and
   // restore state, but at this time it's not clear that this is the
   // right place, and tab is not the right parameter.
-  if (locationBarView_) {
+  if (locationBarView_.get()) {
     locationBarView_->SaveStateToContents(NULL);
   }
 
diff --git a/chrome/browser/status_bubble.h b/chrome/browser/status_bubble.h
index 30aa6db..3f58b9c 100644
--- a/chrome/browser/status_bubble.h
+++ b/chrome/browser/status_bubble.h
@@ -16,6 +16,8 @@ class GURL;
 //
 class StatusBubble {
  public:
+  virtual ~StatusBubble() { };
+
   // Sets the bubble contents to a specific string and causes the bubble
   // to display immediately. Subsequent empty SetURL calls (typically called
   // when the cursor exits a link) will set the status bubble back to its