Fix the placement of dangerous download warning icons in DownloadItemView.

Fix the placement of dangerous download warning icons in DownloadItemView.

BUG=Dangerous download warning in download shelf shows icon a few pixels too low.
TEST=Start downloading something dangerous. Check that the warning icon is centered.


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

[asanka, 2011-11-18 21:57:40]

msw: Would you be willing to review this change?  It's been a while since people
touched this code, so I'm not sure who's canonical.

Screenshots are here : http://folder/asanka/downloads-item-icon/

[msw, 2011-11-21 18:39:20]

This is a prime example of why using LayoutManagers in Views is the right way to
go; manual placement and sizing adds unnecessary complexity. But that's out of
scope of this change.

LGTM with nits.

http://codereview.chromium.org/8548016/diff/4001/chrome/browser/ui/views/down...
File chrome/browser/ui/views/download/download_item_view.cc (right):

http://codereview.chromium.org/8548016/diff/4001/chrome/browser/ui/views/down...
chrome/browser/ui/views/download/download_item_view.cc:832: kLeftPadding +
dangerous_mode_body_image_set_.top_left->width(),
Is there a reason to add the 0px kLeftPadding here?

http://codereview.chromium.org/8548016/diff/4001/chrome/browser/ui/views/down...
chrome/browser/ui/views/download/download_item_view.cc:832: kLeftPadding +
dangerous_mode_body_image_set_.top_left->width(),
Use the body_image_set local instead.
Can you consolidate this code a litte more between the two images?

[asanka, 2011-11-21 20:45:32]

Thanks for the review! Agreed about the use of layout managers.

http://codereview.chromium.org/8548016/diff/4001/chrome/browser/ui/views/down...
File chrome/browser/ui/views/download/download_item_view.cc (right):

http://codereview.chromium.org/8548016/diff/4001/chrome/browser/ui/views/down...
chrome/browser/ui/views/download/download_item_view.cc:832: kLeftPadding +
dangerous_mode_body_image_set_.top_left->width(),
On 2011/11/21 18:39:20, msw wrote:
> Use the body_image_set local instead.
> Can you consolidate this code a litte more between the two images?

Done.

http://codereview.chromium.org/8548016/diff/4001/chrome/browser/ui/views/down...
chrome/browser/ui/views/download/download_item_view.cc:832: kLeftPadding +
dangerous_mode_body_image_set_.top_left->width(),
On 2011/11/21 18:39:20, msw wrote:
> Is there a reason to add the 0px kLeftPadding here?

I'm mirroring the logic in DIV::Layout() and DIV::GetPreferredSize() for
calculating the icon position, which is where kLeftPadding comes from.

[msw, 2011-11-21 20:48:48]

Awesome, LGTM; thanks for the extra cleanup.

[commit-bot: I haz the power, 2011-11-22 15:37:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/asanka@chromium.org/8548016/8001

[commit-bot: I haz the power, 2011-11-22 16:49:21]

Change committed as 111167