Send the referral URL with the client-side phishing detection request.

Send the referral URL with the client-side phishing detection request.

BUG=None
TEST=BrowserFeatureExtractorTest


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

[mattm, 2011-06-20 22:33:06]

http://codereview.chromium.org/7189074/diff/2001/chrome/browser/safe_browsing...
File chrome/browser/safe_browsing/client_side_detection_host.cc (right):

http://codereview.chromium.org/7189074/diff/2001/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/client_side_detection_host.cc:365: <<
browse_info_->url.spec() << " vs. " << verdict->url();
should it return or have the ExtractFeatures in an else block instead of falling
through?

http://codereview.chromium.org/7189074/diff/2001/chrome/browser/safe_browsing...
File chrome/browser/safe_browsing/client_side_detection_host_unittest.cc
(right):

http://codereview.chromium.org/7189074/diff/2001/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/client_side_detection_host_unittest.cc:48:
MATCHER_P(PartiallyEqualVerdict, other, "") {
maybe add a comment about why we only match some parts?

[noelutz, 2011-06-20 22:58:12]

http://codereview.chromium.org/7189074/diff/2001/chrome/browser/safe_browsing...
File chrome/browser/safe_browsing/client_side_detection_host.cc (right):

http://codereview.chromium.org/7189074/diff/2001/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/client_side_detection_host.cc:365: <<
browse_info_->url.spec() << " vs. " << verdict->url();
On 2011/06/20 22:33:06, mattm wrote:
> should it return or have the ExtractFeatures in an else block instead of
falling
> through?

I would like to fall through for now (which is why I added the UMA stat).  I'm
not sure if this ever happens or if there is a legitimate use case when this
happens.  Do you?  I'm wondering whether there could be differences in URL
encoding or something like that between WebKit and the browser.  Thoughts?

http://codereview.chromium.org/7189074/diff/2001/chrome/browser/safe_browsing...
File chrome/browser/safe_browsing/client_side_detection_host_unittest.cc
(right):

http://codereview.chromium.org/7189074/diff/2001/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/client_side_detection_host_unittest.cc:48:
MATCHER_P(PartiallyEqualVerdict, other, "") {
On 2011/06/20 22:33:06, mattm wrote:
> maybe add a comment about why we only match some parts?

Done.

[gcasto (DO NOT USE), 2011-06-20 23:10:50]

LGTM

[mattm, 2011-06-21 00:06:06]

lgtm

http://codereview.chromium.org/7189074/diff/2001/chrome/browser/safe_browsing...
File chrome/browser/safe_browsing/client_side_detection_host.cc (right):

http://codereview.chromium.org/7189074/diff/2001/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/client_side_detection_host.cc:365: <<
browse_info_->url.spec() << " vs. " << verdict->url();
On 2011/06/20 22:58:12, noelutz wrote:
> On 2011/06/20 22:33:06, mattm wrote:
> > should it return or have the ExtractFeatures in an else block instead of
> falling
> > through?
> 
> I would like to fall through for now (which is why I added the UMA stat).  I'm
> not sure if this ever happens or if there is a legitimate use case when this
> happens.  Do you?  I'm wondering whether there could be differences in URL
> encoding or something like that between WebKit and the browser.  Thoughts?

Hm, I don't think there should be, dunno.  Just having the stat to verify seems
fine then.

[Brian Ryner, 2011-06-21 00:20:36]

http://codereview.chromium.org/7189074/diff/3008/chrome/browser/safe_browsing...
File chrome/browser/safe_browsing/browser_feature_extractor.cc (right):

http://codereview.chromium.org/7189074/diff/3008/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/browser_feature_extractor.cc:99:
PageTransition::StripQualifier(info.transition)),
If we ever wanted to implement this in a cross-browser way, it would probably be
better to make this into a token feature with names for each transition, i.e.
"PageTransitionType=Link".

Not critical for this first cut though.

http://codereview.chromium.org/7189074/diff/3008/chrome/browser/safe_browsing...
File chrome/browser/safe_browsing/client_side_detection_host.cc (right):

http://codereview.chromium.org/7189074/diff/3008/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/client_side_detection_host.cc:347:
DCHECK(!cb_factory_.HasPendingCallbacks());
This is trusting that the renderer is well-behaved.  Perhaps we should guard
against a corrupted/compromised renderer sending spurious OnDetectedPhishingSite
IPC's?

[noelutz, 2011-06-21 00:25:17]

http://codereview.chromium.org/7189074/diff/3008/chrome/browser/safe_browsing...
File chrome/browser/safe_browsing/browser_feature_extractor.cc (right):

http://codereview.chromium.org/7189074/diff/3008/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/browser_feature_extractor.cc:99:
PageTransition::StripQualifier(info.transition)),
On 2011/06/21 00:20:36, Brian Ryner wrote:
> If we ever wanted to implement this in a cross-browser way, it would probably
be
> better to make this into a token feature with names for each transition, i.e.
> "PageTransitionType=Link".
> 
> Not critical for this first cut though.

Agreed.  If this will make it to a useful feature I will change that.

http://codereview.chromium.org/7189074/diff/3008/chrome/browser/safe_browsing...
File chrome/browser/safe_browsing/client_side_detection_host.cc (right):

http://codereview.chromium.org/7189074/diff/3008/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/client_side_detection_host.cc:347:
DCHECK(!cb_factory_.HasPendingCallbacks());
On 2011/06/21 00:20:36, Brian Ryner wrote:
> This is trusting that the renderer is well-behaved.  Perhaps we should guard
> against a corrupted/compromised renderer sending spurious
OnDetectedPhishingSite
> IPC's?

Added that check too.

[Brian Ryner, 2011-06-21 00:26:57]

lgtm

[commit-bot: I haz the power, 2011-06-21 15:58:18]

Change committed as 89830