Make RenderView not have to know about how PhishingClassifierDelegate.

chrome/renderer/safe_browsing/phishing_classifier_delegate.cc

Index: chrome/renderer/safe_browsing/phishing_classifier_delegate.cc
===================================================================
--- chrome/renderer/safe_browsing/phishing_classifier_delegate.cc	(revision 73993)
+++ chrome/renderer/safe_browsing/phishing_classifier_delegate.cc	(working copy)
@@ -4,38 +4,101 @@
 
 #include "chrome/renderer/safe_browsing/phishing_classifier_delegate.h"
 
+#include <set>
+
 #include "base/callback.h"
+#include "base/lazy_instance.h"
 #include "base/logging.h"
+#include "base/scoped_callback_factory.h"
 #include "chrome/common/render_messages.h"
 #include "chrome/renderer/navigation_state.h"
+#include "chrome/renderer/render_thread.h"
 #include "chrome/renderer/render_view.h"
 #include "chrome/renderer/safe_browsing/feature_extractor_clock.h"
 #include "chrome/renderer/safe_browsing/phishing_classifier.h"
+#include "chrome/renderer/safe_browsing/scorer.h"
 #include "third_party/WebKit/Source/WebKit/chromium/public/WebFrame.h"
 #include "third_party/WebKit/Source/WebKit/chromium/public/WebURL.h"
 #include "third_party/WebKit/Source/WebKit/chromium/public/WebView.h"
 
 namespace safe_browsing {
 
+typedef std::set<PhishingClassifierDelegate*> PhishingClassifierDelegates;
+static base::LazyInstance<PhishingClassifierDelegates>
+    g_delegates(base::LINKER_INITIALIZED);
+
+static base::LazyInstance<scoped_ptr<const safe_browsing::Scorer> >
+    g_phishing_scorer(base::LINKER_INITIALIZED);
+
+class ScorerCallback {
+ public:
+  static Scorer::CreationCallback* CreateCallback() {
+    ScorerCallback* scorer_callback = new ScorerCallback();
+    return scorer_callback->callback_factory_->NewCallback(
+        &ScorerCallback::PhishingScorerCreated);
+  }
+
+ private:
+  ScorerCallback() {
+    callback_factory_.reset(
+        new base::ScopedCallbackFactory<ScorerCallback>(this));
+  }
+
+  // Callback to be run once the phishing Scorer has been created.
+  void PhishingScorerCreated(safe_browsing::Scorer* scorer) {
+    if (!scorer) {
+      DLOG(ERROR) << "Unable to create a PhishingScorer - corrupt model?";
+      return;
+    }
+
+    g_phishing_scorer.Get().reset(scorer);
+
+    PhishingClassifierDelegates::iterator i;
+    for (i = g_delegates.Get().begin(); i != g_delegates.Get().end(); ++i)
+      (*i)->SetPhishingScorer(scorer);
+
+    delete this;
+  }
+
+  scoped_ptr<base::ScopedCallbackFactory<ScorerCallback> > callback_factory_;
+};
+
+void PhishingClassifierDelegate::SetPhishingModel(
+    IPC::PlatformFileForTransit model_file) {
+  safe_browsing::Scorer::CreateFromFile(
+      IPC::PlatformFileForTransitToPlatformFile(model_file),
+      RenderThread::current()->GetFileThreadMessageLoopProxy(),
+      ScorerCallback::CreateCallback());
+}
+
 PhishingClassifierDelegate::PhishingClassifierDelegate(
     RenderView* render_view,
     PhishingClassifier* classifier)
-    : render_view_(render_view),
+    : RenderViewObserver(render_view),
       last_page_id_sent_to_classifier_(-1),
       pending_classification_(false) {
+  g_delegates.Get().insert(this);
   if (!classifier) {
-    classifier = new PhishingClassifier(render_view_,
+    classifier = new PhishingClassifier(render_view,
                                         new FeatureExtractorClock());
   }
+
+  if (g_phishing_scorer.Get().get())
+    SetPhishingScorer(g_phishing_scorer.Get().get());

[Brian Ryner, 2011/02/07 20:08:19]

I think this needs to be moved down to below where classifier_ is set on line
89.  Otherwise, classifier_ will still be NULL when SetPhishingScorer tries to
access it on line 102.  I'm surprised none of the test cases caught this, I'll
make sure to add one as a follow-up (or feel free to add it yourself if you'd
like)[

[jam, 2011/02/07 20:24:49]

Done.

+
   classifier_.reset(classifier);
 }
 
 PhishingClassifierDelegate::~PhishingClassifierDelegate() {
   CancelPendingClassification();
+  g_delegates.Get().erase(this);
 }
 
 void PhishingClassifierDelegate::SetPhishingScorer(
     const safe_browsing::Scorer* scorer) {
+  if (!render_view()->webview())
+    return;  // RenderView is tearing down.
+
   classifier_->set_phishing_scorer(scorer);
 
   if (pending_classification_) {
@@ -45,15 +108,18 @@
     // classification.  This is because we stop any pending classification on
     // main frame loads in RenderView::didCommitProvisionalLoad().
     DCHECK_EQ(StripToplevelUrl(), last_url_sent_to_classifier_);
-    DCHECK_EQ(render_view_->page_id(), last_page_id_sent_to_classifier_);
+    DCHECK_EQ(render_view()->page_id(), last_page_id_sent_to_classifier_);
     classifier_->BeginClassification(
         &classifier_page_text_,
         NewCallback(this, &PhishingClassifierDelegate::ClassificationDone));
   }
 }
 
-void PhishingClassifierDelegate::CommittedLoadInFrame(
-    WebKit::WebFrame* frame) {
+void PhishingClassifierDelegate::DidCommitProvisionalLoad(
+    WebKit::WebFrame* frame, bool is_new_navigation) {
+  if (!is_new_navigation)
+    return;
+
   // A new page is starting to load.  Unless the load is a navigation within
   // the same page, we need to cancel classification since the content will
   // now be inconsistent with the phishing model.
@@ -64,12 +130,12 @@
   }
 }
 
-void PhishingClassifierDelegate::FinishedLoad(string16* page_text) {
+void PhishingClassifierDelegate::PageCaptured(const string16& page_text) {
   // We check that the page id has incremented so that we don't reclassify
   // pages as the user moves back and forward in session history.  Note: we
   // don't send every page id to the classifier, only those where the toplevel
   // URL changed.
-  int load_id = render_view_->page_id();
+  int load_id = render_view()->page_id();
   if (load_id <= last_page_id_sent_to_classifier_) {
     return;
   }
@@ -84,7 +150,7 @@
 
   last_url_sent_to_classifier_ = url_without_ref;
   last_page_id_sent_to_classifier_ = load_id;
-  classifier_page_text_.swap(*page_text);
+  classifier_page_text_ = page_text;

[Brian Ryner, 2011/02/07 20:08:19]

As I mentioned to you offline, I'd set this up as a swap() to avoid copying a
potentially large string (up to 64KB).  It still seems preferable to avoid
this... what do you think?

[jam, 2011/02/07 20:24:49]

Yeah, I thought about it.  But I decided to skip this optimization because it
breaks the abstraction between RenderView and its observers.  It's preferable to
not add constraints on the ordering of observers.  I think if profiling shows
that this is needed, we can reconsider, but until then best to keep the API
simple.

[Brian Ryner, 2011/02/07 21:39:41]

Ok, sounds reasonable.

 
   if (classifier_->is_ready()) {
     classifier_->BeginClassification(
@@ -105,6 +171,18 @@
   pending_classification_ = false;
 }
 
+bool PhishingClassifierDelegate::OnMessageReceived(
+    const IPC::Message& message) {
+  /*

[Brian Ryner, 2011/02/07 20:08:19]

Did you mean to leave this commented-out code here?

[jam, 2011/02/07 20:24:49]

yep.  I can't uncomment it until a message is added, which is awaiting
http://codereview.chromium.org/6398001/

[Brian Ryner, 2011/02/07 21:39:41]

Ok.  If you end up submitting this first, I'll merge it into my change.

+  bool handled = true;
+  IPC_BEGIN_MESSAGE_MAP(PhishingClassifierDelegate, message)
+    IPC_MESSAGE_UNHANDLED(handled = false)
+  IPC_END_MESSAGE_MAP()
+  return handled;
+  */
+  return false;
+}
+
 void PhishingClassifierDelegate::ClassificationDone(bool is_phishy,
                                                     double phishy_score) {
   // We no longer need the page text.
@@ -114,14 +192,14 @@
     return;
   }
 
-  render_view_->Send(new ViewHostMsg_DetectedPhishingSite(
-      render_view_->routing_id(),
+  render_view()->Send(new ViewHostMsg_DetectedPhishingSite(
+      render_view()->routing_id(),
       last_url_sent_to_classifier_,
       phishy_score));
 }
 
 GURL PhishingClassifierDelegate::StripToplevelUrl() {
-  GURL toplevel_url = render_view_->webview()->mainFrame()->url();
+  GURL toplevel_url = render_view()->webview()->mainFrame()->url();
   GURL::Replacements replacements;
   replacements.ClearRef();
   return toplevel_url.ReplaceComponents(replacements);