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

7 files changed, 316 insertions, 28 deletions

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) {