Update download page to deal with new DANGEROUS_CONTENT state.

Update download page to deal with new DANGEROUS_CONTENT state.

Downloads that are flagged as malicious based on content will have a danger type of DANGEROUS_CONTENT.

BUG=102540
TEST=Downloading something flagged as malicious by the SafeBrowsing service shows a warning dialog in the downloads page.


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

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

Should we go ahead and update strings here?  Otherwise we don't need to
downloads.js.

[noelutz, 2011-11-18 23:21:31]

On 2011/11/18 23:17:08, asanka wrote:
> Should we go ahead and update strings here?  Otherwise we don't need to
> downloads.js.

lgtm,
thanks.

[asanka, 2011-11-18 23:59:36]

[+rdsmith as well]

I added active_downloads.js.  These two are the consumers of
CreateDownloadItemValue()'s output.

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

Exploding the reviewers: Adding Ben for downloads.js (I think of him as the
owner of that file, to the extent we have one) and Achuith for active_downloads*
(ditto).  I don't think I have anything to add beyond those folks, so I'm taking
myself off the reviewers list.  

Achuith, for context: We're adding in safebrowsing checks at the end of the
download which may result in the download transitioning from safe to dangerous
after being displayed to the user if the has matches a malware list.  So this
review is both "does this code look good to you in itself" and "do you think
there will be any particular problems with the active_downloads UI if a
previously safe download transitions to dangerous?"

http://codereview.chromium.org/8558029/diff/4001/chrome/browser/download/down...
File chrome/browser/download/download_util.cc (right):

http://codereview.chromium.org/8558029/diff/4001/chrome/browser/download/down...
chrome/browser/download/download_util.cc:100: // MAYBE_DANGEROUS_CONTENT.
For consistency, I'd think that either NOT_DANGEROUS shouldn't be in the above
list, or MAYBE_DANGEROUS_CONTENT should be combined with NOT_DANGEROUS.

http://codereview.chromium.org/8558029/diff/4001/chrome/browser/resources/dow...
File chrome/browser/resources/downloads.js (right):

