Include full redirect chain in downloads safebrowsing ping.

Include full redirect chain in downloads safebrowsing ping.

BUG=80097
TEST=tcpdump

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

[Brian Ryner, 2011-05-06 00:45:15]

Panayiotis might be a good reviewer for this too.

[noelutz, 2011-05-06 04:24:12]

I'm just hoping to get your thoughts on two 'issues'.

noé.

http://codereview.chromium.org/6935032/diff/1/chrome/browser/safe_browsing/pr...
File chrome/browser/safe_browsing/protocol_manager.cc (right):

http://codereview.chromium.org/6935032/diff/1/chrome/browser/safe_browsing/pr...
chrome/browser/safe_browsing/protocol_manager.cc:672: GURL report_url =
SafeBrowsingHitUrl(url_chain.back(), url_chain.front(),
Do you ever check that there is at least one element in the chain?

http://codereview.chromium.org/6935032/diff/1/chrome/browser/safe_browsing/sa...
File chrome/browser/safe_browsing/safe_browsing_service.h (left):

http://codereview.chromium.org/6935032/diff/1/chrome/browser/safe_browsing/sa...
chrome/browser/safe_browsing/safe_browsing_service.h:256: UrlCheckResult
threat_type);
Have you considered renaming this method to something like
ReportSafeBrowsingHitWithData or simply adding a POST data parameter to this
function?  Note: on the server side I implemented a generic way to support pings
that contain any post data.  The server code is not specific to bad binary hits.

This approach might help to get rid of some of the code duplication.

[panayiotis, 2011-05-09 19:44:16]

http://codereview.chromium.org/6935032/diff/1/chrome/browser/safe_browsing/pr...
File chrome/browser/safe_browsing/protocol_manager.cc (right):

http://codereview.chromium.org/6935032/diff/1/chrome/browser/safe_browsing/pr...
chrome/browser/safe_browsing/protocol_manager.cc:672: GURL report_url =
SafeBrowsingHitUrl(url_chain.back(), url_chain.front(),
A check would be nice yes. 

On 2011/05/06 04:24:12, noelutz wrote:
> Do you ever check that there is at least one element in the chain?

[mattm, 2011-05-10 02:26:09]

http://codereview.chromium.org/6935032/diff/1/chrome/browser/safe_browsing/pr...
File chrome/browser/safe_browsing/protocol_manager.cc (right):

http://codereview.chromium.org/6935032/diff/1/chrome/browser/safe_browsing/pr...
chrome/browser/safe_browsing/protocol_manager.cc:672: GURL report_url =
SafeBrowsingHitUrl(url_chain.back(), url_chain.front(),
On 2011/05/06 04:24:12, noelutz wrote:
> Do you ever check that there is at least one element in the chain?

Added a DCHECK to DownloadSBClient constructor

http://codereview.chromium.org/6935032/diff/1/chrome/browser/safe_browsing/sa...
File chrome/browser/safe_browsing/safe_browsing_service.h (left):

http://codereview.chromium.org/6935032/diff/1/chrome/browser/safe_browsing/sa...
chrome/browser/safe_browsing/safe_browsing_service.h:256: UrlCheckResult
threat_type);
On 2011/05/06 04:24:12, noelutz wrote:
> Have you considered renaming this method to something like
> ReportSafeBrowsingHitWithData or simply adding a POST data parameter to this
> function?  Note: on the server side I implemented a generic way to support
pings
> that contain any post data.  The server code is not specific to bad binary
hits.
> 
> This approach might help to get rid of some of the code duplication.

I like it.  done.

[noelutz, 2011-05-10 07:24:12]

LGTM.

Nice!  Thanks,
noé.

[panayiotis, 2011-05-10 17:38:34]

lgtm