Create a browser feature extractor that runs after the renderer has

Create a browser feature extractor that runs after the renderer has
decided that a page is phishing according to the extracted features
in the renderer.  For now we don't use these features in the classification.
We only use them on the server side.

Currently, the browser feature extractor extracts a bunch of features based
on the history service (e.g., has the user seen this URL before).

BUG=None
TEST=BrowserFeatureExtractorTest


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

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

[Paweł Hajdan Jr., 2011-06-08 08:54:46]

Drive-by with a testing comment.

http://codereview.chromium.org/7119003/diff/4002/chrome/browser/safe_browsing...
File chrome/browser/safe_browsing/browser_feature_extractor_unittest.cc (right):

http://codereview.chromium.org/7119003/diff/4002/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/browser_feature_extractor_unittest.cc:147: // We
have to call Run() twice because we expect ExtractFeaturesDone
Could you rather count the number of finished requests in ExtractFeaturesDone
and only quit the loop when all of the expected ones have finished?

I think it's generally cleaner than running the loop multiple times.

[noelutz, 2011-06-08 16:39:30]

Thanks,
noe.

http://codereview.chromium.org/7119003/diff/4002/chrome/browser/safe_browsing...
File chrome/browser/safe_browsing/browser_feature_extractor_unittest.cc (right):

http://codereview.chromium.org/7119003/diff/4002/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/browser_feature_extractor_unittest.cc:147: // We
have to call Run() twice because we expect ExtractFeaturesDone
On 2011/06/08 08:54:46, Paweł Hajdan Jr. wrote:
> Could you rather count the number of finished requests in ExtractFeaturesDone
> and only quit the loop when all of the expected ones have finished?
> 
> I think it's generally cleaner than running the loop multiple times.

Done.

[mattm, 2011-06-08 23:18:18]

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

http://codereview.chromium.org/7119003/diff/4003/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/browser_feature_extractor.cc:30: const char
kHttpsHostVisitCount[] = "HttpsHostVisitCount=";
Why do these ones have trailing '=' but the next two don't?

http://codereview.chromium.org/7119003/diff/4003/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/browser_feature_extractor.cc:91: DCHECK(1 ==
pending_extractions_.erase(extraction));
DCHECK_EQ

http://codereview.chromium.org/7119003/diff/4003/chrome/browser/safe_browsing...
File chrome/browser/safe_browsing/browser_feature_extractor.h (right):

http://codereview.chromium.org/7119003/diff/4003/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/browser_feature_extractor.h:35: // History featres.
typo

http://codereview.chromium.org/7119003/diff/4003/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/browser_feature_extractor.h:50: extern const char
kUrlHistoryVisitCount24hAgo[];
Maybe this one should have MoreThan in the name too?

http://codereview.chromium.org/7119003/diff/4003/chrome/browser/safe_browsing...
File chrome/browser/safe_browsing/browser_feature_extractor_unittest.cc (right):

http://codereview.chromium.org/7119003/diff/4003/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/browser_feature_extractor_unittest.cc:155:
EXPECT_FALSE(success_[&request2]);
This one is expected to fail since it wasn't added to the history_service? 
maybe add a comment.

[Paweł Hajdan Jr., 2011-06-09 09:13:09]

Code I commented in the drive-by LGTM, thanks!

http://codereview.chromium.org/7119003/diff/4003/chrome/browser/safe_browsing...
File chrome/browser/safe_browsing/browser_feature_extractor_unittest.cc (right):

http://codereview.chromium.org/7119003/diff/4003/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/browser_feature_extractor_unittest.cc:30: :
ui_thread_(BrowserThread::UI, &message_loop_) {
nit: Can it fit in the previous line?

http://codereview.chromium.org/7119003/diff/4003/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/browser_feature_extractor_unittest.cc:31: }
nit: Add an empty line below.

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

http://codereview.chromium.org/7119003/diff/4003/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/client_side_detection_host_unittest.cc:113: 
nit: Why so many empty lines?

[noelutz, 2011-06-09 19:21:39]

Thanks.  Please take another look.
noé.

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

http://codereview.chromium.org/7119003/diff/4003/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/browser_feature_extractor.cc:30: const char
kHttpsHostVisitCount[] = "HttpsHostVisitCount=";
On 2011/06/08 23:18:18, mattm wrote:
> Why do these ones have trailing '=' but the next two don't?

Oups.  I just realized I misunderstood this notation in features.h.  Removed the
= everywhere.

http://codereview.chromium.org/7119003/diff/4003/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/browser_feature_extractor.cc:91: DCHECK(1 ==
pending_extractions_.erase(extraction));
On 2011/06/08 23:18:18, mattm wrote:
> DCHECK_EQ

