Clean up Extension Action Handlers Pt 1/2

Combine ManifestHandlers for PageActions and BrowserActions.
Remove support for the uber-deprecated "page_actions" (plural) manifest key.
Clean up related tests.

TBR=avi@chromium.org (c/b/ui/cocoa)
BUG=419475

Review URL: https://codereview.chromium.org/624453003

Cr-Commit-Position: refs/heads/master@{#297927}

chrome/browser/ui/cocoa/extensions/extension_installed_bubble_controller_unittest.mm

@@ -94,9 +94,7 @@ class ExtensionInstalledBubbleControllerTest : public CocoaProfileTest {
       action->SetString(keys::kPageActionId, "ExtensionActionId");
       action->SetString(keys::kPageActionDefaultTitle, "ExtensionActionTitle");
       action->SetString(keys::kPageActionDefaultIcon, "image1.png");
-      base::ListValue* action_list = new base::ListValue;
-      action_list->Append(action);
-      extension_input_value.Set(keys::kPageActions, action_list);
+      extension_input_value.Set(keys::kPageAction, action);
     } else if (type == extension_installed_bubble::kBrowserAction) {
       extension_input_value.SetString(keys::kName, "browser action extension");
       base::DictionaryValue* browser_action = new base::DictionaryValue;

chrome/chrome_common.gypi

@@ -154,10 +154,6 @@
       'common/extensions/api/commands/commands_handler.h',
       'common/extensions/api/extension_action/action_info.cc',
       'common/extensions/api/extension_action/action_info.h',
-      'common/extensions/api/extension_action/browser_action_handler.cc',
-      'common/extensions/api/extension_action/browser_action_handler.h',
-      'common/extensions/api/extension_action/page_action_handler.cc',
-      'common/extensions/api/extension_action/page_action_handler.h',
       'common/extensions/api/file_browser_handlers/file_browser_handler.cc',
       'common/extensions/api/file_browser_handlers/file_browser_handler.h',
       'common/extensions/api/i18n/default_locale_handler.cc',
@@ -198,6 +194,8 @@
       'common/extensions/manifest_handlers/automation.h',
       'common/extensions/manifest_handlers/content_scripts_handler.cc',
       'common/extensions/manifest_handlers/content_scripts_handler.h',
+      'common/extensions/manifest_handlers/extension_action_handler.cc',
+      'common/extensions/manifest_handlers/extension_action_handler.h',
       'common/extensions/manifest_handlers/mime_types_handler.cc',
       'common/extensions/manifest_handlers/mime_types_handler.h',
       'common/extensions/manifest_handlers/minimum_chrome_version_checker.cc',
@@ -448,9 +446,8 @@
           'sources!': [
             'common/badge_util.cc',
             'common/chrome_version_info_posix.cc',
-            'common/extensions/api/extension_action/browser_action_handler.cc',
-            'common/extensions/api/extension_action/page_action_handler.cc',
             'common/extensions/api/spellcheck/spellcheck_handler.cc',
+            'common/extensions/manifest_handlers/extension_action_handler.cc',
             'common/extensions/manifest_handlers/minimum_chrome_version_checker.cc',
             'common/icon_with_badge_image_source.cc',
             'common/media_galleries/metadata_types.h',

chrome/common/extensions/api/_api_features.json

@@ -580,10 +580,6 @@
     "dependencies": ["manifest:page_action"],
     "contexts": ["blessed_extension"]
   },
-  "pageActions": {
-    "dependencies": ["manifest:page_actions"],
-    "contexts": ["blessed_extension"]
-  },
   "pageCapture": {
     "dependencies": ["permission:pageCapture"],
     "contexts": ["blessed_extension"]

chrome/common/extensions/api/_manifest_features.json

@@ -273,11 +273,6 @@
     "channel": "stable",
     "extension_types": ["extension"]
   },
-  "page_actions": {
-    "channel": "stable",
-    "extension_types": ["extension"],
-    "max_manifest_version": 1
-  },
   "permissions": {
     "channel": "stable",
     "extension_types": [

chrome/common/extensions/api/extension_action/browser_action_handler.h

-// Copyright (c) 2012 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.
-
-#ifndef CHROME_COMMON_EXTENSIONS_API_EXTENSION_ACTION_BROWSER_ACTION_HANDLER_H_
-#define CHROME_COMMON_EXTENSIONS_API_EXTENSION_ACTION_BROWSER_ACTION_HANDLER_H_
-
-#include <string>
-
-#include "chrome/common/extensions/api/extension_action/action_info.h"
-#include "extensions/common/extension.h"
-#include "extensions/common/manifest_handler.h"
-
-namespace extensions {
-
-// Parses the "browser_action" manifest key.
-class BrowserActionHandler : public ManifestHandler {
- public:
-  BrowserActionHandler();
-  virtual ~BrowserActionHandler();
-
-  virtual bool Parse(Extension* extension, base::string16* error) OVERRIDE;
-  virtual bool Validate(const Extension* extension,
-                        std::string* error,
-                        std::vector<InstallWarning>* warnings) const OVERRIDE;
-
- private:
-  virtual const std::vector<std::string> Keys() const OVERRIDE;
-
-  DISALLOW_COPY_AND_ASSIGN(BrowserActionHandler);
-};
-
-}  // namespace extensions
-
-#endif  // CHROME_COMMON_EXTENSIONS_API_EXTENSION_ACTION_BROWSER_ACTION_HANDLER_H_

chrome/common/extensions/api/extension_action/page_action_handler.cc

-// Copyright (c) 2013 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 "chrome/common/extensions/api/extension_action/page_action_handler.h"
-
-#include "base/strings/utf_string_conversions.h"
-#include "base/values.h"
-#include "chrome/common/extensions/extension_constants.h"
-#include "chrome/grit/generated_resources.h"
-#include "extensions/common/extension.h"
-#include "extensions/common/file_util.h"
-#include "extensions/common/manifest_constants.h"
-
-namespace extensions {
-
-namespace keys = manifest_keys;
-namespace errors = manifest_errors;
-
-PageActionHandler::PageActionHandler() {
-}
-
-PageActionHandler::~PageActionHandler() {
-}
-
-bool PageActionHandler::Parse(Extension* extension, base::string16* error) {
-  scoped_ptr<ActionInfo> page_action_info;
-  const base::DictionaryValue* page_action_value = NULL;
-
-  if (extension->manifest()->HasKey(keys::kPageActions)) {
-    const base::ListValue* list_value = NULL;
-    if (!extension->manifest()->GetList(keys::kPageActions, &list_value)) {
-      *error = base::ASCIIToUTF16(errors::kInvalidPageActionsList);
-      return false;
-    }
-
-    size_t list_value_length = list_value->GetSize();
-
-    if (list_value_length == 0u) {
-      // A list with zero items is allowed, and is equivalent to not having
-      // a page_actions key in the manifest.  Don't set |page_action_value|.
-    } else if (list_value_length == 1u) {
-      if (!list_value->GetDictionary(0, &page_action_value)) {
-        *error = base::ASCIIToUTF16(errors::kInvalidPageAction);
-        return false;
-      }
-    } else {  // list_value_length > 1u.
-      *error = base::ASCIIToUTF16(errors::kInvalidPageActionsListSize);
-      return false;
-    }
-  } else if (extension->manifest()->HasKey(keys::kPageAction)) {
-    if (!extension->manifest()->GetDictionary(keys::kPageAction,
-                                              &page_action_value)) {
-      *error = base::ASCIIToUTF16(errors::kInvalidPageAction);
-      return false;
-    }
-  }
-
-  // An extension cannot have both browser and page actions.
-  if (extension->manifest()->HasKey(keys::kBrowserAction)) {
-    *error = base::ASCIIToUTF16(errors::kOneUISurfaceOnly);
-    return false;
-  }
-
-  // If page_action_value is not NULL, then there was a valid page action.
-  if (page_action_value) {
-    page_action_info = ActionInfo::Load(extension, page_action_value, error);
-    if (!page_action_info)
-      return false;  // Failed to parse page action definition.
-  }
-  ActionInfo::SetPageActionInfo(extension, page_action_info.release());
-
-  return true;
-}
-
-bool PageActionHandler::Validate(const Extension* extension,
-                                 std::string* error,
-                                 std::vector<InstallWarning>* warnings) const {
-  const extensions::ActionInfo* action =
-      extensions::ActionInfo::GetPageActionInfo(extension);
-  if (action && !action->default_icon.empty() &&
-      !file_util::ValidateExtensionIconSet(
-          action->default_icon,
-          extension,
-          IDS_EXTENSION_LOAD_ICON_FOR_PAGE_ACTION_FAILED,
-          error)) {
-    return false;
-  }
-  return true;
-}
-
-const std::vector<std::string> PageActionHandler::Keys() const {
-  std::vector<std::string> keys;
-  keys.push_back(keys::kPageAction);
-  keys.push_back(keys::kPageActions);
-  return keys;
-}
-
-}  // namespace extensions

chrome/common/extensions/chrome_manifest_handlers.cc

@@ -5,8 +5,6 @@
 #include "chrome/common/extensions/chrome_manifest_handlers.h"
 
 #include "chrome/common/extensions/api/commands/commands_handler.h"
-#include "chrome/common/extensions/api/extension_action/browser_action_handler.h"
-#include "chrome/common/extensions/api/extension_action/page_action_handler.h"
 #include "chrome/common/extensions/api/file_browser_handlers/file_browser_handler.h"
 #include "chrome/common/extensions/api/i18n/default_locale_handler.h"
 #include "chrome/common/extensions/api/identity/oauth2_manifest_handler.h"
@@ -25,6 +23,7 @@
 #include "chrome/common/extensions/manifest_handlers/app_launch_info.h"
 #include "chrome/common/extensions/manifest_handlers/automation.h"
 #include "chrome/common/extensions/manifest_handlers/content_scripts_handler.h"
+#include "chrome/common/extensions/manifest_handlers/extension_action_handler.h"
 #include "chrome/common/extensions/manifest_handlers/mime_types_handler.h"
 #include "chrome/common/extensions/manifest_handlers/minimum_chrome_version_checker.h"
 #include "chrome/common/extensions/manifest_handlers/settings_overrides_handler.h"
@@ -46,11 +45,11 @@ void RegisterChromeManifestHandlers() {
   (new AppLaunchManifestHandler)->Register();
   (new AutomationHandler)->Register();
   (new BluetoothManifestHandler)->Register();
-  (new BrowserActionHandler)->Register();
   (new CommandsHandler)->Register();
   (new ContentScriptsHandler)->Register();
   (new DefaultLocaleHandler)->Register();
   (new DevToolsPageHandler)->Register();
+  (new ExtensionActionHandler)->Register();
   (new FileBrowserHandlerParser)->Register();
   (new HomepageURLHandler)->Register();
 #if defined(OS_CHROMEOS)
@@ -61,7 +60,6 @@ void RegisterChromeManifestHandlers() {
   (new OAuth2ManifestHandler)->Register();
   (new OmniboxHandler)->Register();
   (new OptionsPageManifestHandler)->Register();
-  (new PageActionHandler)->Register();
   (new PluginsHandler)->Register();
   (new RequirementsHandler)->Register();  // Depends on plugins.
   (new SettingsOverridesHandler)->Register();

chrome/common/extensions/api/extension_action/browser_action_handler.cc → chrome/common/extensions/manifest_handlers/extension_action_handler.cc

-// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Copyright 2014 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 "chrome/common/extensions/api/extension_action/browser_action_handler.h"
+#include "chrome/common/extensions/manifest_handlers/extension_action_handler.h"
 
-#include "base/memory/scoped_ptr.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/values.h"
+#include "chrome/common/extensions/api/extension_action/action_info.h"
+#include "chrome/common/extensions/extension_constants.h"
 #include "chrome/grit/generated_resources.h"
 #include "extensions/common/extension.h"
-#include "extensions/common/feature_switch.h"
 #include "extensions/common/file_util.h"
-#include "extensions/common/manifest.h"
 #include "extensions/common/manifest_constants.h"
 
 namespace extensions {
 
-BrowserActionHandler::BrowserActionHandler() {
+ExtensionActionHandler::ExtensionActionHandler() {
 }
 
-BrowserActionHandler::~BrowserActionHandler() {
+ExtensionActionHandler::~ExtensionActionHandler() {
 }
 
-bool BrowserActionHandler::Parse(Extension* extension,
+bool ExtensionActionHandler::Parse(Extension* extension,
                                    base::string16* error) {
+  const char* key = NULL;
+  const char* error_key = NULL;
+  if (extension->manifest()->HasKey(manifest_keys::kPageAction)) {
+    key = manifest_keys::kPageAction;
+    error_key = manifest_errors::kInvalidPageAction;
+  }
+
+  if (extension->manifest()->HasKey(manifest_keys::kBrowserAction)) {
+    if (key != NULL) {
+      // An extension cannot have both browser and page actions.
+      *error = base::ASCIIToUTF16(manifest_errors::kOneUISurfaceOnly);
+      return false;
+    }
+    key = manifest_keys::kBrowserAction;
+    error_key = manifest_errors::kInvalidBrowserAction;
+  }
+  DCHECK(key);
+
   const base::DictionaryValue* dict = NULL;
-  if (!extension->manifest()->GetDictionary(
-          manifest_keys::kBrowserAction, &dict)) {
-    *error = base::ASCIIToUTF16(manifest_errors::kInvalidBrowserAction);
+  if (!extension->manifest()->GetDictionary(key, &dict)) {
+    *error = base::ASCIIToUTF16(error_key);
     return false;
   }
 
   scoped_ptr<ActionInfo> action_info = ActionInfo::Load(extension, dict, error);
-  if (!action_info.get())
+  if (!action_info)
     return false;  // Failed to parse browser action definition.
 
+  if (key == manifest_keys::kPageAction)
+    ActionInfo::SetPageActionInfo(extension, action_info.release());
+  else
     ActionInfo::SetBrowserActionInfo(extension, action_info.release());
 
   return true;
 }
 
-bool BrowserActionHandler::Validate(
+bool ExtensionActionHandler::Validate(
     const Extension* extension,
     std::string* error,
     std::vector<InstallWarning>* warnings) const {
-  const ActionInfo* action = ActionInfo::GetBrowserActionInfo(extension);
+  int error_message = 0;
+  const ActionInfo* action = ActionInfo::GetPageActionInfo(extension);
+  if (action) {
+    error_message = IDS_EXTENSION_LOAD_ICON_FOR_PAGE_ACTION_FAILED;
+  } else {
+    action = ActionInfo::GetBrowserActionInfo(extension);
+    error_message = IDS_EXTENSION_LOAD_ICON_FOR_BROWSER_ACTION_FAILED;
+  }
+
   if (action && !action->default_icon.empty() &&
       !file_util::ValidateExtensionIconSet(
-          action->default_icon,
-          extension,
-          IDS_EXTENSION_LOAD_ICON_FOR_BROWSER_ACTION_FAILED,
-          error)) {
+          action->default_icon, extension, error_message, error)) {
     return false;
   }
   return true;
 }
 
-const std::vector<std::string> BrowserActionHandler::Keys() const {
-  return SingleKey(manifest_keys::kBrowserAction);
+const std::vector<std::string> ExtensionActionHandler::Keys() const {
+  std::vector<std::string> keys;
+  keys.push_back(manifest_keys::kPageAction);
+  keys.push_back(manifest_keys::kBrowserAction);
+  return keys;
 }
 
 }  // namespace extensions

chrome/common/extensions/api/extension_action/page_action_handler.h → chrome/common/extensions/manifest_handlers/extension_action_handler.h

-// Copyright (c) 2013 The Chromium Authors. All rights reserved.
+// Copyright 2014 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.
 
-#ifndef CHROME_COMMON_EXTENSIONS_API_EXTENSION_ACTION_PAGE_ACTION_HANDLER_H_
-#define CHROME_COMMON_EXTENSIONS_API_EXTENSION_ACTION_PAGE_ACTION_HANDLER_H_
+#ifndef CHROME_COMMON_EXTENSIONS_MANIFEST_HANDLERS_EXTENSION_ACTION_HANDLER_H_
+#define CHROME_COMMON_EXTENSIONS_MANIFEST_HANDLERS_EXTENSION_ACTION_HANDLER_H_
 
 #include <string>
 
-#include "chrome/common/extensions/api/extension_action/action_info.h"
-#include "extensions/common/extension.h"
 #include "extensions/common/manifest_handler.h"
 
 namespace extensions {
 
-// Parses the "page_action" manifest key.
-class PageActionHandler : public ManifestHandler {
+// Parses the "page_action" and "browser_action" manifest keys.
+class ExtensionActionHandler : public ManifestHandler {
  public:
-  PageActionHandler();
-  virtual ~PageActionHandler();
+  ExtensionActionHandler();
+  virtual ~ExtensionActionHandler();
 
   virtual bool Parse(Extension* extension, base::string16* error) OVERRIDE;
   virtual bool Validate(const Extension* extension,
@@ -27,9 +25,9 @@ class PageActionHandler : public ManifestHandler {
  private:
   virtual const std::vector<std::string> Keys() const OVERRIDE;
 
-  DISALLOW_COPY_AND_ASSIGN(PageActionHandler);
+  DISALLOW_COPY_AND_ASSIGN(ExtensionActionHandler);
 };
 
 }  // namespace extensions
 
-#endif  // CHROME_COMMON_EXTENSIONS_API_EXTENSION_ACTION_PAGE_ACTION_HANDLER_H_
+#endif  // CHROME_COMMON_EXTENSIONS_MANIFEST_HANDLERS_EXTENSION_ACTION_HANDLER_H_

chrome/common/extensions/manifest_tests/extension_manifests_initvalue_unittest.cc

@@ -75,8 +75,6 @@ TEST_F(InitValueManifestTest, InitFromValueInvalid) {
              errors::kInvalidPermissions),
     Testcase("init_invalid_permissions_item_invalid.json",
              errors::kInvalidPermission),
-    Testcase("init_invalid_page_actions_multi.json",
-             errors::kInvalidPageActionsListSize),
     Testcase("init_invalid_options_url_invalid.json",
              errors::kInvalidOptionsPage),
     Testcase("init_invalid_locale_invalid.json", errors::kInvalidDefaultLocale),
@@ -129,10 +127,6 @@ TEST_F(InitValueManifestTest, InitFromValueValid) {
   EXPECT_EQ("concise name", extension->short_name());
 
   Testcase testcases[] = {
-    // Test that an empty list of page actions does not stop a browser action
-    // from being loaded.
-    Testcase("init_valid_empty_page_actions.json"),
-
     // Test with a minimum_chrome_version.
     Testcase("init_valid_minimum_chrome.json"),
 

chrome/test/data/extensions/manifest_tests/init_invalid_page_actions_multi.json

-{
-   "name": "my extension",
-   "version": "1.0.0.0",
-   "page_actions": [ {
-      "id": "MyExtensionActionId",
-      "name": "MyExtensionActionName"
-   }, {
-      "id": "MyExtensionActionId",
-      "name": "MyExtensionActionName"
-   } ]
-}

chrome/test/data/extensions/manifest_tests/init_valid_empty_page_actions.json

-{
-   "name": "my extension",
-   "options_page": "options.html",
-   "page_actions": [  ],
-   "version": "1.0.0.0"
-}

extensions/common/manifest_constants.cc

@@ -110,7 +110,6 @@ const char kPageActionIcons[] = "icons";
 const char kPageActionId[] = "id";
 const char kPageActionPopup[] = "popup";
 const char kPageActionPopupPath[] = "path";
-const char kPageActions[] = "page_actions";
 const char kPermissions[] = "permissions";
 const char kPlatformAppBackground[] = "app.background";
 const char kPlatformAppBackgroundPage[] = "app.background.page";
@@ -545,12 +544,6 @@ const char kInvalidPageActionPopup[] =
     "Invalid type for page action popup.";
 const char kInvalidPageActionPopupPath[] =
     "Invalid value for page action popup path [*].";
-const char kInvalidPageActionsList[] =
-    "Invalid value for 'page_actions'.";
-const char kInvalidPageActionsListSize[] =
-    "Invalid value for 'page_actions'. There can be at most one page action.";
-const char kInvalidPageActionTypeValue[] =
-    "Invalid value for 'page_actions[*].type', expected 'tab' or 'permanent'.";
 const char kInvalidPermissionWithDetail[] =
     "Invalid value for 'permissions[*]': *.";
 const char kInvalidPermission[] =

extensions/common/manifest_constants.h

@@ -118,7 +118,6 @@ extern const char kPageActionIcons[];
 extern const char kPageActionId[];
 extern const char kPageActionPopup[];
 extern const char kPageActionPopupPath[];
-extern const char kPageActions[];
 extern const char kPermissions[];
 extern const char kPlatformAppBackground[];
 extern const char kPlatformAppBackgroundPage[];
@@ -396,9 +395,6 @@ extern const char kInvalidPageActionOldAndNewKeys[];
 extern const char kInvalidPageActionPopup[];
 extern const char kInvalidPageActionPopupHeight[];
 extern const char kInvalidPageActionPopupPath[];
-extern const char kInvalidPageActionsList[];
-extern const char kInvalidPageActionsListSize[];
-extern const char kInvalidPageActionTypeValue[];
 extern const char kInvalidPermissionWithDetail[];
 extern const char kInvalidPermission[];
 extern const char kInvalidPermissions[];