Propagate the SafeBrowsing download protection verdict to the DownloadItem.

Propagate the SafeBrowsing download protection verdict to the DownloadItem.
This is where the UI will be notified when a malicious binary is being
downloaded.  The UI currently does not support displaying a warning after
the download started.  We'll fix that in a separate CL.

BUG=102540
TEST=No visible changes.


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

[noelutz, 2011-11-16 00:11:46]

On 2011/11/15 21:50:14, noelutz wrote:

please wait before reviewing this CL.  I made a mistake while merging.

[noelutz, 2011-11-16 00:49:00]

On 2011/11/16 00:11:46, noelutz wrote:
> On 2011/11/15 21:50:14, noelutz wrote:
> 
> please wait before reviewing this CL.  I made a mistake while merging.

CL is ready for review now. Thanks,
noe.

[asanka, 2011-11-16 03:23:48]

http://codereview.chromium.org/8468020/diff/3007/chrome/browser/download/chro...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

http://codereview.chromium.org/8468020/diff/3007/chrome/browser/download/chro...
chrome/browser/download/chrome_download_manager_delegate.cc:299:
ChromeDownloadManagerDelegate::download_protection_service() {
Nit: This is not a trivial accessor.

http://codereview.chromium.org/8468020/diff/3007/chrome/browser/download/chro...
chrome/browser/download/chrome_download_manager_delegate.cc:341: //
TODO(noelutz): ensure that URL warning was not already shown.
Shall we, instead, ensure that the DownloadItem is only marked as (any kind of)
dangerous only once? Otherwise it would be difficult to determine what needs to
be done when an observer notices that a DI was marked as dangerous.

http://codereview.chromium.org/8468020/diff/3007/content/browser/download/dow...
File content/browser/download/download_item.h (right):

http://codereview.chromium.org/8468020/diff/3007/content/browser/download/dow...
content/browser/download/download_item.h:92: DANGEROUS_CONTENT,
Note that this should also be updated in download_extension_api.

[noelutz, 2011-11-16 03:50:26]

Thanks.  Please take another look.
noé.

http://codereview.chromium.org/8468020/diff/3007/chrome/browser/download/chro...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

http://codereview.chromium.org/8468020/diff/3007/chrome/browser/download/chro...
chrome/browser/download/chrome_download_manager_delegate.cc:299:
ChromeDownloadManagerDelegate::download_protection_service() {
On 2011/11/16 03:23:48, asanka wrote:
> Nit: This is not a trivial accessor.

Done.

http://codereview.chromium.org/8468020/diff/3007/chrome/browser/download/chro...
chrome/browser/download/chrome_download_manager_delegate.cc:341: //
TODO(noelutz): ensure that URL warning was not already shown.
On 2011/11/16 03:23:48, asanka wrote:
> Shall we, instead, ensure that the DownloadItem is only marked as (any kind
of)
> dangerous only once? Otherwise it would be difficult to determine what needs
to
> be done when an observer notices that a DI was marked as dangerous.

Much easier. Done.

http://codereview.chromium.org/8468020/diff/3007/content/browser/download/dow...
File content/browser/download/download_item.h (right):

http://codereview.chromium.org/8468020/diff/3007/content/browser/download/dow...
content/browser/download/download_item.h:92: DANGEROUS_CONTENT,
On 2011/11/16 03:23:48, asanka wrote:
> Note that this should also be updated in download_extension_api.

Done.

[asanka, 2011-11-16 04:55:12]

[+benjhayden: for the addition to the extensions API]

Sorry I missed it in the previous pass.
chrome/common/extensions/api/extension_api.json also needs to be updated for the
new danger state constant (add a new property named DANGER_CONTENT to
chrome.experimental.downloads namespace).  I added Ben to the review for his
input on changes needed to the extensions API.

[noelutz, 2011-11-16 05:10:13]

Done.  I couldn't find any documentation to update in
chrome/common/extensions/docs/static/.  Let me know if I'm missing something.

thanks,
noé.

On 2011/11/16 04:55:12, asanka wrote:
> [+benjhayden: for the addition to the extensions API]
> 
> Sorry I missed it in the previous pass.
> chrome/common/extensions/api/extension_api.json also needs to be updated for
the
> new danger state constant (add a new property named DANGER_CONTENT to
> chrome.experimental.downloads namespace).  I added Ben to the review for his
> input on changes needed to the extensions API.

[benjhayden, 2011-11-16 15:45:39]

download_extension_api.cc and extension_api.json LGTM.

[benjhayden, 2011-11-16 15:57:22]

Still LGTM, one minor request.

http://codereview.chromium.org/8468020/diff/3011/content/browser/download/dow...
File content/browser/download/download_item.cc (right):

http://codereview.chromium.org/8468020/diff/3011/content/browser/download/dow...
content/browser/download/download_item.cc:105: // Note: |dangerous_url| takes
precedence over both |dangerous_content| and
Thank you for fixing the incorrect use of the verb "overweight" (which means to
put too much weight on or to overload), but this looks both redundant and
repetitive, and I'm not just being cute: "du before both dc and dc, and du
before dc."
Is this Note even necessary? This method is not that complicated. If you want a
comment here, perhaps explain why we're more sure of url dangers than content
dangers, and content dangers than file dangers.

[noelutz, 2011-11-16 16:48:35]

Thanks,
noé.

[Randy Smith (Not in Mondays), 2011-11-16 17:52:41]

Comments on interface, datastructures, and basic flow.  I don't think these'll
change the detailed flow too much, so I'll do a more detailed round immediately
following this.

http://codereview.chromium.org/8468020/diff/11001/chrome/browser/download/chr...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

http://codereview.chromium.org/8468020/diff/11001/chrome/browser/download/chr...
chrome/browser/download/chrome_download_manager_delegate.cc:438:
service->IsSupportedFileType(state.suggested_path.BaseName())) {
Asanka: I just wanted to explicitly check with you (with your file name
determination hat on) that we have the correct file type (all hints incorporated
in) at this point?

http://codereview.chromium.org/8468020/diff/11001/content/browser/download/do...
File content/browser/download/download_state_info.h (right):

http://codereview.chromium.org/8468020/diff/11001/content/browser/download/do...
content/browser/download/download_state_info.h:62: bool is_dangerous_content;
Any reason not to shift over to using an enum (maybe the standard enum?) for
these three?  The proliferation of bools is a bit disturbing.

http://codereview.chromium.org/8468020/diff/11001/content/browser/download/do...
content/browser/download/download_state_info.h:67: bool needs_quarantine_file;
I'd like to add this to the above enum, if we could (all the way into
DownloadItem is fine)--something like "MAYBE_DANGEROUS_CONTENT"?

[Randy Smith (Not in Mondays), 2011-11-16 18:04:03]

What are the plans for adding automated testing for these cases?

http://codereview.chromium.org/8468020/diff/11001/chrome/browser/download/chr...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

http://codereview.chromium.org/8468020/diff/11001/chrome/browser/download/chr...
chrome/browser/download/chrome_download_manager_delegate.cc:89: #if
defined(ENABLE_SAFE_BROWSING)
GetDownloadProtectionService() isn't inside of an #ifdef; does that mean we
could get rid of this ifdef as well?

http://codereview.chromium.org/8468020/diff/11001/chrome/browser/download/dow...
File chrome/browser/download/download_extension_api.cc (right):

http://codereview.chromium.org/8468020/diff/11001/chrome/browser/download/dow...
chrome/browser/download/download_extension_api.cc:86: case
DownloadItem::DANGEROUS_CONTENT: return kDangerContent;
I think (Asanka?) that we also want to export the "quarantine" info up here,
since that might conceivably have UI effects.  More specifically, I think we
want to export the MAYBE_DANGEROUS_CONTENT enum (if we go that way) so that the
UI knows it might in the future have to show a dangerous download warning for
this item.  This isn't high priority as I don't have a use case, but given that
one of the goals of the extension system is to support everything an alternative
UI might want to have, it seems worth making this visible.

http://codereview.chromium.org/8468020/diff/11001/chrome/common/extensions/ap...
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/8468020/diff/11001/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.json:4323: "description": "The
download's content is malicious."
Why the difference in wording?  ("known to be" vs. "is")

[asanka, 2011-11-16 18:34:05]

http://codereview.chromium.org/8468020/diff/11001/chrome/browser/download/chr...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

http://codereview.chromium.org/8468020/diff/11001/chrome/browser/download/chr...
chrome/browser/download/chrome_download_manager_delegate.cc:438:
service->IsSupportedFileType(state.suggested_path.BaseName())) {
On 2011/11/16 17:52:41, rdsmith wrote:
> Asanka: I just wanted to explicitly check with you (with your file name
> determination hat on) that we have the correct file type (all hints
incorporated
> in) at this point?

Good question.

At this point state.suggested_path contains the filename derived from the
request/response data (URL, Content-Disposition, suggested name, etc.) but not
the filename that the user might choose from a "Save As" dialog.  Note that the
extension might not be what you expect for the content type.

Noe: If we were to determine an innocuous extension (because of a missing or
malicious C-D header, malicious download attribute etc.) and the user later
picks a dangerous extension from the Save As dialog, do we handle it correctly?

http://codereview.chromium.org/8468020/diff/11001/chrome/browser/download/dow...
File chrome/browser/download/download_extension_api.cc (right):

http://codereview.chromium.org/8468020/diff/11001/chrome/browser/download/dow...
chrome/browser/download/download_extension_api.cc:60: const char
kDangerContent[] = "content";
Nit: These should be ordered alphabetically.

http://codereview.chromium.org/8468020/diff/11001/chrome/browser/download/dow...
chrome/browser/download/download_extension_api.cc:86: case
DownloadItem::DANGEROUS_CONTENT: return kDangerContent;
On 2011/11/16 18:04:04, rdsmith wrote:
> I think (Asanka?) that we also want to export the "quarantine" info up here,
> since that might conceivably have UI effects.  More specifically, I think we
> want to export the MAYBE_DANGEROUS_CONTENT enum (if we go that way) so that
the
> UI knows it might in the future have to show a dangerous download warning for
> this item.  This isn't high priority as I don't have a use case, but given
that
> one of the goals of the extension system is to support everything an
alternative
> UI might want to have, it seems worth making this visible.

I can see how it could be useful.  However I'm not quite certain whether we want
to commit to making it a reliable signal.

[Randy Smith (Not in Mondays), 2011-11-16 20:27:58]

On 2011/11/16 18:34:05, asanka wrote:
>
http://codereview.chromium.org/8468020/diff/11001/chrome/browser/download/chr...
> File chrome/browser/download/chrome_download_manager_delegate.cc (right):
> 
>
http://codereview.chromium.org/8468020/diff/11001/chrome/browser/download/chr...
> chrome/browser/download/chrome_download_manager_delegate.cc:438:
> service->IsSupportedFileType(state.suggested_path.BaseName())) {
> On 2011/11/16 17:52:41, rdsmith wrote:
> > Asanka: I just wanted to explicitly check with you (with your file name
> > determination hat on) that we have the correct file type (all hints
> incorporated
> > in) at this point?
> 
> Good question.
> 
> At this point state.suggested_path contains the filename derived from the
> request/response data (URL, Content-Disposition, suggested name, etc.) but not
> the filename that the user might choose from a "Save As" dialog.  Note that
the
> extension might not be what you expect for the content type.
> 
> Noe: If we were to determine an innocuous extension (because of a missing or
> malicious C-D header, malicious download attribute etc.) and the user later
> picks a dangerous extension from the Save As dialog, do we handle it
correctly?

I'm inclined to say that if we've made a judgement based on all the information
we have coming in, and the user chooses a different extension, the results are
their look-out.  They could always use "mv" after it was downloaded, after all. 
I.e. it sounds like this isn't something to worry about--all aspects of our file
picking algorithm have had their say here.

http://codereview.chromium.org/8468020/diff/11001/chrome/browser/download/dow...
> chrome/browser/download/download_extension_api.cc:86: case
> DownloadItem::DANGEROUS_CONTENT: return kDangerContent;
> On 2011/11/16 18:04:04, rdsmith wrote:
> > I think (Asanka?) that we also want to export the "quarantine" info up here,
> > since that might conceivably have UI effects.  More specifically, I think we
> > want to export the MAYBE_DANGEROUS_CONTENT enum (if we go that way) so that
> the
> > UI knows it might in the future have to show a dangerous download warning
for
> > this item.  This isn't high priority as I don't have a use case, but given
> that
> > one of the goals of the extension system is to support everything an
> alternative
> > UI might want to have, it seems worth making this visible.
> 
> I can see how it could be useful.  However I'm not quite certain whether we
want
> to commit to making it a reliable signal.

Hmmm.  Fair; a better interface is probably "be prepared for non-dangerous
downloads to become dangerous at any time".  Should we explicitly sign up for
the behavior that that transition will occur at most once?  (Not your problem,
Noe'; with regard to this CL, the take-home is "Don't expose that state").

[noelutz, 2011-11-16 23:15:07]

Please take another look.
Thanks,
noe.

http://codereview.chromium.org/8468020/diff/11001/chrome/browser/download/chr...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

http://codereview.chromium.org/8468020/diff/11001/chrome/browser/download/chr...
chrome/browser/download/chrome_download_manager_delegate.cc:89: #if
defined(ENABLE_SAFE_BROWSING)
On 2011/11/16 18:04:04, rdsmith wrote:
> GetDownloadProtectionService() isn't inside of an #ifdef; does that mean we
> could get rid of this ifdef as well?

I think it's needed. changed.

http://codereview.chromium.org/8468020/diff/11001/chrome/browser/download/chr...
chrome/browser/download/chrome_download_manager_delegate.cc:438:
service->IsSupportedFileType(state.suggested_path.BaseName())) {
On 2011/11/16 18:34:05, asanka wrote:
> On 2011/11/16 17:52:41, rdsmith wrote:
> > Asanka: I just wanted to explicitly check with you (with your file name
> > determination hat on) that we have the correct file type (all hints
> incorporated
> > in) at this point?
> 
> Good question.
> 
> At this point state.suggested_path contains the filename derived from the
> request/response data (URL, Content-Disposition, suggested name, etc.) but not
> the filename that the user might choose from a "Save As" dialog.  Note that
the
> extension might not be what you expect for the content type.
> 
> Noe: If we were to determine an innocuous extension (because of a missing or
> malicious C-D header, malicious download attribute etc.) and the user later
> picks a dangerous extension from the Save As dialog, do we handle it
correctly?

I don't think so.  I think that's good enough for now though.  Added a TODO. 
Note that the dangerous file warning has the same issue afaik.

http://codereview.chromium.org/8468020/diff/11001/chrome/browser/download/dow...
File chrome/browser/download/download_extension_api.cc (right):

http://codereview.chromium.org/8468020/diff/11001/chrome/browser/download/dow...
chrome/browser/download/download_extension_api.cc:60: const char
kDangerContent[] = "content";
On 2011/11/16 18:34:05, asanka wrote:
> Nit: These should be ordered alphabetically.

Done.

http://codereview.chromium.org/8468020/diff/11001/chrome/browser/download/dow...
chrome/browser/download/download_extension_api.cc:86: case
DownloadItem::DANGEROUS_CONTENT: return kDangerContent;
On 2011/11/16 18:34:05, asanka wrote:
> On 2011/11/16 18:04:04, rdsmith wrote:
> > I think (Asanka?) that we also want to export the "quarantine" info up here,
> > since that might conceivably have UI effects.  More specifically, I think we
> > want to export the MAYBE_DANGEROUS_CONTENT enum (if we go that way) so that
> the
> > UI knows it might in the future have to show a dangerous download warning
for
> > this item.  This isn't high priority as I don't have a use case, but given
> that
> > one of the goals of the extension system is to support everything an
> alternative
> > UI might want to have, it seems worth making this visible.
> 
> I can see how it could be useful.  However I'm not quite certain whether we
want
> to commit to making it a reliable signal.

Propagated the maybe.

http://codereview.chromium.org/8468020/diff/11001/chrome/common/extensions/ap...
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/8468020/diff/11001/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.json:4323: "description": "The
download's content is malicious."
On 2011/11/16 18:04:04, rdsmith wrote:
> Why the difference in wording?  ("known to be" vs. "is")

Done.

http://codereview.chromium.org/8468020/diff/11001/content/browser/download/do...
File content/browser/download/download_state_info.h (right):

http://codereview.chromium.org/8468020/diff/11001/content/browser/download/do...
content/browser/download/download_state_info.h:62: bool is_dangerous_content;
On 2011/11/16 17:52:41, rdsmith wrote:
> Any reason not to shift over to using an enum (maybe the standard enum?) for
> these three?  The proliferation of bools is a bit disturbing.

Done.

http://codereview.chromium.org/8468020/diff/11001/content/browser/download/do...
content/browser/download/download_state_info.h:67: bool needs_quarantine_file;
On 2011/11/16 17:52:41, rdsmith wrote:
> I'd like to add this to the above enum, if we could (all the way into
> DownloadItem is fine)--something like "MAYBE_DANGEROUS_CONTENT"?

Done.

[Randy Smith (Not in Mondays), 2011-11-17 16:49:31]

I'll give my LGTM now, since I'm ok with the code going in with both of the
issues I raise below.  But I'd be curious if we could make the changes I
suggest, and if not, what the problems are with them.

Thanks!

http://codereview.chromium.org/8468020/diff/11001/chrome/browser/download/chr...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

http://codereview.chromium.org/8468020/diff/11001/chrome/browser/download/chr...
chrome/browser/download/chrome_download_manager_delegate.cc:89: #if
defined(ENABLE_SAFE_BROWSING)
On 2011/11/16 23:15:07, noelutz wrote:
> On 2011/11/16 18:04:04, rdsmith wrote:
> > GetDownloadProtectionService() isn't inside of an #ifdef; does that mean we
> > could get rid of this ifdef as well?
> 
> I think it's needed. changed.

Hmmm.  So it would seem to me that we could also arrange for a stub definition
of DownloadProtectionService/GetDownloadProtectionService in the case where
ENABLE_SAFE_BROWSING wasn't defined, which would result in cleaner code at point
of call.  What's the problem with that approach?

Note that this is a nit: I'm ok with the code as written.  I'd just like it
better if more of the #ifdef was in SafeBrowsing specific code.

http://codereview.chromium.org/8468020/diff/19001/content/browser/download/do...
File content/browser/download/download_state_info.h (right):

http://codereview.chromium.org/8468020/diff/19001/content/browser/download/do...
content/browser/download/download_state_info.h:18: enum DangerType {
Given the choice, I'd rather this enum was in DownloadItem rather than
DownloadStateInfo.  The enum has wide visibility (all the way up into the UI)
and I'd rather not increase the number of classes that code that touches it have
to know about.  It's not a big deal--if there's some design reason I'm missing
I'd be ok with it here.  But my sense is that (given current state of code) this
enum belongs in DownloadItem, not here.

[noelutz, 2011-11-17 17:05:44]

Thanks,
noé.

http://codereview.chromium.org/8468020/diff/11001/chrome/browser/download/chr...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

http://codereview.chromium.org/8468020/diff/11001/chrome/browser/download/chr...
chrome/browser/download/chrome_download_manager_delegate.cc:89: #if
defined(ENABLE_SAFE_BROWSING)
On 2011/11/17 16:49:31, rdsmith wrote:
> On 2011/11/16 23:15:07, noelutz wrote:
> > On 2011/11/16 18:04:04, rdsmith wrote:
> > > GetDownloadProtectionService() isn't inside of an #ifdef; does that mean
we
> > > could get rid of this ifdef as well?
> > 
> > I think it's needed. changed.
> 
> Hmmm.  So it would seem to me that we could also arrange for a stub definition
> of DownloadProtectionService/GetDownloadProtectionService in the case where
> ENABLE_SAFE_BROWSING wasn't defined, which would result in cleaner code at
point
> of call.  What's the problem with that approach?
> 
> Note that this is a nit: I'm ok with the code as written.  I'd just like it
> better if more of the #ifdef was in SafeBrowsing specific code.

Agreed.  If that's OK I would like to do that in a separate CL.

http://codereview.chromium.org/8468020/diff/19001/content/browser/download/do...
File content/browser/download/download_state_info.h (right):

http://codereview.chromium.org/8468020/diff/19001/content/browser/download/do...
content/browser/download/download_state_info.h:18: enum DangerType {
On 2011/11/17 16:49:31, rdsmith wrote:
> Given the choice, I'd rather this enum was in DownloadItem rather than
> DownloadStateInfo.  The enum has wide visibility (all the way up into the UI)
> and I'd rather not increase the number of classes that code that touches it
have
> to know about.  It's not a big deal--if there's some design reason I'm missing
> I'd be ok with it here.  But my sense is that (given current state of code)
this
> enum belongs in DownloadItem, not here.

I thought of that but then you have a circular include which isn't ideal. 
DownloadStateInfo includes DownloadItem which includes DownloadStateInfo.

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

Thanks,
noé.

[Randy Smith (Not in Mondays), 2011-11-17 17:14:29]

On 2011/11/17 17:05:44, noelutz wrote:
> Thanks,
> noé.
> 
>
http://codereview.chromium.org/8468020/diff/11001/chrome/browser/download/chr...
> File chrome/browser/download/chrome_download_manager_delegate.cc (right):
> 
>
http://codereview.chromium.org/8468020/diff/11001/chrome/browser/download/chr...
> chrome/browser/download/chrome_download_manager_delegate.cc:89: #if
> defined(ENABLE_SAFE_BROWSING)
> On 2011/11/17 16:49:31, rdsmith wrote:
> > On 2011/11/16 23:15:07, noelutz wrote:
> > > On 2011/11/16 18:04:04, rdsmith wrote:
> > > > GetDownloadProtectionService() isn't inside of an #ifdef; does that mean
> we
> > > > could get rid of this ifdef as well?
> > > 
> > > I think it's needed. changed.
> > 
> > Hmmm.  So it would seem to me that we could also arrange for a stub
definition
> > of DownloadProtectionService/GetDownloadProtectionService in the case where
> > ENABLE_SAFE_BROWSING wasn't defined, which would result in cleaner code at
> point
> > of call.  What's the problem with that approach?
> > 
> > Note that this is a nit: I'm ok with the code as written.  I'd just like it
> > better if more of the #ifdef was in SafeBrowsing specific code.
> 
> Agreed.  If that's OK I would like to do that in a separate CL.

Sure thing.

http://codereview.chromium.org/8468020/diff/19001/content/browser/download/do...
> File content/browser/download/download_state_info.h (right):
> 
>
http://codereview.chromium.org/8468020/diff/19001/content/browser/download/do...
> content/browser/download/download_state_info.h:18: enum DangerType {
> On 2011/11/17 16:49:31, rdsmith wrote:
> > Given the choice, I'd rather this enum was in DownloadItem rather than
> > DownloadStateInfo.  The enum has wide visibility (all the way up into the
UI)
> > and I'd rather not increase the number of classes that code that touches it
> have
> > to know about.  It's not a big deal--if there's some design reason I'm
missing
> > I'd be ok with it here.  But my sense is that (given current state of code)
> this
> > enum belongs in DownloadItem, not here.
> 
> I thought of that but then you have a circular include which isn't ideal. 
> DownloadStateInfo includes DownloadItem which includes DownloadStateInfo.

Gotcha.  Would you then (doesn't have to be in this CL) do a typedef so that
DownloadItem::DangerType still exists, and anything "above" or in DownloadItem
uses that typedef?

[commit-bot: I haz the power, 2011-11-17 17:43:19]

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

[commit-bot: I haz the power, 2011-11-17 17:43:24]

Can't apply patch for file chrome/browser/download/download_manager_unittest.cc.
While running patch -p1 --forward --force;
patching file chrome/browser/download/download_manager_unittest.cc
Hunk #1 succeeded at 143 (offset -2 lines).
Hunk #2 succeeded at 251 with fuzz 1 (offset 7 lines).
Hunk #3 succeeded at 424 (offset -19 lines).
Hunk #4 FAILED at 458.
1 out of 4 hunks FAILED -- saving rejects to file
chrome/browser/download/download_manager_unittest.cc.rej

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

I'm not sure typedefing the enum will help much.  To test if the danger type has
a particular value we would still have to include the state info header.

noé.

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

On 2011/11/17 18:14:21, noelutz wrote:
> I'm not sure typedefing the enum will help much.  To test if the danger type
has
> a particular value we would still have to include the state info header.
> 
> noé.

It's an interface issue.  download_item.h includes download_state_info.h, so
nobody else will need to know about download_state_info (header or structure)
above DownloadItem.

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

On Thu, Nov 17, 2011 at 10:15 AM,  <rdsmith@chromium.org> wrote:
> On 2011/11/17 18:14:21, noelutz wrote:
>>
>> I'm not sure typedefing the enum will help much.  To test if the danger
>> type
>
> has
>>
>> a particular value we would still have to include the state info header.
>
>> noé.
>
> It's an interface issue.  download_item.h includes download_state_info.h, so
> nobody else will need to know about download_state_info (header or
> structure)
> above DownloadItem.

Alright.  To achieve that it sounds like we need to move the enum back
to download_item.h.  I'll send out a separate CL for that.
thanks,
noé.
>
>
> http://codereview.chromium.org/8468020/
>



-- 
-|||--

[Randy Smith (Not in Mondays), 2011-11-17 18:39:07]

On 2011/11/17 18:16:45, noelutz wrote:
> On Thu, Nov 17, 2011 at 10:15 AM,  <mailto:rdsmith@chromium.org> wrote:
> > On 2011/11/17 18:14:21, noelutz wrote:
> >>
> >> I'm not sure typedefing the enum will help much. &nbsp;To test if the
danger
> >> type
> >
> > has
> >>
> >> a particular value we would still have to include the state info header.
> >
> >> noé.
> >
> > It's an interface issue. &nbsp;download_item.h includes
download_state_info.h, so
> > nobody else will need to know about download_state_info (header or
> > structure)
> > above DownloadItem.
> 
> Alright.  To achieve that it sounds like we need to move the enum back
> to download_item.h.  I'll send out a separate CL for that.
> thanks,
> noé.
> >
> >
> > http://codereview.chromium.org/8468020/
> >
> 
> 
> 
> -- 
> -|||--

I'm not sure we're communicating; at least, I don't think we need to move the
enum.  Why don't you commit this CL as it is, and we can work this out later? 
I'll put fifteen minutes into trying to do what I'm imagining and run it by you
sometime late this week early next.

[commit-bot: I haz the power, 2011-11-17 20:20:46]

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

[commit-bot: I haz the power, 2011-11-17 20:20:54]

Presubmit check for 8468020-30001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for: chrome/common/extensions/api/extension_api.json

Presubmit checks took 2.8s to calculate.

[noelutz, 2011-11-17 20:27:31]

+mihaip for LGTM for my changes to extension_api.json.

thanks,
noé.

[Mihai Parparita -not on Chrome, 2011-11-17 20:30:25]

LGTM

http://codereview.chromium.org/8468020/diff/30001/chrome/common/extensions/ap...
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/8468020/diff/30001/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.json:4318: "description": "The
download's URL is malicious."
Is the change to "malicious" here but the keeping of "suspicious" above for
DANGER_FILE intentional?

[noelutz, 2011-11-17 21:25:16]

thanks,
noé.

http://codereview.chromium.org/8468020/diff/30001/chrome/common/extensions/ap...
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/8468020/diff/30001/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.json:4318: "description": "The
download's URL is malicious."
On 2011/11/17 20:30:25, Mihai Parparita wrote:
> Is the change to "malicious" here but the keeping of "suspicious" above for
> DANGER_FILE intentional?

It is.

DANGER_FILE == filename is suspicious. E.g., .exe, .dmg
DANGER_URL == SafeBrowsing servers have seen malware on this URL before.
DANGER_CONTENT == SafeBrowsing servers have seen this content before and flagged
it as malicious.

[commit-bot: I haz the power, 2011-11-17 21:31:04]

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

[commit-bot: I haz the power, 2011-11-17 22:47:43]

Change committed as 110583