Add token field to ClientSafeBrowsingReportReqeust

Add token field to ClientSafeBrowsingReportReqeust.

In order to calculate CTRs of bad downloads, safe browsing needs information to
link ClientSafeBrowsingReportRequest (for downloads) and ClientDownloadResponse.
To achieve this, we add a token field to identify its corresponding
ClientDownloadResponse in the ClientSafeBrowsingReportRequest, and propagate it
with the same string in ClientDownloadResponse.token.

Also add a new download report type (DANGEROUS_DOWNLOAD_BY_API) to separately
track CTR of dangerous downloads go through download API.

BUG=615511
Committed: https://crrev.com/2c26385851a2425cb68a47da7f420c04e28b6c45
Cr-Commit-Position: refs/heads/master@{#398699}

[Jialiu Lin, 2016-06-03 18:09:01]

Description was changed from

==========
Basic logic

BUG=
==========

to

==========
In order to calculate click through rates of bad downloads, safe browsing
backend needs information to link ClientSafeBrowsingReportRequest (for
downloads) and ClientDownloadResponse. Therefore, in the
ClientSafeBrowsingReportRequest, we add a token field to identify its
corresponding ClientDownloadResponse.


BUG=615511
==========

[Jialiu Lin, 2016-06-03 20:29:19]

Description was changed from

==========
In order to calculate click through rates of bad downloads, safe browsing
backend needs information to link ClientSafeBrowsingReportRequest (for
downloads) and ClientDownloadResponse. Therefore, in the
ClientSafeBrowsingReportRequest, we add a token field to identify its
corresponding ClientDownloadResponse.


BUG=615511
==========

to

==========
In order to calculate CTRs of bad downloads, safe browsing needs information to
link ClientSafeBrowsingReportRequest (for downloads) and ClientDownloadResponse.
To achieve this, we add a token field to identify its corresponding
ClientDownloadResponse in the ClientSafeBrowsingReportRequest.

Also add a new download report type (DANGEROUS_DOWNLOAD_BY_API) to track CTR of
downloads go through download API. 

BUG=615511
==========

[Jialiu Lin, 2016-06-03 20:40:05]

Patchset #5 (id:80001) has been deleted

[Jialiu Lin, 2016-06-03 20:40:15]

Patchset #4 (id:60001) has been deleted

[Jialiu Lin, 2016-06-03 20:40:20]

Patchset #2 (id:20001) has been deleted

[Jialiu Lin, 2016-06-03 20:48:01]

Patchset #4 (id:120001) has been deleted

[Jialiu Lin, 2016-06-03 20:51:05]

Description was changed from

==========
In order to calculate CTRs of bad downloads, safe browsing needs information to
link ClientSafeBrowsingReportRequest (for downloads) and ClientDownloadResponse.
To achieve this, we add a token field to identify its corresponding
ClientDownloadResponse in the ClientSafeBrowsingReportRequest.

Also add a new download report type (DANGEROUS_DOWNLOAD_BY_API) to track CTR of
downloads go through download API. 

BUG=615511
==========

to

==========
Add token field to ClientSafeBrowsingReportReqeust.

In order to calculate CTRs of bad downloads, safe browsing needs information to
link ClientSafeBrowsingReportRequest (for downloads) and ClientDownloadResponse.
To achieve this, we add a token field to identify its corresponding
ClientDownloadResponse in the ClientSafeBrowsingReportRequest, and propagate it
with the same string in ClientDownloadResponse.token.

Also add a new download report type (DANGEROUS_DOWNLOAD_BY_API) to separately
track CTR of dangerous downloads go through download API.  

BUG=615511
==========

[Jialiu Lin, 2016-06-03 20:56:09]

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

[Jialiu Lin, 2016-06-03 20:56:09]

Hi asanka@,
This is requested by bineval team for linking ClientDownloadResponse to
ClientSafeBrowsingReportRequest in order to calculate fine-grained CTRs (e.g.
per url or per binary). 
Please take a look. 

Thanks!

[asanka, 2016-06-06 20:39:27]

Sorry about the delay.

A good rule of thumb for this sort of change is that if the setter and the
getter for a DownloadItem property are both in //chrome, then that value
probably doesn't need to be plumbed through //content.

An example of this is how download feedback pings are stored in a DownloadItem
(https://cs.chromium.org/chromium/src/chrome/browser/safe_browsing/download_fe...)

Something like that can be used to get DownlodProtectionService to store the
token directly in the DownloadItem. That way you don't need to plumb the value
to //content nor do you need to change the callback signature for
CheckDownloadCallback. The only place where the token is useful is during a
CheckClientDownloadRequest, which already has a reference to the DownloadItem in
item_. It can set the UserData directly without involving any of the upper
layers.

https://codereview.chromium.org/2029903002/diff/140001/chrome/browser/downloa...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

https://codereview.chromium.org/2029903002/diff/140001/chrome/browser/downloa...
chrome/browser/download/chrome_download_manager_delegate.cc:337:
base::EmptyString());
base::EmptyString() shouldn't be used in function arguments. Use std::string()
instead. Here and elsewhere.

Reasoning for this is explained in strings/string_util.h.

[Jialiu Lin, 2016-06-07 18:15:17]

Patchset #5 (id:160001) has been deleted

[Jialiu Lin, 2016-06-07 18:35:58]

On 2016/06/06 at 20:39:27, asanka wrote:
Ah, Thanks for pointing me to GetUserData/SetUserData. I wish I knew this one
earlier. T_T
I have re-written this CL using GetUserData/SetUserData. 

Thanks! 


> Sorry about the delay.
> 
> A good rule of thumb for this sort of change is that if the setter and the
getter for a DownloadItem property are both in //chrome, then that value
probably doesn't need to be plumbed through //content.
> 
> An example of this is how download feedback pings are stored in a DownloadItem
(https://cs.chromium.org/chromium/src/chrome/browser/safe_browsing/download_fe...)
> 
> Something like that can be used to get DownlodProtectionService to store the
token directly in the DownloadItem. That way you don't need to plumb the value
to //content nor do you need to change the callback signature for
CheckDownloadCallback. The only place where the token is useful is during a
CheckClientDownloadRequest, which already has a reference to the DownloadItem in
item_. It can set the UserData directly without involving any of the upper
layers.
> 
>
https://codereview.chromium.org/2029903002/diff/140001/chrome/browser/downloa...
> File chrome/browser/download/chrome_download_manager_delegate.cc (right):
> 
>
https://codereview.chromium.org/2029903002/diff/140001/chrome/browser/downloa...
> chrome/browser/download/chrome_download_manager_delegate.cc:337:
base::EmptyString());
> base::EmptyString() shouldn't be used in function arguments. Use std::string()
instead. Here and elsewhere.
> 
> Reasoning for this is explained in strings/string_util.h.

[asanka, 2016-06-07 20:26:44]

lgtm

https://codereview.chromium.org/2029903002/diff/180001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/download_protection_service.cc (right):

https://codereview.chromium.org/2029903002/diff/180001/chrome/browser/safe_br...
chrome/browser/safe_browsing/download_protection_service.cc:86: =
&kDownloadPingTokenKey;
Interesting. Have you seen this pattern before?

[Jialiu Lin, 2016-06-07 20:34:01]

Thanks, asanka@!

https://codereview.chromium.org/2029903002/diff/140001/chrome/browser/downloa...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

https://codereview.chromium.org/2029903002/diff/140001/chrome/browser/downloa...
chrome/browser/download/chrome_download_manager_delegate.cc:337:
base::EmptyString());
On 2016/06/06 at 20:39:27, asanka wrote:
> base::EmptyString() shouldn't be used in function arguments. Use std::string()
instead. Here and elsewhere.
> 
> Reasoning for this is explained in strings/string_util.h.

