Add frameId to MessageSender (extension messaging API)

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 68fa08626c5da3eff0e0bf84a630f88e16f384fd..6323291e681f045e96bdc3d398805c1eb8667bfb 100644
--- a/chrome/browser/extensions/api/messaging/message_service.cc
+++ b/chrome/browser/extensions/api/messaging/message_service.cc
@@ -119,6 +119,7 @@ struct MessageService::MessageChannel {
 struct MessageService::OpenChannelParams {
   content::RenderProcessHost* source;
   base::DictionaryValue source_tab;
+  int source_frame_id;
   scoped_ptr<MessagePort> receiver;
   int receiver_port_id;
   std::string source_extension_id;
@@ -131,6 +132,7 @@ struct MessageService::OpenChannelParams {
   // Takes ownership of receiver.
   OpenChannelParams(content::RenderProcessHost* source,
                     scoped_ptr<base::DictionaryValue> source_tab,
+                    int source_frame_id,
                     MessagePort* receiver,
                     int receiver_port_id,
                     const std::string& source_extension_id,
@@ -139,6 +141,7 @@ struct MessageService::OpenChannelParams {
                     const std::string& channel_name,
                     bool include_tls_channel_id)
       : source(source),
+        source_frame_id(source_frame_id),
         receiver(receiver),
         receiver_port_id(receiver_port_id),
         source_extension_id(source_extension_id),
@@ -233,6 +236,7 @@ MessageService* MessageService::Get(BrowserContext* context) {
 
 void MessageService::OpenChannelToExtension(
     int source_process_id, int source_routing_id, int receiver_port_id,
+    int source_frame_id,
     const std::string& source_extension_id,
     const std::string& target_extension_id,
     const GURL& source_url,
@@ -337,10 +341,13 @@ void MessageService::OpenChannelToExtension(
     // unnecessary bits will be stripped out in
     // MessagingBindings::DispatchOnConnect().
     source_tab.reset(ExtensionTabUtil::CreateTabValue(source_contents));
+    DCHECK_GE(source_frame_id, 0);
+  } else {
+    source_frame_id = -1;

[not at google - send to devlin, 2014/11/10 18:36:42]

I don't like rewriting the value of parameters, it's confusing. Parameters
should be implicitly const IMO.

I don't really see any harm in passing the frame ID through here anyway.

[robwu, 2014/11/10 21:43:07]

I wanted to make it clear that the the ID is non-negative iff tab is set. But
this is already checked in the js extension bindings, so I'll remove this line.

   }
 
   OpenChannelParams* params = new OpenChannelParams(
-      source, source_tab.Pass(), receiver, receiver_port_id,
+      source, source_tab.Pass(), source_frame_id, receiver, receiver_port_id,
       source_extension_id, target_extension_id, source_url, channel_name,
       include_tls_channel_id);
 
@@ -476,10 +483,14 @@ void MessageService::OpenChannelToTab(
     return;
   }
 
+  // TODO(robwu): What if the channel to a tab was created from an extension
+  // page in a tab? Then source_tab / source_frame_id / source_url could be
+  // meaningful...

[not at google - send to devlin, 2014/11/10 18:36:42]

I suppose there wouldn't be much harm in setting a source tab or frame ID if the
connection does actually come from a tab, just not a whole lot of point.

This is a good comment to include, though I'd phrase it more strongly. You could
also write it above the parameter, like

      // We don't set the source tab or frame ID for connections
      // from extensions, regardless of whether those connections
      // are from tabs, because there doesn't seem to be much point.
      scoped_ptr<base::DictionaryValue>(), -1,

though I doubt that git cl format will let you put those on the same line,
unfortunately.

[robwu, 2014/11/10 21:43:07]

It is not just those two properties, but also the URL and TLS channel ID. I'll
keep the comments and formatting as is and remove my TODO.

   scoped_ptr<OpenChannelParams> params(new OpenChannelParams(
         source,
         scoped_ptr<base::DictionaryValue>(),  // Source tab doesn't make sense
                                               // for opening to tabs.
+        -1,  // If there is no tab, then there is no frame either.
         receiver.release(),
         receiver_port_id,
         extension_id,
@@ -522,6 +533,7 @@ bool MessageService::OpenChannelImpl(scoped_ptr<OpenChannelParams> params) {
   channel->receiver->DispatchOnConnect(params->receiver_port_id,
                                        params->channel_name,
                                        params->source_tab,
+                                       params->source_frame_id,
                                        params->source_extension_id,
                                        params->target_extension_id,
                                        params->source_url,