Implement safebrowsing download feedback service, enabled for dev & canary only.

Implement safebrowsing download feedback service, enabled for dev & canary only.

For eligible files, the "Discard" button in the download shelf becomes
"Report & Discard".  (Plain "Discard" is still available in the popup menu.)

BUG=169557
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=206571

[mattm, 2013-06-01 02:17:21]

noelutz: safe_browsing/
asanka: chrome/browser/download/ and also download_feedback_service.cc if you
want to look at the downloads api usage (StealDangerousDownload, SetUserData).
sky: chrome/browser/ui/views/

[sky, 2013-06-03 15:43:47]

LGTM

[asanka, 2013-06-03 16:18:31]

chrome/browser/download LGTM

How would a user find out what "Report & Discard" does?

https://codereview.chromium.org/15881012/diff/1/chrome/app/generated_resource...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/15881012/diff/1/chrome/app/generated_resource...
chrome/app/generated_resources.grd:2925: +        Report & Discard
Would this cause the space to be treated as a hotkey?

https://codereview.chromium.org/15881012/diff/1/chrome/browser/safe_browsing/...
File chrome/browser/safe_browsing/download_feedback_service.cc (right):

https://codereview.chromium.org/15881012/diff/1/chrome/browser/safe_browsing/...
chrome/browser/safe_browsing/download_feedback_service.cc:165:
weak_ptr_factory_.GetWeakPtr(),
When would the service be destroyed?

StealDangerousDownload() relies on the caller to clean up the returned file. If
it's possible for the caller to disappear, then it'll be better to bind a
reliable callback target that can either delete the file or call BeginFeedback
depending on the state of the service.

[mattm, 2013-06-07 03:09:02]

On 2013/06/03 16:18:31, asanka wrote:
> How would a user find out what "Report & Discard" does?

A reasonable question. We've been going back and forth with UI design on this
for a while now, trying to not overload it with too much text / etc. So for now
we got okay to just do a dev/canary only launch with very simple text like this
just to trial it.

https://codereview.chromium.org/15881012/diff/1/chrome/app/generated_resource...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/15881012/diff/1/chrome/app/generated_resource...
chrome/app/generated_resources.grd:2925: +        Report & Discard
On 2013/06/03 16:18:32, asanka wrote:
> Would this cause the space to be treated as a hotkey?

Doesn't seem to.

https://codereview.chromium.org/15881012/diff/1/chrome/browser/safe_browsing/...
File chrome/browser/safe_browsing/download_feedback_service.cc (right):

https://codereview.chromium.org/15881012/diff/1/chrome/browser/safe_browsing/...
chrome/browser/safe_browsing/download_feedback_service.cc:165:
weak_ptr_factory_.GetWeakPtr(),
On 2013/06/03 16:18:32, asanka wrote:
> When would the service be destroyed?

On shutdown.

> StealDangerousDownload() relies on the caller to clean up the returned file.
If
> it's possible for the caller to disappear, then it'll be better to bind a
> reliable callback target that can either delete the file or call BeginFeedback
> depending on the state of the service.