Fixed.Thanks!

https://codereview.chromium.org/2029903002/diff/180001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/download_protection_service.cc (right):

https://codereview.chromium.org/2029903002/diff/180001/chrome/browser/safe_br...
chrome/browser/safe_browsing/download_protection_service.cc:86: =
&kDownloadPingTokenKey;
On 2016/06/07 at 20:26:44, asanka wrote:
> Interesting. Have you seen this pattern before?

I copied from the download feedback example you gave me. :-)
https://cs.chromium.org/chromium/src/chrome/browser/safe_browsing/download_fe...

[Jialiu Lin, 2016-06-07 20:37:46]

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

[Jialiu Lin, 2016-06-07 20:37:47]

+sky@ for owner review of
chrome/browser/ui/cocoa/download/download_danger_prompt_impl.cc,
chrome/browser/ui/views/download/download_danger_prompt_view.cc

+nparker@ for safe_browsing related owner review.

Thanks!

[Nathan Parker, 2016-06-07 23:00:54]

lgtm

for safe_browsing, with some nits

https://codereview.chromium.org/2029903002/diff/180001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/download_protection_service.cc (right):

https://codereview.chromium.org/2029903002/diff/180001/chrome/browser/safe_br...
chrome/browser/safe_browsing/download_protection_service.cc:86: =
&kDownloadPingTokenKey;
On 2016/06/07 20:34:01, JialiuLin wrote:
> On 2016/06/07 at 20:26:44, asanka wrote:
> > Interesting. Have you seen this pattern before?
> 
> I copied from the download feedback example you gave me. :-)
>
https://cs.chromium.org/chromium/src/chrome/browser/safe_browsing/download_fe...

