Panos Astithas (he/him) [:past] (please ni?) Reporter
	Description • 13 years ago

As discussed in bug 659910, when multiple Scratchpad windows are open and some of them have web console output, clicking on that output link will focus on the most recent Scratchpad. Rob Campbell suggested that we could make the Scratchpad "URI" correspond to a unique window. Maybe "Scratchpad/n:lineno"?

	Anton Kovalyov (:anton) Assignee
	Comment 1 • 12 years ago

Attached is my proposed patch. Asking for feedback regarding some decisions I made:

1) I'm not sure I like the fact that I had to add Scratchpad specific code into the WebConsoleUtils.abbreviateSourceURL method. Is there a way to avoid that?

2) While testing this patch I noticed that web console combines similar messages even when they originate from different sources. So if you have two different Scratchpad windows printing the same string into the web console, messages will look like they came from the same window. This makes it impossible for us to focus correctly.

To fix that, I added one addition rule to the filterRepeatedConsole: check if messages came from the same source. In my opinion, this new behavior is in general more intuitive. If two separate scripts are printing the same message expect to see them on separate lines and not combined. What do you think?

Thanks!

	Anton Kovalyov (:anton) Assignee
	Comment 2 • 12 years ago

Adding a screenshot showing my patch in action (including new message repeat logic).

	Mihai Sucan [:msucan]
	Comment 3 • 12 years ago

Comment on attachment 660651 [details] [diff] [review]
Proposed patch

Review of attachment 660651 [details] [diff] [review]:
-----------------------------------------------------------------

This patch looks good. I'm giving it an r+ with the comments below addressed.

Please also add a test that actually checks that if you have two scratchpads open and you click a message in the Web Console, the correct one is focused - such that in the future we can avoid regressions.

Thank you!

::: browser/devtools/scratchpad/scratchpad.js
@@ +1116,5 @@
> +    switch (false) {
> +      case "arguments" in window:
> +      case window.arguments[0] instanceof Ci.nsIDialogParamBlock:
> +        Cu.reportError("ERR TODO");
> +    }

I find this switch block confusing. Can you please change this into an if condition? And please provide a meaningful error message. ;)

::: browser/devtools/webconsole/WebConsoleUtils.jsm
@@ +174,5 @@
>    {
> +		// Return Scratchpad URLs with no modifications
> +		if (/^Scratchpad\/\d+$/.test(aSourceURL)) {
> +			return aSourceURL;
> +		}

Indentation is wrong here. Please use two spaces.

You can move this check into webconsole.js, in WCF_createLocationNode(). Just avoid calling abbreviateSourceURl() for Scratchpad URLs.

	Mihai Sucan [:msucan]
	Comment 4 • 12 years ago

(In reply to Anton Kovalyov (:anton) from comment #1)
> Created attachment 660651 [details] [diff] [review]
> Proposed patch
> 
> Attached is my proposed patch. Asking for feedback regarding some decisions
> I made:
> 
> 1) I'm not sure I like the fact that I had to add Scratchpad specific code
> into the WebConsoleUtils.abbreviateSourceURL method. Is there a way to avoid
> that?

True. You can do this in WCF_createLocationNode(), in webconsole.js.

> 2) While testing this patch I noticed that web console combines similar
> messages even when they originate from different sources. So if you have two
> different Scratchpad windows printing the same string into the web console,
> messages will look like they came from the same window. This makes it
> impossible for us to focus correctly.
> 
> To fix that, I added one addition rule to the filterRepeatedConsole: check
> if messages came from the same source. In my opinion, this new behavior is
> in general more intuitive. If two separate scripts are printing the same
> message expect to see them on separate lines and not combined. What do you
> think?

Sounds reasonable for me. Thank you!

	Mihai Sucan [:msucan]
	Comment 5 • 12 years ago

Comment on attachment 660651 [details] [diff] [review]
Proposed patch

Review of attachment 660651 [details] [diff] [review]:
-----------------------------------------------------------------

Just had an idea...

(As you probably noticed web console output code is confusing (to say the least :) ). We've started work in bug 778766 for cleaning it all up.)