Done. (I suppose there may still be an issue if the file thread is also getting
destroyed, haven't thought hard enough about it.)

[asanka, 2013-06-07 15:39:58]

LGTM

https://codereview.chromium.org/15881012/diff/1/chrome/browser/safe_browsing/...
File chrome/browser/safe_browsing/download_feedback_service.cc (right):

https://codereview.chromium.org/15881012/diff/1/chrome/browser/safe_browsing/...
chrome/browser/safe_browsing/download_feedback_service.cc:165:
weak_ptr_factory_.GetWeakPtr(),
On 2013/06/07 03:09:02, mattm wrote:
> On 2013/06/03 16:18:32, asanka wrote:
> > When would the service be destroyed?
> 
> On shutdown.
> 
> > StealDangerousDownload() relies on the caller to clean up the returned file.
> If
> > it's possible for the caller to disappear, then it'll be better to bind a
> > reliable callback target that can either delete the file or call
BeginFeedback
> > depending on the state of the service.
> 
> Done. (I suppose there may still be an issue if the file thread is also
getting
> destroyed, haven't thought hard enough about it.)

Thanks. The handover should be happening in response to a user action. So a
shutdown interfering with the handover should be rare.

[noelutz, 2013-06-12 02:49:47]

lgtm

Really really nice CL!
noé.

https://codereview.chromium.org/15881012/diff/43001/chrome/browser/safe_brows...
File chrome/browser/safe_browsing/download_feedback_service.cc (right):

https://codereview.chromium.org/15881012/diff/43001/chrome/browser/safe_brows...
chrome/browser/safe_browsing/download_feedback_service.cc:125: return
!!DownloadFeedbackPings::FromDownload(download);
Can you educate me and explain what !! does?  Is that because a pointer doesn't
automatically get converted to true if it's not null?

https://codereview.chromium.org/15881012/diff/43001/chrome/browser/safe_brows...
chrome/browser/safe_browsing/download_feedback_service.cc:186:
active_feedback_.front()->Start(base::Bind(
nit: DCHECK(!active_feedback_.empty());

https://codereview.chromium.org/15881012/diff/43001/chrome/browser/ui/views/d...
File chrome/browser/ui/views/download/download_item_view.cc (right):

https://codereview.chromium.org/15881012/diff/43001/chrome/browser/ui/views/d...
chrome/browser/ui/views/download/download_item_view.cc:860: return true;
I didn't know that worked ;).

[mattm, 2013-06-12 04:02:01]

https://codereview.chromium.org/15881012/diff/43001/chrome/browser/safe_brows...
File chrome/browser/safe_browsing/download_feedback_service.cc (right):

https://codereview.chromium.org/15881012/diff/43001/chrome/browser/safe_brows...
chrome/browser/safe_browsing/download_feedback_service.cc:125: return
!!DownloadFeedbackPings::FromDownload(download);
On 2013/06/12 02:49:47, noelutz wrote:
> Can you educate me and explain what !! does?  Is that because a pointer
doesn't
> automatically get converted to true if it's not null?

There is an automatic conversion, but MSVC has a warning about it.  This just
avoids the warning.

https://codereview.chromium.org/15881012/diff/43001/chrome/browser/safe_brows...
chrome/browser/safe_browsing/download_feedback_service.cc:186:
active_feedback_.front()->Start(base::Bind(
On 2013/06/12 02:49:47, noelutz wrote:
> nit: DCHECK(!active_feedback_.empty());

Done.

[mattm, 2013-06-12 04:09:09]

+jar for histograms.xml
note: I had to copy the DownloadItem.DangerType enum from the src-internal
histograms.xml since the downloads histograms are still in there.  Is that okay?

[mattm, 2013-06-14 23:05:24]

+Ilya for histograms review.

Rebased now that downloads histograms are moved.

[Ilya Sherman, 2013-06-14 23:26:28]

LGTM with a question:

https://codereview.chromium.org/15881012/diff/78001/tools/metrics/histograms/...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/15881012/diff/78001/tools/metrics/histograms/...
tools/metrics/histograms/histograms.xml:10619: <histogram
name="SBDownloadFeedback.SizeFailure" units="bytes">
Why are this histogram and the one below in bytes, whereas the one above is in
KB?

[mattm, 2013-06-14 23:37:25]

On 2013/06/14 23:26:28, Ilya Sherman wrote:
> LGTM with a question:
> 
>
https://codereview.chromium.org/15881012/diff/78001/tools/metrics/histograms/...
> File tools/metrics/histograms/histograms.xml (right):
> 
>
https://codereview.chromium.org/15881012/diff/78001/tools/metrics/histograms/...
> tools/metrics/histograms/histograms.xml:10619: <histogram
> name="SBDownloadFeedback.SizeFailure" units="bytes">
> Why are this histogram and the one below in bytes, whereas the one above is in
> KB?

Ah, yeah.  That is a little confusing.  I did that because the SizeEligibleKB
could be any downloaded file, even large ones that would overflow an int if
counted in bytes.  The feature only allows files of 50MB or less to be uploaded,
so SizeFailure and SizeSuccess don't have that issue and could thus be in bytes.

[commit-bot: I haz the power, 2013-06-15 00:55:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mattm@chromium.org/15881012/78001

[commit-bot: I haz the power, 2013-06-15 10:51:36]

Change committed as 206571