Enable devtools via context menu in platform apps and popup extensions.

chrome/browser/tab_contents/render_view_context_menu.cc

Index: chrome/browser/tab_contents/render_view_context_menu.cc
diff --git a/chrome/browser/tab_contents/render_view_context_menu.cc b/chrome/browser/tab_contents/render_view_context_menu.cc
index 3e41d8855baa1137b923cba5d79d8c9d650dd07c..40821e0fd75f6ad385e71b1b4c85ef0edd5eccf0 100644
--- a/chrome/browser/tab_contents/render_view_context_menu.cc
+++ b/chrome/browser/tab_contents/render_view_context_menu.cc
@@ -525,8 +525,12 @@ void RenderViewContextMenu::AppendAllExtensionItems() {
 }
 
 void RenderViewContextMenu::InitMenu() {
-  if (GetPlatformApp()) {
-    AppendPlatformAppItems();
+  const Extension* extension = GetExtension();
+  if (extension) {
+    if (extension->is_platform_app())
+      AppendPlatformAppItems(extension);
+    else
+      AppendPopupExtensionItems();
     return;
   }
 
@@ -630,7 +634,7 @@ void RenderViewContextMenu::InitMenu() {
   observers_.AddObserver(print_preview_menu_observer_.get());
 }
 
-const Extension* RenderViewContextMenu::GetPlatformApp() const {
+const Extension* RenderViewContextMenu::GetExtension() const {
   ExtensionProcessManager* process_manager =
       profile_->GetExtensionProcessManager();
   // There is no process manager in some tests.
@@ -643,20 +647,53 @@ const Extension* RenderViewContextMenu::GetPlatformApp() const {
        ++iter) {
     ExtensionHost* host = *iter;
     if (host->host_contents() == source_web_contents_) {
-      if (host->extension() && host->extension()->is_platform_app()) {
-        return host->extension();
-      }
+      const Extension* extension = host->extension();

[msw, 2012/02/03 07:57:24]

Why not just return host->extension(); here? The NULL check appears pointless.

[benwells, 2012/02/06 03:40:46]

Done.

+      if (extension)
+        return extension;
+      return NULL;
     }
   }
 
   return NULL;
 }
 
-void RenderViewContextMenu::AppendPlatformAppItems() {
-  const Extension* platform_app = GetPlatformApp();
+void RenderViewContextMenu::AppendPlatformAppItems(
+    const Extension* platform_app) {
   DCHECK(platform_app);
   int index = 0;
   AppendExtensionItems(platform_app->id(), &index);
+
+  // Add dev tools for unpacked extensions.
+  if (platform_app->location() == Extension::LOAD)
+    AppendDeveloperItems();
+}
+
+void RenderViewContextMenu::AppendPopupExtensionItems() {
+  bool has_selection = !params_.selection_text.empty();
+
+  if (params_.is_editable) {
+    // Add a menu item that shows suggestions.
+    if (!spelling_menu_observer_.get()) {

[msw, 2012/02/03 07:57:24]

nit: Drop the braces for a single-line if block.

[benwells, 2012/02/06 03:40:46]

Needs braces after change for next comment.

+      spelling_menu_observer_.reset(new SpellingMenuObserver(this));
+    }
+    if (spelling_menu_observer_.get()) {

[msw, 2012/02/03 07:57:24]

Is this check necessary? The above conditional ensures this is non-NULL.
If anything, maybe use a DCHECK instead?

[benwells, 2012/02/06 03:40:46]

I put in an else.

[benwells, 2012/02/06 04:31:27]

Arg, just reread the original code. Will fix as suggested.

+      observers_.AddObserver(spelling_menu_observer_.get());
+      spelling_menu_observer_->InitMenu(params_);
+    }
+  }
+
+  if (params_.is_editable)

[msw, 2012/02/03 07:57:24]

There's an if (params_.is_editable) block just above, please combine them.

[benwells, 2012/02/06 03:40:46]

Done.

+    AppendEditableItems();
+  else if (has_selection)
+    AppendCopyItem();
+
+  if (has_selection)
+    AppendSearchProvider();
+
+  if (!IsDevToolsURL(params_.page_url))
+    AppendAllExtensionItems();
+
+  AppendDeveloperItems();
 }
 
 void RenderViewContextMenu::LookUpInDictionary() {
@@ -1872,17 +1909,33 @@ void RenderViewContextMenu::MenuClosed(ui::SimpleMenuModel* source) {
 
 bool RenderViewContextMenu::IsDevCommandEnabled(int id) const {
   if (id == IDC_CONTENT_CONTEXT_INSPECTELEMENT) {
-    const CommandLine& command_line = *CommandLine::ForCurrentProcess();
-    TabContentsWrapper* tab_contents_wrapper =
-        TabContentsWrapper::GetCurrentWrapperForContents(
-            source_web_contents_);
-    if (!tab_contents_wrapper)
-      return false;
-    // Don't enable the web inspector if JavaScript is disabled.
-    if (!tab_contents_wrapper->prefs_tab_helper()->per_tab_prefs()->GetBoolean(
-            prefs::kWebKitJavascriptEnabled) ||
-        command_line.HasSwitch(switches::kDisableJavaScript))
-      return false;
+    // Don't enable the web inspector if JavaScript is disabled. We don't

[msw, 2012/02/03 07:57:24]

Why skip these checks for extensions? I prefer code simplicity over 'perf' here.
Then, in the non-views block below, just return false if (GetExtension()).

[benwells, 2012/02/06 03:40:46]

The code below doesn't work for popup extensions or platform apps as the
contents_wrapper returned by GetCurrentWrapperForContents is NULL.

In the non-Views case the desired behaviour is for platform apps to have the
command enabled but extension popups not to. I've made this a bit clearer by
using !is_platform_app instead of !is_app. These end up being the same as
GetExtension only returns an extension for platform apps and  popup extensions
but that isn't obvious.

+    // check this when this is the web contents of an extension (e.g.
+    // for a popup extension or a platform app) as they have JavaScript
+    // always enabled.
+    const Extension* extension = GetExtension();
+    if (!extension) {
+      TabContentsWrapper* contents_wrapper =
+          TabContentsWrapper::GetCurrentWrapperForContents(
+              source_web_contents_);
+      if (!contents_wrapper)
+        return false;
+      const CommandLine* command_line = CommandLine::ForCurrentProcess();
+      if (!contents_wrapper->prefs_tab_helper()->per_tab_prefs()->GetBoolean(
+              prefs::kWebKitJavascriptEnabled) ||
+          command_line->HasSwitch(switches::kDisableJavaScript))
+        return false;
+#if defined(TOOLKIT_GTK) || defined(OS_MAC)

[msw, 2012/02/03 07:57:24]

Instead, check #if !defined(TOOLKIT_VIEWS)

[benwells, 2012/02/06 03:40:46]

Done.

+    } else {
+      // Disable dev tools for popup extensions for non-views builds, as the
+      // extension popups for these builds do not support dynamically inspecting
+      // the popups.
+      // TODO(benwells): Add support for these builds and remote this #if.

[msw, 2012/02/03 07:57:24]

s/remote/remove

[benwells, 2012/02/06 03:40:46]

Done.

+      if (!extension->is_app())
+        return false;
+#endif
+    }
+
     // Don't enable the web inspector if the developer tools are disabled via
     // the preference dev-tools-disabled.
     if (profile_->GetPrefs()->GetBoolean(prefs::kDevToolsDisabled))