Factor out a RenderThread interface

- RenderWidget was not unit testable as it was
- Adding the first ever RederWidget unit tests

It is possible to do more. Taking it step by step.

Review URL: http://codereview.chromium.org/4271

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@2649 0039d316-1c4b-4281-b951-d872f2087c98

6 files changed, 212 insertions, 29 deletions

diff --git a/chrome/renderer/render_thread.h b/chrome/renderer/render_thread.h
index 4dc9b99..2d1c198 100644
--- a/chrome/renderer/render_thread.h
+++ b/chrome/renderer/render_thread.h
@@ -19,6 +19,22 @@ struct WebPreferences;
 class RenderDnsMaster;
 class NotificationService;
 
+// The RenderThreadBase is the minimal interface that a RenderWidget expects
+// from a render thread. The interface basically abstracts a way to send and
+// receive messages. It is currently only used for testing.
+class RenderThreadBase : public IPC::Message::Sender {
+ public:
+  virtual ~RenderThreadBase() {}
+
+  // True if currently sending a message.
+  virtual bool InSend() const = 0;
+
+  // Called to add or remove a listener for a particular message routing ID.
+  // These methods normally get delegated to a MessageRouter.
+  virtual void AddRoute(int32 routing_id, IPC::Channel::Listener* listener) = 0;
+  virtual void RemoveRoute(int32 routing_id) = 0;
+};
+
 // The RenderThread class represents a background thread where RenderView
 // instances live.  The RenderThread supports an API that is used by its
 // consumer to talk indirectly to the RenderViews and supporting objects.
@@ -28,13 +44,12 @@ class NotificationService;
 // Most of the communication occurs in the form of IPC messages.  They are
 // routed to the RenderThread according to the routing IDs of the messages.
 // The routing IDs correspond to RenderView instances.
-
 class RenderThread : public IPC::Channel::Listener,
