Always send the full tab object in ExtensionAction click event.

Along the way, decompose a few swiss army knife functions to
simplify and generalize code.

BUG=149020

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

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

11 files changed, 107 insertions, 151 deletions

diff --git a/chrome/browser/extensions/api/extension_action/browser_action_apitest.cc b/chrome/browser/extensions/api/extension_action/browser_action_apitest.cc
index 5844901..11f8ccb 100644
--- a/chrome/browser/extensions/api/extension_action/browser_action_apitest.cc
+++ b/chrome/browser/extensions/api/extension_action/browser_action_apitest.cc
@@ -101,7 +101,7 @@ IN_PROC_BROWSER_TEST_F(BrowserActionApiTest, Basic) {
   ResultCatcher catcher;
   ui_test_utils::NavigateToURL(browser(),
       GURL(extension->GetResourceURL("update.html")));
-  ASSERT_TRUE(catcher.GetNextResult());
+  ASSERT_TRUE(catcher.GetNextResult()) << catcher.message();
 
   // Test that we received the changes.
   ExtensionAction* action = extension->browser_action();
@@ -117,16 +117,7 @@ IN_PROC_BROWSER_TEST_F(BrowserActionApiTest, Basic) {
   ExtensionService* service = browser()->profile()->GetExtensionService();
   service->toolbar_model()->ExecuteBrowserAction(extension, browser(), NULL);
 
-  // Verify the command worked.
-  WebContents* tab = chrome::GetActiveWebContents(browser());
-  bool result = false;
-  ASSERT_TRUE(content::ExecuteJavaScriptAndExtractBool(
-      tab->GetRenderViewHost(), L"",
-      L"setInterval(function(){"
-      L"  if(document.body.bgColor == 'red'){"
-      L"    window.domAutomationController.send(true)}}, 100)",
-      &result));
-  ASSERT_TRUE(result);
+  ASSERT_TRUE(catcher.GetNextResult()) << catcher.message();
 }
 
 IN_PROC_BROWSER_TEST_F(BrowserActionApiTest, DynamicBrowserAction) {
diff --git a/chrome/browser/extensions/api/messaging/message_service.cc b/chrome/browser/extensions/api/messaging/message_service.cc
index 07003e6..e45fb80 100644
--- a/chrome/browser/extensions/api/messaging/message_service.cc
+++ b/chrome/browser/extensions/api/messaging/message_service.cc
@@ -162,9 +162,7 @@ void MessageService::OpenChannelToExtension(
   std::string tab_json = "null";
   if (source_contents) {
     scoped_ptr<DictionaryValue> tab_value(ExtensionTabUtil::CreateTabValue(
-            source_contents,
-            profile->GetExtensionService()->extensions()->GetByID(
-                source_extension_id)));
+        source_contents, ExtensionTabUtil::INCLUDE_PRIVACY_SENSITIVE_FIELDS));
     base::JSONWriter::Write(tab_value.get(), &tab_json);
   }
 
@@ -196,7 +194,6 @@ void MessageService::OpenChannelToNativeApp(
       content::RenderProcessHost::FromID(source_process_id);
   if (!source)
     return;
-  Profile* profile = Profile::FromBrowserContext(source->GetBrowserContext());
 
   WebContents* source_contents = tab_util::GetWebContentsByID(
       source_process_id, source_routing_id);
@@ -205,9 +202,7 @@ void MessageService::OpenChannelToNativeApp(
   std::string tab_json = "null";
   if (source_contents) {
     scoped_ptr<DictionaryValue> tab_value(ExtensionTabUtil::CreateTabValue(
-            source_contents,
-            profile->GetExtensionService()->extensions()->GetByID(
-                source_extension_id)));
+        source_contents, ExtensionTabUtil::INCLUDE_PRIVACY_SENSITIVE_FIELDS));
     base::JSONWriter::Write(tab_value.get(), &tab_json);
   }
 
@@ -292,9 +287,7 @@ void MessageService::OpenChannelToTab(
   std::string tab_json = "null";
   if (source_contents) {
     scoped_ptr<DictionaryValue> tab_value(ExtensionTabUtil::CreateTabValue(
-            source_contents,
-            profile->GetExtensionService()->extensions()->GetByID(
-                extension_id)));
+        source_contents, ExtensionTabUtil::INCLUDE_PRIVACY_SENSITIVE_FIELDS));
     base::JSONWriter::Write(tab_value.get(), &tab_json);
   }
 
