Let RenderProcessHost provide a method HasConnection() instead of exposing

channel().

Motivation:
Currently, components that are using RenderProcessHost are checking its
liveness by null testing on channel(). I'd like to write unittests for those
components, but to mock out RenderProcessHost instances, I have to also
mock out the instance returned by RenderProcessHost::channel(), but there's
no interface class prepared. SyncChannel is directly used in RenderProcessHost.
Instead of dependency injection, I can let mock objects return invalid pointer
such as 0x1, but its bad test design. Rather than that, I'd like to introduce
HasConnection() method and override it to return true.

In fact, most of those components are not accessing channel()'s methods
directry. They're just checking channel() is null or not, and to issue IPCs,
they are calling Send method. So, it's OK to hide channel pointer from users, I
think.

Original review: http://codereview.chromium.org/147077
Patch by tyoshino@google.com

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

12 files changed, 41 insertions, 17 deletions

diff --git a/chrome/browser/browser_accessibility_manager.cc b/chrome/browser/browser_accessibility_manager.cc
index b2945e9..7a7c3b3 100644
--- a/chrome/browser/browser_accessibility_manager.cc
+++ b/chrome/browser/browser_accessibility_manager.cc
@@ -85,12 +85,12 @@ bool BrowserAccessibilityManager::RequestAccessibilityInfo(
 
   // Send accessibility information retrieval message to the renderer.
   bool success = false;
-  if (rvh && rvh->process() && rvh->process()->channel()) {
+  if (rvh && rvh->process() && rvh->process()->HasConnection()) {
     IPC::SyncMessage* msg =
         new ViewMsg_GetAccessibilityInfo(routing_id, in_params, &out_params_);
     // Necessary for the send to keep the UI responsive.
     msg->EnableMessagePumping();
-    success = rvh->process()->channel()->SendWithTimeout(msg,
+    success = rvh->process()->SendWithTimeout(msg,
         kAccessibilityMessageTimeOut);
   }
   return success;
diff --git a/chrome/browser/profile.cc b/chrome/browser/profile.cc
index 54ecdf2..f5182c0 100644
--- a/chrome/browser/profile.cc
+++ b/chrome/browser/profile.cc
@@ -667,7 +667,7 @@ static void BroadcastNewHistoryTable(base::SharedMemory* table_memory) {
   // send to all RenderProcessHosts
   for (RenderProcessHost::iterator i = RenderProcessHost::begin();
        i != RenderProcessHost::end(); i++) {
-    if (!i->second->channel())
+    if (!i->second->HasConnection())
       continue;
 
     base::SharedMemoryHandle new_table;
@@ -679,7 +679,7 @@ static void BroadcastNewHistoryTable(base::SharedMemory* table_memory) {
 
     table_memory->ShareToProcess(process, &new_table);
     IPC::Message* msg = new ViewMsg_VisitedLink_NewTable(new_table);
-    i->second->channel()->Send(msg);
+    i->second->Send(msg);
   }
 }
 
diff --git a/chrome/browser/renderer_host/browser_render_process_host.cc b/chrome/browser/renderer_host/browser_render_process_host.cc
index ecea28c..b4d763e 100644
--- a/chrome/browser/renderer_host/browser_render_process_host.cc
+++ b/chrome/browser/renderer_host/browser_render_process_host.cc
@@ -581,6 +581,15 @@ bool BrowserRenderProcessHost::FastShutdownIfPossible() {
   return true;
 }
 
+bool BrowserRenderProcessHost::SendWithTimeout(IPC::Message* msg,
+                                               int timeout_ms) {
+  if (!channel_.get()) {
+    delete msg;
+    return false;
+  }
+  return channel_->SendWithTimeout(msg, timeout_ms);
+}
+
 // This is a platform specific function for mapping a transport DIB given its id
 TransportDIB* BrowserRenderProcessHost::MapTransportDIB(
     TransportDIB::Id dib_id) {
diff --git a/chrome/browser/renderer_host/browser_render_process_host.h b/chrome/browser/renderer_host/browser_render_process_host.h
index 6c259f0..aee8d2b 100644
--- a/chrome/browser/renderer_host/browser_render_process_host.h
+++ b/chrome/browser/renderer_host/browser_render_process_host.h
@@ -65,6 +65,7 @@ class BrowserRenderProcessHost : public RenderProcessHost,
   virtual void WidgetHidden();
   virtual void AddWord(const std::wstring& word);
   virtual bool FastShutdownIfPossible();
+  virtual bool SendWithTimeout(IPC::Message* msg, int timeout_ms);
   virtual TransportDIB* GetTransportDIB(TransportDIB::Id dib_id);
 
   // IPC::Channel::Sender via RenderProcessHost.
diff --git a/chrome/browser/renderer_host/mock_render_process_host.cc b/chrome/browser/renderer_host/mock_render_process_host.cc
index 8cf0e10..4d4eda2 100644
--- a/chrome/browser/renderer_host/mock_render_process_host.cc
+++ b/chrome/browser/renderer_host/mock_render_process_host.cc
@@ -55,6 +55,13 @@ bool MockRenderProcessHost::FastShutdownIfPossible() {
   return false;
 }
 
+bool MockRenderProcessHost::SendWithTimeout(IPC::Message* msg, int timeout_ms) {
+  // Save the message in the sink. Just ignore timeout_ms.
+  sink_.OnMessageReceived(*msg);
+  delete msg;
+  return true;
+}
+
 bool MockRenderProcessHost::Send(IPC::Message* msg) {
   // Save the message in the sink.
   sink_.OnMessageReceived(*msg);
diff --git a/chrome/browser/renderer_host/mock_render_process_host.h b/chrome/browser/renderer_host/mock_render_process_host.h
index 7aea378..7975636 100644
--- a/chrome/browser/renderer_host/mock_render_process_host.h
+++ b/chrome/browser/renderer_host/mock_render_process_host.h
@@ -40,6 +40,7 @@ class MockRenderProcessHost : public RenderProcessHost {
   virtual void WidgetHidden();
   virtual void AddWord(const std::wstring& word);
   virtual bool FastShutdownIfPossible();
+  virtual bool SendWithTimeout(IPC::Message* msg, int timeout_ms);
   virtual TransportDIB* GetTransportDIB(TransportDIB::Id dib_id);
 
   // IPC::Channel::Sender via RenderProcessHost.
diff --git a/chrome/browser/renderer_host/render_process_host.h b/chrome/browser/renderer_host/render_process_host.h
index ef79711..e661a0f 100644
--- a/chrome/browser/renderer_host/render_process_host.h
+++ b/chrome/browser/renderer_host/render_process_host.h
@@ -54,8 +54,9 @@ class RenderProcessHost : public IPC::Channel::Sender,
   // process.
   const base::Process& process() const { return process_; }
 
-  // May return NULL if there is no connection.
-  IPC::SyncChannel* channel() { return channel_.get(); }
+  // Returns true iff channel_ has been set to non-NULL. Use this for checking
+  // if there is connection or not.
+  bool HasConnection() { return channel_.get() != NULL; }
 
   bool sudden_termination_allowed() const {
     return sudden_termination_allowed_;
@@ -143,6 +144,11 @@ class RenderProcessHost : public IPC::Channel::Sender,
   // Returns True if it was able to do fast shutdown.
   virtual bool FastShutdownIfPossible() = 0;
 
+  // Synchronously sends the message, waiting for the specified timeout. The
+  // implementor takes ownership of the given Message regardless of whether or
+  // not this method succeeds. Returns true on success.
+  virtual bool SendWithTimeout(IPC::Message* msg, int timeout_ms) = 0;
+
   // Transport DIB functions ---------------------------------------------------
 
   // Return the TransportDIB for the given id. On Linux, this can involve
diff --git a/chrome/browser/renderer_host/render_view_host.cc b/chrome/browser/renderer_host/render_view_host.cc
index d41cac3..87f4bf0 100644
--- a/chrome/browser/renderer_host/render_view_host.cc
+++ b/chrome/browser/renderer_host/render_view_host.cc
@@ -139,7 +139,7 @@ bool RenderViewHost::CreateRenderView() {
   // ignored, so this is safe.
   if (!process()->Init())
     return false;
-  DCHECK(process()->channel());
+  DCHECK(process()->HasConnection());
   DCHECK(process()->profile());
 
   if (BindingsPolicy::is_dom_ui_enabled(enabled_bindings_)) {
@@ -197,7 +197,7 @@ bool RenderViewHost::CreateRenderView() {
 }
 
 bool RenderViewHost::IsRenderViewLive() const {
-  return process()->channel() && renderer_initialized_;
+  return process()->HasConnection() && renderer_initialized_;
 }
 
 void RenderViewHost::SetRendererPrefs(
diff --git a/chrome/browser/renderer_host/render_widget_host.cc b/chrome/browser/renderer_host/render_widget_host.cc
index da18bd8..5dec932 100644
--- a/chrome/browser/renderer_host/render_widget_host.cc
+++ b/chrome/browser/renderer_host/render_widget_host.cc
@@ -87,7 +87,7 @@ gfx::NativeViewId RenderWidgetHost::GetNativeViewId() {
 }
 
 void RenderWidgetHost::Init() {
-  DCHECK(process_->channel());
+  DCHECK(process_->HasConnection());
 
   renderer_initialized_ = true;
 
@@ -97,7 +97,7 @@ void RenderWidgetHost::Init() {
 }
 
 void RenderWidgetHost::Shutdown() {
-  if (process_->channel()) {
+  if (process_->HasConnection()) {
     // Tell the renderer object to close.
     process_->ReportExpectingClose(routing_id_);
     bool rv = Send(new ViewMsg_Close(routing_id_));
@@ -180,7 +180,7 @@ void RenderWidgetHost::WasRestored() {
 }
 
 void RenderWidgetHost::WasResized() {
-  if (resize_ack_pending_ || !process_->channel() || !view_ ||
+  if (resize_ack_pending_ || !process_->HasConnection() || !view_ ||
       !renderer_initialized_) {
     return;
   }
@@ -351,7 +351,7 @@ void RenderWidgetHost::ForwardKeyboardEvent(
   // will mess up our key queue.
   if (WebInputEvent::isKeyboardEventType(key_event.type)) {
     // Don't add this key to the queue if we have no way to send the message...
-    if (!process_->channel())
+    if (!process_->HasConnection())
       return;
 
     // Put all WebKeyboardEvent objects in a queue since we can't trust the
@@ -367,7 +367,7 @@ void RenderWidgetHost::ForwardKeyboardEvent(
 
 void RenderWidgetHost::ForwardInputEvent(const WebInputEvent& input_event,
                                          int event_size) {
-  if (!process_->channel())
+  if (!process_->HasConnection())
     return;
 
   IPC::Message* message = new ViewMsg_HandleInputEvent(routing_id_);
diff --git a/chrome/browser/renderer_host/render_widget_host_view_mac.mm b/chrome/browser/renderer_host/render_widget_host_view_mac.mm
index a9d45c8..c13144f 100644
--- a/chrome/browser/renderer_host/render_widget_host_view_mac.mm
+++ b/chrome/browser/renderer_host/render_widget_host_view_mac.mm
@@ -418,7 +418,7 @@ void RenderWidgetHostViewMac::ShutdownHost() {
 }
 
 - (void)drawRect:(NSRect)dirtyRect {
-  DCHECK(renderWidgetHostView_->render_widget_host_->process()->channel());
+  DCHECK(renderWidgetHostView_->render_widget_host_->process()->HasConnection());
   DCHECK(!renderWidgetHostView_->about_to_validate_and_paint_);
 
   renderWidgetHostView_->invalid_rect_ = dirtyRect;
diff --git a/chrome/browser/renderer_host/render_widget_host_view_win.cc b/chrome/browser/renderer_host/render_widget_host_view_win.cc
index e5091d6..f819164 100644
--- a/chrome/browser/renderer_host/render_widget_host_view_win.cc
+++ b/chrome/browser/renderer_host/render_widget_host_view_win.cc
@@ -689,7 +689,7 @@ void RenderWidgetHostViewWin::OnDestroy() {
 }
 
 void RenderWidgetHostViewWin::OnPaint(HDC dc) {
-  DCHECK(render_widget_host_->process()->channel());
+  DCHECK(render_widget_host_->process()->HasConnection());
 
   about_to_validate_and_paint_ = true;
   BackingStore* backing_store = render_widget_host_->GetBackingStore(true);
diff --git a/chrome/browser/tab_contents/render_view_host_delegate_helper.cc b/chrome/browser/tab_contents/render_view_host_delegate_helper.cc
index b8c4c0e..9a7a7c00 100644
--- a/chrome/browser/tab_contents/render_view_host_delegate_helper.cc
+++ b/chrome/browser/tab_contents/render_view_host_delegate_helper.cc
@@ -62,7 +62,7 @@ TabContents* RenderViewHostDelegateViewHelper::GetCreatedWindow(int route_id) {
   pending_contents_.erase(route_id);
 
   if (!new_tab_contents->render_view_host()->view() ||
-      !new_tab_contents->process()->channel()) {
+      !new_tab_contents->process()->HasConnection()) {
     // The view has gone away or the renderer crashed. Nothing to do.
     return NULL;
   }
@@ -85,7 +85,7 @@ RenderWidgetHostView* RenderViewHostDelegateViewHelper::GetCreatedWidget(
   pending_widget_views_.erase(route_id);
 
   RenderWidgetHost* widget_host = widget_host_view->GetRenderWidgetHost();
-  if (!widget_host->process()->channel()) {
+  if (!widget_host->process()->HasConnection()) {
     // The view has gone away or the renderer crashed. Nothing to do.
     return NULL;
   }