Initial download shelf on OS X....

Initial download shelf on OS X.

This has lots of missing stuff (e.g. a custom download item view that shows download progress, the popup is the same for in-progress and completed downloads, no animation, everything looks ugly, the info bubble overlaps the shelf when it's visible, no "open download manager page" link, etc), but the basic functionality is hooked up: The shelf appears when files are downloaded, and something ugly is added to the shelf for each download. The popup's "Reveral in Finder" even works. The shelf is per-window as it should be.

BUG=12500
TEST=Download something and check the shelf appears. Click the close button and make sure it disappears again.

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

[Nico, 2009-04-24 19:19:57]

This is very raw, but it mostly works. I'd like to get this in, to get an idea
of how real reviews look, and to keep me less busy resolving merge conflicts.

[Nico, 2009-04-24 21:42:47]

Assigning thomasvl's favorite reviewers.

[pink (ping after 24hrs), 2009-04-27 17:19:03]

First thing I'd do is check with Glen about how far along things are with one
shelf per window. We really should try to get out in front rather than playing
catch-up.

Barring that re-working, I think you can do a better job getting the
TabContentsController (or some protocol API that it implements) to the shelf.
You shouldn't need to route through TabContents and ifdef things given the
object containment hierarchy.

http://codereview.chromium.org/93129/diff/2005/3010
File chrome/browser/cocoa/download_item_mac.h (right):

http://codereview.chromium.org/93129/diff/2005/3010#newcode17
Line 17: @interface DownloadShelfContextMenuBridge : NSObject {
needs a class-level comment.

http://codereview.chromium.org/93129/diff/2005/3010#newcode17
Line 17: @interface DownloadShelfContextMenuBridge : NSObject {
this seems like an implementation detail. Are you sure it needs to be part of
the public api?

http://codereview.chromium.org/93129/diff/2005/3010#newcode18
Line 18: DownloadShelfContextMenuMac* contextMenu_;  // strong
this should be @private and indented 2 spaces. If it's strong, you should use a
scoped_nsobject or scoped_ptr.

http://codereview.chromium.org/93129/diff/2005/3010#newcode21
Line 21: -
(DownloadShelfContextMenuBridge*)initWithModel:(BaseDownloadItemModel*) model;
no space before |model|

http://codereview.chromium.org/93129/diff/2005/3010#newcode26
Line 26: class DownloadItemMac : DownloadItem::Observer {
class comment needed.

http://codereview.chromium.org/93129/diff/2005/3010#newcode48
Line 48: NSPopUpButton* view_;
strong? use scoped_nsobject if so

http://codereview.chromium.org/93129/diff/2005/3010#newcode51
Line 51: DownloadShelfContextMenuBridge* menu_;
same as above.

http://codereview.chromium.org/93129/diff/2005/3012
File chrome/browser/cocoa/download_item_mac.mm (right):

http://codereview.chromium.org/93129/diff/2005/3012#newcode22
Line 22: class DownloadShelfContextMenuMac : public DownloadShelfContextMenu {
needs class comment

http://codereview.chromium.org/93129/diff/2005/3012#newcode26
Line 26: // |widget| is the button that popped up this context menu, and |e| is
the comment mentions parameters that don't seem to be there.

http://codereview.chromium.org/93129/diff/2005/3012#newcode54
Line 54: -
(DownloadShelfContextMenuBridge*)initWithModel:(BaseDownloadItemModel*) model {
80 cols

http://codereview.chromium.org/93129/diff/2005/3012#newcode63
Line 63: // FIXME(thakis): use a scoped ptr instead?
do that.

http://codereview.chromium.org/93129/diff/2005/3012#newcode90
Line 90: // TODO(thakis): this probably wants to be in a xib file or at least
use
file a bug?

http://codereview.chromium.org/93129/diff/2005/3012#newcode93
Line 93: NSMenu* menu = [[[NSMenu alloc] initWithTitle:@"DownloadItemPopup"]
autorelease];
80cols throughout this method (and file)

http://codereview.chromium.org/93129/diff/2005/3012#newcode136
Line 136: view_ = [[NSPopUpButton alloc] initWithFrame:frame pullsDown:YES];
are you leaking view_? Where does it get released?

http://codereview.chromium.org/93129/diff/2005/3012#newcode142
Line 142: NSString* titleString = nil;
why not do this on 1 line?

http://codereview.chromium.org/93129/diff/2005/3012#newcode150
Line 150: [[view_ itemAtIndex:0] setImage:[[NSWorkspace sharedWorkspace]
iconForFile:titleString]];
if the file doesn't exist, will this be ok? what does setImage do if you pass it
nil? Do we need to be careful?

http://codereview.chromium.org/93129/diff/2005/3006
File chrome/browser/cocoa/download_shelf_mac.mm (right):

http://codereview.chromium.org/93129/diff/2005/3006#newcode22
Line 22: }  // namespace
why leave the empty namespace?

http://codereview.chromium.org/93129/diff/2005/3006#newcode56
Line 56: int startX = 4 + (210) * download_items_.size();
what's with 4 and 210? constants?

http://codereview.chromium.org/93129/diff/2005/3008
File chrome/browser/cocoa/shelf_view.h (right):

http://codereview.chromium.org/93129/diff/2005/3008#newcode13
Line 13: @interface ShelfView : NSView {
this file needs a unit test

http://codereview.chromium.org/93129/diff/2005/3013
File chrome/browser/cocoa/shelf_view.mm (right):

http://codereview.chromium.org/93129/diff/2005/3013#newcode42
Line 42: 
omit one of the blank lines

http://codereview.chromium.org/93129/diff/2005/3013#newcode51
Line 51: 
same here

http://codereview.chromium.org/93129/diff/2005/3014
File chrome/browser/cocoa/tab_contents_controller.h (right):

http://codereview.chromium.org/93129/diff/2005/3014#newcode26
Line 26: IBOutlet ShelfView* /* DownloadShelfView* */ downloadShelfView_;
i know Glen is reworking all this to be per-window, not per-tab. Can we talk to
him before landing this? It might be nice to be ahead of the game rather than
have to change it again once windows changes.

http://codereview.chromium.org/93129/diff/2005/3014#newcode31
Line 31: 
omit blank line

http://codereview.chromium.org/93129/diff/2005/3014#newcode47
Line 47: - (ShelfView*)downloadShelf;
so should you remove this?

http://codereview.chromium.org/93129/diff/2005/3009
File chrome/browser/cocoa/tab_contents_controller.mm (right):

http://codereview.chromium.org/93129/diff/2005/3009#newcode56
Line 56: contents_->set_contents_controller(self);
rather than setting this on the TabContents, why can't you just have the shelf
know about the TabContentsController. Doesn't that own and/or create the
DownloadShelf for the given tab? If nothing else, maybe it can be a delegate
object with a limited @protocol API that it responds to so it doesn't have to
know everything about TabContentsController.

http://codereview.chromium.org/93129/diff/2005/3009#newcode88
Line 88: // "We're too early, but awakeFromNib will call me again" (?)
this came from teh bookmark bar code, right? is it appropriate here at all?

http://codereview.chromium.org/93129/diff/2005/3011
File chrome/browser/cocoa/tab_strip_controller.mm (right):

http://codereview.chromium.org/93129/diff/2005/3011#newcode223
Line 223: contents->set_contents_controller(contentsController);
again, i think there are better ways to do this, per the other comment.

http://codereview.chromium.org/93129/diff/2005/3015
File chrome/browser/download/download_manager.cc (right):

http://codereview.chromium.org/93129/diff/2005/3015#newcode656
Line 656: // TODO(port): port this file -- need dialogs.
dialogs were recently implemented by avi, sure this still needs to be commented
out?

http://codereview.chromium.org/93129/diff/2005/3017
File chrome/chrome.gyp (right):

http://codereview.chromium.org/93129/diff/2005/3017#newcode533
Line 533: 'browser/cocoa/download_item_mac.h',
these both need basic unit tests.

[Nico, 2009-06-17 07:38:38]

Long time no see.

The shelf is now per-window. I also converted the download shelf to the
nib/controller/small bridge object that the bookmark bar and the find bar use. I
want to do that with the download item too, but not in this CL. The shelf
display code is stolen from the bookmark bar code. I didn't bother putting it
somewhere where it can be shared, since that code doesn't scale to more than 1
bar at top and bottom anyway (and that will be required by infobars; I believe
rohit is working on that)

I've added a test for DownloadShelfMac, the rest is covered by the UI tests well
enough in my eyes. I will add some tests for the download item once it's
converted to nib/controller/small bridge too (not in this CL).

Lots of stuff is still missing, I will update the bug with what's still missing
once this CL is committed (stuff still missing includes: reparent grow box when
shelf is visible, put status bubble higher when shelf is visible, make a real
download item with status updates etc, make things pretty, fix the shelf's close
button repainting, add a "show all downloads" link to the shelf, make the
context menu localizable, show that huge download arrow animation, handle item
overflow, support the unsave filename stuff that windows does, etc). But the
foundations are in this CL.

http://codereview.chromium.org/93129/diff/2005/3010
File chrome/browser/cocoa/download_item_mac.h (right):

http://codereview.chromium.org/93129/diff/2005/3010#newcode17
Line 17: @interface DownloadShelfContextMenuBridge : NSObject {
On 2009/04/27 17:19:03, pink wrote:
> this seems like an implementation detail. Are you sure it needs to be part of
> the public api?

Moved it into the mm file.

http://codereview.chromium.org/93129/diff/2005/3010#newcode17
Line 17: @interface DownloadShelfContextMenuBridge : NSObject {
On 2009/04/27 17:19:03, pink wrote:
> needs a class-level comment.

Done.

http://codereview.chromium.org/93129/diff/2005/3010#newcode17
Line 17: @interface DownloadShelfContextMenuBridge : NSObject {
On 2009/04/27 17:19:03, pink wrote:
> needs a class-level comment.

Done.

http://codereview.chromium.org/93129/diff/2005/3010#newcode18
Line 18: DownloadShelfContextMenuMac* contextMenu_;  // strong
On 2009/04/27 17:19:03, pink wrote:
> this should be @private and indented 2 spaces. If it's strong, you should use
a
> scoped_nsobject or scoped_ptr.

Done.

http://codereview.chromium.org/93129/diff/2005/3010#newcode21
Line 21: -
(DownloadShelfContextMenuBridge*)initWithModel:(BaseDownloadItemModel*) model;
On 2009/04/27 17:19:03, pink wrote:
> no space before |model|

Done.

http://codereview.chromium.org/93129/diff/2005/3010#newcode26
Line 26: class DownloadItemMac : DownloadItem::Observer {
On 2009/04/27 17:19:03, pink wrote:
> class comment needed.

Done.

http://codereview.chromium.org/93129/diff/2005/3010#newcode48
Line 48: NSPopUpButton* view_;
On 2009/04/27 17:19:03, pink wrote:
> strong? use scoped_nsobject if so

Done.

http://codereview.chromium.org/93129/diff/2005/3010#newcode51
Line 51: DownloadShelfContextMenuBridge* menu_;
On 2009/04/27 17:19:03, pink wrote:
> same as above.

Done.

http://codereview.chromium.org/93129/diff/2005/3012
File chrome/browser/cocoa/download_item_mac.mm (right):

http://codereview.chromium.org/93129/diff/2005/3012#newcode26
Line 26: // |widget| is the button that popped up this context menu, and |e| is
On 2009/04/27 17:19:03, pink wrote:
> the comment mentions parameters that don't seem to be there.

Done.

http://codereview.chromium.org/93129/diff/2005/3012#newcode54
Line 54: -
(DownloadShelfContextMenuBridge*)initWithModel:(BaseDownloadItemModel*) model {
On 2009/04/27 17:19:03, pink wrote:
> 80 cols

Done.

http://codereview.chromium.org/93129/diff/2005/3012#newcode63
Line 63: // FIXME(thakis): use a scoped ptr instead?
On 2009/04/27 17:19:03, pink wrote:
> do that.

Done.

http://codereview.chromium.org/93129/diff/2005/3012#newcode90
Line 90: // TODO(thakis): this probably wants to be in a xib file or at least
use
On 2009/04/27 17:19:03, pink wrote:
> file a bug?

Will update http://crbug/12500 with what is missing once this is submitted.

http://codereview.chromium.org/93129/diff/2005/3012#newcode93
Line 93: NSMenu* menu = [[[NSMenu alloc] initWithTitle:@"DownloadItemPopup"]
autorelease];
On 2009/04/27 17:19:03, pink wrote:
> 80cols throughout this method (and file)

Done.

http://codereview.chromium.org/93129/diff/2005/3012#newcode136
Line 136: view_ = [[NSPopUpButton alloc] initWithFrame:frame pullsDown:YES];
On 2009/04/27 17:19:03, pink wrote:
> are you leaking view_? Where does it get released?

Done.

http://codereview.chromium.org/93129/diff/2005/3012#newcode142
Line 142: NSString* titleString = nil;
On 2009/04/27 17:19:03, pink wrote:
> why not do this on 1 line?

Done.

http://codereview.chromium.org/93129/diff/2005/3012#newcode150
Line 150: [[view_ itemAtIndex:0] setImage:[[NSWorkspace sharedWorkspace]
iconForFile:titleString]];
On 2009/04/27 17:19:03, pink wrote:
> if the file doesn't exist, will this be ok? what does setImage do if you pass
it
> nil? Do we need to be careful?

Seems like this works. Will be changed anyway when there's a real DownloadItem.

http://codereview.chromium.org/93129/diff/2005/3006
File chrome/browser/cocoa/download_shelf_mac.mm (right):

http://codereview.chromium.org/93129/diff/2005/3006#newcode22
Line 22: }  // namespace
On 2009/04/27 17:19:03, pink wrote:
> why leave the empty namespace?

Done.

http://codereview.chromium.org/93129/diff/2005/3006#newcode56
Line 56: int startX = 4 + (210) * download_items_.size();
On 2009/04/27 17:19:03, pink wrote:
> what's with 4 and 210? constants?

Done.

http://codereview.chromium.org/93129/diff/2005/3008
File chrome/browser/cocoa/shelf_view.h (right):

http://codereview.chromium.org/93129/diff/2005/3008#newcode13
Line 13: @interface ShelfView : NSView {
On 2009/04/27 17:19:03, pink wrote:
> this file needs a unit test

srsly? All it does is override drawRect.

http://codereview.chromium.org/93129/diff/2005/3013
File chrome/browser/cocoa/shelf_view.mm (right):

http://codereview.chromium.org/93129/diff/2005/3013#newcode42
Line 42: 
On 2009/04/27 17:19:03, pink wrote:
> omit one of the blank lines

Done.

http://codereview.chromium.org/93129/diff/2005/3013#newcode51
Line 51: 
On 2009/04/27 17:19:03, pink wrote:
> same here

Done.

http://codereview.chromium.org/93129/diff/2005/3014
File chrome/browser/cocoa/tab_contents_controller.h (right):

http://codereview.chromium.org/93129/diff/2005/3014#newcode26
Line 26: IBOutlet ShelfView* /* DownloadShelfView* */ downloadShelfView_;
On 2009/04/27 17:19:03, pink wrote:
> i know Glen is reworking all this to be per-window, not per-tab. Can we talk
to
> him before landing this? It might be nice to be ahead of the game rather than
> have to change it again once windows changes.

Talked with Glen, he convinced me that I should move the shelf from per-tab to
per-window on windows and linux. Did that.

http://codereview.chromium.org/93129/diff/2005/3014#newcode31
Line 31: 
On 2009/04/27 17:19:03, pink wrote:
> omit blank line

Not longer applicable, this file got reverted.

http://codereview.chromium.org/93129/diff/2005/3014#newcode47
Line 47: - (ShelfView*)downloadShelf;
On 2009/04/27 17:19:03, pink wrote:
> so should you remove this?

See above.

http://codereview.chromium.org/93129/diff/2005/3009
File chrome/browser/cocoa/tab_contents_controller.mm (right):

http://codereview.chromium.org/93129/diff/2005/3009#newcode56
Line 56: contents_->set_contents_controller(self);
On 2009/04/27 17:19:03, pink wrote:
> rather than setting this on the TabContents, why can't you just have the shelf
> know about the TabContentsController. Doesn't that own and/or create the
> DownloadShelf for the given tab? If nothing else, maybe it can be a delegate
> object with a limited @protocol API that it responds to so it doesn't have to
> know everything about TabContentsController.

Since the shelf is now per-window, this is now cleaner (imho).

http://codereview.chromium.org/93129/diff/2005/3009#newcode88
Line 88: // "We're too early, but awakeFromNib will call me again" (?)
On 2009/04/27 17:19:03, pink wrote:
> this came from teh bookmark bar code, right? is it appropriate here at all?

Don't know. This does now happen somewhere else.

http://codereview.chromium.org/93129/diff/2005/3011
File chrome/browser/cocoa/tab_strip_controller.mm (right):

http://codereview.chromium.org/93129/diff/2005/3011#newcode223
Line 223: contents->set_contents_controller(contentsController);
On 2009/04/27 17:19:03, pink wrote:
> again, i think there are better ways to do this, per the other comment.

Yes, this is now done differently.

http://codereview.chromium.org/93129/diff/2005/3017
File chrome/chrome.gyp (right):

http://codereview.chromium.org/93129/diff/2005/3017#newcode533
Line 533: 'browser/cocoa/download_item_mac.h',
On 2009/04/27 17:19:03, pink wrote:
> these both need basic unit tests.

Agreed. Added some tests where it made sense to me. Note that some of this is
covered by the existing ui tests.

[pink (ping after 24hrs), 2009-06-17 21:34:23]

http://codereview.chromium.org/93129/diff/6059/6070
File chrome/browser/cocoa/browser_window_cocoa.mm (right):

http://codereview.chromium.org/93129/diff/6059/6070#newcode102
Line 102: // FIXME(thakis): Not true. Need to set title for expose, dock menu
these should be TODO(thakis) to fit with chrome style.

http://codereview.chromium.org/93129/diff/6059/6070#newcode102
Line 102: // FIXME(thakis): Not true. Need to set title for expose, dock menu
what's not true?

http://codereview.chromium.org/93129/diff/6059/6070#newcode105
Line 105: //if (l10n_util::AdjustStringForLocaleDirection(window_title,
&localized_text))
why is this here if commented out?

http://codereview.chromium.org/93129/diff/6059/6070#newcode109
Line 109: NSString* titleString = nil;
why can't we use |newTitle| from above?

http://codereview.chromium.org/93129/diff/6059/6070#newcode114
Line 114: [window_ setTitle:titleString];
doesn't this cause the title to display in the title bar and cover the tabs?

http://codereview.chromium.org/93129/diff/6059/6070#newcode206
Line 206: //  return download_shelf_ != NULL && download_shelf_->IsShowing();
remove commented out line.

http://codereview.chromium.org/93129/diff/6059/6071
File chrome/browser/cocoa/browser_window_controller.h (right):

http://codereview.chromium.org/93129/diff/6059/6071#newcode104
Line 104: - (DownloadShelfController*)downloadShelf;
you should mention that this creates one lazily if it hasn't already been
created.

http://codereview.chromium.org/93129/diff/6059/6072
File chrome/browser/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/93129/diff/6059/6072#newcode656
Line 656: [downloadShelfController_ resizeDownloadShelf];
omit the if check. it's not necessary (runtime checks for you). omit it on the
next line as well.

http://codereview.chromium.org/93129/diff/6059/6066
File chrome/browser/cocoa/download_item_mac.mm (right):

http://codereview.chromium.org/93129/diff/6059/6066#newcode15
Line 15: #import "chrome/browser/cocoa/download_shelf_controller.h"
these come before chrome/browser/download/... in alphabetical order.

http://codereview.chromium.org/93129/diff/6059/6066#newcode43
Line 43: NSMenuItem* item;
declare |item| where it's initialized, below.

http://codereview.chromium.org/93129/diff/6059/6066#newcode49
Line 49: //  [item addItemWithTitle:@"Open when complete" ...];
why commented out?

http://codereview.chromium.org/93129/diff/6059/6066#newcode54
Line 54: item = [menu addItemWithTitle:@"Always open type"
Normally I'd say to just do the l10n right the first time, but this is such a
huge patch, I'd rather not have it continue to bitrot and pull in more l10n
changes. It shouldn't be that hard though, it'd make my day if you could do this
:)

http://codereview.chromium.org/93129/diff/6059/6066#newcode107
Line 107: - (id)performAction:(id)event {
are you sure the return type isn't IBAction, which is void? Also, the parameter
isn't an event, it's the sender of the message, which in this case would be an
NSMenuItem. Best to rename it |sender| like the rest of cocoa.

http://codereview.chromium.org/93129/diff/6059/6066#newcode131
Line 131: NSPopUpButton* view =
this will leak |view|. Use a scoped_nsobject<T> here.

http://codereview.chromium.org/93129/diff/6059/6061
File chrome/browser/cocoa/download_shelf_controller.h (right):

http://codereview.chromium.org/93129/diff/6059/6061#newcode20
Line 20: // detail of bookmark bar visibility.
bookmark bar? :) copy/paste error? If these classes are similar, is there a way
to have them share anything?

http://codereview.chromium.org/93129/diff/6059/6062
File chrome/browser/cocoa/download_shelf_controller.mm (right):

http://codereview.chromium.org/93129/diff/6059/6062#newcode10
Line 10: 
omit blank line

http://codereview.chromium.org/93129/diff/6059/6062#newcode13
Line 13: #include "base/mac_util.h"
this goes above alphabetically.

http://codereview.chromium.org/93129/diff/6059/6062#newcode26
Line 26: 
omit blank line

http://codereview.chromium.org/93129/diff/6059/6062#newcode43
Line 43: 
omit blank line

http://codereview.chromium.org/93129/diff/6059/6062#newcode55
Line 55: // stay with the top of the contentArea.
s/top/bottom

http://codereview.chromium.org/93129/diff/6059/6062#newcode56
Line 56: - (void)resizeDownloadShelf {
is this really necessary if you set the auto-resize flags correctly on the view?
It should pin itself to the bottom of the content view and stay there, right?

http://codereview.chromium.org/93129/diff/6059/6062#newcode102
Line 102: //  return ![[self view] isHidden];
remove commented out lines? here and below

http://codereview.chromium.org/93129/diff/6059/6068
File chrome/browser/cocoa/download_shelf_mac.mm (right):

http://codereview.chromium.org/93129/diff/6059/6068#newcode8
Line 8: #include "chrome/browser/cocoa/download_item_mac.h"
these go first, alphabetically.

http://codereview.chromium.org/93129/diff/6059/6068#newcode29
Line 29: DownloadShelfMac::DownloadShelfMac(Browser* browser,
DownloadShelfController* controller)
80cols

http://codereview.chromium.org/93129/diff/6059/6068#newcode37
Line 37: // TODO(estade, thakis): we need to delete these at some point. There's
no explicit
80cols

http://codereview.chromium.org/93129/diff/6059/6069
File chrome/browser/cocoa/download_shelf_mac_unittest.mm (right):

http://codereview.chromium.org/93129/diff/6059/6069#newcode6
Line 6: #include "chrome/browser/cocoa/browser_test_helper.h"
alphabetize these

http://codereview.chromium.org/93129/diff/6059/6064
File chrome/browser/cocoa/download_shelf_view.mm (right):

http://codereview.chromium.org/93129/diff/6059/6064#newcode20
Line 20: NSGradient *gradient =
use a scoped_nsobject instead of autoreleasing, since we have it available to
us.

http://codereview.chromium.org/93129/diff/6059/6064#newcode51
Line 51: [[[NSGradient alloc] initWithStartingColor:start endingColor:end]
same as above.

http://codereview.chromium.org/93129/diff/6059/6064#newcode58
Line 58: 
omit empty line.

[Nico, 2009-06-17 23:48:28]

That was quick. Thanks!

http://codereview.chromium.org/93129/diff/6059/6070
File chrome/browser/cocoa/browser_window_cocoa.mm (right):

http://codereview.chromium.org/93129/diff/6059/6070#newcode102
Line 102: // FIXME(thakis): Not true. Need to set title for expose, dock menu
On 2009/06/17 21:34:25, pink wrote:
> these should be TODO(thakis) to fit with chrome style.

Oh, crap, lines 101-114 don't belong to this change. reverted this file of the
CL.

http://codereview.chromium.org/93129/diff/6059/6070#newcode206
Line 206: //  return download_shelf_ != NULL && download_shelf_->IsShowing();
On 2009/06/17 21:34:25, pink wrote:
> remove commented out line.

Done.

http://codereview.chromium.org/93129/diff/6059/6071
File chrome/browser/cocoa/browser_window_controller.h (right):

http://codereview.chromium.org/93129/diff/6059/6071#newcode104
Line 104: - (DownloadShelfController*)downloadShelf;
On 2009/06/17 21:34:25, pink wrote:
> you should mention that this creates one lazily if it hasn't already been
> created.

Done.

http://codereview.chromium.org/93129/diff/6059/6072
File chrome/browser/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/93129/diff/6059/6072#newcode656
Line 656: [downloadShelfController_ resizeDownloadShelf];
On 2009/06/17 21:34:25, pink wrote:
> omit the if check. it's not necessary (runtime checks for you). omit it on the
> next line as well.

Done.

http://codereview.chromium.org/93129/diff/6059/6066
File chrome/browser/cocoa/download_item_mac.mm (right):

http://codereview.chromium.org/93129/diff/6059/6066#newcode15
Line 15: #import "chrome/browser/cocoa/download_shelf_controller.h"
On 2009/06/17 21:34:25, pink wrote:
> these come before chrome/browser/download/... in alphabetical order.

Done.

http://codereview.chromium.org/93129/diff/6059/6066#newcode43
Line 43: NSMenuItem* item;
On 2009/06/17 21:34:25, pink wrote:
> declare |item| where it's initialized, below.

No. It's assigned to multiple times, and all the assignments are independent.
It's nicer this way, and this code is going to go away anyway.

http://codereview.chromium.org/93129/diff/6059/6066#newcode49
Line 49: //  [item addItemWithTitle:@"Open when complete" ...];
On 2009/06/17 21:34:25, pink wrote:
> why commented out?

It's what the text should say for in-progress downloads. Added comment.

http://codereview.chromium.org/93129/diff/6059/6066#newcode54
Line 54: item = [menu addItemWithTitle:@"Always open type"
On 2009/06/17 21:34:25, pink wrote:
> Normally I'd say to just do the l10n right the first time, but this is such a
> huge patch, I'd rather not have it continue to bitrot and pull in more l10n
> changes. It shouldn't be that hard though, it'd make my day if you could do
this
> :)

I like to put the menu into a nib in the future, which makes it autolocalizable
(I think we can't really share strings with windows here anyway, because of
"Reveal in Finder"). Ok?

http://codereview.chromium.org/93129/diff/6059/6066#newcode107
Line 107: - (id)performAction:(id)event {
On 2009/06/17 21:34:25, pink wrote:
> are you sure the return type isn't IBAction, which is void? Also, the
parameter
> isn't an event, it's the sender of the message, which in this case would be an
> NSMenuItem. Best to rename it |sender| like the rest of cocoa.

Done.

http://codereview.chromium.org/93129/diff/6059/6066#newcode131
Line 131: NSPopUpButton* view =
On 2009/06/17 21:34:25, pink wrote:
> this will leak |view|. Use a scoped_nsobject<T> here.

Done. Do we prefer scoped_nsobjects over autorelease in general?

http://codereview.chromium.org/93129/diff/6059/6061
File chrome/browser/cocoa/download_shelf_controller.h (right):

http://codereview.chromium.org/93129/diff/6059/6061#newcode20
Line 20: // detail of bookmark bar visibility.
On 2009/06/17 21:34:25, pink wrote:
> bookmark bar? :) copy/paste error? If these classes are similar, is there a
way
> to have them share anything?

Done. As mentioned in my last mail, the current shelf layout logic won't work
with mutliple shelves (info bars, extension bars, etc), so I didn't bother to
pull it out.

http://codereview.chromium.org/93129/diff/6059/6062
File chrome/browser/cocoa/download_shelf_controller.mm (right):

http://codereview.chromium.org/93129/diff/6059/6062#newcode10
Line 10: 
On 2009/06/17 21:34:25, pink wrote:
> omit blank line

Done.

http://codereview.chromium.org/93129/diff/6059/6062#newcode13
Line 13: #include "base/mac_util.h"
On 2009/06/17 21:34:25, pink wrote:
> this goes above alphabetically.

Done.

http://codereview.chromium.org/93129/diff/6059/6062#newcode26
Line 26: 
On 2009/06/17 21:34:25, pink wrote:
> omit blank line

Done. Apparently, C++ and Java style guides do have its differences ;-)

http://codereview.chromium.org/93129/diff/6059/6062#newcode43
Line 43: 
On 2009/06/17 21:34:25, pink wrote:
> omit blank line

Done.

http://codereview.chromium.org/93129/diff/6059/6062#newcode55
Line 55: // stay with the top of the contentArea.
On 2009/06/17 21:34:25, pink wrote:
> s/top/bottom

Done.

http://codereview.chromium.org/93129/diff/6059/6062#newcode56
Line 56: - (void)resizeDownloadShelf {
On 2009/06/17 21:34:25, pink wrote:
> is this really necessary if you set the auto-resize flags correctly on the
view?
> It should pin itself to the bottom of the content view and stay there, right?

This is so that the download shelf animates in response to the content area
sliding up. It's not needed for window resizing.

http://codereview.chromium.org/93129/diff/6059/6062#newcode102
Line 102: //  return ![[self view] isHidden];
On 2009/06/17 21:34:25, pink wrote:
> remove commented out lines? here and below

Done.

http://codereview.chromium.org/93129/diff/6059/6068
File chrome/browser/cocoa/download_shelf_mac.mm (right):

http://codereview.chromium.org/93129/diff/6059/6068#newcode8
Line 8: #include "chrome/browser/cocoa/download_item_mac.h"
On 2009/06/17 21:34:25, pink wrote:
> these go first, alphabetically.

Done.

http://codereview.chromium.org/93129/diff/6059/6068#newcode29
Line 29: DownloadShelfMac::DownloadShelfMac(Browser* browser,
DownloadShelfController* controller)
On 2009/06/17 21:34:25, pink wrote:
> 80cols

Done.

http://codereview.chromium.org/93129/diff/6059/6068#newcode37
Line 37: // TODO(estade, thakis): we need to delete these at some point. There's
no explicit
On 2009/06/17 21:34:25, pink wrote:
> 80cols

Done :-P

http://codereview.chromium.org/93129/diff/6059/6069
File chrome/browser/cocoa/download_shelf_mac_unittest.mm (right):

http://codereview.chromium.org/93129/diff/6059/6069#newcode6
Line 6: #include "chrome/browser/cocoa/browser_test_helper.h"
On 2009/06/17 21:34:25, pink wrote:
> alphabetize these

Done.

http://codereview.chromium.org/93129/diff/6059/6064
File chrome/browser/cocoa/download_shelf_view.mm (right):

http://codereview.chromium.org/93129/diff/6059/6064#newcode20
Line 20: NSGradient *gradient =
On 2009/06/17 21:34:25, pink wrote:
> use a scoped_nsobject instead of autoreleasing, since we have it available to
> us.

I guess that answers my question a few comments back. Done.

http://codereview.chromium.org/93129/diff/6059/6064#newcode51
Line 51: [[[NSGradient alloc] initWithStartingColor:start endingColor:end]
On 2009/06/17 21:34:25, pink wrote:
> same as above.

Done.

http://codereview.chromium.org/93129/diff/6059/6064#newcode58
Line 58: 
On 2009/06/17 21:34:25, pink wrote:
> omit empty line.

Done.

[pink (ping after 24hrs), 2009-06-18 17:18:23]

http://codereview.chromium.org/93129/diff/9045/8028
File chrome/browser/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/93129/diff/9045/8028#newcode552
Line 552: return downloadShelfController_ != nil && [downloadShelfController_
isVisible];
80cols

http://codereview.chromium.org/93129/diff/9045/8022
File chrome/browser/cocoa/download_item_mac.mm (right):

http://codereview.chromium.org/93129/diff/9045/8022#newcode10
Line 10: 
i'd say get rid of the blank lines. Nobody really needs to read the includes
enough to add whitespace separators.

http://codereview.chromium.org/93129/diff/9045/8018
File chrome/browser/cocoa/download_shelf_controller.mm (right):

http://codereview.chromium.org/93129/diff/9045/8018#newcode99
Line 99: //  return ![[self view] isHidden];
remove commented out line? If you want to leave it, add a comment.

http://codereview.chromium.org/93129/diff/9045/8030
File chrome/chrome.gyp (right):

http://codereview.chromium.org/93129/diff/9045/8030#newcode3201
Line 3201: 'browser/download/download_uitest.cc',
incognito should work, what's the problem here?

[Nico, 2009-06-18 17:36:58]

I'll look into the download_uitest now. Addressed the rest.

http://codereview.chromium.org/93129/diff/9045/8028
File chrome/browser/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/93129/diff/9045/8028#newcode552
Line 552: return downloadShelfController_ != nil && [downloadShelfController_
isVisible];
On 2009/06/18 17:18:24, pink wrote:
> 80cols

Done.

http://codereview.chromium.org/93129/diff/9045/8022
File chrome/browser/cocoa/download_item_mac.mm (right):

http://codereview.chromium.org/93129/diff/9045/8022#newcode10
Line 10: 
On 2009/06/18 17:18:24, pink wrote:
> i'd say get rid of the blank lines. Nobody really needs to read the includes
> enough to add whitespace separators.

Done.

http://codereview.chromium.org/93129/diff/9045/8018
File chrome/browser/cocoa/download_shelf_controller.mm (right):

http://codereview.chromium.org/93129/diff/9045/8018#newcode99
Line 99: //  return ![[self view] isHidden];
On 2009/06/18 17:18:24, pink wrote:
> remove commented out line? If you want to leave it, add a comment.

Done.

http://codereview.chromium.org/93129/diff/9045/8030
File chrome/chrome.gyp (right):

http://codereview.chromium.org/93129/diff/9045/8030#newcode3201
Line 3201: 'browser/download/download_uitest.cc',
On 2009/06/18 17:18:24, pink wrote:
> incognito should work, what's the problem here?

Something failed. I'm rebuilding right now and will test this again.

[pink (ping after 24hrs), 2009-06-18 17:39:37]

cc'ing paulg who will probably be doing a lot of the finishing work on this.

paul, take a look at all the TODOs, not sure if you want to file bugs for them
or what.

[Nico, 2009-06-18 20:42:28]

Ping. Good to go?

[Paul Godavari, 2009-06-18 21:13:41]

I'm okay with it, but there are a ton of TODOs in the code. I think there needs
to be a bug for each one so we can get some parallelization going.

[pink (ping after 24hrs), 2009-06-18 21:20:31]

LGTM