diff --git a/chrome/browser/extensions/browser_event_router.cc b/chrome/browser/extensions/browser_event_router.cc
index af5cf9b..7f69f0c 100644
--- a/chrome/browser/extensions/browser_event_router.cc
+++ b/chrome/browser/extensions/browser_event_router.cc
@@ -166,8 +166,23 @@ void BrowserEventRouter::TabCreatedAt(WebContents* contents,
                                       int index,
                                       bool active) {
   Profile* profile = Profile::FromBrowserContext(contents->GetBrowserContext());
-  DispatchEventWithTab(profile, "", events::kOnTabCreated, contents, active,
-                       EventRouter::USER_GESTURE_NOT_ENABLED);
+  const EventListenerMap::ListenerList& listeners(
+      ExtensionSystem::Get(profile)->event_router()->
+      listeners().GetEventListenersByName(events::kOnTabCreated));
+  for (EventListenerMap::ListenerList::const_iterator it = listeners.begin();
+       it != listeners.end();
+       ++it) {
+    scoped_ptr<ListValue> args(new ListValue());
+    DictionaryValue* tab_value = ExtensionTabUtil::CreateTabValue(
+        contents,
+        profile->GetExtensionService()->extensions()->GetByID(
+            (*it)->extension_id));
+    args->Append(tab_value);
+    tab_value->SetBoolean(tab_keys::kSelectedKey, active);
+    DispatchEventToExtension(profile, (*it)->extension_id,
+                             events::kOnTabCreated, args.Pass(),
+                             EventRouter::USER_GESTURE_NOT_ENABLED);
+  }
 
   RegisterForTabNotifications(contents);
 }
@@ -378,44 +393,6 @@ void BrowserEventRouter::DispatchEventsAcrossIncognito(
       GURL());
 }
 
