Ask for user confirmation when closing a browser with in-progress downloads

This CL adds a confirmation box when closing the browser with in-progress downloads.

BUG=1028
TEST=Start several bug downloads, close the browser. A dialog shows up to warn you are about to lose the downloads. Try the wait and continue button. Try again with several windows, and in Incognito mode.

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

[sky, 2009-04-08 17:35:45]

http://codereview.chromium.org/62131/diff/1036/18
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/62131/diff/1036/18#newcode1347
Line 1347: You have <ph name="DOWNLOAD_COUNT">$1<ex>3</ex></ph> download(s) in
progress. If you close Google Chrome, uncompleted downloads will be removed.
Do you need to use Chromium here, and have the Chrome strings some where else?

I think I would change removed to canceled, or both, canceled and removed.

http://codereview.chromium.org/62131/diff/1036/20
File chrome/browser/browser.cc (right):

http://codereview.chromium.org/62131/diff/1036/20#newcode450
Line 450: if (BrowserList::GetBrowserCount(profile_) == 1 &&
I think this will incorrectly show the dialog if you have an off the record
window open and close your last non off the record window. Also, should this be
before setting is_attempting_to_close_browser_, or at least unset
is_attempting_to_close_browser_ if you return false?

http://codereview.chromium.org/62131/diff/1036/20#newcode2475
Line 2475: DownloadManager* download_manager = profile_->GetDownloadManager();
Be sure and null check this one.

http://codereview.chromium.org/62131/diff/1036/31
File chrome/browser/cocoa/browser_window_cocoa.h (right):

http://codereview.chromium.org/62131/diff/1036/31#newcode68
Line 68: virtual bool ConfirmBrowserCloseWithPendingDownloads();
nit: spacing is off.

http://codereview.chromium.org/62131/diff/1036/22
File chrome/browser/views/frame/browser_view.cc (right):

http://codereview.chromium.org/62131/diff/1036/22#newcode200
Line 200: DownloadInProgressConfirmDialogDelegate(int download_count)
nit: explicit

http://codereview.chromium.org/62131/diff/1036/22#newcode882
Line 882: 
nit: extra line.

[jcampan, 2009-04-08 22:32:08]

OK, per our discussion, I made the call to confirm the browser closure non
blocking.

http://codereview.chromium.org/62131/diff/1036/18
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/62131/diff/1036/18#newcode1347
Line 1347: You have <ph name="DOWNLOAD_COUNT">$1<ex>3</ex></ph> download(s) in
progress. If you close Google Chrome, uncompleted downloads will be removed.
On 2009/04/08 17:35:45, sky wrote:
> Do you need to use Chromium here, and have the Chrome strings some where else?
> 
> I think I would change removed to canceled, or both, canceled and removed.

It seems we have Google Chrome all over that file, so we should be fine.
Changed the text as you suggested.

http://codereview.chromium.org/62131/diff/1036/20
File chrome/browser/browser.cc (right):

http://codereview.chromium.org/62131/diff/1036/20#newcode450
Line 450: if (BrowserList::GetBrowserCount(profile_) == 1 &&
On 2009/04/08 17:35:45, sky wrote:
> I think this will incorrectly show the dialog if you have an off the record
> window open and close your last non off the record window. Also, should this
be
> before setting is_attempting_to_close_browser_, or at least unset
> is_attempting_to_close_browser_ if you return false?
OK, per our discussion, this has changed quiet a lot. The
PromptForInProgressDownloads is not blocking anymore and has been moved to the
top of the method.

http://codereview.chromium.org/62131/diff/1036/20#newcode2475
Line 2475: DownloadManager* download_manager = profile_->GetDownloadManager();
On 2009/04/08 17:35:45, sky wrote:
> Be sure and null check this one.
Null checking the download manager.
We don't need to check the profile_ as I don't think it can be NULL.

http://codereview.chromium.org/62131/diff/1036/31
File chrome/browser/cocoa/browser_window_cocoa.h (right):

http://codereview.chromium.org/62131/diff/1036/31#newcode68
Line 68: virtual bool ConfirmBrowserCloseWithPendingDownloads();
On 2009/04/08 17:35:45, sky wrote:
> nit: spacing is off.
Oops!

http://codereview.chromium.org/62131/diff/1036/22
File chrome/browser/views/frame/browser_view.cc (right):

http://codereview.chromium.org/62131/diff/1036/22#newcode200
Line 200: DownloadInProgressConfirmDialogDelegate(int download_count)
On 2009/04/08 17:35:45, sky wrote:
> nit: explicit

Done.

http://codereview.chromium.org/62131/diff/1036/22#newcode882
Line 882: 
On 2009/04/08 17:35:45, sky wrote:
> nit: extra line.

Done.

[sky, 2009-04-09 00:07:15]

My only comment is the one mentioned in person. Seems like we should not
consider app windows when counting browsers.

http://codereview.chromium.org/62131/diff/38/42
File chrome/browser/browser.h (right):

http://codereview.chromium.org/62131/diff/38/42#newcode191
Line 191: void CancelInProgressDownloadsResponse(bool cancel_downloads);
nit: this name is a bit misleading, perhaps just InProgressDownloadResponse?

[jcampan, 2009-04-09 07:06:26]

Regarding the app windows, it seems they just behave as a regular browser
window. So I don't think they require special treatment.

http://codereview.chromium.org/62131/diff/38/42
File chrome/browser/browser.h (right):

http://codereview.chromium.org/62131/diff/38/42#newcode191
Line 191: void CancelInProgressDownloadsResponse(bool cancel_downloads);
On 2009/04/09 00:07:16, sky wrote:
> nit: this name is a bit misleading, perhaps just InProgressDownloadResponse?
Good idea, done.

[sky, 2009-04-09 19:46:28]

I was thinking we wanted something more complicated for app windows, but on
second thought it's too confusing. So, LGTM.