Allow extensions to add, remove, or change their page action popup.

A similar change will be made for browser action popups.

BUG=27526
TEST=Added unit tests.  Manual testing on linux.

Review URL: http://codereview.chromium.org/545068

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

11 files changed, 438 insertions, 28 deletions

diff --git a/chrome/browser/extensions/extension_function_dispatcher.cc b/chrome/browser/extensions/extension_function_dispatcher.cc
index 6e719fc..c5248cf 100644
--- a/chrome/browser/extensions/extension_function_dispatcher.cc
+++ b/chrome/browser/extensions/extension_function_dispatcher.cc
@@ -113,6 +113,7 @@ void FactoryRegistry::ResetFunctions() {
   RegisterFunction<PageActionHideFunction>();
   RegisterFunction<PageActionSetIconFunction>();
   RegisterFunction<PageActionSetTitleFunction>();
+  RegisterFunction<PageActionSetPopupFunction>();
 
   // Browser Actions.
   RegisterFunction<BrowserActionSetIconFunction>();
diff --git a/chrome/browser/extensions/extension_page_actions_module.cc b/chrome/browser/extensions/extension_page_actions_module.cc
index ace9317..1379b2f 100644
--- a/chrome/browser/extensions/extension_page_actions_module.cc
+++ b/chrome/browser/extensions/extension_page_actions_module.cc
@@ -192,6 +192,29 @@ bool PageActionSetTitleFunction::RunImpl() {
   return true;
 }
 
+bool PageActionSetPopupFunction::RunImpl() {
+  EXTENSION_FUNCTION_VALIDATE(args_->IsType(Value::TYPE_DICTIONARY));
+  const DictionaryValue* args = args_as_dictionary();
+
+  int tab_id;
+  EXTENSION_FUNCTION_VALIDATE(args->GetInteger(L"tabId", &tab_id));
+  if (!InitCommon(tab_id))
+    return false;
+
+  // TODO(skerner): Consider allowing null and undefined to mean the popup
+  // should be removed.
+  std::string popup_string;
+  EXTENSION_FUNCTION_VALIDATE(args->GetString(L"popup", &popup_string));
+
+  GURL popup_url;
+  if (!popup_string.empty())
+    popup_url = GetExtension()->GetResourceURL(popup_string);
+
+  page_action_->SetPopupUrl(tab_id, popup_url);
+  contents_->PageActionStateChanged();
+  return true;
+}
+
 // Not currently exposed to extensions. To re-enable, add mapping in
 // extension_function_dispatcher.
 bool PageActionSetBadgeBackgroundColorFunction::RunImpl() {
diff --git a/chrome/browser/extensions/extension_page_actions_module.h b/chrome/browser/extensions/extension_page_actions_module.h
index 8c0e291..3be9ce1 100644
--- a/chrome/browser/extensions/extension_page_actions_module.h
+++ b/chrome/browser/extensions/extension_page_actions_module.h
@@ -10,6 +10,7 @@
 class TabContents;
 class ExtensionAction;
 
+// Base class for page action APIs.
 class PageActionFunction : public SyncExtensionFunction {
  protected:
   virtual ~PageActionFunction() {}
@@ -22,54 +23,70 @@ class PageActionFunction : public SyncExtensionFunction {
   TabContents* contents_;
 };
 
+// Implement chrome.pageActions.enableForTab().
 class EnablePageActionFunction : public PageActionFunction {
   ~EnablePageActionFunction() {}
   virtual bool RunImpl();
   DECLARE_EXTENSION_FUNCTION_NAME("pageActions.enableForTab")
 };
 
+// Implement chrome.pageActions.disableForTab().
 class DisablePageActionFunction : public PageActionFunction {
   ~DisablePageActionFunction() {}
   virtual bool RunImpl();
   DECLARE_EXTENSION_FUNCTION_NAME("pageActions.disableForTab")
 };
 
+// Implement chrome.pageActions.show().
 class PageActionShowFunction : public PageActionFunction {
   ~PageActionShowFunction() {}
   virtual bool RunImpl();
   DECLARE_EXTENSION_FUNCTION_NAME("pageAction.show")
 };
 
+// Implement chrome.pageActions.hide().
 class PageActionHideFunction : public PageActionFunction {
   ~PageActionHideFunction() {}
   virtual bool RunImpl();
   DECLARE_EXTENSION_FUNCTION_NAME("pageAction.hide")
 };
 
+// Implement chrome.pageActions.setIcon().
 class PageActionSetIconFunction : public PageActionFunction {
   ~PageActionSetIconFunction() {}
   virtual bool RunImpl();
   DECLARE_EXTENSION_FUNCTION_NAME("pageAction.setIcon")
 };
 
+// Implement chrome.pageActions.setTitle().
 class PageActionSetTitleFunction : public PageActionFunction {
   ~PageActionSetTitleFunction() {}
   virtual bool RunImpl();
   DECLARE_EXTENSION_FUNCTION_NAME("pageAction.setTitle")
 };
 
+// Implement chrome.pageActions.setPopup().
+class PageActionSetPopupFunction : public PageActionFunction {
+  ~PageActionSetPopupFunction() {}
+  virtual bool RunImpl();
+  DECLARE_EXTENSION_FUNCTION_NAME("pageAction.setPopup")
+};
+
+// Implement chrome.pageActions.setBadgeBackgroundColor().
 class PageActionSetBadgeBackgroundColorFunction : public PageActionFunction {
   ~PageActionSetBadgeBackgroundColorFunction() {}
   virtual bool RunImpl();
   DECLARE_EXTENSION_FUNCTION_NAME("pageAction.setBadgeBackgroundColor")
 };
 
+// Implement chrome.pageActions.setBadgeTextColor().
 class PageActionSetBadgeTextColorFunction : public PageActionFunction {
   ~PageActionSetBadgeTextColorFunction() {}
   virtual bool RunImpl();
   DECLARE_EXTENSION_FUNCTION_NAME("pageAction.setBadgeTextColor")
 };
 
+// Implement chrome.pageActions.setBadgeText().
 class PageActionSetBadgeTextFunction : public PageActionFunction {
   ~PageActionSetBadgeTextFunction() {}
   virtual bool RunImpl();
diff --git a/chrome/browser/extensions/page_action_apitest.cc b/chrome/browser/extensions/page_action_apitest.cc
index b545558..b5ba0f3 100644
--- a/chrome/browser/extensions/page_action_apitest.cc
+++ b/chrome/browser/extensions/page_action_apitest.cc
@@ -18,6 +18,7 @@ IN_PROC_BROWSER_TEST_F(ExtensionApiTest, PageAction) {
   StartHTTPServer();
   ASSERT_TRUE(RunExtensionTest("page_action/basics")) << message_;
 
+  // TODO(skerner): Move the next four lines into a helper method.
   ExtensionsService* service = browser()->profile()->GetExtensionsService();
   ASSERT_EQ(1u, service->extensions()->size());
   Extension* extension = service->extensions()->at(0);
@@ -61,6 +62,86 @@ IN_PROC_BROWSER_TEST_F(ExtensionApiTest, PageAction) {
   EXPECT_FALSE(action->GetIcon(tab_id).isNull());
 }
 
+// Test that calling chrome.pageAction.setPopup() can enable a popup.
+IN_PROC_BROWSER_TEST_F(ExtensionApiTest, PageActionAddPopup) {
+  // Load the extension, which has no default popup.
+  ASSERT_TRUE(RunExtensionTest("page_action/add_popup")) << message_;
+
+  ExtensionsService* service = browser()->profile()->GetExtensionsService();
+  ASSERT_EQ(1u, service->extensions()->size());
+  Extension* extension = service->extensions()->at(0);
+  ASSERT_TRUE(extension);
+
+  int tab_id = ExtensionTabUtil::GetTabId(browser()->GetSelectedTabContents());
+
+  ExtensionAction* page_action = extension->page_action();
+  ASSERT_TRUE(page_action)
+      << "Page action test extension should have a page action.";
+
+  ASSERT_FALSE(page_action->HasPopup(tab_id));
+
+  // Simulate the page action being clicked.  The resulting event should
+  // install a page action popup.
+  {
+    ResultCatcher catcher;
+    ExtensionBrowserEventRouter::GetInstance()->PageActionExecuted(
+        browser()->profile(), extension->id(), "action", tab_id, "", 1);
+    ASSERT_TRUE(catcher.GetNextResult());
+  }
+
+  ASSERT_TRUE(page_action->HasPopup(tab_id))
+      << "Clicking on the page action should have caused a popup to be added.";
+
+  ASSERT_STREQ("/a_popup.html",
+               page_action->GetPopupUrl(tab_id).path().c_str());
+
+  // Now change the popup from a_popup.html to a_second_popup.html .
+  // Load a page which removes the popup using chrome.pageAction.setPopup().
+  {
+    ResultCatcher catcher;
+    ui_test_utils::NavigateToURL(
+        browser(),
+        GURL(extension->GetResourceURL("change_popup.html")));
+    ASSERT_TRUE(catcher.GetNextResult());
+  }
+
+  ASSERT_TRUE(page_action->HasPopup(tab_id));
+  ASSERT_STREQ("/another_popup.html",
+               page_action->GetPopupUrl(tab_id).path().c_str());
+}
+
+// Test that calling chrome.pageAction.setPopup() can remove a popup.
+IN_PROC_BROWSER_TEST_F(ExtensionApiTest, PageActionRemovePopup) {
+  // Load the extension, which has a page action with a default popup.
+  ASSERT_TRUE(RunExtensionTest("page_action/remove_popup")) << message_;
+
+  ExtensionsService* service = browser()->profile()->GetExtensionsService();
+  ASSERT_EQ(1u, service->extensions()->size());
+  Extension* extension = service->extensions()->at(0);
+  ASSERT_TRUE(extension);
+
+  int tab_id = ExtensionTabUtil::GetTabId(browser()->GetSelectedTabContents());
+
+  ExtensionAction* page_action = extension->page_action();
+  ASSERT_TRUE(page_action)
+      << "Page action test extension should have a page action.";
+
+  ASSERT_TRUE(page_action->HasPopup(tab_id))
+      << "Expect a page action popup before the test removes it.";
+
+  // Load a page which removes the popup using chrome.pageAction.setPopup().
+  {
+    ResultCatcher catcher;
+    ui_test_utils::NavigateToURL(
+        browser(),
+        GURL(extension->GetResourceURL("remove_popup.html")));
+    ASSERT_TRUE(catcher.GetNextResult());
+  }
+
+  ASSERT_FALSE(page_action->HasPopup(tab_id))
+      << "Page action popup should have been removed.";
+}
+
 // Tests old-style pageActions API that is deprecated but we don't want to
 // break.
 IN_PROC_BROWSER_TEST_F(ExtensionApiTest, OldPageActions) {
diff --git a/chrome/common/extensions/api/extension_api.json b/chrome/common/extensions/api/extension_api.json
index 346971c..e1f0d14 100755
--- a/chrome/common/extensions/api/extension_api.json
+++ b/chrome/common/extensions/api/extension_api.json
@@ -1002,13 +1002,31 @@
             }
           }
         ]
+      },
+      {
+        "name": "setPopup",
+        "type": "function",
+        "description": "Sets the html document to be opened as a popup when the user clicks on the page action's icon.",
+        "parameters": [
+          {
+            "name": "details",
+            "type": "object",
+            "properties": {
+              "tabId": {"type": "integer", "minimum": 0, "description": "The id of the tab for which you want to modify the page action."},
+              "popup": {
+                "type": "string",
+                "description": "The html file to show in a popup.  If set to the empty string (''), no popup is shown."
+              }
+            }
+          }
+        ]
       }
     ],
     "events": [
       {
         "name": "onClicked",
         "type": "function",
-        "description": "Fired when a page action icon is clicked.",
+        "description": "Fired when a page action icon is clicked if the page action doesn't show a popup.",
         "parameters": [
           {
             "name": "tab",
diff --git a/chrome/common/extensions/docs/pageAction.html b/chrome/common/extensions/docs/pageAction.html
index 15b693c..5f9f1f6 100644
--- a/chrome/common/extensions/docs/pageAction.html
+++ b/chrome/common/extensions/docs/pageAction.html
@@ -243,6 +243,8 @@
                       </li><li>
                         <a href="#method-setIcon">setIcon</a>
                       </li><li>
+                        <a href="#method-setPopup">setPopup</a>
+                      </li><li>
                         <a href="#method-setTitle">setTitle</a>
                       </li><li>
                         <a href="#method-show">show</a>
@@ -316,7 +318,7 @@ like this:
   <b>"page_action": {
     "default_icon": "icons/foo.png", <em>// <b>required</b></em>
     "default_title": "Do action",    <em>// optional; shown in tooltip</em>
-    "popup": "popup.html"            <em>// optional</em>
+    "default_popup": "popup.html"    <em>// optional</em>
   }</b>,
   ...
 }</pre>
@@ -773,6 +775,182 @@ For other examples and for help in viewing the source code, see
               </div> <!-- /description -->
 
             </div><div class="apiItem">
+              <a name="method-setPopup"></a> <!-- method-anchor -->
+              <h4>setPopup</h4>
+
+              <div class="summary"><span style="display: none; ">void</span>
+                  <!-- Note: intentionally longer 80 columns -->
+                  <span>chrome.pageAction.setPopup</span>(<span class="null"><span style="display: none; ">, </span><span>object</span>
+                      <var><span>details</span></var></span>)</div>
+
+              <div class="description">
+                <p class="todo" style="display: none; ">Undocumented.</p>
+                <p>Sets the html document to be opened as a popup when the user clicks on the page action's icon.</p>
+
+                <!-- PARAMETERS -->
+                <h4>Parameters</h4>
+                <dl>
+                  <div>
+                    <div>
+          <dt>
+            <var>details</var>
+              <em>
+
+                <!-- TYPE -->
+                <div style="display:inline">
+                  (
+                    <span class="optional" style="display: none; ">optional</span>
+                    <span id="typeTemplate">
+                      <span style="display: none; ">
+                        <a> Type</a>
+                      </span>
+                      <span>
+                        <span style="display: none; ">
+                          array of <span><span></span></span>
+                        </span>
+                        <span>object</span>
+                      </span>
+                    </span>
+                  )
+                </div>
+
+              </em>
+          </dt>
+          <dd class="todo">
+            Undocumented.
+          </dd>
+          <dd style="display: none; ">
+            Description of this parameter from the json schema.
+          </dd>
+
+          <!-- OBJECT PROPERTIES -->
+          <dd>
+            <dl>
+              <div>
+                <div>
+          <dt>
+            <var>tabId</var>
+              <em>
+
+                <!-- TYPE -->
+                <div style="display:inline">
+                  (
+                    <span class="optional" style="display: none; ">optional</span>
+                    <span id="typeTemplate">
+                      <span style="display: none; ">
+                        <a> Type</a>
+                      </span>
+                      <span>
+                        <span style="display: none; ">
+                          array of <span><span></span></span>
+                        </span>
+                        <span>integer</span>
+                      </span>
+                    </span>
+                  )
+                </div>
+
+              </em>
+          </dt>
+          <dd class="todo" style="display: none; ">
+            Undocumented.
+          </dd>
+          <dd>The id of the tab for which you want to modify the page action.</dd>
+
+          <!-- OBJECT PROPERTIES -->
+          <dd style="display: none; ">
+            <dl>
+              <div>
+                <div>
+                </div>
+              </div>
+            </dl>
+          </dd>
+        </div>
+              </div><div>
+                <div>
+          <dt>
+            <var>popup</var>
+              <em>
+
+                <!-- TYPE -->
+                <div style="display:inline">
+                  (
+                    <span class="optional" style="display: none; ">optional</span>
+                    <span id="typeTemplate">
+                      <span style="display: none; ">
+                        <a> Type</a>
+                      </span>
+                      <span>
+                        <span style="display: none; ">
+                          array of <span><span></span></span>
+                        </span>
+                        <span>string</span>
+                      </span>
+                    </span>
+                  )
+                </div>
+
+              </em>
+          </dt>
+          <dd class="todo" style="display: none; ">
+            Undocumented.
+          </dd>
+          <dd>The html file to show in a popup.  If set to the empty string (''), no popup is shown.</dd>
+
+          <!-- OBJECT PROPERTIES -->
+          <dd style="display: none; ">
+            <dl>
+              <div>
+                <div>
+                </div>
+              </div>
+            </dl>
+          </dd>
+        </div>
+              </div>
+            </dl>
+          </dd>
+        </div>
+                  </div>
+                </dl>
+
+                <!-- RETURNS -->
+                <h4 style="display: none; ">Returns</h4>
+                <dl>
+                  <div style="display: none; ">
+                    <div>
+                    </div>
+                  </div>
+                </dl>
+
+                <!-- CALLBACK -->
+                <div style="display: none; ">
+                  <div>
+                  <h4>Callback function</h4>
+                  <p>
+                    The callback <em>parameter</em> should specify a function
+                    that looks like this:
+                  </p>
+                  <p>
+                    If you specify the <em>callback</em> parameter, it should
+                    specify a function that looks like this:
+                  </p>
+
+                  <!-- Note: intentionally longer 80 columns -->
+                  <pre>function(<span>Type param1, Type param2</span>) <span class="subdued">{...}</span>);</pre>
+                  <dl>
+                    <div>
+                      <div>
+                      </div>
+                    </div>
+                  </dl>
+                  </div>
+                </div>
+
+              </div> <!-- /description -->
+
+            </div><div class="apiItem">
               <a name="method-setTitle"></a> <!-- method-anchor -->
               <h4>setTitle</h4>
 
@@ -1064,7 +1242,7 @@ For other examples and for help in viewing the source code, see
 
               <div class="description">
                 <p class="todo" style="display: none; ">Undocumented.</p>
-                <p>Fired when a page action icon is clicked.</p>
+                <p>Fired when a page action icon is clicked if the page action doesn't show a popup.</p>
 
                 <!-- PARAMETERS -->
                 <h4>Parameters</h4>
diff --git a/chrome/common/extensions/docs/static/pageAction.html b/chrome/common/extensions/docs/static/pageAction.html
index 24d54ae..0c515b9 100755
--- a/chrome/common/extensions/docs/static/pageAction.html
+++ b/chrome/common/extensions/docs/static/pageAction.html
@@ -43,7 +43,7 @@ like this:
   <b>"page_action": {
     "default_icon": "icons/foo.png", <em>// <b>required</b></em>
     "default_title": "Do action",    <em>// optional; shown in tooltip</em>
-    "popup": "popup.html"            <em>// optional</em>
+    "default_popup": "popup.html"    <em>// optional</em>
   }</b>,
   ...
 }</pre>
diff --git a/chrome/common/extensions/extension.cc b/chrome/common/extensions/extension.cc
index f6de4ab..4b2992d 100644
--- a/chrome/common/extensions/extension.cc
+++ b/chrome/common/extensions/extension.cc
@@ -436,36 +436,55 @@ ExtensionAction* Extension::LoadExtensionActionHelper(
   result->SetTitle(ExtensionAction::kDefaultTabId, title);
 
   // Read the action's |popup| (optional).
-  DictionaryValue* popup = NULL;
-  std::string url_str;
-  if (extension_action->HasKey(keys::kPageActionPopup) &&
-     !extension_action->GetDictionary(keys::kPageActionPopup, &popup) &&
-     !extension_action->GetString(keys::kPageActionPopup, &url_str)) {
-    *error = errors::kInvalidPageActionPopup;
-    return NULL;
-  }
-  if (popup) {
-    // TODO(EXTENSIONS_DEPRECATED): popup is a string only
-    if (!popup->GetString(keys::kPageActionPopupPath, &url_str)) {
+  const wchar_t* popup_key = NULL;
+  if (extension_action->HasKey(keys::kPageActionDefaultPopup))
+    popup_key = keys::kPageActionDefaultPopup;
+
+  // For backward compatibility, alias old key "popup" to new
+  // key "default_popup".
+  if (extension_action->HasKey(keys::kPageActionPopup)) {
+    if (popup_key) {
       *error = ExtensionErrorUtils::FormatErrorMessage(
-          errors::kInvalidPageActionPopupPath, "<missing>");
+          errors::kInvalidPageActionOldAndNewKeys,
+          WideToASCII(keys::kPageActionDefaultPopup),
+          WideToASCII(keys::kPageActionPopup));
       return NULL;
     }
-    GURL url = GetResourceURL(url_str);
-    if (!url.is_valid()) {
-      *error = ExtensionErrorUtils::FormatErrorMessage(
-          errors::kInvalidPageActionPopupPath, url_str);
+    popup_key = keys::kPageActionPopup;
+  }
+
+  if (popup_key) {
+    DictionaryValue* popup = NULL;
+    std::string url_str;
+
+    if (extension_action->GetString(popup_key, &url_str)) {
+      // On success, |url_str| is set.  Nothing else to do.
+    } else if (extension_action->GetDictionary(popup_key, &popup)) {
+      // TODO(EXTENSIONS_DEPRECATED): popup is now a string only.
+      // Support the old dictionary format for backward compatibility.
+      if (!popup->GetString(keys::kPageActionPopupPath, &url_str)) {
+        *error = ExtensionErrorUtils::FormatErrorMessage(
+            errors::kInvalidPageActionPopupPath, "<missing>");
+        return NULL;
+      }
+    } else {
+      *error = errors::kInvalidPageActionPopup;
       return NULL;
     }
-    result->SetPopupUrl(ExtensionAction::kDefaultTabId, url);
-  } else if (!url_str.empty()) {
-    GURL url = GetResourceURL(url_str);
-    if (!url.is_valid()) {
-      *error = ExtensionErrorUtils::FormatErrorMessage(
-          errors::kInvalidPageActionPopupPath, url_str);
-      return NULL;
+
+    if (!url_str.empty()) {
+      // An empty string is treated as having no popup.
+      GURL url = GetResourceURL(url_str);
+      if (!url.is_valid()) {
+        *error = ExtensionErrorUtils::FormatErrorMessage(
+            errors::kInvalidPageActionPopupPath, url_str);
+        return NULL;
+      }
+      result->SetPopupUrl(ExtensionAction::kDefaultTabId, url);
+    } else {
+      DCHECK(!result->HasPopup(ExtensionAction::kDefaultTabId))
+          << "Shouldn't be posible for the popup to be set.";
     }
-    result->SetPopupUrl(ExtensionAction::kDefaultTabId, url);
   }
 
   return result.release();
diff --git a/chrome/common/extensions/extension_constants.cc b/chrome/common/extensions/extension_constants.cc
index d203552..3621b9f 100644
--- a/chrome/common/extensions/extension_constants.cc
+++ b/chrome/common/extensions/extension_constants.cc
@@ -31,6 +31,7 @@ const wchar_t* kPageAction = L"page_action";
 const wchar_t* kPageActions = L"page_actions";
 const wchar_t* kPageActionIcons = L"icons";
 const wchar_t* kPageActionDefaultIcon = L"default_icon";
+const wchar_t* kPageActionDefaultPopup = L"default_popup";
 const wchar_t* kPageActionDefaultTitle = L"default_title";
 const wchar_t* kPageActionPopup = L"popup";
 const wchar_t* kPageActionPopupHeight = L"height";
@@ -133,6 +134,9 @@ const char* kInvalidPageActionId =
     "Required value 'id' is missing or invalid.";
 const char* kInvalidPageActionDefaultTitle =
     "Invalid value for 'default_title'.";
+const char* kInvalidPageActionOldAndNewKeys =
+    "Key \"*\" is deprecated.  Key \"*\" has the same meaning.  You can not "
+    "use both.";
 const char* kInvalidPageActionPopup =
     "Invalid type for page action popup.";
 const char* kInvalidPageActionPopupHeight =
diff --git a/chrome/common/extensions/extension_constants.h b/chrome/common/extensions/extension_constants.h
index c28c583..113cfda 100644
--- a/chrome/common/extensions/extension_constants.h
+++ b/chrome/common/extensions/extension_constants.h
@@ -32,6 +32,7 @@ namespace extension_manifest_keys {
   extern const wchar_t* kPageActions;
   extern const wchar_t* kPageActionIcons;
   extern const wchar_t* kPageActionDefaultIcon;
+  extern const wchar_t* kPageActionDefaultPopup;
   extern const wchar_t* kPageActionDefaultTitle;
   extern const wchar_t* kPageActionPopup;
   extern const wchar_t* kPageActionPopupHeight;
@@ -111,6 +112,7 @@ namespace extension_manifest_errors {
   extern const char* kInvalidPageActionIconPath;
   extern const char* kInvalidPageActionId;
   extern const char* kInvalidPageActionDefaultTitle;
+  extern const char* kInvalidPageActionOldAndNewKeys;
   extern const char* kInvalidPageActionPopup;
   extern const char* kInvalidPageActionPopupHeight;
   extern const char* kInvalidPageActionPopupPath;
diff --git a/chrome/common/extensions/extension_unittest.cc b/chrome/common/extensions/extension_unittest.cc
index d0e14e4..d0d1f89 100644
--- a/chrome/common/extensions/extension_unittest.cc
+++ b/chrome/common/extensions/extension_unittest.cc
@@ -12,7 +12,9 @@
 #include "chrome/common/chrome_paths.h"
 #include "chrome/common/extensions/extension_constants.h"
 #include "chrome/common/extensions/extension_error_reporter.h"
+#include "chrome/common/extensions/extension_error_utils.h"
 #include "chrome/common/json_value_serializer.h"
+#include "chrome/common/url_constants.h"
 #include "net/base/mime_sniffer.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -406,6 +408,7 @@ TEST(ExtensionTest, LoadPageActionHelper) {
   // Now setup some values to use in the page action.
   const std::string kTitle("MyExtensionActionTitle");
   const std::string kIcon("image1.png");
+  const std::string kPopupHtmlFile("a_popup.html");
 
   // Add the dictionary for the contextual action.
   input.Clear();
@@ -441,6 +444,70 @@ TEST(ExtensionTest, LoadPageActionHelper) {
   ASSERT_TRUE(NULL == action.get());
   ASSERT_STREQ(errors::kInvalidPageActionName, error_msg.c_str());
   error_msg = "";
+
+  // Test that keys "popup" and "default_popup" both work, but can not
+  // be used at the same time.
+  input.Clear();
+  input.SetString(keys::kPageActionDefaultTitle, kTitle);
+  input.SetString(keys::kPageActionDefaultIcon, kIcon);
+
+  // LoadExtensionActionHelper expects the extension member |extension_url_|
+  // to be set.
+  extension.extension_url_ = GURL(std::string(chrome::kExtensionScheme) +
+                                  chrome::kStandardSchemeSeparator +
+                                  "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa/");
+
+  // Add key "popup", expect success.
+  input.SetString(keys::kPageActionPopup, kPopupHtmlFile);
+  action.reset(extension.LoadExtensionActionHelper(&input, &error_msg));
+  ASSERT_TRUE(NULL != action.get());
+  ASSERT_TRUE(error_msg.empty());
+  ASSERT_STREQ(
+      extension.extension_url_.Resolve(kPopupHtmlFile).spec().c_str(),
+      action->GetPopupUrl(ExtensionAction::kDefaultTabId).spec().c_str());
+
+  // Add key "default_popup", expect failure.
+  input.SetString(keys::kPageActionDefaultPopup, kPopupHtmlFile);
+  action.reset(extension.LoadExtensionActionHelper(&input, &error_msg));
+  ASSERT_TRUE(NULL == action.get());
+  ASSERT_STREQ(
+      ExtensionErrorUtils::FormatErrorMessage(
+          errors::kInvalidPageActionOldAndNewKeys,
+          WideToASCII(keys::kPageActionDefaultPopup),
+          WideToASCII(keys::kPageActionPopup)).c_str(),
+      error_msg.c_str());
+  error_msg = "";
+
+  // Remove key "popup", expect success.
+  input.Remove(keys::kPageActionPopup, NULL);
+  action.reset(extension.LoadExtensionActionHelper(&input, &error_msg));
+  ASSERT_TRUE(NULL != action.get());
+  ASSERT_TRUE(error_msg.empty());
+  ASSERT_STREQ(
+      extension.extension_url_.Resolve(kPopupHtmlFile).spec().c_str(),
+      action->GetPopupUrl(ExtensionAction::kDefaultTabId).spec().c_str());
+
+  // Setting default_popup to "" is the same as having no popup.
+  input.Remove(keys::kPageActionDefaultPopup, NULL);
+  input.SetString(keys::kPageActionDefaultPopup, "");
+  action.reset(extension.LoadExtensionActionHelper(&input, &error_msg));
+  ASSERT_TRUE(NULL != action.get());
+  ASSERT_TRUE(error_msg.empty());
+  EXPECT_FALSE(action->HasPopup(ExtensionAction::kDefaultTabId));
+  ASSERT_STREQ(
+      "",
+      action->GetPopupUrl(ExtensionAction::kDefaultTabId).spec().c_str());
+
+  // Setting popup to "" is the same as having no popup.
+  input.Remove(keys::kPageActionDefaultPopup, NULL);
+  input.SetString(keys::kPageActionPopup, "");
+  action.reset(extension.LoadExtensionActionHelper(&input, &error_msg));
+  ASSERT_TRUE(NULL != action.get());
+  ASSERT_TRUE(error_msg.empty());
+  EXPECT_FALSE(action->HasPopup(ExtensionAction::kDefaultTabId));
+  ASSERT_STREQ(
+      "",
+      action->GetPopupUrl(ExtensionAction::kDefaultTabId).spec().c_str());
 }
 
 TEST(ExtensionTest, IdIsValid) {