Report malware redirectors as well.

Report malware redirectors as well.
BUG=none
TEST=unit_tests

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

[lzheng, 2010-08-03 01:33:58]

http://codereview.chromium.org/3028040/diff/1/2
File chrome/browser/safe_browsing/safe_browsing_service.cc (right):

http://codereview.chromium.org/3028040/diff/1/2#newcode779
chrome/browser/safe_browsing/safe_browsing_service.cc:779: // Sometimes there is
no useful information to report
I wonder why you don't want to check with the Database anymore? Will that
increase the reporting requests?
What's the qps for this reporting request for now?

[panayiotis, 2010-08-03 17:19:15]

http://codereview.chromium.org/3028040/diff/1/2
File chrome/browser/safe_browsing/safe_browsing_service.cc (right):

http://codereview.chromium.org/3028040/diff/1/2#newcode779
chrome/browser/safe_browsing/safe_browsing_service.cc:779: // Sometimes there is
no useful information to report
Ah yes, I should have explained this change.

I think this would rarely happen. If page_url was in the list, the user would
see a warning immediately -- and we would not end up loading any resources. 

Maybe it will happen if the user clicks through the first warning -- but do we
show yet another warning in that case? 

I think we are at about 5QPS currently. 



On 2010/08/03 01:33:58, lzheng wrote:
> I wonder why you don't want to check with the Database anymore? Will that
> increase the reporting requests?
> What's the qps for this reporting request for now?

[panayiotis, 2010-08-03 17:28:33]

http://codereview.chromium.org/3028040/diff/1/2
File chrome/browser/safe_browsing/safe_browsing_service.cc (right):

http://codereview.chromium.org/3028040/diff/1/2#newcode779
chrome/browser/safe_browsing/safe_browsing_service.cc:779: // Sometimes there is
no useful information to report
On 2010/08/03 17:19:15, panayiotis wrote:
> Ah yes, I should have explained this change.
> 
> I think this would rarely happen. If page_url was in the list, the user would
> see a warning immediately -- and we would not end up loading any resources. 
> 
> Maybe it will happen if the user clicks through the first warning -- but do we
> show yet another warning in that case? 
> 
> I think we are at about 5QPS currently. 
> 
> 
> 
> On 2010/08/03 01:33:58, lzheng wrote:
> > I wonder why you don't want to check with the Database anymore? Will that
> > increase the reporting requests?
> > What's the qps for this reporting request for now?
> 
> 

I just confirmed that if you ignore a warning for a blacklisted page, you still
get one for a blacklisted subresource, so this might actually happen -- thus I
reverted this part of the change.

[lzheng, 2010-08-05 21:40:22]

Sorry for the delay.

http://codereview.chromium.org/3028040/diff/3/6001
File chrome/browser/safe_browsing/safe_browsing_service.cc (right):

http://codereview.chromium.org/3028040/diff/3/6001#newcode779
chrome/browser/safe_browsing/safe_browsing_service.cc:779: // Sometimes there is
no useful information to report
nit: put "." at the end.

http://codereview.chromium.org/3028040/diff/3/6001#newcode780
chrome/browser/safe_browsing/safe_browsing_service.cc:780: if (malware_url ==
page_url && referrer_url.is_empty())
Can we do this comparison in DoDisplayBlockingPage before we call
ChromeThread::PostTask? So we could avoid some task scheduling when "malware_url
== page_url && referrer_url.is_empty()".

[panayiotis, 2010-08-06 01:48:51]

http://codereview.chromium.org/3028040/diff/3/6001
File chrome/browser/safe_browsing/safe_browsing_service.cc (right):

http://codereview.chromium.org/3028040/diff/3/6001#newcode780
chrome/browser/safe_browsing/safe_browsing_service.cc:780: if (malware_url ==
page_url && referrer_url.is_empty())
On 2010/08/05 21:40:22, lzheng wrote:
> Can we do this comparison in DoDisplayBlockingPage before we call
> ChromeThread::PostTask? So we could avoid some task scheduling when
"malware_url
> == page_url && referrer_url.is_empty()". 
> 

Done.

[lzheng, 2010-08-06 02:32:28]

http://codereview.chromium.org/3028040/diff/9001/10001
File chrome/browser/safe_browsing/safe_browsing_service.cc (right):

http://codereview.chromium.org/3028040/diff/9001/10001#newcode762
chrome/browser/safe_browsing/safe_browsing_service.cc:762: if (!(resource.url ==
page_url && referrer_url.is_empty())) {
Would you mind changing this to  if (resource.url != page_url ||
!referrer_url.is_empty()))? I feel it is less mind twisting  that way :-)

[panayiotis, 2010-08-06 17:09:22]

http://codereview.chromium.org/3028040/diff/9001/10001
File chrome/browser/safe_browsing/safe_browsing_service.cc (right):

http://codereview.chromium.org/3028040/diff/9001/10001#newcode762
chrome/browser/safe_browsing/safe_browsing_service.cc:762: if (!(resource.url ==
page_url && referrer_url.is_empty())) {
On 2010/08/06 02:32:28, lzheng wrote:
> Would you mind changing this to  if (resource.url != page_url ||
> !referrer_url.is_empty()))? I feel it is less mind twisting  that way :-)

Done. :)

[lzheng, 2010-08-06 17:12:09]

LGTM.

[panayiotis, 2010-08-10 20:11:38]

One more thing: We'd like to distinguish between the two kinds of reports.
Please have another look, thanks!

http://codereview.chromium.org/3028040/diff/17001/18003
File chrome/browser/safe_browsing/protocol_manager_unittest.cc (right):

http://codereview.chromium.org/3028040/diff/17001/18003#newcode216
chrome/browser/safe_browsing/protocol_manager_unittest.cc:216:
"url.com%2F&evtb=1",
I've asked around, these parameters don't mean anything special, so keeping up
with the tradition of non-meaningful params ...

[lzheng, 2010-08-11 06:11:10]

http://codereview.chromium.org/3028040/diff/17001/18003
File chrome/browser/safe_browsing/protocol_manager_unittest.cc (right):

http://codereview.chromium.org/3028040/diff/17001/18003#newcode218
chrome/browser/safe_browsing/protocol_manager_unittest.cc:218: true).spec());
Can you make this a "false" and test evtb=0?

[panayiotis, 2010-08-11 17:43:45]

http://codereview.chromium.org/3028040/diff/17001/18003
File chrome/browser/safe_browsing/protocol_manager_unittest.cc (right):

http://codereview.chromium.org/3028040/diff/17001/18003#newcode218
chrome/browser/safe_browsing/protocol_manager_unittest.cc:218: true).spec());
On 2010/08/11 06:11:10, lzheng wrote:
> Can you make this a "false" and test evtb=0?

done

[lzheng, 2010-08-11 17:49:09]

LGTM