Done.

http://codereview.chromium.org/7119003/diff/4003/chrome/browser/safe_browsing...
File chrome/browser/safe_browsing/browser_feature_extractor.h (right):

http://codereview.chromium.org/7119003/diff/4003/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/browser_feature_extractor.h:35: // History featres.
On 2011/06/08 23:18:18, mattm wrote:
> typo

Done.

http://codereview.chromium.org/7119003/diff/4003/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/browser_feature_extractor.h:50: extern const char
kUrlHistoryVisitCount24hAgo[];
On 2011/06/08 23:18:18, mattm wrote:
> Maybe this one should have MoreThan in the name too?

Done.

http://codereview.chromium.org/7119003/diff/4003/chrome/browser/safe_browsing...
File chrome/browser/safe_browsing/browser_feature_extractor_unittest.cc (right):

http://codereview.chromium.org/7119003/diff/4003/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/browser_feature_extractor_unittest.cc:30: :
ui_thread_(BrowserThread::UI, &message_loop_) {
On 2011/06/09 09:13:10, Paweł Hajdan Jr. wrote:
> nit: Can it fit in the previous line?

81 chars :-(

http://codereview.chromium.org/7119003/diff/4003/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/browser_feature_extractor_unittest.cc:31: }
On 2011/06/09 09:13:10, Paweł Hajdan Jr. wrote:
> nit: Add an empty line below.

Done.

http://codereview.chromium.org/7119003/diff/4003/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/browser_feature_extractor_unittest.cc:155:
EXPECT_FALSE(success_[&request2]);
On 2011/06/08 23:18:18, mattm wrote:
> This one is expected to fail since it wasn't added to the history_service? 
> maybe add a comment.

Done.

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

http://codereview.chromium.org/7119003/diff/4003/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/client_side_detection_host_unittest.cc:113: 
On 2011/06/09 09:13:10, Paweł Hajdan Jr. wrote:
> nit: Why so many empty lines?

Removed.

[mattm, 2011-06-09 19:55:56]

lgtm

[gcasto (DO NOT USE), 2011-06-09 21:39:55]

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

http://codereview.chromium.org/7119003/diff/8002/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/browser_feature_extractor.cc:25: const char
kUrlHistoryVisitCount[] = "UrlHistoryVisitCount";
It might be nice to be able to easily distinguish features like this (i.e. those
that will not be used by the model) without having to hard code them. Maybe some
prefix like "BrowserFeatures" or something. Then again I'm not sure if all the
features of this type are going to be coming from the browser, though I think
that they are for the moment. Any thoughts?

http://codereview.chromium.org/7119003/diff/8002/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/browser_feature_extractor.cc:61: for
(PendingQueriesMap::iterator it = pending_queries_.begin();
I think that it would be cleaner for this to be merged in with the above loop,
and just have the history check in the loop.

http://codereview.chromium.org/7119003/diff/8002/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/browser_feature_extractor.cc:125: // that URL.
Just to make sure, is the store that the browser does not asynchronous, or is
somehow guaranteed to finish before we do our extraction?

http://codereview.chromium.org/7119003/diff/8002/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/browser_feature_extractor.cc:129:
ClientPhishingRequest::Feature* feature = request->add_feature_map();
Might be worth making these 4 lines a function, AddFeature(request, name,
value). It saves a few lines, including the copy and paste for the logging.

http://codereview.chromium.org/7119003/diff/8002/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/browser_feature_extractor.cc:169: // Issue next
history lookup for hist visits.
hist -> host

http://codereview.chromium.org/7119003/diff/8002/chrome/browser/safe_browsing...
File chrome/browser/safe_browsing/browser_feature_extractor.h (right):

http://codereview.chromium.org/7119003/diff/8002/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/browser_feature_extractor.h:84: // object.  This
method must run on the UI thread.
Maybe mention something about how we take ownership of the request, but only
until the callback is called? The comment currently seems slightly misleading.

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

http://codereview.chromium.org/7119003/diff/8002/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/client_side_detection_host.cc:389: VLOG(2) <<
"Feature extraction done (success:" << success << ") for URL: "
Do we want a histogram here so we can see how often we are failing in practice?

[noelutz, 2011-06-09 22:52:08]

Please take another look.
thanks,
noé.

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

http://codereview.chromium.org/7119003/diff/8002/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/browser_feature_extractor.cc:25: const char
kUrlHistoryVisitCount[] = "UrlHistoryVisitCount";
On 2011/06/09 21:39:55, gcasto wrote:
> It might be nice to be able to easily distinguish features like this (i.e.
those
> that will not be used by the model) without having to hard code them. Maybe
some
> prefix like "BrowserFeatures" or something. Then again I'm not sure if all the
> features of this type are going to be coming from the browser, though I think
> that they are for the moment. Any thoughts?

Done.

http://codereview.chromium.org/7119003/diff/8002/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/browser_feature_extractor.cc:61: for
(PendingQueriesMap::iterator it = pending_queries_.begin();
On 2011/06/09 21:39:55, gcasto wrote:
> I think that it would be cleaner for this to be merged in with the above loop,
> and just have the history check in the loop.

Done.

http://codereview.chromium.org/7119003/diff/8002/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/browser_feature_extractor.cc:125: // that URL.
On 2011/06/09 21:39:55, gcasto wrote:
> Just to make sure, is the store that the browser does not asynchronous, or is
> somehow guaranteed to finish before we do our extraction?

It looks like adding to the history service is async but I'm not sure if queries
return data for pending adds (some data is synchronously stored in a map and
then an async operation is started).

That said it's safe to assume that the page should already be in the history I
think.  The history is updated when we start with the pre-classification checks.
 This particular code is called much much later (once the entire page has
loaded).  I'm pretty sure that's enough time to commit changes to the history.

If it's not enough time we will know about it because we'll see that there
aren't any browser features stored in the request protocol buffer.

http://codereview.chromium.org/7119003/diff/8002/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/browser_feature_extractor.cc:129:
ClientPhishingRequest::Feature* feature = request->add_feature_map();
On 2011/06/09 21:39:55, gcasto wrote:
> Might be worth making these 4 lines a function, AddFeature(request, name,
> value). It saves a few lines, including the copy and paste for the logging.

Done.

http://codereview.chromium.org/7119003/diff/8002/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/browser_feature_extractor.cc:169: // Issue next
history lookup for hist visits.
On 2011/06/09 21:39:55, gcasto wrote:
> hist -> host

Done.

http://codereview.chromium.org/7119003/diff/8002/chrome/browser/safe_browsing...
File chrome/browser/safe_browsing/browser_feature_extractor.h (right):

http://codereview.chromium.org/7119003/diff/8002/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/browser_feature_extractor.h:84: // object.  This
method must run on the UI thread.
On 2011/06/09 21:39:55, gcasto wrote:
> Maybe mention something about how we take ownership of the request, but only
> until the callback is called? The comment currently seems slightly misleading.

Done.

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

http://codereview.chromium.org/7119003/diff/8002/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/client_side_detection_host.cc:389: VLOG(2) <<
"Feature extraction done (success:" << success << ") for URL: "
On 2011/06/09 21:39:55, gcasto wrote:
> Do we want a histogram here so we can see how often we are failing in
practice?

Maybe?  As of right now we can tell how often it happens by looking at the
requests that we see on the server.

[Brian Ryner, 2011-06-10 04:45:10]

Looks nice.  Just a few comments, mostly minor.

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

http://codereview.chromium.org/7119003/diff/9008/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/browser_feature_extractor.cc:62:
DCHECK(GetHistoryService(&history) || pending_queries_.size() == 0);
This won't work in release builds because GetHistoryService will never run.

http://codereview.chromium.org/7119003/diff/9008/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/browser_feature_extractor.cc:80:
DCHECK(request->url().find("http:") == 0);
DCHECK_EQ

http://codereview.chromium.org/7119003/diff/9008/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/browser_feature_extractor.cc:99: DCHECK_EQ(1U,
pending_extractions_.erase(extraction));
Ditto here, the erase will not happen in release builds.

http://codereview.chromium.org/7119003/diff/9008/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/browser_feature_extractor.cc:265: DCHECK(0 ==
pending_queries_.count(handle));
DCHECK_EQ

http://codereview.chromium.org/7119003/diff/9008/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/browser_feature_extractor.cc:286: *history =
tab_->profile()->GetHistoryService(Profile::EXPLICIT_ACCESS);
I'm not totally clear on whether EXPLICIT or IMPLICIT is correct here.

http://codereview.chromium.org/7119003/diff/9008/chrome/browser/safe_browsing...
File chrome/browser/safe_browsing/browser_feature_extractor_unittest.cc (right):

http://codereview.chromium.org/7119003/diff/9008/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/browser_feature_extractor_unittest.cc:28: public:
Aren't all of the test fixture methods normally protected or private?

http://codereview.chromium.org/7119003/diff/9008/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/browser_feature_extractor_unittest.cc:75:
success_[request] = success;
Do you want to assert that request is not already in success_ ?

http://codereview.chromium.org/7119003/diff/9008/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/browser_feature_extractor_unittest.cc:104:
history::SOURCE_BROWSED);  // different domain.
I think you mean "different host".  All of these examples are on the domain
foo.com.

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

http://codereview.chromium.org/7119003/diff/9008/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/client_side_detection_host.cc:393: request,  // The
service takes ownership of the verdict object.
nit: maybe say "the request object" just so we're using the same terminology
everywhere.

http://codereview.chromium.org/7119003/diff/9008/chrome/browser/safe_browsing...
File chrome/browser/safe_browsing/client_side_detection_host.h (right):

http://codereview.chromium.org/7119003/diff/9008/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/client_side_detection_host.h:61: void
FeatureExtractionDone(bool success, ClientPhishingRequest* request);
Add a brief comment for this method?

[gcasto (DO NOT USE), 2011-06-10 20:28:17]

LGTM

[noelutz, 2011-06-14 01:10:10]

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

http://codereview.chromium.org/7119003/diff/9008/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/browser_feature_extractor.cc:62:
DCHECK(GetHistoryService(&history) || pending_queries_.size() == 0);
On 2011/06/10 04:45:10, Brian Ryner wrote:
> This won't work in release builds because GetHistoryService will never run.

Good catch. Done.

http://codereview.chromium.org/7119003/diff/9008/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/browser_feature_extractor.cc:80:
DCHECK(request->url().find("http:") == 0);
On 2011/06/10 04:45:10, Brian Ryner wrote:
> DCHECK_EQ

Done.

http://codereview.chromium.org/7119003/diff/9008/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/browser_feature_extractor.cc:99: DCHECK_EQ(1U,
pending_extractions_.erase(extraction));
On 2011/06/10 04:45:10, Brian Ryner wrote:
> Ditto here, the erase will not happen in release builds.