-                     public IPC::Message::Sender,
+                     public RenderThreadBase,
                      public base::Thread {
  public:
   RenderThread(const std::wstring& channel_name);
-  ~RenderThread();
+  virtual ~RenderThread();
 
   // IPC::Channel::Listener implementation:
   virtual void OnMessageReceived(const IPC::Message& msg);
@@ -55,8 +70,8 @@ class RenderThread : public IPC::Channel::Listener,
   void Resolve(const char* name, size_t length);
 
   // See documentation on MessageRouter for AddRoute and RemoveRoute
-  void AddRoute(int32 routing_id, IPC::Channel::Listener* listener);
-  void RemoveRoute(int32 routing_id);
+  virtual void AddRoute(int32 routing_id, IPC::Channel::Listener* listener);
+  virtual void RemoveRoute(int32 routing_id);
 
   // Invokes InformHostOfCacheStats after a short delay.  Used to move this
   // bookkeeping operation off the critical latency path.
@@ -65,7 +80,7 @@ class RenderThread : public IPC::Channel::Listener,
   MessageLoop* owner_loop() { return owner_loop_; }
 
   // Indicates if RenderThread::Send() is on the call stack.
-  bool in_send() const { return in_send_ != 0; }
+  virtual bool InSend() const { return in_send_ != 0; }
 
  protected:
   // Called by the thread base class
diff --git a/chrome/renderer/render_view.cc b/chrome/renderer/render_view.cc
index 9bfed29..1ed80c7 100644
--- a/chrome/renderer/render_view.cc
+++ b/chrome/renderer/render_view.cc
@@ -134,7 +134,7 @@ class RenderViewExtraRequestData : public WebRequest::ExtraData {
 ///////////////////////////////////////////////////////////////////////////////
 
 RenderView::RenderView()
-  : RenderWidget(),
+  : RenderWidget(RenderThread::current()),
     is_loading_(false),
     page_id_(-1),
     last_page_id_sent_to_browser_(-1),
@@ -1658,7 +1658,8 @@ WebView* RenderView::CreateWebView(WebView* webview, bool user_gesture) {
 }
 
 WebWidget* RenderView::CreatePopupWidget(WebView* webview) {
-  RenderWidget* widget = RenderWidget::Create(routing_id_);
+  RenderWidget* widget = RenderWidget::Create(routing_id_,
+                                              RenderThread::current());
   return widget->webwidget();
 }
 
diff --git a/chrome/renderer/render_widget.cc b/chrome/renderer/render_widget.cc
index 95b1050..abfe528 100644
--- a/chrome/renderer/render_widget.cc
+++ b/chrome/renderer/render_widget.cc
@@ -12,6 +12,8 @@
 #include "base/message_loop.h"
 #include "base/gfx/platform_canvas_win.h"
 #include "base/scoped_ptr.h"
+#include "chrome/renderer/render_process.h"
+
 #include "webkit/glue/webinputevent.h"
 #include "webkit/glue/webwidget.h"
 
@@ -33,7 +35,6 @@ class DeferredCloses : public Task {
 
   // Called to trigger any deferred closes to be run.
   static void Post() {
-    DCHECK(!RenderThread::current()->in_send());
     if (current_) {
       MessageLoop::current()->PostTask(FROM_HERE, current_);
       current_ = NULL;
@@ -46,7 +47,7 @@ class DeferredCloses : public Task {
     // that is true, then we need to re-queue the widgets to be closed and try
     // again later.
     while (!queue_.empty()) {
-      if (RenderThread::current()->in_send()) {
+      if (queue_.front()->InSend()) {
         Push(queue_.front());
       } else {
         queue_.front()->Close();
@@ -68,9 +69,10 @@ DeferredCloses* DeferredCloses::current_ = NULL;
 
 ///////////////////////////////////////////////////////////////////////////////
 
-RenderWidget::RenderWidget()
+RenderWidget::RenderWidget(RenderThreadBase* render_thread)
     : routing_id_(MSG_ROUTING_NONE),
       opener_id_(MSG_ROUTING_NONE),
+      render_thread_(render_thread),
       host_window_(NULL),
       current_paint_buf_(NULL),
       current_scroll_buf_(NULL),
@@ -88,6 +90,7 @@ RenderWidget::RenderWidget()
       ime_control_new_state_(false),
       ime_control_updated_(false) {
   RenderProcess::AddRefProcess();
+  DCHECK(render_thread_);
 }
 
 RenderWidget::~RenderWidget() {
@@ -103,9 +106,10 @@ RenderWidget::~RenderWidget() {
 }
 
 /*static*/
-RenderWidget* RenderWidget::Create(int32 opener_id) {
+RenderWidget* RenderWidget::Create(int32 opener_id,
+                                   RenderThreadBase* render_thread) {
   DCHECK(opener_id != MSG_ROUTING_NONE);
-  scoped_refptr<RenderWidget> widget = new RenderWidget();
+  scoped_refptr<RenderWidget> widget = new RenderWidget(render_thread);
   widget->Init(opener_id);  // adds reference
   return widget;
 }
@@ -120,10 +124,10 @@ void RenderWidget::Init(int32 opener_id) {
   WebWidget* webwidget = WebWidget::Create(this);
   webwidget_.swap(&webwidget);
 
-  bool result = RenderThread::current()->Send(
+  bool result = render_thread_->Send(
       new ViewHostMsg_CreateWidget(opener_id, &routing_id_));
   if (result) {
-    RenderThread::current()->AddRoute(routing_id_, this);
+    render_thread_->AddRoute(routing_id_, this);
     // Take a reference on behalf of the RenderThread.  This will be balanced
     // when we receive ViewMsg_Close.
     AddRef();
@@ -172,17 +176,21 @@ bool RenderWidget::Send(IPC::Message* message) {
   if (message->routing_id() == MSG_ROUTING_NONE)
     message->set_routing_id(routing_id_);
 
-  bool rv = RenderThread::current()->Send(message);
+  bool rv = render_thread_->Send(message);
 
   // If there aren't any more RenderThread::Send calls on the stack, then we
   // can go ahead and schedule Close to be called on any RenderWidget objects
   // that received a ViewMsg_Close while we were inside Send.
-  if (!RenderThread::current()->in_send())
+  if (!render_thread_->InSend())
     DeferredCloses::Post();
 
   return rv;
 }
 
+bool RenderWidget::InSend() const {
+  return render_thread_->InSend();
+}
+
 // Got a response from the browser after the renderer decided to create a new
 // view.
 void RenderWidget::OnCreatingNewAck(HWND parent) {
@@ -198,7 +206,7 @@ void RenderWidget::OnClose() {
 
   // Browser correspondence is no longer needed at this point.
   if (routing_id_ != MSG_ROUTING_NONE)
-    RenderThread::current()->RemoveRoute(routing_id_);
+    render_thread_->RemoveRoute(routing_id_);
 
   // Balances the AddRef taken when we called AddRoute.  This release happens
   // via the MessageLoop since it may cause our destruction.
@@ -206,7 +214,7 @@ void RenderWidget::OnClose() {
 
   // If there is a Send call on the stack, then it could be dangerous to close
   // now.  Instead, we wait until we get out of Send.
-  if (RenderThread::current()->in_send()) {
+  if (render_thread_->InSend()) {
     DeferredCloses::Push(this);
   } else {
     Close();
@@ -593,7 +601,7 @@ void RenderWidget::Show(WebWidget* webwidget,
     // NOTE: initial_pos_ may still have its default values at this point, but
     // that's okay.  It'll be ignored if as_popup is false, or the browser
     // process will impose a default position otherwise.
-    RenderThread::current()->Send(new ViewHostMsg_ShowWidget(
+    render_thread_->Send(new ViewHostMsg_ShowWidget(
         opener_id_, routing_id_, initial_pos_));
   }
 }
diff --git a/chrome/renderer/render_widget.h b/chrome/renderer/render_widget.h
index ea1e4f7e..c9bdc39 100644
--- a/chrome/renderer/render_widget.h
+++ b/chrome/renderer/render_widget.h
@@ -13,11 +13,13 @@
 #include "base/ref_counted.h"
 #include "chrome/common/ipc_channel.h"
 #include "chrome/common/render_messages.h"
-#include "chrome/renderer/render_process.h"
+
 #include "webkit/glue/webwidget_delegate.h"
 #include "webkit/glue/webcursor.h"
 #include "webkit/glue/webplugin.h"
 
+class RenderThreadBase;
+
 // RenderWidget provides a communication bridge between a WebWidget and
 // a RenderWidgetHost, the latter of which lives in a different process.
 class RenderWidget : public IPC::Channel::Listener,
@@ -26,8 +28,9 @@ class RenderWidget : public IPC::Channel::Listener,
                      public base::RefCounted<RenderWidget> {
  public:
   // Creates a new RenderWidget.  The opener_id is the routing ID of the
-  // RenderView that this widget lives inside.
-  static RenderWidget* Create(int32 opener_id);
+  // RenderView that this widget lives inside. The render_thread is any
+  // RenderThreadBase implementation, mostly commonly RenderThread::current().
+  static RenderWidget* Create(int32 opener_id, RenderThreadBase* render_thread);
 
   // The routing ID assigned by the RenderProcess. Will be MSG_ROUTING_NONE if
   // not yet assigned a view ID, in which case, the process MUST NOT send
@@ -55,6 +58,9 @@ class RenderWidget : public IPC::Channel::Listener,
   // IPC::Message::Sender
   virtual bool Send(IPC::Message* msg);
 
+  // True if the underlying IPC is currently sending data.
+  bool InSend() const;
+
   // WebWidgetDelegate
   virtual HWND GetContainingWindow(WebWidget* webwidget);
   virtual void DidInvalidateRect(WebWidget* webwidget, const gfx::Rect& rect);
@@ -71,14 +77,16 @@ class RenderWidget : public IPC::Channel::Listener,
   virtual void DidMove(WebWidget* webwidget, const WebPluginGeometry& move);
   virtual void RunModal(WebWidget* webwidget) {}
 
-  // Do not delete directly.  This class is reference counted.
-  virtual ~RenderWidget();
-
   // Close the underlying WebWidget.
   void Close();
 
  protected:
-  RenderWidget();
+  // Friend RefCounted so that the dtor can be non-public. Using this class
+  // without ref-counting is an error.
+  friend class base::RefCounted<RenderWidget>;
+
+  RenderWidget(RenderThreadBase* render_thread);
+  virtual ~RenderWidget();
 
   // Initializes this view with the given opener.  CompleteInit must be called
   // later.
@@ -176,6 +184,9 @@ class RenderWidget : public IPC::Channel::Listener,
   // view is.
   int32 opener_id_;
 
+  // The thread that does our IPC.
+  RenderThreadBase* render_thread_;
+
   // The position where this view should be initially shown.
   gfx::Rect initial_pos_;
 
diff --git a/chrome/renderer/render_widget_unittest.cc b/chrome/renderer/render_widget_unittest.cc
new file mode 100644
index 0000000..9638547
--- /dev/null
+++ b/chrome/renderer/render_widget_unittest.cc
@@ -0,0 +1,140 @@
+// Copyright (c) 2006-2008 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.
+
+#include "testing/gtest/include/gtest/gtest.h"
+
+#include "base/ref_counted.h"
+#include "chrome/renderer/render_widget.h"
+#include "chrome/renderer/render_thread.h"
+
+// This class is very simple mock of RenderThread. It simulates an IPC channel
+// which supports only two messages:
+// ViewHostMsg_CreateWidget : sync message sent by the Widget.
+// ViewMsg_Close : async, send to the Widget.
+class MockRenderThread : public RenderThreadBase {
+ public:
+  MockRenderThread() 
+      : routing_id_(0), opener_id_(0), widget_(NULL),
+        reply_deserializer_(NULL) {
+  }
+  virtual ~MockRenderThread() {
+  }
+
+  // Called by the Widget. Not used in the test.
+  virtual bool InSend() const {
+    return false;
+  }
+
+  // Called by the Widget. The routing_id must match the routing id assigned
+  // to the Widget in reply to ViewHostMsg_CreateWidget message.
+  virtual void AddRoute(int32 routing_id, IPC::Channel::Listener* listener) {
+    EXPECT_EQ(routing_id_, routing_id);
+    widget_ = listener;
+  }
+
+  // Called by the Widget. The routing id must match the routing id of AddRoute.
+  virtual void RemoveRoute(int32 routing_id) {
+    EXPECT_EQ(routing_id_, routing_id);
+    widget_ = NULL;
+  }
+
+  // Called by the Widget. Used to send messages to the browser.
+  // We short-circuit the mechanim and handle the messages right here on this
+  // class.
+  virtual bool Send(IPC::Message* msg) {
+    // We need to simulate a synchronous channel, thus we are going to receive
+    // through this function messages, messages with reply and reply messages.
+    // We can only handle one synchronous message at a time.
+    if (msg->is_reply()) {
+      if (reply_deserializer_)
+        reply_deserializer_->SerializeOutputParameters(*msg);
+        reply_deserializer_ = NULL;
+    } else {
+      if (msg->is_sync()) {
+        reply_deserializer_ = 
+            static_cast<IPC::SyncMessage*>(msg)->GetReplyDeserializer();
+      }
+      OnMessageReceived(*msg);
+    }
+    delete msg;
+    return true;
+  }
+
+  //////////////////////////////////////////////////////////////////////////
+  // The following functions are called by the test itself.
+
+  void set_routing_id(int32 id) {
+    routing_id_ = id;
+  }
+
+  int32 opener_id() const {
+    return opener_id_;
+  }
+
+  bool has_widget() const {
+    return widget_ ? true : false;
+  }
+
+  // Simulates the Widget receiving a close message. This should result
+  // on releasing the internal reference counts and destroying the internal
+  // state.
+  void SendCloseMessage() {
+    ViewMsg_Close msg(routing_id_);
+    widget_->OnMessageReceived(msg);
+  }
+
+ private:
+
+  // This function operates as a regular IPC listener.
+  void OnMessageReceived(const IPC::Message& msg) {
+    bool handled = true;
+    bool msg_is_ok = true;
+    IPC_BEGIN_MESSAGE_MAP_EX(MockRenderThread, msg, msg_is_ok)
+      IPC_MESSAGE_HANDLER(ViewHostMsg_CreateWidget, OnMsgCreateWidget);
+      IPC_MESSAGE_UNHANDLED(handled = false)
+    IPC_END_MESSAGE_MAP_EX()
+
+    // This test must handle all messages that RenderWidget generates.
+    EXPECT_TRUE(handled);
+  }
+
+  // The Widget expects to be returned valid route_id.
+  void OnMsgCreateWidget(int opener_id, int* route_id) {
+    opener_id_ = opener_id;
+    *route_id = routing_id_;
+  }
+
+  // Routing id what will be assigned to the Widget.
+  int32 routing_id_;
+  // Opener id reported by the Widget.
+  int32 opener_id_;
+  // We only keep track of one Widget, we learn its pointer when it
+  // adds a new route.
+  IPC::Channel::Listener* widget_;
+  // The last known good deserializer for sync messages.
+  IPC::MessageReplyDeserializer* reply_deserializer_;
+};
+
+TEST(RenderWidgetTest, CreateAndCloseWidget) {
+  MessageLoop msg_loop;
+  MockRenderThread render_thread;
+
+  const int32 kRouteId = 5;
+  const int32 kOpenerId = 7;
+
+  render_thread.set_routing_id(kRouteId);
+  scoped_refptr<RenderWidget> rw =
+      RenderWidget::Create(kOpenerId, &render_thread);
+  ASSERT_TRUE(rw != NULL);
+
+  // After the RenderWidget it must have sent a message to the render thread
+  // that sets the opener id.
+  EXPECT_EQ(kOpenerId, render_thread.opener_id());
+  ASSERT_TRUE(render_thread.has_widget());
+
+  // Now simulate a close of the Widget.
+  render_thread.SendCloseMessage();
+  EXPECT_FALSE(render_thread.has_widget());  
+}
+
diff --git a/chrome/test/unit/unittests.vcproj b/chrome/test/unit/unittests.vcproj
index 64e7d57..0a3260b 100644
--- a/chrome/test/unit/unittests.vcproj
+++ b/chrome/test/unit/unittests.vcproj
@@ -762,11 +762,11 @@
 				>
 			</File>
 			<File
-				RelativePath="..\..\browser\safe_browsing\safe_browsing_database_unittest.cc"
+				RelativePath="..\..\browser\safe_browsing\safe_browsing_database_impl_unittest.cc"
 				>
 			</File>
 			<File
-				RelativePath="..\..\browser\safe_browsing\safe_browsing_database_impl_unittest.cc"
+				RelativePath="..\..\browser\safe_browsing\safe_browsing_database_unittest.cc"
 				>
 			</File>
 			<File
@@ -954,6 +954,14 @@
 				>
 			</File>
 		</Filter>
+		<Filter
+			Name="TestRenderWidget"
+			>
+			<File
+				RelativePath="..\..\renderer\render_widget_unittest.cc"
+				>
+			</File>
+		</Filter>
 	</Files>
 	<Globals>
 	</Globals>