::: browser/devtools/webconsole/webconsole.js
@@ +964,5 @@
> +        // childNodes[2] is the description element
> +        if (nodes[2].textContent === lastNodes[2].textContent) {
> +          // childNodes[4] is the location element  
> +          let loc = nodes[4] && nodes[4].getAttribute("value");
> +          let lastLoc = lastNodes[4] && lastNodes[4].getAttribute("value");

To make this a bit better you can find the location node by doing aNode.querySelector(".webconsole-location"), and lastMessage.querySelector(".webconsole-location").

	Mihai Sucan [:msucan]
	Comment 6 • 12 years ago

Comment on attachment 660651 [details] [diff] [review]
Proposed patch

Been playing with this patch more. It works as a user, but I see some test failures. Copying them here, for reference:

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/scratchpad/test/browser_scratchpad_bug690552_display_outputs_errors.js | error display output - Got throw new Error("Ouch!")
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/scratchpad/test/browser_scratchpad_bug690552_display_outputs_errors.js | error display output - Got throw new Error("Ouch!")
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/scratchpad/test/browser_scratchpad_bug756681_display_non_error_exceptions.js | error display output - Got throw new Error("Ouch!")
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/scratchpad/test/browser_scratchpad_bug756681_display_non_error_exceptions.js | error run output - Got throw new Error("Ouch!")

I also see:

TEST-INFO | chrome://mochitests/content/browser/browser/devtools/scratchpad/test/browser_scratchpad_restore.js | Console message: [JavaScript Error: "TypeError: this.editor is null" {file: "chrome://browser/content/scratchpad.js" line: 136}]

Not sure if this is new or not. It seems getText() should return "" if !this.editor.

Changing from f+ to r+, due to the test failures, but I hope they'll be easy to fix. Please let me know if you need any help. Thanks!

(sorry for the bugspam!)

	Anton Kovalyov (:anton) Assignee
	Comment 7 • 12 years ago

Putting the patch out for the review. I cleaned up the code per Mihai's comments and wrote a test case.

	Panos Astithas (he/him) [:past] (please ni?) Reporter
	Comment 8 • 12 years ago

Wouldn't it make sense to display the unique name in the Scratchpad title bar? People who have long coding sessions with multiple scratchpads may start to get confused after a while.

	Girish Sharma [:Optimizer]
	Comment 9 • 12 years ago

(In reply to Panos Astithas [:past] from comment #8)
> Wouldn't it make sense to display the unique name in the Scratchpad title
> bar? People who have long coding sessions with multiple scratchpads may
> start to get confused after a while.

That might be an awesome idea, most of the advanced editors show a new name for new instance/file created. Notepad++ shows "new 0", "new 1" etc. Eclipse also follows the same. Scratchpad should also show a "Scratchpad - New Window x" on the title bar. If a user opens another file (via Open File, or from recent list) or saves the file, then x does not gets incremented, rather gets reverted to last x if that Scratchpad window is the most recent one (highest x). x only increments if a new instance of Scratchpad is opened.

Overall a sample user flow could be :
1. First ever Scratchpad window : "Scratchpad - New Window 1", x = 1
  1.a. User saves the file after typing some code: x reverts back to 0 
    1.a.i. User opens a Scratchpad again: x increments to 1 ... and so on.
  1.b. User opens a new Scratchpad window: x increments to 2.
    1.b.i. User saves the second scratchpad: x reverts to 1.
      1.b.i.A. User opens a new scratchpad: x becomes 2 again.
    1.b.ii. User saves the first scratchpad: x remains 2.
      1.b.ii.A. User opens a new scratchpad: x becomes 3.

I hope I made sense there.

This should call for a new bug itself.

	Mihai Sucan [:msucan]
	Comment 10 • 12 years ago

Comment on attachment 661086 [details] [diff] [review]
Cleaned up code, wrote tests

Review of attachment 661086 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for your update!

General comments:

- This patch has trailing whitespaces. Please remove them.

- The new test fails on my machine (Ubuntu 12.04):

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/scratchpad/test/browser_scratchpad_bug_661762_wrong_window_focus.js | there are active Scratchpad windows
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/scratchpad/test/browser_scratchpad_bug_661762_wrong_window_focus.js | Test timed out
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/scratchpad/test/browser_scratchpad_bug_661762_wrong_window_focus.js | Found a devtools:scratchpad after previous test timed out

Please fix the test, then the patch is pretty much ready. Below I add comments for small things.

Thank you!

::: browser/devtools/scratchpad/test/browser_scratchpad_bug_661762_wrong_window_focus.js
@@ +11,5 @@
> +  waitForExplicitFinish();
> +
> +  // To test for this bug we open a Scratchpad window, save its
> +  // reference and then open another one. This way the first window
> +  // goes loses its focus.

s/goes//

@@ +30,5 @@
> +      let sw = gScratchpadWindow;
> +      
> +      openScratchpad(function () {
> +        function onWebConsoleOpen(subj, topic) {
> +          if (topic !== "web-console-created") {

Is this needed? The observer should be called only for the topic you registered it for.

@@ +51,5 @@
> +      });
> +    });
> +  }, true);
> +
> +  content.location = "data:text/html,<p>test window focus for Scratchpad.";