Done.

http://codereview.chromium.org/7119003/diff/9008/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/browser_feature_extractor.cc:265: DCHECK(0 ==
pending_queries_.count(handle));
On 2011/06/10 04:45:10, Brian Ryner wrote:
> DCHECK_EQ

Done.

http://codereview.chromium.org/7119003/diff/9008/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/browser_feature_extractor.cc:286: *history =
tab_->profile()->GetHistoryService(Profile::EXPLICIT_ACCESS);
On 2011/06/10 04:45:10, Brian Ryner wrote:
> I'm not totally clear on whether EXPLICIT or IMPLICIT is correct here.

Only EXPLICIT_ACCESS does something.  See the profile code.

http://codereview.chromium.org/7119003/diff/9008/chrome/browser/safe_browsing...
File chrome/browser/safe_browsing/browser_feature_extractor_unittest.cc (right):

http://codereview.chromium.org/7119003/diff/9008/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/browser_feature_extractor_unittest.cc:28: public:
On 2011/06/10 04:45:10, Brian Ryner wrote:
> Aren't all of the test fixture methods normally protected or private?

Done.

http://codereview.chromium.org/7119003/diff/9008/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/browser_feature_extractor_unittest.cc:75:
success_[request] = success;
On 2011/06/10 04:45:10, Brian Ryner wrote:
> Do you want to assert that request is not already in success_ ?

