Fix a couple of bugs relating to sending Tab info with chrome.runtime.connect and

chrome/browser/extensions/api/messaging/message_service.cc

Index: chrome/browser/extensions/api/messaging/message_service.cc
diff --git a/chrome/browser/extensions/api/messaging/message_service.cc b/chrome/browser/extensions/api/messaging/message_service.cc
index e3075f9eb6672a06da1ca3aa71a9f85bd97658f6..3c72ffb1ec50ffe63640d039c21a5d13c1f30b88 100644
--- a/chrome/browser/extensions/api/messaging/message_service.cc
+++ b/chrome/browser/extensions/api/messaging/message_service.cc
@@ -32,6 +32,7 @@
 #include "content/public/browser/render_view_host.h"
 #include "content/public/browser/site_instance.h"
 #include "content/public/browser/web_contents.h"
+#include "googleurl/src/gurl.h"
 
 using content::SiteInstance;
 using content::WebContents;
@@ -66,28 +67,36 @@ struct MessageService::MessageChannel {
 
 struct MessageService::OpenChannelParams {
   content::RenderProcessHost* source;
-  std::string tab_json;
+  DictionaryValue source_tab;
   scoped_ptr<MessagePort> receiver;
   int receiver_port_id;
   std::string source_extension_id;
   std::string target_extension_id;
+  GURL source_url;
   std::string channel_name;
 
   // Takes ownership of receiver.
   OpenChannelParams(content::RenderProcessHost* source,
-                    const std::string& tab_json,
+                    scoped_ptr<DictionaryValue> source_tab,
                     MessagePort* receiver,
                     int receiver_port_id,
                     const std::string& source_extension_id,
                     const std::string& target_extension_id,
+                    const GURL& source_url,
                     const std::string& channel_name)
       : source(source),
-        tab_json(tab_json),
         receiver(receiver),
         receiver_port_id(receiver_port_id),
         source_extension_id(source_extension_id),
         target_extension_id(target_extension_id),
-        channel_name(channel_name) {}
+        source_url(source_url),
+        channel_name(channel_name) {
+    if (source_tab)
+      this->source_tab.Swap(source_tab.get());
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(OpenChannelParams);
 };
 
 namespace {
@@ -152,6 +161,7 @@ void MessageService::OpenChannelToExtension(
     int source_process_id, int source_routing_id, int receiver_port_id,
     const std::string& source_extension_id,
     const std::string& target_extension_id,
+    const GURL& source_url,
     const std::string& channel_name) {
   content::RenderProcessHost* source =
       content::RenderProcessHost::FromID(source_process_id);
@@ -169,17 +179,21 @@ void MessageService::OpenChannelToExtension(
       source_process_id, source_routing_id);
 
   // Include info about the opener's tab (if it was a tab).
-  std::string tab_json = "null";
-  if (source_contents) {
-    scoped_ptr<DictionaryValue> tab_value(ExtensionTabUtil::CreateTabValue(
-        source_contents));
-    base::JSONWriter::Write(tab_value.get(), &tab_json);
+  scoped_ptr<DictionaryValue> source_tab;
+  GURL source_url_for_tab;
+
+  if (source_contents && ExtensionTabUtil::GetTabId(source_contents) >= 0) {
+    source_tab.reset(ExtensionTabUtil::CreateTabValue(source_contents));
+    source_url_for_tab = source_url;
   }
 
-  OpenChannelParams* params = new OpenChannelParams(source, tab_json, receiver,
+  OpenChannelParams* params = new OpenChannelParams(source,
+                                                    source_tab.Pass(),
+                                                    receiver,
                                                     receiver_port_id,
                                                     source_extension_id,
                                                     target_extension_id,
+                                                    source_url_for_tab,
                                                     channel_name);
 
   // The target might be a lazy background page. In that case, we have to check
@@ -189,7 +203,7 @@ void MessageService::OpenChannelToExtension(
     return;
   }
 
-  OpenChannelImpl(scoped_ptr<OpenChannelParams>(params));
+  OpenChannelImpl(make_scoped_ptr(params));
 }
 
 void MessageService::OpenChannelToNativeApp(
@@ -222,17 +236,6 @@ void MessageService::OpenChannelToNativeApp(
     return;
   }
 
-  WebContents* source_contents = tab_util::GetWebContentsByID(
-      source_process_id, source_routing_id);
-
-  // Include info about the opener's tab (if it was a tab).
-  std::string tab_json = "null";
-  if (source_contents) {
-    scoped_ptr<DictionaryValue> tab_value(ExtensionTabUtil::CreateTabValue(
-        source_contents));
-    base::JSONWriter::Write(tab_value.get(), &tab_json);
-  }
-

[Matt Perry, 2013/04/23 18:00:17]

Why delete this?

[not at google - send to devlin, 2013/04/23 18:30:18]

Maybe I'm missing something subtle (or obvious), but it seems like a copy-paste
gone wrong. Tab value isn't actually used anywhere.

[Matt Perry, 2013/04/23 18:39:37]

Oh right, good point :)

   scoped_ptr<MessageChannel> channel(new MessageChannel());
   channel->opener.reset(new ExtensionMessagePort(source, MSG_ROUTING_CONTROL,
                                                  source_extension_id));
@@ -298,19 +301,20 @@ void MessageService::OpenChannelToTab(
       source_process_id, source_routing_id);
 
   // Include info about the opener's tab (if it was a tab).
-  std::string tab_json = "null";
-  if (source_contents) {
-    scoped_ptr<DictionaryValue> tab_value(ExtensionTabUtil::CreateTabValue(
-        source_contents));
-    base::JSONWriter::Write(tab_value.get(), &tab_json);
-  }
-
-  scoped_ptr<OpenChannelParams> params(new OpenChannelParams(source, tab_json,
-                                                             receiver.release(),
-                                                             receiver_port_id,
-                                                             extension_id,
-                                                             extension_id,
-                                                             channel_name));
+  scoped_ptr<DictionaryValue> source_tab;
+
+  if (source_contents && ExtensionTabUtil::GetTabId(source_contents) >= 0)
+    source_tab.reset(ExtensionTabUtil::CreateTabValue(source_contents));
+
+  scoped_ptr<OpenChannelParams> params(new OpenChannelParams(
+        source,
+        source_tab.Pass(),
+        receiver.release(),
+        receiver_port_id,
+        extension_id,
+        extension_id,
+        GURL(),  // source URL doesn't make sense for opening to tabs

[Matt Perry, 2013/04/23 18:00:17]

Source tab doesn't make sense to me either. Do you know why we included it
before? I can't think of a case where it would happen.

[not at google - send to devlin, 2013/04/23 18:30:18]

Oh, right. Yeah we presumably included it before because there were no tests to
assert what we documented. I think that GetTabId above will always be -1 in this
case though, since these calls can only come from extension processes, so
source_tab will always be NULL here. I'll make it explicit, thanks.

+        channel_name));
   OpenChannelImpl(params.Pass());
 }
 
@@ -346,9 +350,11 @@ bool MessageService::OpenChannelImpl(scoped_ptr<OpenChannelParams> params) {
   // Send the connect event to the receiver.  Give it the opener's port ID (the
   // opener has the opposite port ID).
   channel->receiver->DispatchOnConnect(params->receiver_port_id,
-                                       params->channel_name, params->tab_json,
+                                       params->channel_name,
+                                       params->source_tab,
                                        params->source_extension_id,
-                                       params->target_extension_id);
+                                       params->target_extension_id,
+                                       params->source_url);
 
   // Keep both ends of the channel alive until the channel is closed.
   channel->opener->IncrementLazyKeepaliveCount();