I suppose it is guaranteed to be unique in this process.

https://codereview.chromium.org/2029903002/diff/180001/chrome/browser/safe_br...
chrome/browser/safe_browsing/download_protection_service.cc:482: token =
response.has_token() ? response.token() : std::string();
nit: Could this be just
  token = response.token(); ?
if it's not present, the default should be empty string.

https://codereview.chromium.org/2029903002/diff/180001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/download_protection_service.h (right):

https://codereview.chromium.org/2029903002/diff/180001/chrome/browser/safe_br...
chrome/browser/safe_browsing/download_protection_service.h:226: static const
void* const kDownloadPingTokenKey;
Could all of this be in an anonymous namespace in the .cc file?  If you're using
it in tests, then no (and never mind).

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

https://codereview.chromium.org/2029903002/diff/180001/chrome/browser/safe_br...
chrome/browser/safe_browsing/download_protection_service_unittest.cc:1859:
DownloadProtectionService::SetDownloadPingToken(&item, token);
How about checking it before setting it, to ensure it's empty?

[Jialiu Lin, 2016-06-08 00:06:16]

Thanks, nparker@! Your comments are all addressed.

https://codereview.chromium.org/2029903002/diff/180001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/download_protection_service.cc (right):

https://codereview.chromium.org/2029903002/diff/180001/chrome/browser/safe_br...
chrome/browser/safe_browsing/download_protection_service.cc:86: =
&kDownloadPingTokenKey;
On 2016/06/07 at 23:00:53, Nathan Parker wrote:
> On 2016/06/07 20:34:01, JialiuLin wrote:
> > On 2016/06/07 at 20:26:44, asanka wrote:
> > > Interesting. Have you seen this pattern before?
> > 
> > I copied from the download feedback example you gave me. :-)
> >
https://cs.chromium.org/chromium/src/chrome/browser/safe_browsing/download_fe...
> 
> I suppose it is guaranteed to be unique in this process.
I suppose so.

https://codereview.chromium.org/2029903002/diff/180001/chrome/browser/safe_br...
chrome/browser/safe_browsing/download_protection_service.cc:482: token =
response.has_token() ? response.token() : std::string();
On 2016/06/07 at 23:00:53, Nathan Parker wrote:
> nit: Could this be just
>   token = response.token(); ?
> if it's not present, the default should be empty string.

Fixed. Thanks!

https://codereview.chromium.org/2029903002/diff/180001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/download_protection_service.h (right):

https://codereview.chromium.org/2029903002/diff/180001/chrome/browser/safe_br...
chrome/browser/safe_browsing/download_protection_service.h:226: static const
void* const kDownloadPingTokenKey;
On 2016/06/07 at 23:00:53, Nathan Parker wrote:
> Could all of this be in an anonymous namespace in the .cc file?  If you're
using it in tests, then no (and never mind).