-void BrowserEventRouter::DispatchEventWithTab(
-    Profile* profile,
-    const std::string& extension_id,
-    const char* event_name,
-    const WebContents* web_contents,
-    bool active,
-    EventRouter::UserGestureState user_gesture,
-    scoped_ptr<ListValue> event_args) {
-  if (!profile_->IsSameProfile(profile))
-    return;
-
-  if (!extension_id.empty()) {
-    event_args->Append(ExtensionTabUtil::CreateTabValueActive(
-        web_contents,
-        active,
-        profile->GetExtensionService()->extensions()->GetByID(extension_id)));
-    DispatchEventToExtension(profile, extension_id, event_name,
-                             event_args.Pass(), user_gesture);
-  } else {
-    const EventListenerMap::ListenerList& listeners(
-        ExtensionSystem::Get(profile)->event_router()->
-        listeners().GetEventListenersByName(event_name));
-
-    for (EventListenerMap::ListenerList::const_iterator it = listeners.begin();
-         it != listeners.end();
-         ++it) {
-      scoped_ptr<ListValue> args(event_args->DeepCopy());
-      args->Append(ExtensionTabUtil::CreateTabValueActive(
-          web_contents,
-          active,
-          profile->GetExtensionService()->extensions()->GetByID(
-              (*it)->extension_id)));
-      DispatchEventToExtension(profile, (*it)->extension_id, event_name,
-                               args.Pass(), user_gesture);
-    }
-  }
-}
-
 void BrowserEventRouter::DispatchSimpleBrowserEvent(
     Profile* profile, const int window_id, const char* event_name) {
   if (!profile_->IsSameProfile(profile))
@@ -435,19 +412,33 @@ void BrowserEventRouter::DispatchTabUpdatedEvent(
 
   // The state of the tab (as seen from the extension point of view) has
   // changed.  Send a notification to the extension.
-  scoped_ptr<ListValue> args(new ListValue());
+  scoped_ptr<ListValue> args_base(new ListValue());
 
   // First arg: The id of the tab that changed.
-  args->Append(Value::CreateIntegerValue(ExtensionTabUtil::GetTabId(contents)));
+  args_base->AppendInteger(ExtensionTabUtil::GetTabId(contents));
 
   // Second arg: An object containing the changes to the tab state.
-  args->Append(changed_properties);
+  args_base->Append(changed_properties);
 
   // Third arg: An object containing the state of the tab.
   Profile* profile = Profile::FromBrowserContext(contents->GetBrowserContext());
 
-  DispatchEventWithTab(profile, "", events::kOnTabUpdated, contents, true,
-                       EventRouter::USER_GESTURE_UNKNOWN, args.Pass());
+  const EventListenerMap::ListenerList& listeners(
+      ExtensionSystem::Get(profile)->event_router()->
+      listeners().GetEventListenersByName(events::kOnTabUpdated));
+  for (EventListenerMap::ListenerList::const_iterator it = listeners.begin();
+       it != listeners.end();
+       ++it) {
+    scoped_ptr<ListValue> args(args_base->DeepCopy());
+    DictionaryValue* tab_value = ExtensionTabUtil::CreateTabValue(
+        contents,
+        profile->GetExtensionService()->extensions()->GetByID(
+            (*it)->extension_id));
+    args->Append(tab_value);
+    DispatchEventToExtension(profile, (*it)->extension_id,
+                             events::kOnTabUpdated, args.Pass(),
+                             EventRouter::USER_GESTURE_UNKNOWN);
+  }
 }
 
 BrowserEventRouter::TabEntry* BrowserEventRouter::GetTabEntry(
@@ -597,12 +588,17 @@ void BrowserEventRouter::ExtensionActionExecuted(
   }
 
   if (event_name) {
-    DispatchEventWithTab(profile,
-                         extension_action.extension_id(),
-                         event_name,
-                         tab_contents->web_contents(),
-                         true,
-                         EventRouter::USER_GESTURE_ENABLED);
+    scoped_ptr<ListValue> args(new ListValue());
+    DictionaryValue* tab_value = ExtensionTabUtil::CreateTabValue(
+        tab_contents->web_contents(),
+        ExtensionTabUtil::INCLUDE_PRIVACY_SENSITIVE_FIELDS);
+    args->Append(tab_value);
+
+    DispatchEventToExtension(profile,
+                             extension_action.extension_id(),
+                             event_name,
+                             args.Pass(),
+                             EventRouter::USER_GESTURE_ENABLED);
   }
 }
 
diff --git a/chrome/browser/extensions/browser_event_router.h b/chrome/browser/extensions/browser_event_router.h
index 69732c2..0ea37b3 100644
--- a/chrome/browser/extensions/browser_event_router.h
+++ b/chrome/browser/extensions/browser_event_router.h
@@ -122,26 +122,6 @@ class BrowserEventRouter : public TabStripModelObserver,
       scoped_ptr<base::ListValue> event_args,
       scoped_ptr<base::ListValue> cross_incognito_args);
 
