Update the windows UI to support displaying a warning after the

Update the windows UI to support displaying a warning after the
download has started.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=110750

[noelutz, 2011-11-17 22:10:59]

This seems to work as expected.  Is there anything else that needs to changes?

Thanks,
noé.

[Randy Smith (Not in Mondays), 2011-11-18 00:00:19]

LGTM, but I wouldn't put a lot of faith in that--Asanka's going to have to be
primary reviewer for this change.

[noelutz, 2011-11-18 01:05:02]

On 2011/11/18 00:00:19, rdsmith wrote:
> LGTM, but I wouldn't put a lot of faith in that--Asanka's going to have to be
> primary reviewer for this change.

Funny... he said the same thing :).  Asanka saw that the icon is slightly lower
than in the current UI.  I'll try to fix that first and then update the CL.

thanks,
noé.

[Randy Smith (Not in Mondays), 2011-11-18 01:13:56]

On 2011/11/18 01:05:02, noelutz wrote:
> On 2011/11/18 00:00:19, rdsmith wrote:
> > LGTM, but I wouldn't put a lot of faith in that--Asanka's going to have to
be
> > primary reviewer for this change.
> 
> Funny... he said the same thing :).  Asanka saw that the icon is slightly
lower
> than in the current UI.  I'll try to fix that first and then update the CL.

Hopefully in the same direction; if Asanka thinks I know this code better than
he does, we've *all* got problems :-} :-}.

[noelutz, 2011-11-18 01:59:21]

On 2011/11/18 01:13:56, rdsmith wrote:
> On 2011/11/18 01:05:02, noelutz wrote:
> > On 2011/11/18 00:00:19, rdsmith wrote:
> > > LGTM, but I wouldn't put a lot of faith in that--Asanka's going to have to
> be
> > > primary reviewer for this change.
> > 
> > Funny... he said the same thing :).  Asanka saw that the icon is slightly
> lower
> > than in the current UI.  I'll try to fix that first and then update the CL.
> 
> Hopefully in the same direction; if Asanka thinks I know this code better than
> he does, we've *all* got problems :-} :-}.

Weird. Even in the dangerous file UI case the warning icon is slightly low even
though the code for this case is exactly identical to the code before that CL. 
I'm wondering if this misalignment is related to my change at all.  Screenshot:
http://www/~noelutz/screenshot.png.

Thoughts?

thanks,
noé.

[noelutz, 2011-11-18 03:11:28]

Had nothing better to do than reverting my CL rebuilding Chrome and comparing
the warning messages.  Looks like my change is not responsible for this
mis-alignment: http://www/~noelutz/side-by-side.png

Thanks,
noé.

[noelutz, 2011-11-18 04:47:15]

Please take a look.  This CL will show the malicious download content warning on
windows.

noé.

[asanka, 2011-11-18 17:17:18]

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

http://codereview.chromium.org/8587042/diff/8001/chrome/browser/ui/views/down...
chrome/browser/ui/views/download/download_item_view.cc:216: }
Minor nit: No braces needed.

http://codereview.chromium.org/8587042/diff/8001/chrome/browser/ui/views/down...
chrome/browser/ui/views/download/download_item_view.cc:282:
LoadIconIfItemPathChanged();
If the download path changes on an IN_PROGRESS download while we showing the
dangerous download prompt and we validate the download, we'll miss out on the
(potentially new) icon until we get another OnDownloadUpdated() notification.

This doesn't currently happen, but I'm not sure we can guarantee that with some
of the upcoming extensions API work.

[asanka, 2011-11-18 17:19:10]

Also, LGTM.

[noelutz, 2011-11-18 17:45:18]

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

http://codereview.chromium.org/8587042/diff/8001/chrome/browser/ui/views/down...
chrome/browser/ui/views/download/download_item_view.cc:216: }
On 2011/11/18 17:17:18, asanka wrote:
> Minor nit: No braces needed.

Done.

