enable download protection on CrOS

Enable download protection on CrOS

BUG=523413
Committed: https://crrev.com/44f338d2d3e5b0a93de2d48a0202d0aa69f439f0
Cr-Commit-Position: refs/heads/master@{#355483}

[Jialiu Lin, 2015-09-25 00:09:22]

Patchset #2 (id:20001) has been deleted

[Jialiu Lin, 2015-09-25 17:37:58]

Patchset #2 (id:40001) has been deleted

[Jialiu Lin, 2015-09-25 20:01:25]

Patchset #2 (id:60001) has been deleted

[Jialiu Lin, 2015-09-25 20:01:29]

Patchset #2 (id:80001) has been deleted

[Jialiu Lin, 2015-09-25 23:09:52]

Patchset #2 (id:100001) has been deleted

[Jialiu Lin, 2015-09-28 21:22:51]

Patchset #2 (id:120001) has been deleted

[Jialiu Lin, 2015-10-05 19:51:12]

Patchset #5 (id:190001) has been deleted

[Jialiu Lin, 2015-10-05 19:51:17]

Patchset #4 (id:140002) has been deleted

[Jialiu Lin, 2015-10-05 19:51:21]

Patchset #3 (id:160001) has been deleted

[Jialiu Lin, 2015-10-05 19:51:27]

Patchset #2 (id:140001) has been deleted

[Jialiu Lin, 2015-10-05 20:45:32]

Patchset #2 (id:210001) has been deleted

[Jialiu Lin, 2015-10-08 21:49:14]

Patchset #2 (id:230001) has been deleted

[Jialiu Lin, 2015-10-08 22:27:38]

jialiul@chromium.org changed reviewers:
+ mattm@chromium.org, nparker@chromium.org

[Jialiu Lin, 2015-10-14 21:44:48]

jialiul@chromium.org changed reviewers:
+ yoshiki@chromium.org

[Jialiu Lin, 2015-10-14 21:50:26]

Hi yoshiki,
Could you take a look at this CL?  I made some changes to the
download_notification_browsertest.cc, I wanna make sure this CL will not break
your code. 
Also, really appreciate if you can let me know how to enable
download_notification on Chrome OS ( I tried set up
--enable-download-notification in /etc/chrome_dev.conf  file on Chrome OS, but
seems not working). 

You can refer to bug #523413 for more info. 

Thanks,
Jialiu

[Jialiu Lin, 2015-10-14 23:23:46]

On 2015/10/14 21:50:26, JialiuLin wrote:
> Hi yoshiki,
> Could you take a look at this CL?  I made some changes to the
> download_notification_browsertest.cc, I wanna make sure this CL will not break
> your code. 
> Also, really appreciate if you can let me know how to enable
> download_notification on Chrome OS ( I tried set up
> --enable-download-notification in /etc/chrome_dev.conf  file on Chrome OS, but
> seems not working). 
> 
> You can refer to bug #523413 for more info. 
> 
> Thanks,
> Jialiu

Hi Yoshiki, 
Just realized that enable-download-notification is already in chrome://flags. My
bad. 
I tested this CL against testsafebrowsing.appspot.com (Download Warnings 1-3)
when download notification is enabled. Everything looks fine. 
Let me know if I missed anything related to download notification. 

Thanks,

[Jialiu Lin, 2015-10-19 17:10:41]

A gentle ping to yoshiki~~
I wanna make sure this CL will not break download notification. Or let me know
who should I direct this CL to.  

Thanks,
Jialiu

[yoshiki, 2015-10-21 12:16:53]

yoshiki@chromium.org changed reviewers:
+ asanka@chromium.org

[yoshiki, 2015-10-21 12:16:54]

lgtm. Thank you for doing this.

Could you get an approval from asanka@? (the download owner)

[asanka, 2015-10-21 13:53:21]

Overall, at this point it might be worth it to introduce some macro to indicate
that client side checks are supported on a platform. But I don't feel too
strongly about it since it's only exposed out of safe_browsing in
DownloadItemImpl. For the latter, we should consider removing the #if defined()
conditional, and just consider a download as dangerous if it gets flagged by SB.
I'd do that in a separate change since that's a behavior change on Linux.

LGTM for this CL.

https://codereview.chromium.org/1348683003/diff/250001/chrome/browser/downloa...
File chrome/browser/download/notification/download_notification_browsertest.cc
(right):

https://codereview.chromium.org/1348683003/diff/250001/chrome/browser/downloa...
chrome/browser/download/notification/download_notification_browsertest.cc:237:
return NULL;
Nit: nullptr instead of NULL.

[Jialiu Lin, 2015-10-21 20:14:55]

Thank you, asanka and yoshiki for reviewing this. 
 
nparker and mattm, any additional comments?

https://codereview.chromium.org/1348683003/diff/250001/chrome/browser/downloa...
File chrome/browser/download/notification/download_notification_browsertest.cc
(right):

https://codereview.chromium.org/1348683003/diff/250001/chrome/browser/downloa...
chrome/browser/download/notification/download_notification_browsertest.cc:237:
return NULL;
On 2015/10/21 13:53:21, asanka wrote:
> Nit: nullptr instead of NULL.

Done.

[Nathan Parker, 2015-10-21 21:10:39]

lgtm

just nits

https://codereview.chromium.org/1348683003/diff/270001/chrome/browser/downloa...
File chrome/browser/download/notification/download_notification_browsertest.cc
(right):

https://codereview.chromium.org/1348683003/diff/270001/chrome/browser/downloa...
chrome/browser/download/notification/download_notification_browsertest.cc:234:
// Disable DownloadProtectionService in order to disable content checking.
Is this necessary just on ChromOS, or everywhere?

https://codereview.chromium.org/1348683003/diff/270001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/download_protection_service_unittest.cc
(right):

https://codereview.chromium.org/1348683003/diff/270001/chrome/browser/safe_br...
chrome/browser/safe_browsing/download_protection_service_unittest.cc:538: #if
defined(OS_WIN) || defined(OS_MACOSX) || defined(OS_CHROMEOS)
nit: How about replacing all of these with "#if OS_SUPPORTS_DPS" and define that
up top?  Makes it less likely we'll miss one.

https://codereview.chromium.org/1348683003/diff/270001/chrome/browser/safe_br...
chrome/browser/safe_browsing/download_protection_service_unittest.cc:689: // On
!(OS_WIN || OS_MACOSX) || defined(OS_CHROMEOS),
Should OS_CHROMEOS be in the ()'s?  Same below.

https://codereview.chromium.org/1348683003/diff/270001/content/browser/downlo...
File content/browser/download/download_item_impl.cc (right):

https://codereview.chromium.org/1348683003/diff/270001/content/browser/downlo...
content/browser/download/download_item_impl.cc:670: // TODO(noelutz): At this
point only the windows views and OSX UI supports
Comment needs updating, or removing.

[Jialiu Lin, 2015-10-22 01:38:54]

Thanks, Nathan!

https://codereview.chromium.org/1348683003/diff/270001/chrome/browser/downloa...
File chrome/browser/download/notification/download_notification_browsertest.cc
(right):

https://codereview.chromium.org/1348683003/diff/270001/chrome/browser/downloa...
chrome/browser/download/notification/download_notification_browsertest.cc:234:
// Disable DownloadProtectionService in order to disable content checking.
On 2015/10/21 21:10:39, nparker wrote:
> Is this necessary just on ChromOS, or everywhere?

Just for ChromeOS. In fact, this browsertest, as well as all the
download_notification code only run on ChromeOS.

https://codereview.chromium.org/1348683003/diff/270001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/download_protection_service_unittest.cc
(right):

https://codereview.chromium.org/1348683003/diff/270001/chrome/browser/safe_br...
chrome/browser/safe_browsing/download_protection_service_unittest.cc:538: #if
defined(OS_WIN) || defined(OS_MACOSX) || defined(OS_CHROMEOS)
On 2015/10/21 21:10:39, nparker wrote:
> nit: How about replacing all of these with "#if OS_SUPPORTS_DPS" and define
that
> up top?  Makes it less likely we'll miss one.
Resolved in person. Will clean up all the MACRO in a separate CL.

https://codereview.chromium.org/1348683003/diff/270001/chrome/browser/safe_br...
chrome/browser/safe_browsing/download_protection_service_unittest.cc:689: // On
!(OS_WIN || OS_MACOSX) || defined(OS_CHROMEOS),
On 2015/10/21 21:10:39, nparker wrote:
> Should OS_CHROMEOS be in the ()'s?  Same below.

Oops. nice catch

[mattm, 2015-10-22 01:43:28]

lgtm

[Jialiu Lin, 2015-10-22 02:29:31]

The CQ bit was checked by jialiul@chromium.org

[Jialiu Lin, 2015-10-22 02:29:31]

The patchset sent to the CQ was uploaded after l-g-t-m from asanka@chromium.org,
yoshiki@chromium.org, nparker@chromium.org
Link to the patchset: https://codereview.chromium.org/1348683003/#ps290001
(title: "nit")

[commit-bot: I haz the power, 2015-10-22 02:30:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1348683003/290001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1348683003/290001

[commit-bot: I haz the power, 2015-10-22 02:35:05]

Committed patchset #4 (id:290001)

[commit-bot: I haz the power, 2015-10-22 02:36:35]

Patchset 4 (id:??) landed as
https://crrev.com/44f338d2d3e5b0a93de2d48a0202d0aa69f439f0
Cr-Commit-Position: refs/heads/master@{#355483}