Run pre-classification checks in the browser before starting client-side phishing detection.

chrome/browser/safe_browsing/client_side_detection_service_unittest.cc

Index: chrome/browser/safe_browsing/client_side_detection_service_unittest.cc
diff --git a/chrome/browser/safe_browsing/client_side_detection_service_unittest.cc b/chrome/browser/safe_browsing/client_side_detection_service_unittest.cc
index b418d14a6e4de6fc90c6b4f9204e0aba1c50eb19..c54cf39ec55f1aee1dfe25fdbc97b3d9280180ee 100644
--- a/chrome/browser/safe_browsing/client_side_detection_service_unittest.cc
+++ b/chrome/browser/safe_browsing/client_side_detection_service_unittest.cc
@@ -19,11 +19,15 @@
 #include "base/time.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "chrome/browser/browser_thread.h"
+#include "chrome/browser/renderer_host/test/test_render_view_host.h"
 #include "chrome/browser/safe_browsing/client_side_detection_service.h"
 #include "chrome/browser/safe_browsing/csd.pb.h"
+#include "chrome/common/render_messages.h"
 #include "chrome/common/net/test_url_fetcher_factory.h"
 #include "chrome/common/net/url_fetcher.h"
 #include "googleurl/src/gurl.h"
+#include "ipc/ipc_channel.h"
+#include "ipc/ipc_test_sink.h"
 #include "net/url_request/url_request_status.h"
 
 namespace safe_browsing {
@@ -224,4 +228,69 @@ TEST_F(ClientSideDetectionServiceTest, GetNumReportTest) {
   EXPECT_EQ(2, GetNumReportsPerDay());
 }
 
+// We use a separate test fixture for testing the ClientSideDetectionService's
+// handling of load notifications from TabContents.  This uses
+// RenderViewHostTestHarness to set up a fake TabContents and related objects.
+class ClientSideDetectionServiceHooksTest : public RenderViewHostTestHarness,
+                                            public IPC::Channel::Listener {
+ public:
+  // IPC::Channel::Listener
+  virtual bool OnMessageReceived(const IPC::Message& msg) {
+    if (msg.type() == ViewMsg_StartPhishingDetection::ID) {
+      received_msg_ = msg;
+      MessageLoop::current()->Quit();
+      return true;
+    }
+    return false;
+  }
+
+ protected:
+  virtual void SetUp() {
+    RenderViewHostTestHarness::SetUp();
+    file_thread_.reset(new BrowserThread(BrowserThread::FILE, &message_loop_));
+    ui_thread_.reset(new BrowserThread(BrowserThread::UI, &message_loop_));
+
+    // We're not exercising model fetching here, so just set up a canned
+    // success response.
+    factory_.reset(new FakeURLFetcherFactory());
+    factory_->SetFakeResponse(ClientSideDetectionService::kClientModelUrl,
+                              "dummy model data", true);
+    URLFetcher::set_factory(factory_.get());
+
+    process()->sink().AddFilter(this);
+  }
+
+  virtual void TearDown() {
+    process()->sink().RemoveFilter(this);
+    URLFetcher::set_factory(NULL);
+    file_thread_.reset();
+    ui_thread_.reset();
+    RenderViewHostTestHarness::TearDown();
+  }
+
+  scoped_ptr<FakeURLFetcherFactory> factory_;
+  scoped_ptr<BrowserThread> ui_thread_;
+  scoped_ptr<BrowserThread> file_thread_;
+  IPC::Message received_msg_;
+};
+
+TEST_F(ClientSideDetectionServiceHooksTest, ShouldClassifyUrl) {
+  ScopedTempDir tmp_dir;
+  ASSERT_TRUE(tmp_dir.CreateUniqueTempDir());
+  FilePath model_path = tmp_dir.path().AppendASCII("model");
+
+  scoped_ptr<ClientSideDetectionService> csd_service(
+      ClientSideDetectionService::Create(model_path, NULL));

[noelutz, 2011/02/10 01:36:07]

I created a separate empty constructor for this in my CL.  Feel free to do the
same and I will merge.

[Brian Ryner, 2011/02/10 21:15:20]

I'm not so sure I'm a fan of the second constructor.  It duplicates all of the
initialization, which makes it more likely that we'll forget at some point to
add a new initializer to both constructers.  Do you actually need to have the
empty constructor for your test, or could you just have your
MockClientSideDetectionService initialize the base class with an empty FilePath
and a NULL URLRequestContextGetter?

+
+  // Navigate the tab to a page.  We should see a StartPhishingDetection IPC.
+  NavigateAndCommit(GURL("http://host.com/"));
+  // Wait for the StartPhishingDetection message to arrive.
+  MessageLoop::current()->Run();
+
+  Tuple1<GURL> url;
+  ViewMsg_StartPhishingDetection::Read(&received_msg_, &url);
+  EXPECT_EQ(GURL("http://host.com/"), url.a);
+  EXPECT_EQ(rvh()->routing_id(), received_msg_.routing_id());

[noelutz, 2011/02/10 01:36:07]

Maybe do a reload and check that you don't send an IPC in this case?

[Brian Ryner, 2011/02/10 21:15:20]

Since I'm no longer doing anything different on back/forward navigation, there's
probably no need to test that case explicitly.

I wouldn't mind doing a test for the in-page navigation case, but it would be a
little difficult -- since I wouldn't expect an IPC, I wouldn't be able to quit
the message loop the same way as I'm doing above.  I could possibly call
RunAllPending() (more than once if needed)... thoughts?

+}
+
 }  // namespace safe_browsing