http://codereview.chromium.org/8587042/diff/8001/chrome/browser/ui/views/down...
chrome/browser/ui/views/download/download_item_view.cc:282:
LoadIconIfItemPathChanged();
On 2011/11/18 17:17:18, asanka wrote:
> If the download path changes on an IN_PROGRESS download while we showing the
> dangerous download prompt and we validate the download, we'll miss out on the
> (potentially new) icon until we get another OnDownloadUpdated() notification.
> 
> This doesn't currently happen, but I'm not sure we can guarantee that with
some
> of the upcoming extensions API work.
> 

Are you suggesting I should change something here?  Also, I'm not entirely sure
I follow your point.  If the download path changes on an IN_PROGRESS download
while we show the warning I believe this code is still called and
LoadIconIfItemPathChanged() will be called.  Isn't that what we want?

[Randy Smith (Not in Mondays), 2011-11-18 18:15:25]

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

http://codereview.chromium.org/8587042/diff/8001/chrome/browser/ui/views/down...
chrome/browser/ui/views/download/download_item_view.cc:282:
LoadIconIfItemPathChanged();
On 2011/11/18 17:17:18, asanka wrote:
> If the download path changes on an IN_PROGRESS download while we showing the
> dangerous download prompt and we validate the download, we'll miss out on the
> (potentially new) icon until we get another OnDownloadUpdated() notification.
> 
> This doesn't currently happen, but I'm not sure we can guarantee that with
some
> of the upcoming extensions API work.
> 

Could this be fixed by putting a LoadIconIfItemPathChanged in the Dangerous
conditional, above?  Also, isn't this vulnerability there before Noe's change? 
(In the sense that you have to loosen the currently fairly tight linkage between
path changes and transitions into/out of the dangerous state for it to happen,
and if you do that, I think we have the same problem in the current code).

More generally, could you spell out a use case you think would trigger this
problem, just so I'm sure I have a good understanding of it?

[asanka, 2011-11-18 18:19:01]

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

http://codereview.chromium.org/8587042/diff/8001/chrome/browser/ui/views/down...
chrome/browser/ui/views/download/download_item_view.cc:282:
LoadIconIfItemPathChanged();
On 2011/11/18 17:45:18, noelutz wrote:
> On 2011/11/18 17:17:18, asanka wrote:
> > If the download path changes on an IN_PROGRESS download while we showing the
> > dangerous download prompt and we validate the download, we'll miss out on
the
> > (potentially new) icon until we get another OnDownloadUpdated()
notification.
> > 
> > This doesn't currently happen, but I'm not sure we can guarantee that with
> some
> > of the upcoming extensions API work.
> > 
> 
> Are you suggesting I should change something here?  Also, I'm not entirely
sure
> I follow your point.  If the download path changes on an IN_PROGRESS download
> while we show the warning I believe this code is still called and
> LoadIconIfItemPathChanged() will be called.  Isn't that what we want?

The code in the 'else' block here used to run immediately after
ClearDangerousMode().  It doesn't anymore.  I was thinking about potential
behavior changes due to that.

We don't automatically call OnDownloadUpdated() immediately after the path
changes, but there's a state change soon afterwards which winds up sending an
update.  So this code is safe.

Apologies for the confusion.

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

http://codereview.chromium.org/8587042/diff/9005/chrome/browser/ui/views/down...
chrome/browser/ui/views/download/download_item_view.cc:976: ResourceBundle &rb =
ResourceBundle::GetSharedInstance();
Lint error here: ResouceBundle& rb

[commit-bot: I haz the power, 2011-11-18 19:00:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/noelutz@google.com/8587042/6003

[commit-bot: I haz the power, 2011-11-18 20:09:40]

Try job failure for 8587042-6003 (retry) on linux_rel for step "ui_tests".
It's a second try, previously, step "ui_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[noelutz, 2011-11-18 20:17:00]

On 2011/11/18 20:09:40, I haz the power (commit-bot) wrote:
> Try job failure for 8587042-6003 (retry) on linux_rel for step "ui_tests".
> It's a second try, previously, step "ui_tests" failed.
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

I don't think this failure is related.  The rest looks good.  Submitting
manually.