diff options
author | pinkerton@chromium.org <pinkerton@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2009-02-18 21:51:39 +0000 |
---|---|---|
committer | pinkerton@chromium.org <pinkerton@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2009-02-18 21:51:39 +0000 |
commit | 41741a96ce6b7b3cfca3d59ef3bb59064dc939e0 (patch) | |
tree | 04691674fee10a438145d1bc42325c9ea00d2f7a /chrome/browser | |
parent | 4daf03583a367047c06cbc05aace8f9edc355237 (diff) | |
download | chromium_src-41741a96ce6b7b3cfca3d59ef3bb59064dc939e0.zip chromium_src-41741a96ce6b7b3cfca3d59ef3bb59064dc939e0.tar.gz chromium_src-41741a96ce6b7b3cfca3d59ef3bb59064dc939e0.tar.bz2 |
Fix window close and application quit on Mac to call the proper Browser machinery in the proper order. Add comments to BrowserWindow::Close to stress its assumptions. DCHECK that nobody adds a NULL browser to the browser list.
Review URL: http://codereview.chromium.org/20460
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@9973 0039d316-1c4b-4281-b951-d872f2087c98
Diffstat (limited to 'chrome/browser')
-rw-r--r-- | chrome/browser/app_controller_mac.mm | 7 | ||||
-rw-r--r-- | chrome/browser/browser_list.cc | 1 | ||||
-rw-r--r-- | chrome/browser/browser_window.h | 6 | ||||
-rw-r--r-- | chrome/browser/browser_window_cocoa.mm | 5 | ||||
-rw-r--r-- | chrome/browser/browser_window_controller.mm | 48 |
5 files changed, 54 insertions, 13 deletions
diff --git a/chrome/browser/app_controller_mac.mm b/chrome/browser/app_controller_mac.mm index 5a9609a..d7b0a72 100644 --- a/chrome/browser/app_controller_mac.mm +++ b/chrome/browser/app_controller_mac.mm @@ -8,6 +8,7 @@ #import "chrome/app/chrome_dll_resource.h" #import "chrome/browser/browser.h" #import "chrome/browser/browser_list.h" +#include "chrome/browser/browser_shutdown.h" #import "chrome/browser/command_updater.h" #import "chrome/browser/profile_manager.h" #import "chrome/common/temp_scaffolding_stubs.h" @@ -48,6 +49,12 @@ // handle it. If it says to continue, post the quit message, otherwise // go back to normal. + // TODO(pinkerton): Not sure where this should live, including it here + // causes all sorts of asserts from the open renderers. On Windows, it + // lives in Browser::OnWindowClosing, but that's not appropriate on Mac + // since we don't shut down when we reach zero windows. + // browser_shutdown::OnShutdownStarting(browser_shutdown::WINDOW_CLOSE); + // Close all the windows. BrowserList::CloseAllBrowsers(true); diff --git a/chrome/browser/browser_list.cc b/chrome/browser/browser_list.cc index 5769c1b..de85eba0 100644 --- a/chrome/browser/browser_list.cc +++ b/chrome/browser/browser_list.cc @@ -27,6 +27,7 @@ std::vector<BrowserList::Observer*> BrowserList::observers_; // static void BrowserList::AddBrowser(Browser* browser) { + DCHECK(browser); browsers_.push_back(browser); g_browser_process->AddRefModule(); diff --git a/chrome/browser/browser_window.h b/chrome/browser/browser_window.h index 902cbf5..6a1215f 100644 --- a/chrome/browser/browser_window.h +++ b/chrome/browser/browser_window.h @@ -37,7 +37,11 @@ class BrowserWindow { // Closes the frame as soon as possible. If the frame is not in a drag // session, it will close immediately; otherwise, it will move offscreen (so - // events are still fired) until the drag ends, then close. + // events are still fired) until the drag ends, then close. This assumes + // that the Browser is not immediately destroyed, but will be eventually + // destroyed by other means (eg, the tab strip going to zero elements). + // Bad things happen if the Browser dtor is called directly as a result of + // invoking this method. virtual void Close() = 0; // Activates (brings to front) the window. Restores the window from minimized diff --git a/chrome/browser/browser_window_cocoa.mm b/chrome/browser/browser_window_cocoa.mm index 0780734..bbf6735 100644 --- a/chrome/browser/browser_window_cocoa.mm +++ b/chrome/browser/browser_window_cocoa.mm @@ -31,6 +31,9 @@ void BrowserWindowCocoa::SetBounds(const gfx::Rect& bounds) { [screen frame].size.height - bounds.height() - bounds.y(); } +// Callers assume that this doesn't immediately delete the Browser object. +// The controller implementing the window delegate methods called from +// |-performClose:| must take precautiions to ensure that. void BrowserWindowCocoa::Close() { [window_ orderOut:controller_]; [window_ performClose:controller_]; @@ -97,6 +100,8 @@ bool BrowserWindowCocoa::IsFullscreen() const { } gfx::Rect BrowserWindowCocoa::GetRootWindowResizerRect() const { + // TODO(pinkerton): fill this in so scrollbars go in the correct places + NOTIMPLEMENTED(); return gfx::Rect(); } diff --git a/chrome/browser/browser_window_controller.mm b/chrome/browser/browser_window_controller.mm index 101fc9c..282193b 100644 --- a/chrome/browser/browser_window_controller.mm +++ b/chrome/browser/browser_window_controller.mm @@ -24,9 +24,9 @@ - (void)dealloc { browser_->CloseAllTabs(); + [tabStripController_ release]; delete browser_; delete windowShim_; - [tabStripController_ release]; [super dealloc]; } @@ -56,24 +56,48 @@ } - (void)destroyBrowser { - // we need the window to go away now, other areas of code will be checking - // the number of browser objects remaining after we finish so we can't defer - // deletion via autorelease. + // We need the window to go away now. [self autorelease]; } -// Called when the window is closing from Cocoa. Destroy this controller, -// which will tear down the rest of the infrastructure as the Browser is -// itself destroyed. +// Called when the window meets the criteria to be closed (ie, +// |-windowShoudlClose:| returns YES). We must be careful to preserve the +// semantics of BrowserWindow::Close() and not call the Browser's dtor directly +// from this method. - (void)windowWillClose:(NSNotification *)notification { - [self autorelease]; + DCHECK(!browser_->tabstrip_model()->count()); + + // We can't acutally use |-autorelease| here because there's an embedded + // run loop in the |-performClose:| which contains its own autorelease pool. + // Instead we use call it after a zero-length delay, which gets us back + // to the main event loop. + [self performSelector:@selector(autorelease) + withObject:nil + afterDelay:0]; } -// Called when the user wants to close a window. Usually it's ok, but we may -// want to prompt the user when they have multiple tabs open, for example. +// Called when the user wants to close a window or from the shutdown process. +// The Browser object is in control of whether or not we're allowed to close. It +// may defer closing due to several states, such as onUnload handlers needing to +// be fired. If closing is deferred, the Browser will handle the processing +// required to get us to the closing state and (by watching for all the tabs +// going away) will again call to close the window when it's finally ready. - (BOOL)windowShouldClose:(id)sender { - // TODO(pinkerton): check tab model to see if it's ok to close the - // window. Use NSGetAlertPanel() and runModalForWindow:. + // Give beforeunload handlers the chance to cancel the close before we hide + // the window below. + if (!browser_->ShouldCloseWindow()) + return NO; + + if (!browser_->tabstrip_model()->empty()) { + // Tab strip isn't empty. Hide the frame (so it appears to have closed + // immediately) and close all the tabs, allowing the renderers to shut + // down. When the tab strip is empty we'll be called back again. + [[self window] orderOut:self]; + browser_->OnWindowClosing(); + return NO; + } + + // the tab strip is empty, it's ok to close the window return YES; } |