Pull shell window stuff out of ExtensionHost and put in ShellWindow

chrome/browser/ui/extensions/shell_window.cc

Index: chrome/browser/ui/extensions/shell_window.cc
diff --git a/chrome/browser/ui/extensions/shell_window.cc b/chrome/browser/ui/extensions/shell_window.cc
index f624f95f2636ad32e41dbe970f8f3d201304b42c..941cbcdfd7db5dbd877d28321439eb6e7d40da28 100644
--- a/chrome/browser/ui/extensions/shell_window.cc
+++ b/chrome/browser/ui/extensions/shell_window.cc
@@ -11,11 +11,19 @@
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/sessions/session_id.h"
 #include "chrome/common/chrome_notification_types.h"
+#include "chrome/common/chrome_view_type.h"
 #include "chrome/common/extensions/extension.h"
+#include "chrome/common/extensions/extension_messages.h"
 #include "content/public/browser/notification_details.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/notification_source.h"
 #include "content/public/browser/notification_types.h"
+#include "content/public/browser/render_view_host.h"
+#include "content/public/browser/site_instance.h"
+#include "content/public/browser/web_contents.h"
+#include "content/public/common/renderer_preferences.h"
+
+using content::WebContents;
 
 namespace internal {
 
@@ -83,24 +91,38 @@ ShellWindow* ShellWindow::Create(Profile* profile,
   DCHECK(manager);
 
   // This object will delete itself when the window is closed.
-  return ShellWindow::CreateShellWindow(
-      manager->CreateShellHost(extension, url));
+  return ShellWindow::CreateShellWindow(profile, extension, url);
+}
+
+WebContents* ShellWindow::GetAssociatedWebContents() const {
+  return NULL;
+}
+
+bool ShellWindow::OnMessageReceived(const IPC::Message& message) {
+  bool handled = true;
+  IPC_BEGIN_MESSAGE_MAP(ShellWindow, message)
+    IPC_MESSAGE_HANDLER(ExtensionHostMsg_Request, OnRequest)
+    IPC_MESSAGE_UNHANDLED(handled = false)
+  IPC_END_MESSAGE_MAP()
+  return handled;
+}
+
+void ShellWindow::CloseContents(WebContents* contents) {
+  Close();
+}
+
+bool ShellWindow::ShouldSuppressDialogs() {
+  return true;
 }
 
 void ShellWindow::Observe(int type,
                           const content::NotificationSource& source,
                           const content::NotificationDetails& details) {
   switch (type) {
-    case chrome::NOTIFICATION_EXTENSION_HOST_VIEW_SHOULD_CLOSE:
-      if (content::Details<ExtensionHost>(host_.get()) == details)
-        Close();
-      break;
     case chrome::NOTIFICATION_EXTENSION_UNLOADED: {
       const Extension* unloaded_extension =
           content::Details<UnloadedExtensionInfo>(details)->extension;
-      // We compare extension IDs and not Extension pointers since ExtensionHost
-      // nulls out its Extension pointer when it gets this notification.
-      if (host_->extension_id() == unloaded_extension->id())
+      if (extension_ == unloaded_extension)
         Close();
       break;
     }
@@ -112,15 +134,28 @@ void ShellWindow::Observe(int type,
   }
 }
 
-ShellWindow::ShellWindow(ExtensionHost* host)
-    : host_(host) {
-  // Close the window in response to window.close() and the like.
-  registrar_.Add(this, chrome::NOTIFICATION_EXTENSION_HOST_VIEW_SHOULD_CLOSE,
-                 content::Source<Profile>(host->profile()));
-  // Also close if the window if the extension has been unloaded (parallels
-  // NOTIFICATION_EXTENSION_UNLOADED closing the app's tabs in TabStripModel).
+ShellWindow::ShellWindow(Profile* profile,
+                         const Extension* extension,
+                         const GURL& url)
+    : profile_(profile),
+      extension_(extension),
+      ALLOW_THIS_IN_INITIALIZER_LIST(
+          extension_function_dispatcher_(profile, this)),
+      site_instance_(content::SiteInstance::Create(profile)) {
+  web_contents_.reset(WebContents::Create(
+      profile, site_instance_.get(), MSG_ROUTING_NONE, NULL, NULL));
+  content::WebContentsObserver::Observe(web_contents_.get());
+  web_contents_->SetDelegate(this);
+  web_contents_->SetViewType(chrome::VIEW_TYPE_APP_SHELL);
+  web_contents_->GetMutableRendererPrefs()->browser_handles_all_requests =
+      true;
+  web_contents_->GetRenderViewHost()->SyncRendererPrefs();

[Matt Perry, 2012/04/25 19:31:58]

This line is a good example of why I'm reluctant to pull apart ExtensionHost
into multiple things. This one line represents several hours of debugging to
figure out why selected text in file dialogs on chrome OS were white on white.
How many other lines are like that? Pulling apart ExtensionHost has 2 main
disadvantages:
1. It's not immediately obvious what bits are needed when breaking apart the
class. What if we had omitted this line when we first created ShellWindow? Would
we have noticed right away? If not, there goes another couple hours when
something subtle breaks.
2. If we end up with multiple copies of code that's basically "configure this
web contents to be sane", those copies are going to diverge as we change exactly
how best to configure it, creating more work.

I agree with the complaint that ExtensionHost is the wrong abstraction - these
"configure this web contents to be sane" bits are not really specific to
extensions. But I think splitting it apart into multiple duplicate copies is a
step in the wrong direction. We need some shared object or piece of code that is
extension agnostic.

[Matt Perry, 2012/04/25 22:35:00]

BTW I'm just voicing my grievances :). I realize Aaron suggested you go this
route, so I'll defer to him on this review.

[benwells, 2012/04/25 23:25:25]

For context, this patch is why I want to split out ShellWindow:
http://codereview.chromium.org/10134026/. As platform apps get further refined I
think the hacks to keep them in ExtensionHost will get nastier.

I agree with the philosophy of "don't refactor unless there is a specific need,
as you can lose accumulated wisdom / bug fixes", but I think there is a specific
need here for platform app windows to be different from the other windows /
views covered by ExtensionHost. (sorry, I don't mean to start a philosophical
discussion :) ).

I don't have an opinion on whether ExtensionHost should be further split up, I
don't know enough about the other types of extensions.

[Aaron Boodman, 2012/04/27 16:20:31]

I hear you. We have many options when these things come up:
- If they are configuration, push them down into WebContents and give them
default values
- Implement isolated helpers (like EFD) for specific groups of behaviors
- Make the APIs WebContents exposes harder to mess up

In this case, it seems like the latter applies. GetMutableRendererPrefs() is a
trap. It doesn't actually update the renderer's prefs. It should be modified to
make it less easy to screw up, like:

WebContents::ScopedRenderPrefUpdate update;
update.render_prefs().browser_handles_all_requests = true;
// sync happens when update goes out of scope.

+
+  web_contents_->GetController().LoadURL(
+      url, content::Referrer(), content::PAGE_TRANSITION_LINK,
+      std::string());
   registrar_.Add(this, chrome::NOTIFICATION_EXTENSION_UNLOADED,
-                 content::Source<Profile>(host->profile()));
+                 content::Source<Profile>(profile_));
   // Close when the browser is exiting.
   // TODO(mihaip): we probably don't want this in the long run (when platform
   // apps are no longer tied to the browser process).
@@ -132,7 +167,7 @@ ShellWindow::ShellWindow(ExtensionHost* host)
 
   // Make this window available to the extension API.
   extension_window_controller_.reset(
-      new internal::ShellWindowController(this, host->profile()));
+      new internal::ShellWindowController(this, profile_));
 }
 
 ShellWindow::~ShellWindow() {
@@ -143,3 +178,12 @@ ShellWindow::~ShellWindow() {
   // Remove shutdown prevention.
   BrowserList::EndKeepAlive();
 }
+
+Browser* ShellWindow::GetBrowser() {
+  return NULL;
+}
+
+void ShellWindow::OnRequest(const ExtensionHostMsg_Request_Params& params) {
+  extension_function_dispatcher_.Dispatch(params,
+                                          web_contents_->GetRenderViewHost());
+}