Yes, I'm using it in download_danger_prompt_browsertest

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

https://codereview.chromium.org/2029903002/diff/180001/chrome/browser/safe_br...
chrome/browser/safe_browsing/download_protection_service_unittest.cc:1859:
DownloadProtectionService::SetDownloadPingToken(&item, token);
On 2016/06/07 at 23:00:53, Nathan Parker wrote:
> How about checking it before setting it, to ensure it's empty?
Sure.

[sky, 2016-06-08 16:12:18]

https://codereview.chromium.org/2029903002/diff/220001/chrome/browser/ui/view...
File chrome/browser/ui/views/download/download_danger_prompt_views.cc (right):

https://codereview.chromium.org/2029903002/diff/220001/chrome/browser/ui/view...
chrome/browser/ui/views/download/download_danger_prompt_views.cc:78: bool
show_context_;
Would it make more sense to have this be
ClientSafeBrowsingReportRequest::ReportType ?

[Jialiu Lin, 2016-06-08 17:46:29]

Thanks, sky@!

https://codereview.chromium.org/2029903002/diff/220001/chrome/browser/ui/view...
File chrome/browser/ui/views/download/download_danger_prompt_views.cc (right):

https://codereview.chromium.org/2029903002/diff/220001/chrome/browser/ui/view...
chrome/browser/ui/views/download/download_danger_prompt_views.cc:78: bool
show_context_;
On 2016/06/08 16:12:17, sky wrote:
> Would it make more sense to have this be
> ClientSafeBrowsingReportRequest::ReportType ?
Hmm... probably not. Report type does depend on show_context_ , and so does
dialog UI. And show_context_ is actually independent from safe browsing service.

 
Personally, I found this variable name very confusing too, so I added some
comments for future developer to have a better sense of what's going on here.
Anyway, we already have a bug (https://crbug.com/308762) aims to refactor this
file and completely separates download confirmation dialog and download recovery
dialog. After it gets fixed, bool show_context_ should be gone.

[sky, 2016-06-08 20:11:05]

Ok, LGTM

[Jialiu Lin, 2016-06-08 20:29:15]

The CQ bit was checked by jialiul@chromium.org

[Jialiu Lin, 2016-06-08 20:29:16]

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

[commit-bot: I haz the power, 2016-06-08 20:29:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2029903002/220001

[commit-bot: I haz the power, 2016-06-08 22:18:26]

Committed patchset #7 (id:220001)

[commit-bot: I haz the power, 2016-06-08 22:19:47]

Description was changed from

==========
Add token field to ClientSafeBrowsingReportReqeust.

In order to calculate CTRs of bad downloads, safe browsing needs information to
link ClientSafeBrowsingReportRequest (for downloads) and ClientDownloadResponse.
To achieve this, we add a token field to identify its corresponding
ClientDownloadResponse in the ClientSafeBrowsingReportRequest, and propagate it
with the same string in ClientDownloadResponse.token.

Also add a new download report type (DANGEROUS_DOWNLOAD_BY_API) to separately
track CTR of dangerous downloads go through download API.  

BUG=615511
==========

to

==========
Add token field to ClientSafeBrowsingReportReqeust.

In order to calculate CTRs of bad downloads, safe browsing needs information to
link ClientSafeBrowsingReportRequest (for downloads) and ClientDownloadResponse.
To achieve this, we add a token field to identify its corresponding
ClientDownloadResponse in the ClientSafeBrowsingReportRequest, and propagate it
with the same string in ClientDownloadResponse.token.

Also add a new download report type (DANGEROUS_DOWNLOAD_BY_API) to separately
track CTR of dangerous downloads go through download API.

BUG=615511

Committed: https://crrev.com/2c26385851a2425cb68a47da7f420c04e28b6c45
Cr-Commit-Position: refs/heads/master@{#398699}
==========

[commit-bot: I haz the power, 2016-06-08 22:19:48]

Patchset 7 (id:??) landed as
https://crrev.com/2c26385851a2425cb68a47da7f420c04e28b6c45
Cr-Commit-Position: refs/heads/master@{#398699}