http://codereview.chromium.org/8558029/diff/4001/chrome/browser/resources/dow...
chrome/browser/resources/downloads.js:308: DANGEROUS_CONTENT:
"DANGEROUS_CONTENT"
I presume you've thought about the downloads page and don't believe that the
safe->dangerous transition will cause any UI problems?  (I don't think so, but I
think you're in a better position to say than I am.)

[Randy Smith (Not in Mondays), 2011-11-21 01:19:02]

Argh.  Let's try that again, thsi time with real reviewer edits :-J.

Exploding the reviewers: Adding Ben for downloads.js (I think of him as the
owner of that file, to the extent we have one) and Achuith for active_downloads*
(ditto).  I don't think I have anything to add beyond those folks, so I'm taking
myself off the reviewers list.  

Achuith, for context: We're adding in safebrowsing checks at the end of the
download which may result in the download transitioning from safe to dangerous
after being displayed to the user if the has matches a malware list.  So this
review is both "does this code look good to you in itself" and "do you think
there will be any particular problems with the active_downloads UI if a
previously safe download transitions to dangerous?"

[benjhayden, 2011-11-21 16:21:55]

http://codereview.chromium.org/8558029/diff/4001/chrome/browser/resources/act...
File chrome/browser/resources/active_downloads.js (right):

http://codereview.chromium.org/8558029/diff/4001/chrome/browser/resources/act...
chrome/browser/resources/active_downloads.js:386: dangerText =
localStrings.getString('dangerousurl');
Do we not want a different string for dangerous content? I thought that
dangerous content was a stronger signal than dangerous url?

http://codereview.chromium.org/8558029/diff/4001/chrome/browser/resources/dow...
File chrome/browser/resources/downloads.js (right):

http://codereview.chromium.org/8558029/diff/4001/chrome/browser/resources/dow...
chrome/browser/resources/downloads.js:308: DANGEROUS_CONTENT:
"DANGEROUS_CONTENT"
On 2011/11/21 01:18:15, rdsmith wrote:
> I presume you've thought about the downloads page and don't believe that the
> safe->dangerous transition will cause any UI problems?  (I don't think so, but
I
> think you're in a better position to say than I am.)

It looks to me like Download.prototype.update supports this, but a manual test
would make me feel better. :-)

[asanka, 2011-11-21 17:08:57]

http://codereview.chromium.org/8558029/diff/4001/chrome/browser/download/down...
File chrome/browser/download/download_util.cc (right):

http://codereview.chromium.org/8558029/diff/4001/chrome/browser/download/down...
chrome/browser/download/download_util.cc:100: // MAYBE_DANGEROUS_CONTENT.
On 2011/11/21 01:18:15, rdsmith wrote:
> For consistency, I'd think that either NOT_DANGEROUS shouldn't be in the above
> list, or MAYBE_DANGEROUS_CONTENT should be combined with NOT_DANGEROUS.

Good point. I'll remove the NOT_DANGEROUS string, rather than introduce a string
for MAYBE_DANGEROUS_CONTENT so that we don't define strings we don't use.

http://codereview.chromium.org/8558029/diff/4001/chrome/browser/resources/act...
File chrome/browser/resources/active_downloads.js (right):

http://codereview.chromium.org/8558029/diff/4001/chrome/browser/resources/act...
chrome/browser/resources/active_downloads.js:386: dangerText =
localStrings.getString('dangerousurl');
On 2011/11/21 16:21:55, b s h wrote:
> Do we not want a different string for dangerous content? I thought that
> dangerous content was a stronger signal than dangerous url?

I'll defer to Noe' on whether we are going to have different  messages for
DANGEROUS_URL and DANGEROUS_CONTENT (my understanding is we are going to use the
same string).  However, I think we do need a new string here that incorporates
(at least) the download filename.  This will probably happen in a different CL.

http://codereview.chromium.org/8558029/diff/4001/chrome/browser/resources/dow...
File chrome/browser/resources/downloads.js (right):

http://codereview.chromium.org/8558029/diff/4001/chrome/browser/resources/dow...
chrome/browser/resources/downloads.js:308: DANGEROUS_CONTENT:
"DANGEROUS_CONTENT"
On 2011/11/21 16:21:55, b s h wrote:
> On 2011/11/21 01:18:15, rdsmith wrote:
> > I presume you've thought about the downloads page and don't believe that the
> > safe->dangerous transition will cause any UI problems?  (I don't think so,
but
> I
> > think you're in a better position to say than I am.)
> 
> It looks to me like Download.prototype.update supports this, but a manual test
> would make me feel better. :-)

I verified the behavior manually for the downloads page. I've yet to verify it
for the active_downloads page.

[noelutz, 2011-11-21 18:17:52]

http://codereview.chromium.org/8558029/diff/4001/chrome/browser/resources/act...
File chrome/browser/resources/active_downloads.js (right):

http://codereview.chromium.org/8558029/diff/4001/chrome/browser/resources/act...
chrome/browser/resources/active_downloads.js:386: dangerText =
localStrings.getString('dangerousurl');
On 2011/11/21 17:08:57, asanka wrote:
> On 2011/11/21 16:21:55, b s h wrote:
> > Do we not want a different string for dangerous content? I thought that
> > dangerous content was a stronger signal than dangerous url?
> 
> I'll defer to Noe' on whether we are going to have different  messages for
> DANGEROUS_URL and DANGEROUS_CONTENT (my understanding is we are going to use
the
> same string).  However, I think we do need a new string here that incorporates
> (at least) the download filename.  This will probably happen in a different
CL.

We can use the same warning for now.  I like the idea of adding the file name
though.

[benjhayden, 2011-11-21 18:20:40]

LGTM

[achuithb, 2011-11-21 21:34:36]

Do we have a test for the situation Randy describes (even a manual test) - I'd
like to test DANGEROUS_CONTENT and make sure it works as expected on cros once
this lands.

http://codereview.chromium.org/8558029/diff/7003/chrome/browser/download/down...
File chrome/browser/download/download_util.cc (right):

http://codereview.chromium.org/8558029/diff/7003/chrome/browser/download/down...
chrome/browser/download/download_util.cc:102: // NOT_DANGEROUS or
MAYBE_DANGEROUS_CONTENT.
Does a NOTREACHED() make sense here? If we ever get here the UI will probably
not print anything but have the confirm/deny buttons.

http://codereview.chromium.org/8558029/diff/7003/chrome/browser/resources/dow...
File chrome/browser/resources/downloads.js (right):

http://codereview.chromium.org/8558029/diff/7003/chrome/browser/resources/dow...
chrome/browser/resources/downloads.js:467: // danger_url_desc is also used by
DANGEROUS_CONTENT
nit: period at the end of comment?

[asanka, 2011-11-21 22:46:58]

Thanks for the reviews!

http://codereview.chromium.org/8558029/diff/7003/chrome/browser/download/down...
File chrome/browser/download/download_util.cc (right):

http://codereview.chromium.org/8558029/diff/7003/chrome/browser/download/down...
chrome/browser/download/download_util.cc:102: // NOT_DANGEROUS or
MAYBE_DANGEROUS_CONTENT.
On 2011/11/21 21:34:36, achuith.bhandarkar wrote:
> Does a NOTREACHED() make sense here? If we ever get here the UI will probably
> not print anything but have the confirm/deny buttons.

It did have a NOTREACHED(), but I removed it because the call-site has a DCHECK
for checking that |danger_type| is one of the three.  But it makes sense to have
NOTREACHED() here as well.

http://codereview.chromium.org/8558029/diff/7003/chrome/browser/resources/dow...
File chrome/browser/resources/downloads.js (right):

http://codereview.chromium.org/8558029/diff/7003/chrome/browser/resources/dow...
chrome/browser/resources/downloads.js:467: // danger_url_desc is also used by
DANGEROUS_CONTENT
On 2011/11/21 21:34:36, achuith.bhandarkar wrote:
> nit: period at the end of comment?

Done.

[commit-bot: I haz the power, 2011-11-21 23:10:19]

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

[commit-bot: I haz the power, 2011-11-22 00:20:44]

Change committed as 111051