-  void DispatchEventWithTab(Profile* profile,
-                            const std::string& extension_id,
-                            const char* event_name,
-                            const content::WebContents* web_contents,
-                            bool active,
-                            EventRouter::UserGestureState user_gesture,
-                            scoped_ptr<ListValue> event_args);
-
-  // DispatchEvent with a tab value appended as the last argument.
-  void DispatchEventWithTab(Profile* profile,
-                            const std::string& extension_id,
-                            const char* event_name,
-                            const content::WebContents* web_contents,
-                            bool active,
-                            EventRouter::UserGestureState user_gesture) {
-    DispatchEventWithTab(profile, extension_id, event_name, web_contents,
-                         active, user_gesture,
-                         scoped_ptr<ListValue>(new ListValue).Pass());
-  }
-
   void DispatchSimpleBrowserEvent(Profile* profile,
                                   const int window_id,
                                   const char* event_name);
diff --git a/chrome/browser/extensions/extension_tab_util.cc b/chrome/browser/extensions/extension_tab_util.cc
index 09d20e0..416a07b 100644
--- a/chrome/browser/extensions/extension_tab_util.cc
+++ b/chrome/browser/extensions/extension_tab_util.cc
@@ -62,19 +62,17 @@ int ExtensionTabUtil::GetWindowIdOfTab(const WebContents* web_contents) {
 
 DictionaryValue* ExtensionTabUtil::CreateTabValue(
     const WebContents* contents,
+    TabStripModel* tab_strip,
+    int tab_index,
     const Extension* extension) {
-  // Find the tab strip and index of this guy.
-  TabStripModel* tab_strip = NULL;
-  int tab_index;
-  if (ExtensionTabUtil::GetTabStripModel(contents, &tab_strip, &tab_index)) {
-    return ExtensionTabUtil::CreateTabValue(contents,
-                                            tab_strip,
-                                            tab_index,
-                                            extension);
-  }
+  // Only add privacy-sensitive data if the requesting extension has the tabs
+  // permission.
+  bool has_permission = extension && extension->HasAPIPermissionForTab(
+      GetTabId(contents), APIPermission::kTab);
 
-  // Couldn't find it.  This can happen if the tab is being dragged.
-  return ExtensionTabUtil::CreateTabValue(contents, NULL, -1, extension);
+  return CreateTabValue(contents, tab_strip, tab_index,
+                        has_permission ? INCLUDE_PRIVACY_SENSITIVE_FIELDS :
+                            OMIT_PRIVACY_SENSITIVE_FIELDS);
 }
 
 ListValue* ExtensionTabUtil::CreateTabList(
@@ -97,7 +95,10 @@ DictionaryValue* ExtensionTabUtil::CreateTabValue(
     const WebContents* contents,
     TabStripModel* tab_strip,
     int tab_index,
-    const Extension* extension) {
+    IncludePrivacySensitiveFields include_privacy_sensitive_fields) {
+  if (!tab_strip)
+    ExtensionTabUtil::GetTabStripModel(contents, &tab_strip, &tab_index);
+
   DictionaryValue* result = new DictionaryValue();
   bool is_loading = contents->IsLoading();
   result->SetInteger(keys::kIdKey, GetTabId(contents));
@@ -115,24 +116,7 @@ DictionaryValue* ExtensionTabUtil::CreateTabValue(
   result->SetBoolean(keys::kIncognitoKey,
                      contents->GetBrowserContext()->IsOffTheRecord());
 
-  // Only add privacy-sensitive data if the requesting extension has the tabs
-  // permission.
-  bool has_permission = false;
-  if (extension) {
-    if (tab_index >= 0) {
-      has_permission =
-          extension->HasAPIPermissionForTab(
-              tab_index, APIPermission::kTab) ||
-          extension->HasAPIPermissionForTab(
-              tab_index, APIPermission::kWebNavigation);
-    } else {
-      has_permission =
-          extension->HasAPIPermission(APIPermission::kTab) ||
-          extension->HasAPIPermission(APIPermission::kWebNavigation);
-    }
-  }
-
-  if (has_permission) {
+  if (include_privacy_sensitive_fields == INCLUDE_PRIVACY_SENSITIVE_FIELDS) {
     result->SetString(keys::kUrlKey, contents->GetURL().spec());
     result->SetString(keys::kTitleKey, contents->GetTitle());
     if (!is_loading) {
@@ -154,15 +138,6 @@ DictionaryValue* ExtensionTabUtil::CreateTabValue(
   return result;
 }
 
-DictionaryValue* ExtensionTabUtil::CreateTabValueActive(
-    const WebContents* contents,
-    bool active,
-    const extensions::Extension* extension) {
-  DictionaryValue* result = CreateTabValue(contents, extension);
-  result->SetBoolean(keys::kSelectedKey, active);
-  return result;
-}
-
 bool ExtensionTabUtil::GetTabStripModel(const WebContents* web_contents,
                                         TabStripModel** tab_strip_model,
                                         int* tab_index) {
diff --git a/chrome/browser/extensions/extension_tab_util.h b/chrome/browser/extensions/extension_tab_util.h
index 0261164..de81e8b 100644
--- a/chrome/browser/extensions/extension_tab_util.h
+++ b/chrome/browser/extensions/extension_tab_util.h
@@ -45,19 +45,33 @@ class ExtensionTabUtil {
   static base::ListValue* CreateTabList(
       const Browser* browser,
       const extensions::Extension* extension);
+
   static base::DictionaryValue* CreateTabValue(
       const content::WebContents* web_contents,
-      const extensions::Extension* extension);
+      const extensions::Extension* extension) {
+    return CreateTabValue(web_contents, NULL, -1, extension);
+  }
   static base::DictionaryValue* CreateTabValue(
       const content::WebContents* web_contents,
       TabStripModel* tab_strip,
       int tab_index,
       const extensions::Extension* extension);
-  // Create a tab value, overriding its kSelectedKey to the provided boolean.
-  static base::DictionaryValue* CreateTabValueActive(
+
+  enum IncludePrivacySensitiveFields {
+    INCLUDE_PRIVACY_SENSITIVE_FIELDS,
+    OMIT_PRIVACY_SENSITIVE_FIELDS
+  };
+  static base::DictionaryValue* CreateTabValue(
       const content::WebContents* web_contents,
-      bool active,
-      const extensions::Extension* extension);
+      IncludePrivacySensitiveFields include_privacy_sensitive_fields) {
+    return CreateTabValue(web_contents, NULL, -1,
+                          include_privacy_sensitive_fields);
+  }
+  static base::DictionaryValue* CreateTabValue(
+      const content::WebContents* web_contents,
+      TabStripModel* tab_strip,
+      int tab_index,
+      IncludePrivacySensitiveFields include_privacy_sensitive_fields);
 
   // Gets the |tab_strip_model| and |tab_index| for the given |web_contents|.
   static bool GetTabStripModel(const content::WebContents* web_contents,
diff --git a/chrome/browser/extensions/extension_tab_util_android.cc b/chrome/browser/extensions/extension_tab_util_android.cc
index 2e0b51a..7cd8377 100644
--- a/chrome/browser/extensions/extension_tab_util_android.cc
+++ b/chrome/browser/extensions/extension_tab_util_android.cc
@@ -34,6 +34,8 @@ int ExtensionTabUtil::GetWindowIdOfTab(const WebContents* web_contents) {
 
 DictionaryValue* ExtensionTabUtil::CreateTabValue(
     const WebContents* contents,
+    TabStripModel* tab_strip,
+    int tab_index,
     const extensions::Extension* extension) {
   NOTIMPLEMENTED();
   return NULL;
@@ -50,15 +52,7 @@ DictionaryValue* ExtensionTabUtil::CreateTabValue(
     const WebContents* contents,
     TabStripModel* tab_strip,
     int tab_index,
-    const extensions::Extension* extension) {
-  NOTIMPLEMENTED();
-  return NULL;
-}
-
-DictionaryValue* ExtensionTabUtil::CreateTabValueActive(
-    const WebContents* contents,
-    bool active,
-    const extensions::Extension* extension) {
+    IncludePrivacySensitiveFields include_privacy_sensitive_fields) {
   NOTIMPLEMENTED();
   return NULL;
 }
diff --git a/chrome/browser/extensions/menu_manager.cc b/chrome/browser/extensions/menu_manager.cc
index bd2e0eb..33fcb08 100644
--- a/chrome/browser/extensions/menu_manager.cc
+++ b/chrome/browser/extensions/menu_manager.cc
@@ -643,10 +643,12 @@ void MenuManager::ExecuteCommand(Profile* profile,
   // No tab info in a platform app.
   if (!extension || !extension->is_platform_app()) {
     // Note: web_contents are NULL in unit tests :(
-    if (web_contents)
-      args->Append(ExtensionTabUtil::CreateTabValue(web_contents, extension));
-    else
+    if (web_contents) {
+      args->Append(ExtensionTabUtil::CreateTabValue(
+          web_contents, ExtensionTabUtil::INCLUDE_PRIVACY_SENSITIVE_FIELDS));
+    } else {
       args->Append(new DictionaryValue());
+    }
   }
 
   if (item->type() == MenuItem::CHECKBOX ||
diff --git a/chrome/test/data/extensions/api_test/browser_action/basics/background.js b/chrome/test/data/extensions/api_test/browser_action/basics/background.js
index 4160911..db07f43 100644
--- a/chrome/test/data/extensions/api_test/browser_action/basics/background.js
+++ b/chrome/test/data/extensions/api_test/browser_action/basics/background.js
@@ -3,8 +3,16 @@
 // found in the LICENSE file.
 
 // Called when the user clicks on the browser action.
-chrome.browserAction.onClicked.addListener(function(windowId) {
-  chrome.tabs.executeScript(null, {code:"document.body.bgColor='red'"});
+chrome.browserAction.onClicked.addListener(function(tab) {
+  // Privacy-sensitive properties are treated specially. Ensure they are
+  // present.
+  chrome.test.assertTrue(Boolean(tab.url.length));
+  chrome.test.assertTrue(Boolean(tab.title.length));
+
+  // Everything else is handled in a general way and should of course also
+  // be present.
+  chrome.test.assertTrue(Boolean(tab.id));
+  chrome.test.notifyPass();
 });
 
 chrome.test.notifyPass();
diff --git a/chrome/test/data/extensions/api_test/browser_action/basics/manifest.json b/chrome/test/data/extensions/api_test/browser_action/basics/manifest.json
index c9acc10..e67eaa8 100644
--- a/chrome/test/data/extensions/api_test/browser_action/basics/manifest.json
+++ b/chrome/test/data/extensions/api_test/browser_action/basics/manifest.json
@@ -6,7 +6,7 @@
     "scripts": ["background.js"]
   },
   "permissions": [
-    "tabs", "http://*/*"
+    "http://*/*"
   ],
   "browser_action": {
     "default_title": "Make this page red",
diff --git a/chrome/test/data/extensions/api_test/messaging/connect/test.js b/chrome/test/data/extensions/api_test/messaging/connect/test.js
index 54c58a2..95a9d1d 100644
--- a/chrome/test/data/extensions/api_test/messaging/connect/test.js
+++ b/chrome/test/data/extensions/api_test/messaging/connect/test.js
@@ -65,6 +65,9 @@ chrome.test.getConfig(function(config) {
     // Tests that postMessage from the tab and its response works.
     function postMessageFromTab() {
       chrome.extension.onConnect.addListener(function(port) {
+        chrome.test.assertTrue(Boolean(port.sender.tab.url));
+        chrome.test.assertTrue(Boolean(port.sender.tab.title));
+        chrome.test.assertTrue(Boolean(port.sender.tab.id));
         port.onMessage.addListener(function(msg) {
           chrome.test.assertTrue(msg.testPostMessageFromTab);
           port.postMessage({success: true, portName: port.name});