Implement the downloads in progress at exit dialog for Mac

Implement the quit dialog on Mac displayed when the user has in progress
Downloads.

BUG=15899
TEST=A dialog asking the user if they want to quit when there are downloads
     in progress will be displayed when the user tries to exit under that
     condition.

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

[pink (ping after 24hrs), 2009-07-30 14:42:56]

http://codereview.chromium.org/159424/diff/26/1034
File chrome/browser/app_controller_mac.mm (right):

http://codereview.chromium.org/159424/diff/26/1034#newcode252
Line 252: BrowserList::const_iterator it = BrowserList::begin();
why not have this in the for statement? it's ok if it wraps. otherwise it leaks
|it| outside the for loop scope for no reason.

http://codereview.chromium.org/159424/diff/26/1034#newcode266
Line 266: // progress.
this comment doesn't seem to apply. We're not passing any of the particular
windows down, and the download manager should be the same for all open windows.

http://codereview.chromium.org/159424/diff/26/1027
File chrome/browser/cocoa/download_in_progress_controller.h (right):

http://codereview.chromium.org/159424/diff/26/1027#newcode14
Line 14: // Stores the user's choice to quit or not.
these should be @private

http://codereview.chromium.org/159424/diff/26/1027#newcode26
Line 26: // Use |multiple| to load different strings for the UI.
i don't see |multiple| here anywhere.

http://codereview.chromium.org/159424/diff/26/1027#newcode33
Line 33: - (void)runModalDialog;
comment since it's part of the public api?

http://codereview.chromium.org/159424/diff/26/1027#newcode36
Line 36: @property (readwrite) int downloadCount;
is this really readwrite?

http://codereview.chromium.org/159424/diff/26/1029
File chrome/browser/cocoa/download_in_progress_controller.mm (right):

http://codereview.chromium.org/159424/diff/26/1029#newcode17
Line 17: - (id)initWithDownloadCount:(int)downloadCount {
maybe use unsigned?

http://codereview.chromium.org/159424/diff/26/1029#newcode22
Line 22: shouldQuit_ = NO;
not technically necessary, the runtime does this for you

http://codereview.chromium.org/159424/diff/26/1029#newcode30
Line 30: NSString* titleString;
init to nil just to be safe?

http://codereview.chromium.org/159424/diff/26/1029#newcode35
Line 35: titleString = base::SysWideToNSString(
rather than manually filling in all these strings with code, we should use the
localizer that TVL has written which just makes it happen automatically given
text/constants specified in the xib.

http://codereview.chromium.org/159424/diff/31/1043#newcode19
Line 19: pathForResource:@"DownloadInProgress"
i haven't seen the nib, but can this just be a NSAlert? Does it need its own
dialog? It seems like a text field and ok/cancel buttons no?

[Paul Godavari, 2009-08-01 00:20:06]

[Adding Rohit since Mike is on vacation].

Adressed all feedback from previous review by moving the dialog from a custom
window to using a built in alert panel.

[rohitrao (ping after 24h), 2009-08-03 21:12:40]

LGTM

This looks fine to me.  I don't understand the quit workflow in AppController,
but based on the surrounding comments, it looks like you're putting this check
in the right place.

http://codereview.chromium.org/159424/diff/1078/1080
File chrome/browser/app_controller_mac.mm (right):

http://codereview.chromium.org/159424/diff/1078/1080#newcode267
Line 267: // want to prompt for each one. Use the first browser with downloads
in
Should we be adding up the total number of current browsers across all windows
and displaying that?

http://codereview.chromium.org/159424/diff/1078/1079
File chrome/browser/cocoa/browser_window_cocoa.mm (right):

http://codereview.chromium.org/159424/diff/1078/1079#newcode249
Line 249: browser_->InProgressDownloadResponse(true);
Can you add a comment saying that it's safe to always close here because the
real close/noclose decision is made before this method is even called, in
[AppController quit:]?

[Nico, 2009-08-04 04:26:07]

http://codereview.chromium.org/159424/diff/1082/46
File chrome/browser/app_controller_mac.mm (right):

http://codereview.chromium.org/159424/diff/1082/46#newcode342
Line 342: return;
Isn't this something that should be done in |applicationShouldTerminate:|? I'm
asking because I'm about to add something there, and I'm wondering if there's a
problem with this method in chromiume.