Intergration of the client-side phishing detection.

chrome/browser/safe_browsing/client_side_detection_host.h

+// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROME_BROWSER_SAFE_BROWSING_CLIENT_SIDE_DETECTION_HOST_H_
+#define CHROME_BROWSER_SAFE_BROWSING_CLIENT_SIDE_DETECTION_HOST_H_
+#pragma once
+
+#include "base/basictypes.h"
+#include "base/gtest_prod_util.h"
+#include "base/ref_counted.h"
+#include "base/scoped_callback_factory.h"
+#include "base/task.h"
+#include "chrome/browser/safe_browsing/safe_browsing_service.h"
+#include "chrome/browser/tab_contents/tab_contents_observer.h"
+#include "googleurl/src/gurl.h"
+
+class TabContents;
+
+namespace safe_browsing {
+
+class ClientSideDetectionService;
+class CsdClient;
+
+// This class is used to receive the IPC from the renderer which
+// notifies the browser that a URL was classified as phishing.  This
+// class relays this information to the client-side detection service
+// class which sends a ping to a server to validate the verdict.
+// TODO(noelutz): move all client-side detection IPCs to this class.

[lzheng, 2011/02/11 19:39:42]

What are other IPC messages that would be moved here? 
Looking at Brian's change http://codereview.chromium.org/6398001, 
does it make sense to move the ClientSideDetectionService::Observe here? 

[noelutz, 2011/02/15 23:00:55]

I guess we have three IPCs?

Browser -> Renderer:
  - send model file to the renderer
  - tell the renderer to start classification

Renderer -> Browser:
  - renderer tells the browser that a page is phishing.

I was thinking we could move the other two IPCs here but definitely in a
separate CL.  Thoughts?

[lzheng, 2011/02/16 19:09:32]

I agree.
On 2011/02/15 23:00:55, noelutz wrote:

+class ClientSideDetectionHost : public TabContentsObserver {
+ public:
+  // The caller keeps ownership of the tab object and is responsible for
+  // ensuring that it stays valid for the entire lifetime of this object.
+  explicit ClientSideDetectionHost(TabContents* tab);
+  virtual ~ClientSideDetectionHost();
+
+  // From TabContentsObserver.
+  virtual bool OnMessageReceived(const IPC::Message& message);
+
+  // From TabContentsObserver.  If we navigate way we cancel all pending

[Brian Ryner, 2011/02/11 01:30:39]

way -> away

[noelutz, 2011/02/15 23:00:55]

Done.

+  // callbacks that could show an interstitial.
+  virtual void DidNavigateMainFramePostCommit(
+      const NavigationController::LoadCommittedDetails& details,
+      const ViewHostMsg_FrameNavigate_Params& params);
+
+ private:
+  friend class ClientSideDetectionHostTest;
+  FRIEND_TEST_ALL_PREFIXES(ClientSideDetectionHostTest, OnDetectedPhishingSite);
+
+  // Handles the IPC that is sent from the renderer the a URL is

[jam, 2011/02/15 18:54:53]

nit: no need to comment IPC message handlers, since the message itself is
commented

[noelutz, 2011/02/15 23:00:55]

Done.

[jam, 2011/02/15 23:34:42]

nit: there's still a comment, what i mean is nothing is needed at all, i.e. to
say that a function handles message X.  we normally don't comment that, since
it's something that's apparent from the function name

[noelutz, 2011/02/16 21:24:43]

Done.

+  // classified as phishing.  This method will send a server ping to validate
+  // the phishing verdict.
+  void OnDetectedPhishingSite(const GURL& phishing_url, double phishing_score);
+
+  // Callback that is called when the server ping back is
+  // done. Display an interstitial if |is_phishing| is true.
+  // Otherwise, we do nothgin.  Called in UI thread.
+  void MaybeShowPhishingWarning(GURL phishing_url, bool is_phishing);
+
+  // Used for testing.  This function does not take ownership of the service
+  // class.
+  void set_client_side_detection_service(ClientSideDetectionService* service);
+
+  // Used for testing.  This function does not take ownership of the service
+  // class.
+  void set_safe_browsing_service(SafeBrowsingService* service);

[lzheng, 2011/02/11 19:39:42]

set_client_side_detection_service and set_safe_browsing_service could be removed
if you check the comments I left in .cc.

[noelutz, 2011/02/15 23:00:55]

see comment in the cc file.

+
+  // We are not responsible for deleting these objects.  The tab contents as
+  // well as the service object outlive this object.
+  TabContents* tab_;
+  ClientSideDetectionService* service_;  // might be NULL.

[Brian Ryner, 2011/02/11 01:30:39]

Here and on line 73 - can you explain briefly when these would be null?

[noelutz, 2011/02/15 23:00:55]

Done.

+
+  scoped_refptr<SafeBrowsingService> sb_service_;  // might be NULL.
+
+  base::ScopedCallbackFactory<ClientSideDetectionHost> cb_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(ClientSideDetectionHost);
+};
+
+// This class is used to display the phishing interstitial.
+// TODO(noelutz): might want to move this class to the CC file.

[Brian Ryner, 2011/02/11 01:30:39]

Seems reasonable to me to move it.

[noelutz, 2011/02/15 23:00:55]

Done.

+class CsdClient : public SafeBrowsingService::Client {
+ public:
+  explicit CsdClient();

[Brian Ryner, 2011/02/11 01:30:39]

"explicit" is only needed for 1-argument constructors.

[noelutz, 2011/02/15 23:00:55]

Sure. Leftover from previous version of the code.  Removed.

+
+  // Method from SafeBrowsingService::Client.  This method is called on the
+  // IO thread once the interstitial is going away.  This method simply deletes
+  // the CsdClient object.
+  virtual void OnBlockingPageComplete(bool proceed);
+
+ private:
+  friend class DeleteTask<CsdClient>;  // Calls the private destructor.
+
+  // We're taking care of deleting this object.  No-one else should delete
+  // this object.
+  ~CsdClient();

[Brian Ryner, 2011/02/11 01:30:39]

This should be declared virtual since it's virtual in the base class.

[noelutz, 2011/02/15 23:00:55]

Nice catch.

+
+  DISALLOW_COPY_AND_ASSIGN(CsdClient);
+};
+
+}  // namespace safe_browsing
+
+#endif  // CHROME_BROWSER_SAFE_BROWSING_CLIENT_SIDE_DETECTION_HOST_H_