Add frameId to MessageSender (extension messaging API)

chrome/test/data/extensions/api_test/messaging/connect/test.js

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 var listenOnce = chrome.test.listenOnce;
 var listenForever = chrome.test.listenForever;
 
 JSON.parse = function() {
   return "JSON.parse clobbered by extension.";
 };
@@ skipping 52 matching lines @@
         chrome.test.assertEq(msg.portName, portName);
         port.disconnect();
       });
     },
 
     // Tests that postMessage from the tab and its response works.
     function postMessageFromTab() {
       listenOnce(chrome.runtime.onConnect, function(port) {
         chrome.test.assertEq({
           tab: testTab,
+          frameId: 0, // Main frame
           url: testTab.url,
            id: chrome.runtime.id
         }, port.sender);
         listenOnce(port.onMessage, function(msg) {
           chrome.test.assertTrue(msg.testPostMessageFromTab);
           port.postMessage({success: true, portName: port.name});
           chrome.test.log("postMessageFromTab: got message from tab");
         });
       });
 
       var port = chrome.tabs.connect(testTab.id);
       port.postMessage({testPostMessageFromTab: true});
       chrome.test.log("postMessageFromTab: sent first message to tab");
       listenOnce(port.onMessage, function(msg) {
         port.disconnect();
       });
     },
 
     // Tests receiving a request from a content script and responding.
     function sendMessageFromTab() {
       var doneListening = listenForever(
         chrome.runtime.onMessage,
         function(request, sender, sendResponse) {
           chrome.test.assertEq({
             tab: testTab,
+            frameId: 0, // Main frame
             url: testTab.url,
              id: chrome.runtime.id
           }, sender);
           if (request.step == 1) {
             // Step 1: Page should send another request for step 2.
             chrome.test.log("sendMessageFromTab: got step 1");
             sendResponse({nextStep: true});
           } else {
             // Step 2.
             chrome.test.assertEq(request.step, 2);
             sendResponse();
             doneListening();
           }
       });
 
       var port = chrome.tabs.connect(testTab.id);
       port.postMessage({testSendMessageFromTab: true});
       port.disconnect();
       chrome.test.log("sendMessageFromTab: sent first message to tab");
     },
 
+    // Tests that a message from a child frame has a correct frameId.
+    function sendMessageFromFrameInTab() {
+      var senderFrameUrl = {};
+      listenForever(
+        chrome.runtime.onMessage,
+        function(request, sender, sendResponse) {
+          // Child frames have a positive frameId.
+          chrome.test.assertTrue(sender.frameId >= 1,
+              'frameId must be positive, but it is ' + sender.frameId);
+          chrome.test.assertFalse(sender.frameId in messageSenders,
+              'frameId ' + sender.frameId + ' was already seen before!');
+          senderFrameUrl[sender.frameId] = request.frameUrl;
+          delete sender.frameId;
+          chrome.test.assertEq({

[not at google - send to devlin, 2014/11/11 22:23:00]

Test is better now, but I think you can make it even clearer. I just don't like
this "delete sender.frameId" part, and the "frameId >= 1" assertion seems
redundant if your assertions are useful (like I mentioned).

Basically:

1. Remove lines 127-131 and 134-138 (leaving just 132).
2. Rename senderFrameUrl to "senders" and just make it a list of senders, then
line 132 becomes just appending the sender to it.
3. When you're done and you fetch the frames, rather than iterating over the
details, transform it into a list of what the senders *should* have been. Call
it expectedSenders or something.
4. Sort each list so you can do just assertEq(expectedSenders, senders).

Does that make sense?

Sorry to keep on banging on about the test, but I'd like this to be as solid as
possible.

[robwu, 2014/11/11 22:44:38]

The list of frames *could* potentially contain more frames (due to the way
navigations are handled). I've applied your changes for now, but if the test
fail or become flaky, then I'll update the test to explicitly filter frames.
Yes. Done.
No problem, I'm not offended at all.

+            tab: testTab,
+            url: request.frameUrl,
+            id: chrome.runtime.id
+          }, sender);
+
+          if (Object.keys(senderFrameUrl).length == 2) {
+            // Now test whether the frameId that we got back makes sense...
+            chrome.webNavigation.getAllFrames({
+              tabId: testTab.id
+            }, function(details) {
+              var framesFound = 0;
+              details.forEach(function(frame) {
+                var url = senderFrameUrl[frame.frameId];
+                if (url) {
+                  chrome.test.assertEq(frame.url, url);
+                  ++framesFound;
+                }
+              });
+              chrome.test.assertEq(framesFound, 2);
+              doneListening();
+            });
+          }
+        }
+      );
+
+      var port = chrome.tabs.connect(testTab.id);
+      port.postMessage({testSendMessageFromFrame: true});
+      port.disconnect();
+      chrome.test.log("sendMessageFromFrameInTab: send 1st message to tab");
+    },
+
     // Tests error handling when sending a request from a content script to an
     // invalid extension.
     function sendMessageFromTabError() {
       listenOnce(
         chrome.runtime.onMessage,
         function(request, sender, sendResponse) {
           if (!request.success)
             chrome.test.fail();
         }
       );
@@ skipping 76 matching lines @@
       } catch(e) {
         error = e;
       }
       chrome.test.assertTrue(error != undefined);
 
       chrome.test.succeed();
     },
 
   ]);
 });