Create a browser feature extractor that runs after the renderer has

chrome/browser/safe_browsing/client_side_detection_host.cc

Index: chrome/browser/safe_browsing/client_side_detection_host.cc
diff --git a/chrome/browser/safe_browsing/client_side_detection_host.cc b/chrome/browser/safe_browsing/client_side_detection_host.cc
index 4cb448c5a77dc99262b3a93e1b982adf17e73257..cf2d7579bebc43950401ca7a006d11671f685912 100644
--- a/chrome/browser/safe_browsing/client_side_detection_host.cc
+++ b/chrome/browser/safe_browsing/client_side_detection_host.cc
@@ -13,6 +13,7 @@
 #include "base/task.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/safe_browsing/browser_feature_extractor.h"
 #include "chrome/browser/safe_browsing/client_side_detection_service.h"
 #include "chrome/browser/safe_browsing/safe_browsing_service.h"
 #include "chrome/common/chrome_switches.h"
@@ -264,18 +265,14 @@ ClientSideDetectionHost* ClientSideDetectionHost::Create(
 ClientSideDetectionHost::ClientSideDetectionHost(TabContents* tab)
     : TabContentsObserver(tab),
       csd_service_(g_browser_process->safe_browsing_detection_service()),
+      feature_extractor_(new BrowserFeatureExtractor(tab)),
       cb_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)) {
   DCHECK(tab);
   // Note: csd_service_ and sb_service_ might be NULL.
   sb_service_ = g_browser_process->safe_browsing_service();
 }
 
-ClientSideDetectionHost::~ClientSideDetectionHost() {
-  // Tell any pending classification request that it is being canceled.
-  if (classification_request_.get()) {
-    classification_request_->Cancel();
-  }
-}
+ClientSideDetectionHost::~ClientSideDetectionHost() {}

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

So it's guaranteed that TabContentsDestroyed is called before the destructor?
(Do the tests follow this too?)

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

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.

 
 bool ClientSideDetectionHost::OnMessageReceived(const IPC::Message& message) {
   bool handled = true;
@@ -324,6 +321,16 @@ void ClientSideDetectionHost::DidNavigateMainFramePostCommit(
   }
 }
 
+void ClientSideDetectionHost::TabContentsDestroyed(TabContents* tab) {
+  DCHECK(tab);
+  // Tell any pending classification request that it is being canceled.
+  if (classification_request_.get()) {
+    classification_request_->Cancel();
+  }
+  // Cancel all pending feature extractions.
+  feature_extractor_.reset();
+}
+
 void ClientSideDetectionHost::OnDetectedPhishingSite(
     const std::string& verdict_str) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
@@ -340,12 +347,12 @@ void ClientSideDetectionHost::OnDetectedPhishingSite(
     // There shouldn't be any pending requests because we revoke them everytime
     // we navigate away.
     DCHECK(!cb_factory_.HasPendingCallbacks());
-    VLOG(2) << "Start sending client phishing request for URL: "
-            << verdict->url();
-    csd_service_->SendClientReportPhishingRequest(
-        verdict.release(),  // The service takes ownership of the verdict.
-        cb_factory_.NewCallback(
-            &ClientSideDetectionHost::MaybeShowPhishingWarning));
+
+    // Start browser-side feature extraction.  Once we're done it will send
+    // the client verdict request.
+    feature_extractor_->ExtractFeatures(
+        verdict.release(),
+        NewCallback(this, &ClientSideDetectionHost::FeatureExtractionDone));
   }
 }
 
@@ -377,6 +384,22 @@ void ClientSideDetectionHost::MaybeShowPhishingWarning(GURL phishing_url,
   }
 }
 
+void ClientSideDetectionHost::FeatureExtractionDone(
+    bool success,
+    ClientPhishingRequest* request) {
+  if (!request) {
+    DLOG(FATAL) << "Invalid request object in FeatureExtractionDone";
+    return;
+  }
+  VLOG(2) << "Feature extraction done (success:" << success << ") for URL: "
+          << request->url() << ". Start sending client phishing request.";
+  // Send ping no matter what - even if the browser feature extraction failed.
+  csd_service_->SendClientReportPhishingRequest(
+      request,  // The service takes ownership of the request object.
+      cb_factory_.NewCallback(
+          &ClientSideDetectionHost::MaybeShowPhishingWarning));
+}
+
 void ClientSideDetectionHost::set_client_side_detection_service(
     ClientSideDetectionService* service) {
   csd_service_ = service;