Add "dangerous download" view on OS X, for now for dmg files. Also fix download item layout.

Add "dangerous download" view on OS X, for now for dmg files. Also fix download item layout.

BUG=14667, 17831
TEST=Download a dmg file. Make sure the "dangerous download" view appears. Check that both buttons work, and that the dmg filename is elided correctly. Also test that "Remove" on a normal download item's context menu relayouts the remaining items on the shelf.

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

[Nico, 2009-08-11 08:10:16]

Last substantial piece for the download shelf. Hooray!

[Paul Godavari, 2009-08-11 21:34:49]

Mostly just nits:

http://codereview.chromium.org/165295/diff/11/13
File chrome/browser/cocoa/download_item_controller.h (right):

http://codereview.chromium.org/165295/diff/11/13#newcode16
Line 16: 
nit: remove blanks lines (should only be one).

http://codereview.chromium.org/165295/diff/11/13#newcode41
Line 41: base::Time creation_time_;
nit: creationTime_?

http://codereview.chromium.org/165295/diff/11/13#newcode47
Line 47: } state_;
With only two states, perhaps this should be:
BOOL isDangerous_;

http://codereview.chromium.org/165295/diff/11/14
File chrome/browser/cocoa/download_item_controller.mm (right):

http://codereview.chromium.org/165295/diff/11/14#newcode76
Line 76: gfx::Font font_chr = gfx::Font::CreateFont(
Nit: fontChr?

http://codereview.chromium.org/165295/diff/11/14#newcode129
Line 129: if (state_ == kNormal)
nit: 'if' clause needs braces if the 'else' clause uses them, although in this
case you don't even need the else clause since you've returned above.

http://codereview.chromium.org/165295/diff/11/14#newcode161
Line 161: // The user has confirmed a dangerous download.  We'd record how
quickly the
nit: "We record"

http://codereview.chromium.org/165295/diff/11/14#newcode174
Line 174: == DownloadItem::IN_PROGRESS)
nit: '==' goes on the previous line.

http://codereview.chromium.org/165295/diff/11/18
File chrome/browser/cocoa/download_shelf_controller.mm (right):

http://codereview.chromium.org/165295/diff/11/18#newcode191
Line 191: - (void)layoutItems:(BOOL)skipFirst {
Add a comment about the parameter. Should this be listed in a 'Private'
@interface in this file?

http://codereview.chromium.org/165295/diff/11/18#newcode198
Line 198: if (!skipFirst) {
nit: braces not needed.

http://codereview.chromium.org/165295/diff/11/19
File chrome/browser/download/download_exe.cc (right):

http://codereview.chromium.org/165295/diff/11/19#newcode151
Line 151: "dmg",
Maybe file a bug to track populating this list (as opposed to implementing
dangerous support)?

[Nico, 2009-08-11 22:37:27]

Thanks for the review. All addressed.

http://codereview.chromium.org/165295/diff/11/13
File chrome/browser/cocoa/download_item_controller.h (right):

http://codereview.chromium.org/165295/diff/11/13#newcode16
Line 16: 
On 2009/08/11 21:34:49, Paul Godavari wrote:
> nit: remove blanks lines (should only be one).

Done.

http://codereview.chromium.org/165295/diff/11/13#newcode41
Line 41: base::Time creation_time_;
On 2009/08/11 21:34:49, Paul Godavari wrote:
> nit: creationTime_?

Done.

http://codereview.chromium.org/165295/diff/11/13#newcode47
Line 47: } state_;
On 2009/08/11 21:34:49, Paul Godavari wrote:
> With only two states, perhaps this should be:
> BOOL isDangerous_;

I generally prefer enums over bools (because the make function calls much more
readable, especially with c functions that take multiple params – compare
doSomething(kNoTimeout, kUseLasers) with doSomething(false, true)). I agree that
it's less necessary with objc function names, but would like to leave this as is
anyway if you don't mind.

http://codereview.chromium.org/165295/diff/11/14
File chrome/browser/cocoa/download_item_controller.mm (right):

http://codereview.chromium.org/165295/diff/11/14#newcode76
Line 76: gfx::Font font_chr = gfx::Font::CreateFont(
On 2009/08/11 21:34:49, Paul Godavari wrote:
> Nit: fontChr?

Done.

http://codereview.chromium.org/165295/diff/11/14#newcode129
Line 129: if (state_ == kNormal)
On 2009/08/11 21:34:49, Paul Godavari wrote:
> nit: 'if' clause needs braces if the 'else' clause uses them, although in this
> case you don't even need the else clause since you've returned above.

Done.

http://codereview.chromium.org/165295/diff/11/14#newcode161
Line 161: // The user has confirmed a dangerous download.  We'd record how
quickly the
On 2009/08/11 21:34:49, Paul Godavari wrote:
> nit: "We record"

Done.

http://codereview.chromium.org/165295/diff/11/14#newcode174
Line 174: == DownloadItem::IN_PROGRESS)
On 2009/08/11 21:34:49, Paul Godavari wrote:
> nit: '==' goes on the previous line.

Done.

http://codereview.chromium.org/165295/diff/11/18
File chrome/browser/cocoa/download_shelf_controller.mm (right):

http://codereview.chromium.org/165295/diff/11/18#newcode191
Line 191: - (void)layoutItems:(BOOL)skipFirst {
On 2009/08/11 21:34:49, Paul Godavari wrote:
> Add a comment about the parameter. Should this be listed in a 'Private'
> @interface in this file?

Done.

http://codereview.chromium.org/165295/diff/11/18#newcode198
Line 198: if (!skipFirst) {
On 2009/08/11 21:34:49, Paul Godavari wrote:
> nit: braces not needed.

Done.

http://codereview.chromium.org/165295/diff/11/19
File chrome/browser/download/download_exe.cc (right):

http://codereview.chromium.org/165295/diff/11/19#newcode151
Line 151: "dmg",
On 2009/08/11 21:34:49, Paul Godavari wrote:
> Maybe file a bug to track populating this list (as opposed to implementing
> dangerous support)?

Done.

[Paul Godavari, 2009-08-11 22:44:48]

LGTM!