Please add ;charset=utf8, otherwise we always get a warning about missing charset.

@@ +58,5 @@
> +function testFocus(sw, hud) {
> +  let sp = sw.Scratchpad;
> +
> +  function onMessage(subj, topic) {
> +    if (topic !== "web-console-message-created") {

Same concern as mentioned above.

@@ +70,5 @@
> +    is(loc.value, sw.Scratchpad.uniqueName + ":1",
> +        "location value is correct");
> +
> +    // Simulate a click on the "Scratchpad/N:1" link.
> +    EventUtils.sendMouseEvent({ type: "click" }, loc, gBrowser.contentWindow);

Can you use EventUtils.synthesizeMouse() here?

@@ +77,5 @@
> +      getZOrderDOMWindowEnumerator("devtools:scratchpad", true);
> +
> +    ok(wins.hasMoreElements(), "there are active Scratchpad windows");
> +    is(wins.getNext().Scratchpad.uniqueName, sw.Scratchpad.uniqueName,
> +        "correct window is in focus");

Maybe the fix for the test failure is to not assume that the click event is sync and that focus is sync. Before making the click, try adding a focus event listener to the scratchpad window.

::: browser/devtools/webconsole/webconsole.js
@@ +962,5 @@
> +
> +    let body = aNode.querySelector(".webconsole-msg-body");
> +    let lastBody = lastMessage.querySelector(".webconsole-msg-body");
> +
> +    if (aNode.classList.contains("webconsole-msg-inspector")) {

Why not add this condition to the previous if block?

@@ +966,5 @@
> +    if (aNode.classList.contains("webconsole-msg-inspector")) {
> +      return false;
> +    }
> +
> +    if (body.textContent === lastBody.textContent) {

Nit: generally we don't use strict comparisons unless needed.

@@ +971,5 @@
> +      let loc = aNode.querySelector(".webconsole-location");
> +      let lastLoc = lastMessage.querySelector(".webconsole-location");
> +
> +      if (loc && lastLoc) {
> +        if (loc.getAttribute("value") !== lastLoc.getAttribute("value")) {

Why not combine these two ifs?

@@ +2168,5 @@
> +    let text;
> +
> +    if (/^Scratchpad\/\d+$/.test(aSourceURL)) {
> +      text = aSourceURL;
> +    } else {

Nit: please follow file coding style.

if (foo) {
  bar();
}
else {
  baz();
}

	Anton Kovalyov (:anton) Assignee
	Comment 11 • 12 years ago

Thanks! Going to work on trying to reproduce and fix this test failure today.

(In reply to Mihai Sucan [:msucan] from comment #10)
> 
> @@ +30,5 @@
> > +      let sw = gScratchpadWindow;
> > +      
> > +      openScratchpad(function () {
> > +        function onWebConsoleOpen(subj, topic) {
> > +          if (topic !== "web-console-created") {
> 
> Is this needed? The observer should be called only for the topic you
> registered it for.

I am not sure. I copied the logic from browser/devtools/webconsole/test/head.js.

> 
> @@ +70,5 @@
> > +    is(loc.value, sw.Scratchpad.uniqueName + ":1",
> > +        "location value is correct");
> > +
> > +    // Simulate a click on the "Scratchpad/N:1" link.
> > +    EventUtils.sendMouseEvent({ type: "click" }, loc, gBrowser.contentWindow);
> 
> Can you use EventUtils.synthesizeMouse() here?

Sure. I took EventUtils.sendMouseEvent from one of the test files—is there any documentation for EventUtils? (I couldn't find)

> @@ +77,5 @@
> > +      getZOrderDOMWindowEnumerator("devtools:scratchpad", true);
> > +
> > +    ok(wins.hasMoreElements(), "there are active Scratchpad windows");
> > +    is(wins.getNext().Scratchpad.uniqueName, sw.Scratchpad.uniqueName,
> > +        "correct window is in focus");
> 
> Maybe the fix for the test failure is to not assume that the click event is
> sync and that focus is sync. Before making the click, try adding a focus
> event listener to the scratchpad window.

I think I started with adding a focus event listener but it didn't work. That said, it was before other fixes so I'll try that approach again.

> 
> ::: browser/devtools/webconsole/webconsole.js
> @@ +962,5 @@
> > +
> > +    let body = aNode.querySelector(".webconsole-msg-body");
> > +    let lastBody = lastMessage.querySelector(".webconsole-msg-body");
> > +
> > +    if (aNode.classList.contains("webconsole-msg-inspector")) {
> 
> Why not add this condition to the previous if block?
> 
> @@ +971,5 @@
> > +      let loc = aNode.querySelector(".webconsole-location");
> > +      let lastLoc = lastMessage.querySelector(".webconsole-location");
> > +
> > +      if (loc && lastLoc) {
> > +        if (loc.getAttribute("value") !== lastLoc.getAttribute("value")) {
> 
> Why not combine these two ifs?

Old habit of mine—I prefer to break if's when they have long multi-line conditions in them. I can change it, if you want, it's just a matter of personal style.

	Mihai Sucan [:msucan]
	Comment 12 • 12 years ago

(In reply to Anton Kovalyov (:anton) from comment #11)
> Thanks! Going to work on trying to reproduce and fix this test failure today.
> 
> (In reply to Mihai Sucan [:msucan] from comment #10)
> > @@ +30,5 @@
> > > +      let sw = gScratchpadWindow;
> > > +      
> > > +      openScratchpad(function () {
> > > +        function onWebConsoleOpen(subj, topic) {
> > > +          if (topic !== "web-console-created") {
> > 
> > Is this needed? The observer should be called only for the topic you
> > registered it for.
> 
> I am not sure. I copied the logic from
> browser/devtools/webconsole/test/head.js.

Yeah, we have the copy/paste issue in our codebase, sometimes. ;) I think you don't need the topic check if you register one handler for one topic.


> > @@ +70,5 @@
> > > +    is(loc.value, sw.Scratchpad.uniqueName + ":1",
> > > +        "location value is correct");
> > > +
> > > +    // Simulate a click on the "Scratchpad/N:1" link.
> > > +    EventUtils.sendMouseEvent({ type: "click" }, loc, gBrowser.contentWindow);
> > 
> > Can you use EventUtils.synthesizeMouse() here?
> 
> Sure. I took EventUtils.sendMouseEvent from one of the test files—is there
> any documentation for EventUtils? (I couldn't find)

No docs, but you can search mxr:

http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/SimpleTest/EventUtils.js#175


> > ::: browser/devtools/webconsole/webconsole.js
> > @@ +962,5 @@
> > > +
> > > +    let body = aNode.querySelector(".webconsole-msg-body");
> > > +    let lastBody = lastMessage.querySelector(".webconsole-msg-body");
> > > +
> > > +    if (aNode.classList.contains("webconsole-msg-inspector")) {
> > 
> > Why not add this condition to the previous if block?
> > 
> > @@ +971,5 @@
> > > +      let loc = aNode.querySelector(".webconsole-location");
> > > +      let lastLoc = lastMessage.querySelector(".webconsole-location");
> > > +
> > > +      if (loc && lastLoc) {
> > > +        if (loc.getAttribute("value") !== lastLoc.getAttribute("value")) {
> > 
> > Why not combine these two ifs?
> 
> Old habit of mine—I prefer to break if's when they have long multi-line
> conditions in them. I can change it, if you want, it's just a matter of
> personal style.

Yeah, it's not important. You can leave it either way.

	Anton Kovalyov (:anton) Assignee
	Comment 13 • 12 years ago

Made changes requested by Mihai. (That said, I didn't find an Ubuntu box to test it yet)

By the way, what is the difference between EventUtils.sendMouseEvent and synthesizeMouse?

	Mihai Sucan [:msucan]
	Comment 14 • 12 years ago

(In reply to Anton Kovalyov (:anton) from comment #13)
> Created attachment 661342 [details] [diff] [review]
> Changes requested by Mihai
> 
> Made changes requested by Mihai. (That said, I didn't find an Ubuntu box to
> test it yet)

Thank you! I will test when I get to review the patch.

> By the way, what is the difference between EventUtils.sendMouseEvent and
> synthesizeMouse?

synthesizeMouse() does it at a lower level in Gecko. sendMouseEvent() only does DOM events.

	Anton Kovalyov (:anton) Assignee
	Comment 15 • 12 years ago

Comment on attachment 661342 [details] [diff] [review]
Changes requested by Mihai

Heather pushed my patch to try for me and I can see now that this test still fails on Linux. :-(

	Anton Kovalyov (:anton) Assignee
	Comment 16 • 12 years ago

Update on this bug: I switched to another bug (638949) while I'm waiting on a non-Mac computer to debug test failures on Linux.

	Anton Kovalyov (:anton) Assignee
	Comment 17 • 12 years ago

Finally got my Linux box. The problem was with |getZOrderDOMWindowEnumerator| being broken on Linux (bug 462222). I changed the test to use |getMostRecentWindow| and now all Scratchpad tests pass both on Mac and Linux.

	Mihai Sucan [:msucan]
	Comment 18 • 12 years ago

Comment on attachment 669370 [details] [diff] [review]
Focus the correct Scratchpad window

Review of attachment 669370 [details] [diff] [review]:
-----------------------------------------------------------------

Patch looks good. I like how the test is structured.

The patch does not apply - there's a failure in webconsole.js. I was not able to test. Please rebase and I will retest. Also, please push to the try servers. Thanks!

	Anton Kovalyov (:anton) Assignee
	Comment 19 • 12 years ago

Patch applied cleanly for me. :-\ Pushed it to the try servers here: https://tbpl.mozilla.org/?tree=Try&rev=c3e234062082

	Mihai Sucan [:msucan]
	Comment 20 • 12 years ago

(In reply to Anton Kovalyov (:anton) from comment #19)
> Patch applied cleanly for me. :-\ Pushed it to the try servers here:
> https://tbpl.mozilla.org/?tree=Try&rev=c3e234062082

Thanks for the try push. I see an orange on Mac OS X:

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/webconsole/test/browser_webconsole_bug_588730_text_node_insertion.js | an unexpected uncaught JS exception reported through window.onerror - TypeError: lastBody is null at chrome://browser/content/devtools/webconsole.js:986

	Anton Kovalyov (:anton) Assignee
	Comment 21 • 12 years ago

Yeah I saw that orange yesterday—it doesn't seem related to my change but I'll investigate today.

	Anton Kovalyov (:anton) Assignee
	Comment 22 • 12 years ago

Rebased with the latest trunk and submitted again to the try server: https://tbpl.mozilla.org/?tree=Try&rev=a952af97ac4c

	Anton Kovalyov (:anton) Assignee
	Comment 23 • 12 years ago

Hm, it still fails on OSX debug (even though it passed locally). I will try to kill $OBJDIR tomorrow and see if I can reproduce this.

	Anton Kovalyov (:anton) Assignee
	Comment 24 • 12 years ago

Very minor change to the patch to fix try failures. New try build: https://tbpl.mozilla.org/?tree=Try&rev=f96858219740

Carrying over r+ since the page is very minor.

	Anton Kovalyov (:anton) Assignee
	Comment 25 • 12 years ago

Marking it as [land-in-fx-team] since the try build above succeeded (all oranges are known and unrelated).

	Mozilla RelEng Bot
	Comment 26 • 12 years ago

Try run for f96858219740 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=f96858219740
Results (out of 108 total builds):
    success: 100
    warnings: 8
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/akovalyov@mozilla.com-f96858219740

	Mihai Sucan [:msucan]
	Comment 27 • 12 years ago

Landed:
https://hg.mozilla.org/integration/fx-team/rev/f36f62550c6e

Thank you Anton!

	Ryan VanderMeulen [:RyanVM]
	Comment 28 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/f36f62550c6e