Done.

http://codereview.chromium.org/7119003/diff/9008/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/browser_feature_extractor_unittest.cc:104:
history::SOURCE_BROWSED);  // different domain.
On 2011/06/10 04:45:10, Brian Ryner wrote:
> I think you mean "different host".  All of these examples are on the domain
> http://foo.com.

Done.

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

http://codereview.chromium.org/7119003/diff/9008/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/client_side_detection_host.cc:393: request,  // The
service takes ownership of the verdict object.
On 2011/06/10 04:45:10, Brian Ryner wrote:
> nit: maybe say "the request object" just so we're using the same terminology
> everywhere.

Done.

http://codereview.chromium.org/7119003/diff/9008/chrome/browser/safe_browsing...
File chrome/browser/safe_browsing/client_side_detection_host.h (right):

http://codereview.chromium.org/7119003/diff/9008/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/client_side_detection_host.h:61: void
FeatureExtractionDone(bool success, ClientPhishingRequest* request);
On 2011/06/10 04:45:10, Brian Ryner wrote:
> Add a brief comment for this method?

Done.

[Brian Ryner, 2011-06-14 01:25:15]

LGTM

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

http://codereview.chromium.org/7119003/diff/9008/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/browser_feature_extractor.cc:286: *history =
tab_->profile()->GetHistoryService(Profile::EXPLICIT_ACCESS);
On 2011/06/14 01:10:10, noelutz wrote:
> On 2011/06/10 04:45:10, Brian Ryner wrote:
> > I'm not totally clear on whether EXPLICIT or IMPLICIT is correct here.
> 
> Only EXPLICIT_ACCESS does something.  See the profile code.

