Bug 661762 - Scratchpad source link may focus the wrong Scratchpad window; r=msucan

browser/devtools/scratchpad/scratchpad-manager.jsm

@@ -23,6 +23,7 @@ Cu.import("resource://gre/modules/Services.jsm");
  */
 var ScratchpadManager = {
 
+  _nextUid: 1,
   _scratchpads: [],
 
   /**
@@ -89,16 +90,20 @@ var ScratchpadManager = {
    */
   openScratchpad: function SPM_openScratchpad(aState)
   {
-    let params = null;
+    let params = Cc["@mozilla.org/embedcomp/dialogparam;1"]
+                 .createInstance(Ci.nsIDialogParamBlock);
+
+    params.SetNumberStrings(2);
+    params.SetString(0, JSON.stringify(this._nextUid++));
+
     if (aState) {
       if (typeof aState != 'object') {
         return;
       }
-      params = Cc["@mozilla.org/embedcomp/dialogparam;1"]
-               .createInstance(Ci.nsIDialogParamBlock);
-      params.SetNumberStrings(1);
-      params.SetString(0, JSON.stringify(aState));
+
+      params.SetString(1, JSON.stringify(aState));
     }
+
     let win = Services.ww.openWindow(null, SCRATCHPAD_WINDOW_URL, "_blank",
                                      SCRATCHPAD_WINDOW_FEATURES, params);
     // Only add the shutdown observer if we've opened a scratchpad window.

browser/devtools/scratchpad/scratchpad.js

@@ -41,6 +41,7 @@ const BUTTON_POSITION_REVERT=0;
  * The scratchpad object handles the Scratchpad window functionality.
  */
 var Scratchpad = {
+  _instanceId: null,
   _initialWindowTitle: document.title,
 
   /**
@@ -204,6 +205,15 @@ var Scratchpad = {
    */
   _contentSandbox: null,
 
+  /**
+   * Unique name for the current Scratchpad instance. Used to distinguish
+   * Scratchpad windows between each other. See bug 661762.
+   */
+  get uniqueName()
+  {
+    return "Scratchpad/" + this._instanceId;
+  },
+
   /**
    * Get the Cu.Sandbox object for the active tab content window object. Note
    * that the returned object is cached for later reuse. The cached object is
@@ -317,7 +327,7 @@ var Scratchpad = {
     let error, result;
     try {
       result = Cu.evalInSandbox(aString, this.contentSandbox, "1.8",
-                                "Scratchpad", 1);
+                                this.uniqueName, 1);
     }
     catch (ex) {
       error = ex;
@@ -339,7 +349,7 @@ var Scratchpad = {
     let error, result;
     try {
       result = Cu.evalInSandbox(aString, this.chromeSandbox, "1.8",
-                                "Scratchpad", 1);
+                                this.uniqueName, 1);
     }
     catch (ex) {
       error = ex;
@@ -1111,6 +1121,7 @@ var Scratchpad = {
     if (aEvent.target != document) {
       return;
     }
+
     let chrome = Services.prefs.getBoolPref(DEVTOOLS_CHROME_ENABLED);
     if (chrome) {
       let environmentMenu = document.getElementById("sp-environment-menu");
@@ -1121,8 +1132,6 @@ var Scratchpad = {
       errorConsoleCommand.removeAttribute("disabled");
     }
 
-    let state = null;
-
     let initialText = this.strings.formatStringFromName(
       "scratchpadIntro1",
       [LayoutHelpers.prettyKey(document.getElementById("sp-key-run")),
@@ -1130,9 +1139,21 @@ var Scratchpad = {
        LayoutHelpers.prettyKey(document.getElementById("sp-key-display"))],
       3);
 
-    if ("arguments" in window &&
-         window.arguments[0] instanceof Ci.nsIDialogParamBlock) {
-      state = JSON.parse(window.arguments[0].GetString(0));
+    let args = window.arguments;
+
+    if (args && args[0] instanceof Ci.nsIDialogParamBlock) {
+      args = args[0];
+    } else {
+      // If this Scratchpad window doesn't have any arguments, horrible
+      // things might happen so we need to report an error.
+      Cu.reportError(this.strings. GetStringFromName("scratchpad.noargs"));
+    }
+
+    this._instanceId = args.GetString(0);
+
+    let state = args.GetString(1) || null;
+    if (state) {
+      state = JSON.parse(state);
       this.setState(state);
       initialText = state.text;
     }

browser/devtools/scratchpad/test/Makefile.in

@@ -35,6 +35,7 @@ MOCHITEST_BROWSER_FILES = \
 		browser_scratchpad_bug756681_display_non_error_exceptions.js \
 		browser_scratchpad_bug_751744_revert_to_saved.js \
 		browser_scratchpad_bug740948_reload_and_run.js \
+		browser_scratchpad_bug_661762_wrong_window_focus.js \
 		head.js \
 
 include $(topsrcdir)/config/rules.mk

browser/devtools/scratchpad/test/browser_scratchpad_bug690552_display_outputs_errors.js

@@ -36,7 +36,8 @@ function runTests()
   scratchpad.setText(error);
   scratchpad.display();
   is(scratchpad.getText(), 
-      error + openComment + "Exception: Ouch!\n@Scratchpad:1" + closeComment,
+      error + openComment +
+      "Exception: Ouch!\n@" + scratchpad.uniqueName + ":1" + closeComment,
       "error display output");
 
   scratchpad.setText(message);
@@ -46,7 +47,8 @@ function runTests()
   scratchpad.setText(error);
   scratchpad.run();
   is(scratchpad.getText(), 
-      error + openComment + "Exception: Ouch!\n@Scratchpad:1" + closeComment,
+      error + openComment +
+      "Exception: Ouch!\n@" + scratchpad.uniqueName + ":1" + closeComment,
       "error display output");
 
   finish();

browser/devtools/scratchpad/test/browser_scratchpad_bug756681_display_non_error_exceptions.js

@@ -41,7 +41,8 @@ function runTests()
   scratchpad.setText(error1);
   scratchpad.display();
   is(scratchpad.getText(),
-      error1 + openComment + "Exception: Ouch!\n@Scratchpad:1" + closeComment,
+      error1 + openComment +
+      "Exception: Ouch!\n@" + scratchpad.uniqueName + ":1" + closeComment,
       "error display output");
 
   // Display error2, throw "A thrown string"
@@ -75,7 +76,8 @@ function runTests()
   scratchpad.setText(error1);
   scratchpad.run();
   is(scratchpad.getText(),
-      error1 + openComment + "Exception: Ouch!\n@Scratchpad:1" + closeComment,
+      error1 + openComment +
+      "Exception: Ouch!\n@" + scratchpad.uniqueName + ":1" + closeComment,
       "error run output");
 
   // Run error2, throw "A thrown string"

browser/devtools/scratchpad/test/browser_scratchpad_bug_661762_wrong_window_focus.js

@@ -0,0 +1,93 @@
+/* vim:set ts=2 sw=2 sts=2 et: */
+/* Any copyright is dedicated to the Public Domain.
+   http://creativecommons.org/publicdomain/zero/1.0/ */
+
+let tempScope = {};
+Cu.import("resource:///modules/HUDService.jsm", tempScope);
+let HUDService = tempScope.HUDService;
+
+function test()
+{
+  waitForExplicitFinish();
+
+  // To test for this bug we open a Scratchpad window, save its
+  // reference and then open another one. This way the first window
+  // loses its focus.
+  //
+  // Then we open a web console and execute a console.log statement
+  // from the first Scratch window (that's why we needed to save its
+  // reference).
+  //
+  // Then we wait for our message to appear in the console and click
+  // on the location link. After that we check which Scratchpad window
+  // is currently active (it should be the older one).
+
+  gBrowser.selectedTab = gBrowser.addTab();
+  gBrowser.selectedBrowser.addEventListener("load", function onLoad() {
+    gBrowser.selectedBrowser.removeEventListener("load", onLoad, true);
+
+    openScratchpad(function () {
+      let sw = gScratchpadWindow;
+
+      openScratchpad(function () {
+        function onWebConsoleOpen(subj) {
+          Services.obs.removeObserver(onWebConsoleOpen,
+            "web-console-created");
+          subj.QueryInterface(Ci.nsISupportsString);
+
+          let hud = HUDService.getHudReferenceById(subj.data);
+          hud.jsterm.clearOutput(true);
+          executeSoon(testFocus.bind(null, sw, hud));
+        }
+
+        Services.obs.
+          addObserver(onWebConsoleOpen, "web-console-created", false);
+
+        HUDService.consoleUI.toggleHUD();
+      });
+    });
+  }, true);
+
+  content.location = "data:text/html;charset=utf8,<p>test window focus for Scratchpad.";
+}
+
+function testFocus(sw, hud) {
+  let sp = sw.Scratchpad;
+
+  function onMessage(subj) {
+    Services.obs.removeObserver(onMessage, "web-console-message-created");
+
+    var loc = hud.jsterm.outputNode.querySelector(".webconsole-location");
+    ok(loc, "location element exists");
+    is(loc.value, sw.Scratchpad.uniqueName + ":1",
+        "location value is correct");
+
+    sw.addEventListener("focus", function onFocus() {
+      sw.removeEventListener("focus", onFocus, true);
+
+      let win = Services.wm.getMostRecentWindow("devtools:scratchpad");
+
+      ok(win, "there are active Scratchpad windows");
+      is(win.Scratchpad.uniqueName, sw.Scratchpad.uniqueName,
+          "correct window is in focus");
+
+      // gScratchpadWindow will be closed automatically but we need to
+      // close the second window ourselves.
+      sw.close();
+      finish();
+    }, true);
+
+    // Simulate a click on the "Scratchpad/N:1" link.
+    EventUtils.synthesizeMouse(loc, 2, 2, {}, hud.iframeWindow);
+  }
+
+  // Sending messages to web console is an asynchronous operation. That's
+  // why we have to setup an observer here.
+  Services.obs.addObserver(onMessage, "web-console-message-created", false);
+
+  sp.setText("console.log('foo');");
+  let [selection, error, result] = sp.run();
+  is(selection, "console.log('foo');", "selection is correct");
+  is(error, undefined, "error is correct");
+  is(result, undefined, "result is correct");
+}

browser/devtools/scratchpad/test/browser_scratchpad_open.js

@@ -5,6 +5,7 @@
 // only finish() when correct number of tests are done
 const expected = 3;
 var count = 0;
+var lastUniqueName = null;
 
 function done()
 {
@@ -21,6 +22,19 @@ function test()
   testOpenInvalidState();
 }
 
+function testUniqueName(name)
+{
+  ok(name, "Scratchpad has a uniqueName");
+
+  if (lastUniqueName === null) {
+    lastUniqueName = name;
+    return;
+  }
+
+  ok(name !== lastUniqueName,
+      "Unique name for this instance differs from the last one.");
+}
+
 function testOpen()
 {
   openScratchpad(function(win) {
@@ -28,6 +42,7 @@ function testOpen()
     isnot(win.Scratchpad.getText(), null, "Default text should not be null");
     is(win.Scratchpad.executionContext, win.SCRATCHPAD_CONTEXT_CONTENT,
       "Default execution context is content");
+    testUniqueName(win.Scratchpad.uniqueName);
 
     win.close();
     done();
@@ -46,6 +61,7 @@ function testOpenWithState()
     is(win.Scratchpad.filename, state.filename, "Filename loaded from state");
     is(win.Scratchpad.executionContext, state.executionContext, "Execution context loaded from state");
     is(win.Scratchpad.getText(), state.text, "Content loaded from state");
+    testUniqueName(win.Scratchpad.uniqueName);
 
     win.close();
     done();

browser/devtools/webconsole/webconsole.js

@@ -965,9 +965,9 @@ WebConsoleFrame.prototype = {
 
   /**
    * Filter the console node from the output node if it is a repeat. Console
-   * messages are filtered from the output if and only if they match the
-   * immediately preceding message. The output node's last occurrence should
-   * have its timestamp updated.
+   * messages are filtered from the output if they match the immediately
+   * preceding message that came from the same source. The output node's
+   * last occurrence should have its timestamp updated.
    *
    * @param nsIDOMNode aNode
    *        The message node to be filtered or not.
@@ -978,11 +978,31 @@ WebConsoleFrame.prototype = {
   {
     let lastMessage = this.outputNode.lastChild;
 
-    // childNodes[2] is the description element
-    if (lastMessage && lastMessage.childNodes[2] &&
-        !aNode.classList.contains("webconsole-msg-inspector") &&
-        aNode.childNodes[2].textContent ==
-        lastMessage.childNodes[2].textContent) {
+    if (!lastMessage) {
+      return false;
+    }
+
+    let body = aNode.querySelector(".webconsole-msg-body");
+    let lastBody = lastMessage.querySelector(".webconsole-msg-body");
+
+    if (aNode.classList.contains("webconsole-msg-inspector")) {
+      return false;
+    }
+
+    if (!body || !lastBody) {
+      return false;
+    }
+
+    if (body.textContent == lastBody.textContent) {
+      let loc = aNode.querySelector(".webconsole-location");
+      let lastLoc = lastMessage.querySelector(".webconsole-location");
+
+      if (loc && lastLoc) {
+        if (loc.getAttribute("value") !== lastLoc.getAttribute("value")) {
+          return false;
+        }
+      }
+
       this.mergeFilteredMessageNode(lastMessage, aNode);
       return true;
     }
@@ -2373,11 +2393,20 @@ WebConsoleFrame.prototype = {
     let locationNode = this.document.createElementNS(XUL_NS, "label");
 
     // Create the text, which consists of an abbreviated version of the URL
-    // plus an optional line number.
-    let text = WebConsoleUtils.abbreviateSourceURL(aSourceURL);
+    // plus an optional line number. Scratchpad URLs should not be abbreviated.
+    let text;
+
+    if (/^Scratchpad\/\d+$/.test(aSourceURL)) {
+      text = aSourceURL;
+    }
+    else {
+      text = WebConsoleUtils.abbreviateSourceURL(aSourceURL);
+    }
+
     if (aSourceLine) {
       text += ":" + aSourceLine;
     }
+
     locationNode.setAttribute("value", text);
 
     // Style appropriately.
@@ -2389,10 +2418,16 @@ WebConsoleFrame.prototype = {
 
     // Make the location clickable.
     locationNode.addEventListener("click", function() {
-      if (aSourceURL == "Scratchpad") {
-        let win = Services.wm.getMostRecentWindow("devtools:scratchpad");
-        if (win) {
-          win.focus();
+      if (/^Scratchpad\/\d+$/.test(aSourceURL)) {
+        let wins = Services.wm.getEnumerator("devtools:scratchpad");
+
+        while (wins.hasMoreElements()) {
+          let win = wins.getNext();
+
+          if (win.Scratchpad.uniqueName === aSourceURL) {
+            win.focus();
+            return;
+          }
         }
       }
       else if (locationNode.parentNode.category == CATEGORY_CSS) {

browser/locales/en-US/chrome/browser/devtools/scratchpad.properties

@@ -71,6 +71,11 @@ confirmRevert.title=Revert Changes
 # comment inside /* and */.
 scratchpadIntro1=/*\n * This is a JavaScript Scratchpad.\n *\n * Enter some JavaScript, then Right Click or choose from the Execute Menu:\n * 1. Run to evaluate the selected text (%1$S),\n * 2. Inspect to bring up an Object Inspector on the result (%2$S), or,\n * 3. Display to insert the result in a comment after the selection. (%3$S)\n */\n\n
 
+# LOCALIZATION NOTE  (scratchpad.noargs): This error message is shown when
+# Scratchpad instance is created without any arguments. Scratchpad window
+# expects to receive its unique identifier as the first window argument.
+scratchpad.noargs=Scratchpad was created without any arguments.
+
 # LOCALIZATION NOTE  (notification.browserContext): This is the message displayed
 # over the top of the editor when the user has switched to browser context.
 browserContext.notification=This scratchpad executes in the Browser context.