AppWindowContents: Clean up unnecessary SuspendRenderViewHost.

apps/app_window_contents.cc

Index: apps/app_window_contents.cc
diff --git a/apps/app_window_contents.cc b/apps/app_window_contents.cc
index 5004c6fd5fd80291f3e75792153572791c7401ce..dc732509927c09a87889c8872941161a29acd5e6 100644
--- a/apps/app_window_contents.cc
+++ b/apps/app_window_contents.cc
@@ -8,12 +8,7 @@
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/common/extensions/api/app_window.h"
 #include "content/public/browser/browser_context.h"
-#include "content/public/browser/browser_thread.h"
-#include "content/public/browser/notification_details.h"
-#include "content/public/browser/notification_source.h"
-#include "content/public/browser/render_process_host.h"
 #include "content/public/browser/render_view_host.h"
-#include "content/public/browser/resource_dispatcher_host.h"
 #include "content/public/browser/site_instance.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/common/renderer_preferences.h"
@@ -44,34 +39,10 @@ void AppWindowContentsImpl::Initialize(content::BrowserContext* context,
   web_contents_->GetRenderViewHost()->SyncRendererPrefs();
 }
 
-void AppWindowContentsImpl::LoadContents(int32 creator_process_id) {
-  // If the new view is in the same process as the creator, block the created
-  // RVH from loading anything until the background page has had a chance to

[not at google - send to devlin, 2014/07/09 13:48:50]

RVH is the browser side of a RV. Examples of RVs include normal web pages,
iframes, app windows, popups, background pages, whatever.

It doesn't seem possible that the opener can be in a different process than the
creator, since the only thing that can create app windows must be in an
extension process (since it's a privileged function call). And all views for an
extension must live in the same process.

I may be misunderstanding, or wrong, so maybe this should be a [D]CHECK_EQ,
though given it doesn't matter anyway, what you've done seems fine.

[not at google - send to devlin, 2014/07/09 13:51:30]

Actually one thing that does invalidate my claim will be service workers, which
will be able to use app APIs but I'm not sure are guaranteed to be in the same
process. Jeffrey/Vincent do you know?

[scheib, 2014/07/09 17:31:55]

I'm not certain that the Service worker will always be in the same render
processes as an extension. I'm also not clear now on what the implication here
is if it isn't in the same process.

[Matt Giuca, 2014/07/10 03:37:06]

Well it seems that I'm not changing the behaviour when they aren't in the same
process, only when they are (which is true in the normal app case).

That's true for this change; not so sure about the removal of the
NOTIFICATION_RENDER_VIEW_HOST_CHANGED handler.

-  // do any initialization it wants. If it's a different process, the new RVH
-  // shouldn't communicate with the background page anyway (e.g. sandboxed).
-  if (web_contents_->GetRenderViewHost()->GetProcess()->GetID() ==
-      creator_process_id) {
-    SuspendRenderViewHost(web_contents_->GetRenderViewHost());
-  } else {
-    VLOG(1) << "AppWindow created in new process ("
-            << web_contents_->GetRenderViewHost()->GetProcess()->GetID()
-            << ") != creator (" << creator_process_id << "). Routing disabled.";
-  }
-
-  // TODO(jeremya): there's a bug where navigating a web contents to an
-  // extension URL causes it to create a new RVH and discard the old (perfectly
-  // usable) one. To work around this, we watch for a
-  // NOTIFICATION_RENDER_VIEW_HOST_CHANGED message from the web contents (which
-  // will be sent during LoadURL) and suspend resource requests on the new RVH
-  // to ensure that we block the new RVH from loading anything. It should be
-  // okay to remove the NOTIFICATION_RENDER_VIEW_HOST_CHANGED registration once
-  // http://crbug.com/123007 is fixed.
-  registrar_.Add(this, content::NOTIFICATION_RENDER_VIEW_HOST_CHANGED,
-                 content::Source<content::WebContents>(web_contents()));
+void AppWindowContentsImpl::LoadContents() {
   web_contents_->GetController().LoadURL(
       url_, content::Referrer(), content::PAGE_TRANSITION_LINK,
       std::string());
-  registrar_.RemoveAll();
 }
 
 void AppWindowContentsImpl::NativeWindowChanged(
@@ -110,27 +81,6 @@ content::WebContents* AppWindowContentsImpl::GetWebContents() const {
   return web_contents_.get();
 }
 
-void AppWindowContentsImpl::Observe(
-    int type,
-    const content::NotificationSource& source,
-    const content::NotificationDetails& details) {
-  switch (type) {
-    case content::NOTIFICATION_RENDER_VIEW_HOST_CHANGED: {
-      // TODO(jeremya): once http://crbug.com/123007 is fixed, we'll no longer
-      // need to suspend resource requests here (the call in the constructor
-      // should be enough).
-      content::Details<std::pair<content::RenderViewHost*,
-                                 content::RenderViewHost*> >
-          host_details(details);
-      if (host_details->first)
-        SuspendRenderViewHost(host_details->second);
-      break;
-    }
-    default:
-      NOTREACHED() << "Received unexpected notification";
-  }
-}
-
 bool AppWindowContentsImpl::OnMessageReceived(const IPC::Message& message) {
   bool handled = true;
   IPC_BEGIN_MESSAGE_MAP(AppWindowContentsImpl, message)
@@ -162,14 +112,4 @@ void AppWindowContentsImpl::UpdateDraggableRegions(
   host_->UpdateDraggableRegions(regions);
 }
 
-void AppWindowContentsImpl::SuspendRenderViewHost(
-    content::RenderViewHost* rvh) {
-  DCHECK(rvh);
-  content::BrowserThread::PostTask(
-      content::BrowserThread::IO, FROM_HERE,
-      base::Bind(&content::ResourceDispatcherHost::BlockRequestsForRoute,
-                 base::Unretained(content::ResourceDispatcherHost::Get()),
-                 rvh->GetProcess()->GetID(), rvh->GetRoutingID()));
-}
-
 }  // namespace apps