In incognito mode (OffTheRecordProfileImpl), that's correct.  In normal mode
(ProfileImpl), it looks like it will return a HistoryService either way, unless
you have turned off saving history.

For now it doesn't really matter since we disable detection in incognito mode,
but we'll want to revisit this if we ever want to use history features with
incognito mode.

[commit-bot: I haz the power, 2011-06-14 20:33:59]

Try job failure for 7119003-11003 on linux_clang for step compile:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...

[commit-bot: I haz the power, 2011-06-15 08:42:08]

Failed to apply the patch.
Adding         chrome/browser/safe_browsing/browser_feature_extractor.cc
Adding         chrome/browser/safe_browsing/browser_feature_extractor.h
Adding        
chrome/browser/safe_browsing/browser_feature_extractor_unittest.cc
Sending        chrome/browser/safe_browsing/client_side_detection_host.cc
Sending        chrome/browser/safe_browsing/client_side_detection_host.h
Sending       
chrome/browser/safe_browsing/client_side_detection_host_unittest.cc
Sending        chrome/chrome_browser.gypi
Sending        chrome/chrome_tests.gypi
Sending        chrome/common/safe_browsing/csd.proto
Transmitting file data .........svn: Commit failed (details follow):
svn: File '/trunk/src/chrome/chrome_browser.gypi' is out of date

[M-A Ruel, 2011-06-15 12:42:13]

On 2011/06/15 08:42:08, I haz the power (commit-bot) wrote:
> Failed to apply the patch.
> Adding         chrome/browser/safe_browsing/browser_feature_extractor.cc
> Adding         chrome/browser/safe_browsing/browser_feature_extractor.h
> Adding        
> chrome/browser/safe_browsing/browser_feature_extractor_unittest.cc
> Sending        chrome/browser/safe_browsing/client_side_detection_host.cc
> Sending        chrome/browser/safe_browsing/client_side_detection_host.h
> Sending       
> chrome/browser/safe_browsing/client_side_detection_host_unittest.cc
> Sending        chrome/chrome_browser.gypi
> Sending        chrome/chrome_tests.gypi
> Sending        chrome/common/safe_browsing/csd.proto
> Transmitting file data .........svn: Commit failed (details follow):
> svn: File '/trunk/src/chrome/chrome_browser.gypi' is out of date

<3 svn.

Re-checking the commit box.

[commit-bot: I haz the power, 2011-06-15 13:32:26]

Change committed as 89178

[noelutz, 2011-06-15 22:01:28]

Fixed the valgrind issues (which were really bugs in my code).  Please take
another look.

Thanks,
noe.

On 2011/06/15 13:32:26, I haz the power (commit-bot) wrote:
> Change committed as 89178

[mattm, 2011-06-15 22:34:12]

http://codereview.chromium.org/7119003/diff/22004/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/browser_feature_extractor.cc (right):

http://codereview.chromium.org/7119003/diff/22004/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/browser_feature_extractor.cc:17: #include
"content/common/notification_details.h"
Could include of notification_details be omitted?

http://codereview.chromium.org/7119003/diff/22004/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/browser_feature_extractor.cc:18: #include
"content/common/notification_service.h"
this too?

http://codereview.chromium.org/7119003/diff/22004/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/browser_feature_extractor.cc:234:
callback->Run(false, request);
this one doesn't delete callback?

[noelutz, 2011-06-15 22:39:24]

Thanks.  Please take another look.
noe.

http://codereview.chromium.org/7119003/diff/22004/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/browser_feature_extractor.cc (right):

http://codereview.chromium.org/7119003/diff/22004/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/browser_feature_extractor.cc:17: #include
"content/common/notification_details.h"
On 2011/06/15 22:34:12, mattm wrote:
> Could include of notification_details be omitted?

I actually moved the notification to the host.

http://codereview.chromium.org/7119003/diff/22004/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/browser_feature_extractor.cc:18: #include
"content/common/notification_service.h"
On 2011/06/15 22:34:12, mattm wrote:
> this too?

same as above.

http://codereview.chromium.org/7119003/diff/22004/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/browser_feature_extractor.cc:234:
callback->Run(false, request);
On 2011/06/15 22:34:12, mattm wrote:
> this one doesn't delete callback?

Nice catch.

[mattm, 2011-06-15 23:22:39]

http://codereview.chromium.org/7119003/diff/24001/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/browser_feature_extractor.cc (right):

http://codereview.chromium.org/7119003/diff/24001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/browser_feature_extractor.cc:20: #include
"content/browser/tab_contents/navigation_controller.h"
unused now?

http://codereview.chromium.org/7119003/diff/24001/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/client_side_detection_host.cc (right):

http://codereview.chromium.org/7119003/diff/24001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/client_side_detection_host.cc:275:
ClientSideDetectionHost::~ClientSideDetectionHost() {}
So it's guaranteed that TabContentsDestroyed is called before the destructor?
(Do the tests follow this too?)

[noelutz, 2011-06-16 17:01:39]

http://codereview.chromium.org/7119003/diff/24001/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/browser_feature_extractor.cc (right):

http://codereview.chromium.org/7119003/diff/24001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/browser_feature_extractor.cc:20: #include
"content/browser/tab_contents/navigation_controller.h"
On 2011/06/15 23:22:39, mattm wrote:
> unused now?

Done.

http://codereview.chromium.org/7119003/diff/24001/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/client_side_detection_host.cc (right):

http://codereview.chromium.org/7119003/diff/24001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/client_side_detection_host.cc:275:
ClientSideDetectionHost::~ClientSideDetectionHost() {}
On 2011/06/15 23:22:39, mattm wrote:
> So it's guaranteed that TabContentsDestroyed is called before the destructor?
> (Do the tests follow this too?)

AFAIK yes.  All the code uses TabContentsWrapper now which owns the TabContents
object as a member variable.  In tab_contents_wrapper.h it says explicitly that
the TabContents will be deleted before all the observers (i.e., the
ClientSideDetectionHost in this case).   TabContentsDestroyed is called in the
TabContents destructor hence it should always be called before this destructor.

In the unit-test we use TabContentsWrapperTestHarness which uses the
TestTabContentsWrapper which also follows this pattern.

[mattm, 2011-06-16 20:28:00]

thanks, LGTM

[Brian Ryner, 2011-06-16 20:45:39]

LGTM

[commit-bot: I haz the power, 2011-06-18 17:10:58]

Change committed as 89603