1005471 - Scratchpad "Jump to line" should preset input value based on current selection, handle LINE:COLUMN as well

Status: RESOLVED FIXED

(DevTools Graveyard :: Scratchpad, defect)

Description

[Adrian Aichner [:anaran]]

Scratchpad Jump to line (Ctrl+J) should preset input.value tp current selection.

This can be put to good use in combination with error comments inserted into scratchpad by Execute menu commands Display, Inspect, Run.

See also Bug 1005469, where I argue for support of the :LINE:CHARACTER syntax.

Comment 1

[Adrian Aichner [:anaran]]

Let me propose this first version of a patch.

I tested it in a local build of firefox under windows xpp using sources of git branch fx-team in my clone of https://git.mozilla.org/integration/gecko-dev.git

I have been informed about https://github.com/mozilla/moz-git-tools but have not looked into that yet.

Is the patch attachment good enough for a reviewer to test it?

Comment 2

[Adrian Aichner [:anaran]]

Attachment: Bug1005471.patch

Produced with
git diff browser/devtools/sourceeditor/ > Bug1005471.patch

Comment 3

[Adrian Aichner [:anaran]]

Attachment: Bug1005471.patch

This patch find LINE:COLUMN in selections inserted by running or pretty-printing code with errors in scratchpad.

e.g. try the two test cases in following scratchpad example code:

// Use Case 1: Run (Ctrl+R), then immediate use Jump to line... (Ctrl+J)
// Use Case 2: Pretty Print (Ctrl+P), then immediate use Jump to line... (Ctrl+J)
var re = /(\d+)?/:?(\d+)?/);

/*
Exception: missing ; before statement
@Scratchpad/2:3
*/
/*
Exception: Expecting Unicode escape sequence \uXXXX (3:21)
raise@resource://gre/modules/devtools/acorn/acorn.js:231:9
readWord1@resource://gre/modules/devtools/acorn/acorn.js:919:11
readWord@resource://gre/modules/devtools/acorn/acorn.js:939:9
readToken@resource://gre/modules/devtools/acorn/acorn.js:729:59
getToken@resource://gre/modules/devtools/acorn/acorn.js:146:7
prettyFast@resource://gre/modules/devtools/pretty-fast.js:770:7
self.onmessage@resource://gre/modules/devtools/server/actors/pretty-print-worker.js:39:7

*/

Comment 4

[Adrian Aichner [:anaran]]

Hi Nick, I would like to "take it and work on it".

Does my patch look OK?

I think it would greatly speed up debugging of syntax errors in scratchpad and eliminate errors selecting/typing the correct lines manually.

Comment 5

[Marco Zehe (:MarcoZ)]

I don't see a problem with that, on the contrary, preset values often help a user get the expected format right.

Comment 6

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8420977 [details] [diff] [review]
Bug1005471.patch

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

This looks pretty good to me, but I think :bgrins should review it, because he is more familiar with the editor than I am. When you have test(s) for this patch, flag him for review.

Comment 7

[Adrian Aichner [:anaran]]

(In reply to Nick Fitzgerald [:fitzgen] from comment #6)
> Comment on attachment 8420977 [details] [diff] [review]
> Bug1005471.patch
> 
> Review of attachment 8420977 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks pretty good to me, but I think :bgrins should review it, because

Ah, I know bgrins from devtools-snippets galore. Hi!

> he is more familiar with the editor than I am. When you have test(s) for
> this patch, flag him for review.

Thanks for the quick feedback! What kind of test would be appropriate here?

A quick look at git.mozilla.org/gecko-dev/browser/devtools/sourceeditor/test/ on the fx-team branch suggests there are no "Jump to line..." tests yet?

I could try to test whether the input value gets set correctly based on selected editor text.

The tests should currently fail, then pass with my patch?

Could you please assign the bug to me?

Comment 8

[Brian Grinstead [:bgrins]]

(In reply to adrian from comment #7)
> (In reply to Nick Fitzgerald [:fitzgen] from comment #6)
> > Comment on attachment 8420977 [details] [diff] [review]
> > Bug1005471.patch
> > 
> > Review of attachment 8420977 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > This looks pretty good to me, but I think :bgrins should review it, because
> 
> Ah, I know bgrins from devtools-snippets galore. Hi!
> 
> > he is more familiar with the editor than I am. When you have test(s) for
> > this patch, flag him for review.
> 
> Thanks for the quick feedback! What kind of test would be appropriate here?
> 
> A quick look at
> git.mozilla.org/gecko-dev/browser/devtools/sourceeditor/test/ on the fx-team
> branch suggests there are no "Jump to line..." tests yet?
> 
> I could try to test whether the input value gets set correctly based on
> selected editor text.
> 
> The tests should currently fail, then pass with my patch?
> 
> Could you please assign the bug to me?

Hi, yes I would add a new test called browser_editor_goto_line.js (and refer to it in the browser.ini file in the test directory).  It would be kind of like browser_editor_cursor.js, but it would test editor.jumpToLine() directly.  It would be great to have a basic test for this jump to line functionality that passes currently, then to also add some assertions that only pass with the rest of the patch applied.

The thing about jumpToLine is that it waits for input before proceeding, so it may be a bit tricky to figure out how to get this to work inside of the test suite.  I would figure out which window/dom element to send keys to  - it will probably look something like this (I doubt this code will work as-is though): 

  let editorWin = win.document.querySelector("iframe").contentWindow;
  editor.jumpToLine();
  EventUtils.synthesizeKey("2", {}, editorWin);
  EventUtils.synthesizeKey("VK_RETURN", {}, editorWin);

I will add a couple of notes about the patch also.

Comment 9

[Brian Grinstead [:bgrins]]

Comment on attachment 8420977 [details] [diff] [review]
Bug1005471.patch

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

This patch changes the permission on editor.js (you can see with old mode / new mode in https://bug1005471.bugzilla.mozilla.org/attachment.cgi?id=8420977).  Please reset the permissions so that isn't part of the patch.

::: browser/devtools/sourceeditor/editor.js
@@ +730,5 @@
>      let txt = doc.createTextNode(L10N.GetStringFromName("gotoLineCmd.promptTitle"));
> +    // Match @Scratchpad/N:LINE[:COLUMN], (LINE[:COLUMN]) anywhere,
> +    // or LINE[:COLUMN] just at begin of text selection.
> +    let reLineSpec = /(?:@Scratchpad\/\d+:|\(|^)(\d+):?(\d+)?/g;
> +    var match, line, column;

You can remove `var match, line, column;` here and instead use `let [match, line, column] = lineSpec;` inside of the if

@@ +731,5 @@
> +    // Match @Scratchpad/N:LINE[:COLUMN], (LINE[:COLUMN]) anywhere,
> +    // or LINE[:COLUMN] just at begin of text selection.
> +    let reLineSpec = /(?:@Scratchpad\/\d+:|\(|^)(\d+):?(\d+)?/g;
> +    var match, line, column;
> +    

Remove the trailing whitespace here

Comment 10

[Adrian Aichner [:anaran]]

(In reply to Brian Grinstead [:bgrins] from comment #9)
> Comment on attachment 8420977 [details] [diff] [review]
> Bug1005471.patch
> 
> Review of attachment 8420977 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This patch changes the permission on editor.js (you can see with old mode /
> new mode in
> https://bug1005471.bugzilla.mozilla.org/attachment.cgi?id=8420977).  Please
> reset the permissions so that isn't part of the patch.
> 
> ::: browser/devtools/sourceeditor/editor.js
> @@ +730,5 @@
> >      let txt = doc.createTextNode(L10N.GetStringFromName("gotoLineCmd.promptTitle"));
> > +    // Match @Scratchpad/N:LINE[:COLUMN], (LINE[:COLUMN]) anywhere,
> > +    // or LINE[:COLUMN] just at begin of text selection.
> > +    let reLineSpec = /(?:@Scratchpad\/\d+:|\(|^)(\d+):?(\d+)?/g;
> > +    var match, line, column;
> 
> You can remove `var match, line, column;` here and instead use `let [match,
> line, column] = lineSpec;` inside of the if
> 
> @@ +731,5 @@
> > +    // Match @Scratchpad/N:LINE[:COLUMN], (LINE[:COLUMN]) anywhere,
> > +    // or LINE[:COLUMN] just at begin of text selection.
> > +    let reLineSpec = /(?:@Scratchpad\/\d+:|\(|^)(\d+):?(\d+)?/g;
> > +    var match, line, column;
> > +    
> 
> Remove the trailing whitespace here

Thanks for the partial review and guidance.

I hope https://github.com/mozilla/gecko-dev/pull/27 addresses your feedback.

Test case is next on my list.

Comment 11

[Brian Grinstead [:bgrins]]

(In reply to adrian from comment #10)
> (In reply to Brian Grinstead [:bgrins] from comment #9)
> > Comment on attachment 8420977 [details] [diff] [review]
> > Bug1005471.patch
> > 
> > Review of attachment 8420977 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > This patch changes the permission on editor.js (you can see with old mode /
> > new mode in
> > https://bug1005471.bugzilla.mozilla.org/attachment.cgi?id=8420977).  Please
> > reset the permissions so that isn't part of the patch.
> > 
> > ::: browser/devtools/sourceeditor/editor.js
> > @@ +730,5 @@
> > >      let txt = doc.createTextNode(L10N.GetStringFromName("gotoLineCmd.promptTitle"));
> > > +    // Match @Scratchpad/N:LINE[:COLUMN], (LINE[:COLUMN]) anywhere,
> > > +    // or LINE[:COLUMN] just at begin of text selection.
> > > +    let reLineSpec = /(?:@Scratchpad\/\d+:|\(|^)(\d+):?(\d+)?/g;
> > > +    var match, line, column;
> > 
> > You can remove `var match, line, column;` here and instead use `let [match,
> > line, column] = lineSpec;` inside of the if
> > 
> > @@ +731,5 @@
> > > +    // Match @Scratchpad/N:LINE[:COLUMN], (LINE[:COLUMN]) anywhere,
> > > +    // or LINE[:COLUMN] just at begin of text selection.
> > > +    let reLineSpec = /(?:@Scratchpad\/\d+:|\(|^)(\d+):?(\d+)?/g;
> > > +    var match, line, column;
> > > +    
> > 
> > Remove the trailing whitespace here
> 
> Thanks for the partial review and guidance.
> 
> I hope https://github.com/mozilla/gecko-dev/pull/27 addresses your feedback.
> 
> Test case is next on my list.

We can use this bug for all the discussion as opposed to a PR.  You can generate the patch and upload it here then mark your old one as obsolete.

Comment 12

[Adrian Aichner [:anaran]]

Attachment: 0001-Address-https-bugzilla.mozilla.org-show_bug.cgi-id-1.patch

Comment 13

[Adrian Aichner [:anaran]]

Attachment: 0002-Bug-1005471-Add-test-coverage-passing-prior-to-featu.patch

Please note 
Depends on: 	1009996

The associated proposed patch is
https://bugzilla.mozilla.org/attachment.cgi?id=8422162

I'll attach my passing log as well.

Comment 14

[Adrian Aichner [:anaran]]

Attachment: mochitest-browser_editor_goto_line-20140514T035940+0000.txt

Comment 15

[Adrian Aichner [:anaran]]

I have updated patches, added basic test coverage.

Haven't figured out how to to "Run" and "Pretty Print" programmatically, which would nicely test the new features I added.

Basically I would need to access the Scratchpad object either from the ed or win setup arguments.

Looks like I was the first one to use EventUtils.synthesizeKeyExpectEvent, which I tried to fix along the way, in Bug 1009996.

Comment 16

[Brian Grinstead [:bgrins]]

Comment on attachment 8422184 [details] [diff] [review]
0002-Bug-1005471-Add-test-coverage-passing-prior-to-featu.patch

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

::: browser/devtools/sourceeditor/test/browser_editor_goto_line.js
@@ +10,5 @@
> +  setup((ed, win) => {
> +    info("\ned.getText():\n" + ed.getText());
> +    ed.setText("// line 1\n// line 2\n/a bad /regexp/; // line 3 is an obvious syntax error!\n// line 4\n// line 5\n");
> +    info("\ned.getText():\n" + ed.getText());
> +    waitForFocus(function () {

Cool, so it looks like you figured out how to fill in the dialog.  Could you break this piece out into a function that takes an input string (in this case "4")?  Then add quite a few more assertions to test the "Line:Col" format, and a variety of invalid inputs

@@ +16,5 @@
> +      let editorDoc = ed.container.contentDocument;
> +      let lineInput = editorDoc.querySelector("input");
> +      let four = "4";
> +      let enter = "VK_RETURN";
> +      EventUtils.synthesizeKeyExpectEvent(four, {}, lineInput, "keypress", "Press " + four, win);

Why are you using synthesizeKeyExpectEvent instead of just synthesizeKey here?  It looks like this is adding an extra test to make sure the event happened, which is implied later with your ed.getCursor assertion.

@@ +20,5 @@
> +      EventUtils.synthesizeKeyExpectEvent(four, {}, lineInput, "keypress", "Press " + four, win);
> +      // keypress is never seen, because keydown process the event.
> +      EventUtils.synthesizeKeyExpectEvent(enter, {}, lineInput, "keydown", "Press " + enter, win);
> +    });
> +    waitForFocus(function () {

Ideally, this line and the teardown will be moved into the single waitForFocus callback.  If this causes failures, we may to see if there is an event or promise we could use for jumpToLine() to have a more explicit time when it is OK to proceed after synthesizing the keys.

Comment 17

[Brian Grinstead [:bgrins]]

(In reply to adrian from comment #15)
> I have updated patches, added basic test coverage.
> 
> Haven't figured out how to to "Run" and "Pretty Print" programmatically,
> which would nicely test the new features I added.
> 
> Basically I would need to access the Scratchpad object either from the ed or
> win setup arguments.

With regards to the scratchpad test, I agree that this would be a nice test to add.  But I would keep them separate - use this first test for testing jumpToLine and various inputs directly, then add a new test in the scratchpad suite that tests the scratchpad workflow more directly.  Check out browser_scratchpad_execute_print.js for an example that runs the code from a test.

Comment 18

[Adrian Aichner [:anaran]]

(In reply to Brian Grinstead [:bgrins] from comment #17)
> (In reply to adrian from comment #15)
> > I have updated patches, added basic test coverage.
> > 
> > Haven't figured out how to to "Run" and "Pretty Print" programmatically,
> > which would nicely test the new features I added.
> > 
> > Basically I would need to access the Scratchpad object either from the ed or
> > win setup arguments.
> 
> With regards to the scratchpad test, I agree that this would be a nice test
> to add.  But I would keep them separate - use this first test for testing
> jumpToLine and various inputs directly, then add a new test in the
> scratchpad suite that tests the scratchpad workflow more directly.  Check
> out browser_scratchpad_execute_print.js for an example that runs the code
> from a test.

Sounds good. I will continue working on the tomorrow.

What about setting
inputElement.value directly
and dispatching a DOM_VK_RETURN KeyEvent to it?
instead of using EventUtils?

Comment 19

[Brian Grinstead [:bgrins]]

> What about setting
> inputElement.value directly
> and dispatching a DOM_VK_RETURN KeyEvent to it?
> instead of using EventUtils?

Yeah, that sounds good to me.  That will be easier for the inputs with > 1 character.

Comment 20

[Adrian Aichner [:anaran]]

(In reply to Brian Grinstead [:bgrins] from comment #8)
> (In reply to adrian from comment #7)
> > (In reply to Nick Fitzgerald [:fitzgen] from comment #6)
> > > Comment on attachment 8420977 [details] [diff] [review]
> > > Bug1005471.patch
> > > 
> > > Review of attachment 8420977 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > This looks pretty good to me, but I think :bgrins should review it, because
> > 
> > Ah, I know bgrins from devtools-snippets galore. Hi!
> > 
> > > he is more familiar with the editor than I am. When you have test(s) for
> > > this patch, flag him for review.
> > 
> > Thanks for the quick feedback! What kind of test would be appropriate here?
> > 
> > A quick look at
> > git.mozilla.org/gecko-dev/browser/devtools/sourceeditor/test/ on the fx-team
> > branch suggests there are no "Jump to line..." tests yet?
> > 
> > I could try to test whether the input value gets set correctly based on
> > selected editor text.
> > 
> > The tests should currently fail, then pass with my patch?
> > 
> > Could you please assign the bug to me?
> 
> Hi, yes I would add a new test called browser_editor_goto_line.js (and refer
> to it in the browser.ini file in the test directory).  It would be kind of
> like browser_editor_cursor.js, but it would test editor.jumpToLine()
> directly.  It would be great to have a basic test for this jump to line
> functionality that passes currently, then to also add some assertions that
> only pass with the rest of the patch applied.

I'm unsure about this "add some assertions" thing.

I currently have these implemented as todo_... tests.

Is that what you had in mind?

I'll upload and updated patch soon.

> 
> The thing about jumpToLine is that it waits for input before proceeding, so
> it may be a bit tricky to figure out how to get this to work inside of the
> test suite.  I would figure out which window/dom element to send keys to  -
> it will probably look something like this (I doubt this code will work as-is
> though): 
> 
>   let editorWin = win.document.querySelector("iframe").contentWindow;
>   editor.jumpToLine();
>   EventUtils.synthesizeKey("2", {}, editorWin);
>   EventUtils.synthesizeKey("VK_RETURN", {}, editorWin);
> 
> I will add a couple of notes about the patch also.

Comment 21

[Brian Grinstead [:bgrins]]

> > directly.  It would be great to have a basic test for this jump to line
> > functionality that passes currently, then to also add some assertions that
> > only pass with the rest of the patch applied.
> 
> I'm unsure about this "add some assertions" thing.
> 
> I currently have these implemented as todo_... tests.
> 
> Is that what you had in mind?

Yeah, so if you are testing jumpToLine with the input string "4", this should pass already without any new patches applied (the editor cursor location should be line 4, col 0 or whatever).  But for other inputs, like "1:2", these will only pass once your new changes are in.  So ideally the test would make it easy to add these extra checks.

Some input types that we should probably test:

"4" (line 4 col 0)
"100000" (should go to last line if this is bigger than the # of lines)
"1:2" (line 1 col 2)
"1:100000" (should go to end of line 1 even if this # is bigger than the length of the line)
"asdf" (don't know, probably it doesn't do anything)
"-1" (don't know, probably it doesn't do anything)
"Exception: asdfa is not defined\n@Scratchpad/2:10:5" (should go to line 10 col 5 I guess)

Comment 22

[Adrian Aichner [:anaran]]

(In reply to Brian Grinstead [:bgrins] from comment #21)
> > > directly.  It would be great to have a basic test for this jump to line
> > > functionality that passes currently, then to also add some assertions that
> > > only pass with the rest of the patch applied.
> > 
> > I'm unsure about this "add some assertions" thing.
> > 
> > I currently have these implemented as todo_... tests.
> > 
> > Is that what you had in mind?
> 
> Yeah, so if you are testing jumpToLine with the input string "4", this
> should pass already without any new patches applied (the editor cursor
> location should be line 4, col 0 or whatever).  But for other inputs, like
> "1:2", these will only pass once your new changes are in.  So ideally the
> test would make it easy to add these extra checks.
> 
> Some input types that we should probably test:
> 
> "4" (line 4 col 0)
> "100000" (should go to last line if this is bigger than the # of lines)
> "1:2" (line 1 col 2)
> "1:100000" (should go to end of line 1 even if this # is bigger than the
> length of the line)
> "asdf" (don't know, probably it doesn't do anything)
> "-1" (don't know, probably it doesn't do anything)
> "Exception: asdfa is not defined\n@Scratchpad/2:10:5" (should go to line 10
> col 5 I guess)

I have got similar cases covered.

The question was if my patch should use the todo_ variants of tests failing right now.

If my whole patch goes in, then those todo_ prefixes should be removed.
So what is the best approach?

Comment 23

[Brian Grinstead [:bgrins]]

(In reply to adrian from comment #22)
> (In reply to Brian Grinstead [:bgrins] from comment #21)
> > > > directly.  It would be great to have a basic test for this jump to line
> > > > functionality that passes currently, then to also add some assertions that
> > > > only pass with the rest of the patch applied.
> > > 
> > > I'm unsure about this "add some assertions" thing.
> > > 
> > > I currently have these implemented as todo_... tests.
> > > 
> > > Is that what you had in mind?
> > 
> > Yeah, so if you are testing jumpToLine with the input string "4", this
> > should pass already without any new patches applied (the editor cursor
> > location should be line 4, col 0 or whatever).  But for other inputs, like
> > "1:2", these will only pass once your new changes are in.  So ideally the
> > test would make it easy to add these extra checks.
> > 
> > Some input types that we should probably test:
> > 
> > "4" (line 4 col 0)
> > "100000" (should go to last line if this is bigger than the # of lines)
> > "1:2" (line 1 col 2)
> > "1:100000" (should go to end of line 1 even if this # is bigger than the
> > length of the line)
> > "asdf" (don't know, probably it doesn't do anything)
> > "-1" (don't know, probably it doesn't do anything)
> > "Exception: asdfa is not defined\n@Scratchpad/2:10:5" (should go to line 10
> > col 5 I guess)
> 
> I have got similar cases covered.
> 
> The question was if my patch should use the todo_ variants of tests failing
> right now.
> 
> If my whole patch goes in, then those todo_ prefixes should be removed.
> So what is the best approach?

I'm not sure exactly what you mean by the todo_ prefixes.  We will land the patch all as one (code and tests), so no worries about the test having to support the older code, if that's what you mean.

Comment 24

[Adrian Aichner [:anaran]]

See(In reply to Brian Grinstead [:bgrins] from comment #23)
> (In reply to adrian from comment #22)
> > (In reply to Brian Grinstead [:bgrins] from comment #21)
> > > > > directly.  It would be great to have a basic test for this jump to line
> > > > > functionality that passes currently, then to also add some assertions that
> > > > > only pass with the rest of the patch applied.
> > > > 
> > > > I'm unsure about this "add some assertions" thing.
> > > > 
> > > > I currently have these implemented as todo_... tests.
> > > > 
> > > > Is that what you had in mind?
> > > 
> > > Yeah, so if you are testing jumpToLine with the input string "4", this
> > > should pass already without any new patches applied (the editor cursor
> > > location should be line 4, col 0 or whatever).  But for other inputs, like
> > > "1:2", these will only pass once your new changes are in.  So ideally the
> > > test would make it easy to add these extra checks.
> > > 
> > > Some input types that we should probably test:
> > > 
> > > "4" (line 4 col 0)
> > > "100000" (should go to last line if this is bigger than the # of lines)
> > > "1:2" (line 1 col 2)
> > > "1:100000" (should go to end of line 1 even if this # is bigger than the
> > > length of the line)
> > > "asdf" (don't know, probably it doesn't do anything)
> > > "-1" (don't know, probably it doesn't do anything)
> > > "Exception: asdfa is not defined\n@Scratchpad/2:10:5" (should go to line 10
> > > col 5 I guess)
> > 
> > I have got similar cases covered.
> > 
> > The question was if my patch should use the todo_ variants of tests failing
> > right now.
> > 
> > If my whole patch goes in, then those todo_ prefixes should be removed.
> > So what is the best approach?
> 
> I'm not sure exactly what you mean by the todo_ prefixes.  We will land the
> patch all as one (code and tests), so no worries about the test having to
> support the older code, if that's what you mean.

See
https://developer.mozilla.org/en-US/docs/Mochitest#Test_functions

OK, based on your reply I will remove the todo_ prefixes.

They will still be there in the history of my Bug1005471 branch.

Clean patch forthcoming...

Comment 25

[Adrian Aichner [:anaran]]

Attachment: 0001-Bug-1005471-Scratchpad-Jump-to-line-should-preset-in.patch

What took me so long is that I can't fix two leaks of
nsGlobalWindow in the pprint test.
I would appreciate help on that one.
All test logs are clean except for that.
I am uploading them next.

Comment 26

[Adrian Aichner [:anaran]]

Attachment: mochitest-devtools-browser_editor_goto_line-20140516T031205+0000.txt

Comment 27

[Adrian Aichner [:anaran]]

Attachment: mochitest-devtools-browser_scratchpad_run_error_goto_line-20140516T031310+0000.txt

Comment 28

[Adrian Aichner [:anaran]]

Attachment: mochitest-devtools-browser_scratchpad_pprint_error_goto_line-20140516T031240+0000.txt

Here I could need some help.
The prettyPrint fails, as desired, because of a JS syntax error.
jumpToLine can automatically goto that syntax error and that functionality passes.
I could swear this worked without leaked windows before, but now I have two, reproducibly.
Perhaps you can spot it fairly easily.

Comment 29

[Adrian Aichner [:anaran]]

(In reply to adrian from comment #28)
> Created attachment 8423595 [details]
> mochitest-devtools-browser_scratchpad_pprint_error_goto_line-
> 20140516T031240+0000.txt
> 
> Here I could need some help.
> The prettyPrint fails, as desired, because of a JS syntax error.
> jumpToLine can automatically goto that syntax error and that functionality
> passes.
> I could swear this worked without leaked windows before, but now I have two,
> reproducibly.
> Perhaps you can spot it fairly easily.

This change to
browser_scratchpad_pprint_error_goto_line.js
eliminates the window leaks for me:

@@ -57,7 +57,8 @@ function runTests(sw)
     let cursor = sp.editor.getCursor();
     is(inputLine, cursor.line + 1, "jumpToLine goto error location (line)");
     is(inputColumn, cursor.ch + 1, "jumpToLine goto error location (column)");
+    info(sw);
     sw.close();
     finish();
   });
 }

Does that look like a timing issue?
An unasked for promise perhaps?

Comment 30

[Adrian Aichner [:anaran]]

Comment on attachment 8423590 [details] [diff] [review]
0001-Bug-1005471-Scratchpad-Jump-to-line-should-preset-in.patch

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

What's your take on ordering of entries in browser.ini?

::: browser/devtools/scratchpad/test/browser.ini
@@ -22,3 @@
>  [browser_scratchpad_falsy.js]
>  [browser_scratchpad_edit_ui_updates.js]
>  [browser_scratchpad_revert_to_saved.js]

According to
https://wiki.mozilla.org/DevTools/mochitests_coding_standards#Referencing_the_new_file
these files should be in alphabetical order.

Not so in this file. Is this a problem?

Should the mochitests_coding_standards clarify *why* the alphabetical order is necessary?

::: browser/devtools/sourceeditor/test/browser.ini
@@ -22,1 @@
>  

This file violates
https://wiki.mozilla.org/DevTools/mochitests_coding_standards#Referencing_the_new_file
as well.

[browser_codemirror.js]
, to name just one, appears later in the file

Comment 31

[Brian Grinstead [:bgrins]]

Comment on attachment 8423594 [details]
mochitest-devtools-browser_scratchpad_run_error_goto_line-20140516T031310+0000.txt

You only need to ask for review on patches

Comment 32

[Brian Grinstead [:bgrins]]

Comment on attachment 8423590 [details] [diff] [review]
0001-Bug-1005471-Scratchpad-Jump-to-line-should-preset-in.patch

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

Please update the commit message to read:
Bug 1005471 - Scratchpad "Jump to line" should preset input value based on current selection, handle LINE:COLUMN as well;r=bgrins

Please make the updates, then we can try to sort out the leak after that.

::: browser/devtools/scratchpad/test/browser.ini
@@ -22,3 @@
>  [browser_scratchpad_falsy.js]
>  [browser_scratchpad_edit_ui_updates.js]
>  [browser_scratchpad_revert_to_saved.js]

I think the alphabetical ordering is just to keep things organized.  I'd put your new lines in alphabetical, even if the rest of the file isn't

::: browser/devtools/scratchpad/test/browser_scratchpad_pprint_error_goto_line.js
@@ +32,5 @@
> +    // stack are 1-based.
> +    is(/Invalid regexp flag \(3:10\)/.test(error), true, "prettyPrint expects error in editor text:\n" + error);
> +    let [ , errorLine, errorColumn ] = error.match(/\((\d+):(\d+)\)/);
> +    let editorDoc = sp.editor.container.contentDocument;
> +    let enter = editorDoc.createEvent("KeyboardEvent");

This is where using EventUtils.synthesizeKey can clean up the code quite a bit.  This whole `enter` block should be able to be removed, then replace lineInput.dispatchEvent(enter) to:

  lineInput.focus(); /* You may or may not need this */
  EventUtils.synthesizeKey("VK_RETURN", { }, editorDoc.defaultView);

Do the same in the other two tests

@@ +56,5 @@
> +    // stack are 1-based.
> +    let cursor = sp.editor.getCursor();
> +    is(inputLine, cursor.line + 1, "jumpToLine goto error location (line)");
> +    is(inputColumn, cursor.ch + 1, "jumpToLine goto error location (column)");
> +    sw.close();

Is this sw.close() needed?  Looks like only one other test uses that

::: browser/devtools/scratchpad/test/browser_scratchpad_run_error_goto_line.js
@@ +26,5 @@
> +    "// line 4",
> +    "// line 5",
> +    ""].join("\n"));
> +  sp.run().then(() => {
> +    error = sp.editor.getSelection();

use `let error = sp.editor.getSelection();` and get rid of that `delete error` below

@@ +52,5 @@
> +    // CodeMirror lines and columns are 0-based, Scratchpad UI and error
> +    // stack are 1-based.
> +    let cursor = sp.editor.getCursor();
> +    is(inputLine, cursor.line + 1, "jumpToLine goto error location (line)");
> +    is(1, cursor.ch + 1, "jumpToLine goto error location (column)");

Switch the order of the parameters here and other places following this ordering.  The first should be the actual value, the second should be the expected value

@@ +54,5 @@
> +    let cursor = sp.editor.getCursor();
> +    is(inputLine, cursor.line + 1, "jumpToLine goto error location (line)");
> +    is(1, cursor.ch + 1, "jumpToLine goto error location (column)");
> +    delete error;
> +    // sp.editor.destroy();

Remove the commented out line

::: browser/devtools/sourceeditor/editor.js
@@ +734,5 @@
>      let inp = doc.createElement("input");
>      let txt = doc.createTextNode(L10N.GetStringFromName("gotoLineCmd.promptTitle"));
> +    // Match @Scratchpad/N:LINE[:COLUMN], (LINE[:COLUMN]) anywhere,
> +    // or LINE[:COLUMN] just at begin of text selection.
> +    let reLineSpec = /(?:@Scratchpad\/\d+:|\(|^)(\d+):?(\d+)?/g;

Make this regex a const at the the top of the file, and name it something like RE_SCRATCHPAD_ERROR

@@ +739,5 @@
>      inp.type = "text";
>      inp.style.width = "10em";
>      inp.style.MozMarginStart = "1em";
> +    let cm = editors.get(this);
> +    let sel = cm.listSelections().length === 1 ? cm.getSelection() : undefined;

You can use this.hasMultipleSelections() instead of hitting the cm.listSelections() method here.

@@ +746,5 @@
> +      // e.g. inserted by running or pretty-printing code with errors.
> +      let lineSpec = reLineSpec.exec(sel);
> +      if (lineSpec) {
> +        let [ match, line, column ] = lineSpec;
> +        inp.value = column ? line+":"+column : line;

Add spaces between the `+` operators

@@ +755,5 @@
>      div.appendChild(inp);
>  
> +    this.openDialog(div, (line) => {
> +      // Handle LINE:COLUMN as well as LINE
> +      let [ match, line, column ] = line.match(/^(\d+)?:?(\d+)?/);

Move this regex up with the other one, and give it a name like RE_JUMP_TO_LINE

::: browser/devtools/sourceeditor/test/browser.ini
@@ -22,1 @@
>  

Yeah, not everything is order.  But I'd keep yours in order like you have

::: browser/devtools/sourceeditor/test/browser_editor_goto_line.js
@@ +16,5 @@
> +  ed.jumpToLine();
> +  let editorDoc = ed.container.contentDocument;
> +  let lineInput = editorDoc.querySelector("input");
> +  let enter = editorDoc.createEvent("KeyboardEvent");
> +  enter.initKeyEvent(

use synthesizeKey

@@ +46,5 @@
> +      "//     line 5",
> +      ""];
> +    ed.setText(textLines.join("\n"));
> +    waitForFocus(function () {
> +      let tailpipe =

I would be a bit more explicit with what is expected here, instead of generating the expected value in testJumpToLine.  Then pass the expected value into testJumpToLine

let testData = [
  {input: "", expected: {line: 0, ch: 0},
  {input: ":", expected: {line: 0, ch: 0},
  {input: "1:1", expected: {line: 1, ch: 1},
....
]

@@ +64,5 @@
> +        ];
> +      tailpipe.forEach(function (line) {
> +        testJumpToLine(ed, line);
> +      });
> +      textLines.map(function (line, index, object) {

I'm not sure of the point of the mapping on textLines here.  Seems like you could cover all the same cases more simply with the test data above (currently called tailpipe), by adding in things like "1:2", "10000:1", "1:100000", etc.

Comment 33

[Adrian Aichner [:anaran]]

(In reply to Brian Grinstead [:bgrins] from comment #32)
> Comment on attachment 8423590 [details] [diff] [review]
> 0001-Bug-1005471-Scratchpad-Jump-to-line-should-preset-in.patch
> 
> Review of attachment 8423590 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please update the commit message to read:
> Bug 1005471 - Scratchpad "Jump to line" should preset input value based on
> current selection, handle LINE:COLUMN as well;r=bgrins
> 
> Please make the updates, then we can try to sort out the leak after that.

Should that be the full commit message?

Should the squashed details not be part of the commit message?

> 
> ::: browser/devtools/scratchpad/test/browser.ini
> @@ -22,3 @@
> >  [browser_scratchpad_falsy.js]
> >  [browser_scratchpad_edit_ui_updates.js]
> >  [browser_scratchpad_revert_to_saved.js]
> 
> I think the alphabetical ordering is just to keep things organized.  I'd put
> your new lines in alphabetical, even if the rest of the file isn't
> 
> :::
> browser/devtools/scratchpad/test/browser_scratchpad_pprint_error_goto_line.js
> @@ +32,5 @@
> > +    // stack are 1-based.
> > +    is(/Invalid regexp flag \(3:10\)/.test(error), true, "prettyPrint expects error in editor text:\n" + error);
> > +    let [ , errorLine, errorColumn ] = error.match(/\((\d+):(\d+)\)/);
> > +    let editorDoc = sp.editor.container.contentDocument;
> > +    let enter = editorDoc.createEvent("KeyboardEvent");
> 
> This is where using EventUtils.synthesizeKey can clean up the code quite a
> bit.  This whole `enter` block should be able to be removed, then replace
> lineInput.dispatchEvent(enter) to:
> 
>   lineInput.focus(); /* You may or may not need this */
>   EventUtils.synthesizeKey("VK_RETURN", { }, editorDoc.defaultView);
> 
> Do the same in the other two tests

Sounds good, will do.

> 
> @@ +56,5 @@
> > +    // stack are 1-based.
> > +    let cursor = sp.editor.getCursor();
> > +    is(inputLine, cursor.line + 1, "jumpToLine goto error location (line)");
> > +    is(inputColumn, cursor.ch + 1, "jumpToLine goto error location (column)");
> > +    sw.close();
> 
> Is this sw.close() needed?  Looks like only one other test uses that

The leaks go away when I use
    info(sw);
in place of
    sw.close();

What could info do to the argument to make it get collected?

> 
> :::
> browser/devtools/scratchpad/test/browser_scratchpad_run_error_goto_line.js
> @@ +26,5 @@
> > +    "// line 4",
> > +    "// line 5",
> > +    ""].join("\n"));
> > +  sp.run().then(() => {
> > +    error = sp.editor.getSelection();
> 
> use `let error = sp.editor.getSelection();` and get rid of that `delete
> error` below

Ah, yes, I saw that already later today. Will do.

> 
> @@ +52,5 @@
> > +    // CodeMirror lines and columns are 0-based, Scratchpad UI and error
> > +    // stack are 1-based.
> > +    let cursor = sp.editor.getCursor();
> > +    is(inputLine, cursor.line + 1, "jumpToLine goto error location (line)");
> > +    is(1, cursor.ch + 1, "jumpToLine goto error location (column)");
> 
> Switch the order of the parameters here and other places following this
> ordering.  The first should be the actual value, the second should be the
> expected value

Yep

> 
> @@ +54,5 @@
> > +    let cursor = sp.editor.getCursor();
> > +    is(inputLine, cursor.line + 1, "jumpToLine goto error location (line)");
> > +    is(1, cursor.ch + 1, "jumpToLine goto error location (column)");
> > +    delete error;
> > +    // sp.editor.destroy();
> 
> Remove the commented out line
> 
> ::: browser/devtools/sourceeditor/editor.js
> @@ +734,5 @@
> >      let inp = doc.createElement("input");
> >      let txt = doc.createTextNode(L10N.GetStringFromName("gotoLineCmd.promptTitle"));
> > +    // Match @Scratchpad/N:LINE[:COLUMN], (LINE[:COLUMN]) anywhere,
> > +    // or LINE[:COLUMN] just at begin of text selection.
> > +    let reLineSpec = /(?:@Scratchpad\/\d+:|\(|^)(\d+):?(\d+)?/g;
> 
> Make this regex a const at the the top of the file, and name it something
> like RE_SCRATCHPAD_ERROR
> 
> @@ +739,5 @@
> >      inp.type = "text";
> >      inp.style.width = "10em";
> >      inp.style.MozMarginStart = "1em";
> > +    let cm = editors.get(this);
> > +    let sel = cm.listSelections().length === 1 ? cm.getSelection() : undefined;
> 
> You can use this.hasMultipleSelections() instead of hitting the
> cm.listSelections() method here.
> 
> @@ +746,5 @@
> > +      // e.g. inserted by running or pretty-printing code with errors.
> > +      let lineSpec = reLineSpec.exec(sel);
> > +      if (lineSpec) {
> > +        let [ match, line, column ] = lineSpec;
> > +        inp.value = column ? line+":"+column : line;
> 
> Add spaces between the `+` operators

Will do.

I had hoped I could use Scratchpad to pretty print the code, but it enforces conflicting coding conventions, like single quotes intead of double quotes.

Shouldn't those conventions be in sync?

> 
> @@ +755,5 @@
> >      div.appendChild(inp);
> >  
> > +    this.openDialog(div, (line) => {
> > +      // Handle LINE:COLUMN as well as LINE
> > +      let [ match, line, column ] = line.match(/^(\d+)?:?(\d+)?/);
> 
> Move this regex up with the other one, and give it a name like
> RE_JUMP_TO_LINE

Will do this and the other one.

> 
> ::: browser/devtools/sourceeditor/test/browser.ini
> @@ -22,1 @@
> >  
> 
> Yeah, not everything is order.  But I'd keep yours in order like you have

OK, easy enough

> 
> ::: browser/devtools/sourceeditor/test/browser_editor_goto_line.js
> @@ +16,5 @@
> > +  ed.jumpToLine();
> > +  let editorDoc = ed.container.contentDocument;
> > +  let lineInput = editorDoc.querySelector("input");
> > +  let enter = editorDoc.createEvent("KeyboardEvent");
> > +  enter.initKeyEvent(
> 
> use synthesizeKey
> 
> @@ +46,5 @@
> > +      "//     line 5",
> > +      ""];
> > +    ed.setText(textLines.join("\n"));
> > +    waitForFocus(function () {
> > +      let tailpipe =
> 
> I would be a bit more explicit with what is expected here, instead of
> generating the expected value in testJumpToLine.  Then pass the expected
> value into testJumpToLine
> 
> let testData = [
>   {input: "", expected: {line: 0, ch: 0},
>   {input: ":", expected: {line: 0, ch: 0},
>   {input: "1:1", expected: {line: 1, ch: 1},
> ....
> ]
> 
> @@ +64,5 @@
> > +        ];
> > +      tailpipe.forEach(function (line) {
> > +        testJumpToLine(ed, line);
> > +      });
> > +      textLines.map(function (line, index, object) {
> 
> I'm not sure of the point of the mapping on textLines here.  Seems like you
> could cover all the same cases more simply with the test data above
> (currently called tailpipe), by adding in things like "1:2", "10000:1",
> "1:100000", etc.

The idea is to jump to beginning, one before end, end, one beyond end of line.
Seemed logical to let JS do the calculations, driven by the text content.
If I expand this into static tests then they would break when we decide "textLines" should be replaced with something better.

Would it be acceptable to leave in a helper function which prints out the algorithmically generated expect data, only to be uncommented when an update of the expect data is needed?

Comment 34

[Brian Grinstead [:bgrins]]

(In reply to adrian from comment #33)
> (In reply to Brian Grinstead [:bgrins] from comment #32)
> > Comment on attachment 8423590 [details] [diff] [review]
> > 0001-Bug-1005471-Scratchpad-Jump-to-line-should-preset-in.patch
> > 
> > Review of attachment 8423590 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Please update the commit message to read:
> > Bug 1005471 - Scratchpad "Jump to line" should preset input value based on
> > current selection, handle LINE:COLUMN as well;r=bgrins
> > 
> > Please make the updates, then we can try to sort out the leak after that.
> 
> Should that be the full commit message?
> 
> Should the squashed details not be part of the commit message?
> 

Yes, that should be the full commit message (no squashed details).
 
> > @@ +56,5 @@
> > > +    // stack are 1-based.
> > > +    let cursor = sp.editor.getCursor();
> > > +    is(inputLine, cursor.line + 1, "jumpToLine goto error location (line)");
> > > +    is(inputColumn, cursor.ch + 1, "jumpToLine goto error location (column)");
> > > +    sw.close();
> > 
> > Is this sw.close() needed?  Looks like only one other test uses that
> 
> The leaks go away when I use
>     info(sw);
> in place of
>     sw.close();
> 
> What could info do to the argument to make it get collected?
> 

I'm not sure off the top of my head, we will have to get to the bottom of it before landing.  The other tests are working, and it doesn't seem like this is doing anything too much different so we should be able to track it down - I usually just comment stuff out until it goes away to narrow it down to a particular line and go from there.  I'd remove the sw.close() call though, unless if it is needed.

> > @@ +64,5 @@
> > > +        ];
> > > +      tailpipe.forEach(function (line) {
> > > +        testJumpToLine(ed, line);
> > > +      });
> > > +      textLines.map(function (line, index, object) {
> > 
> > I'm not sure of the point of the mapping on textLines here.  Seems like you
> > could cover all the same cases more simply with the test data above
> > (currently called tailpipe), by adding in things like "1:2", "10000:1",
> > "1:100000", etc.
> 
> The idea is to jump to beginning, one before end, end, one beyond end of
> line.
> Seemed logical to let JS do the calculations, driven by the text content.
> If I expand this into static tests then they would break when we decide
> "textLines" should be replaced with something better.
> 
> Would it be acceptable to leave in a helper function which prints out the
> algorithmically generated expect data, only to be uncommented when an update
> of the expect data is needed?

Feel free to come up with the test data however you want.  Not sure how frequently we will need to be changing the test data in the future (unless if we catch some edge case that it isn't being handled right).  The tests would be more likely to be expanded wrt to handling 'bad' input than to deal with off by one cases at the end of lines.  Once that part works, it kind of just works for all types of lines.

Comment 35

[Adrian Aichner [:anaran]]

(In reply to Brian Grinstead [:bgrins] from comment #34)
> (In reply to adrian from comment #33)
> > (In reply to Brian Grinstead [:bgrins] from comment #32)
> > > Comment on attachment 8423590 [details] [diff] [review]
> > > 0001-Bug-1005471-Scratchpad-Jump-to-line-should-preset-in.patch
> > > 
> > > Review of attachment 8423590 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > Please update the commit message to read:
> > > Bug 1005471 - Scratchpad "Jump to line" should preset input value based on
> > > current selection, handle LINE:COLUMN as well;r=bgrins
> > > 
> > > Please make the updates, then we can try to sort out the leak after that.
> > 
> > Should that be the full commit message?
> > 
> > Should the squashed details not be part of the commit message?
> > 
> 
> Yes, that should be the full commit message (no squashed details).
>  
> > > @@ +56,5 @@
> > > > +    // stack are 1-based.
> > > > +    let cursor = sp.editor.getCursor();
> > > > +    is(inputLine, cursor.line + 1, "jumpToLine goto error location (line)");
> > > > +    is(inputColumn, cursor.ch + 1, "jumpToLine goto error location (column)");
> > > > +    sw.close();
> > > 
> > > Is this sw.close() needed?  Looks like only one other test uses that
> > 
> > The leaks go away when I use
> >     info(sw);
> > in place of
> >     sw.close();
> > 
> > What could info do to the argument to make it get collected?
> > 
> 
> I'm not sure off the top of my head, we will have to get to the bottom of it
> before landing.  The other tests are working, and it doesn't seem like this
> is doing anything too much different so we should be able to track it down -
> I usually just comment stuff out until it goes away to narrow it down to a
> particular line and go from there.  I'd remove the sw.close() call though,
> unless if it is needed.

Thanks.

I decided to understand promises a bit better and I hope I do now.

The root cause, though, seems to that function runTests holds on the to the ChomeWindow via its this.

When I call finishTest() instead of finish() directly from runTests, then the leaks go away.

The this reported in finishTest is just Object, not ChomeWindow.

function finishTest()
{
  info(this);
  finish();
}

> 
> > > @@ +64,5 @@
> > > > +        ];
> > > > +      tailpipe.forEach(function (line) {
> > > > +        testJumpToLine(ed, line);
> > > > +      });
> > > > +      textLines.map(function (line, index, object) {
> > > 
> > > I'm not sure of the point of the mapping on textLines here.  Seems like you
> > > could cover all the same cases more simply with the test data above
> > > (currently called tailpipe), by adding in things like "1:2", "10000:1",
> > > "1:100000", etc.
> > 
> > The idea is to jump to beginning, one before end, end, one beyond end of
> > line.
> > Seemed logical to let JS do the calculations, driven by the text content.
> > If I expand this into static tests then they would break when we decide
> > "textLines" should be replaced with something better.
> > 
> > Would it be acceptable to leave in a helper function which prints out the
> > algorithmically generated expect data, only to be uncommented when an update
> > of the expect data is needed?
> 
> Feel free to come up with the test data however you want.  Not sure how
> frequently we will need to be changing the test data in the future (unless
> if we catch some edge case that it isn't being handled right).  The tests
> would be more likely to be expanded wrt to handling 'bad' input than to deal
> with off by one cases at the end of lines.  Once that part works, it kind of
> just works for all types of lines.

Yeah, I'll keep a help in my work branch, not to be delivered in the squashed commit.

Comment 36

[Adrian Aichner [:anaran]]

Attachment: 0001-Bug-1005471-Scratchpad-Jump-to-line-should-preset-in.patch

This patch addresses all your feedback.
I have kept the
  info(sw)
workaround in place in
  browser_scratchpad_pprint_error_goto_line.js
to fix the window leaks reliably.
All my promising solutions turned out to still leak intermittently.
I have come to the temporary conclusion that this must be a race or insufficient timeout issue.
One thing I noticed: When the leaks happen the browser stays up until the searchbar icon refreshes from a vanilla image to the google search image.

Comment 37

[Adrian Aichner [:anaran]]

Attachment: mochitest-devtools-browser_editor_goto_line-20140517T020503+0000.txt

Comment 38

[Adrian Aichner [:anaran]]

Attachment: 0001-Bug-1005471-Scratchpad-Jump-to-line-should-preset-in.patch

Sorry, I had slipped in a one character showstopper bug.
I have re-run all my tests add they all pass now.

Comment 39

[Adrian Aichner [:anaran]]

Attachment: mochitest-devtools-browser_editor_goto_line-20140517T030449+0000.txt

Comment 40

[Adrian Aichner [:anaran]]

Attachment: mochitest-devtools-browser_scratchpad_pprint_error_goto_line-20140517T025942+0000.txt

Comment 41

[Adrian Aichner [:anaran]]

Attachment: mochitest-devtools-browser_scratchpad_run_error_goto_line-20140517T025840+0000.txt

Comment 42

[Brian Grinstead [:bgrins]]

Comment on attachment 8424269 [details]
mochitest-devtools-browser_editor_goto_line-20140517T030449+0000.txt

FYI - you don't need to include the test logs as attachments

Comment 43

[Adrian Aichner [:anaran]]

Found a bug with my implementation:

My use of the global flag and RE.exec(str) intead of str.match(RE) is a leftover from when I thought I needed positions of matches, which I don't.

Due to the that implementation jumpToLine fails to extract the LINE:COLUMN match form the selected text every other time.

My minimal fix is just to drop the g flag in
const RE_SCRATCHPAD_ERROR = /(?:@Scratchpad\/\d+:|\(|^)(\d+):?(\d+)?/g;

I added test coverage and the fix works.

However, if you agree, I would like to change that use of .exec to .match as well.

Regarding "leaked until shutdown nsGlobalWindow" I have not made much progress, but the issue seems wide spread.

Also these look potentially related:
Bug 941787
Bug 937912
Bug 937913
Bug 964369

I was able to simplify my workaround to

function finishTest()
{
  finish();
  info(gScratchpadWindow);
}

and I now use objects provided by openScratchpad(runTests)

function runTests(sw, sp)
{

That's it for now

Comment 44

[Adrian Aichner [:anaran]]

Attachment: 0001-Bug-1005471-Scratchpad-Jump-to-line-should-preset-in.patch

This patch implements what I outlined in comment 43.
I hope this patch is getting ready for landing.

Comment 45

[Brian Grinstead [:bgrins]]

Comment on attachment 8427158 [details] [diff] [review]
0001-Bug-1005471-Scratchpad-Jump-to-line-should-preset-in.patch

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

The tests are looking better - I've made a few notes throughout the code.  Please also update commit message to be on one line.

::: browser/devtools/scratchpad/test/browser_scratchpad_pprint_error_goto_line.js
@@ +52,5 @@
> +    ok(false, "Expecting Invalid regexp flag (3:10)");
> +    finishTest();
> +  }, error => {
> +    testJumpToPrettyPrintError(sp, error, " (Bug 1005471, first time)");
> +    let [ from, to ] = sp.editor.getPosition(0, sp.getText().length);

Looks like some variables are assigned that are not checked.  Were you intending to add more assertions for it here?  If not then remove the line

@@ +60,5 @@
> +    finishTest();
> +  }, error => {
> +    // Second time verifies bug in earlier implementation fixed.
> +    testJumpToPrettyPrintError(sp, error, " (second time)");
> +    let [ from, to ] = sp.editor.getPosition(0, sp.getText().length);

Extra variables again

::: browser/devtools/scratchpad/test/browser_scratchpad_run_error_goto_line.js
@@ +41,5 @@
> +    // stack are 1-based.
> +    let cursor = sp.editor.getCursor();
> +    is(cursor.line + 1, inputLine, "jumpToLine goto error location (line)");
> +    is(cursor.ch + 1, 1, "jumpToLine goto error location (column)");
> +    finish();

So this file doesn't need the magic 'info()' call to prevent leaks?  Interesting - it makes me think there may be something triggered by the calls in the pprint test that is causing the leak.  You may try to comment out functionality in the pprint test until the leak goes away to narrow it down to a particular line.

::: browser/devtools/sourceeditor/editor.js
@@ +742,5 @@
>      inp.style.width = "10em";
>      inp.style.MozMarginStart = "1em";
> +    let cm = editors.get(this);
> +    let sel = !this.hasMultipleSelections() ? cm.getSelection() : undefined;
> +    if (sel) {

getSelection() will not be null so I would change this a bit instead of assigning sel in a ternary:

if (!this.hasMultipleSelections()) {
  let cm = editors.get(this);
  let sel = cm.getSelection();
  ....
}

@@ +757,5 @@
>      div.appendChild(inp);
>  
> +    this.openDialog(div, (line) => {
> +      // Handle LINE:COLUMN as well as LINE
> +      let [ match, line, column ] = line.match(RE_JUMP_TO_LINE);

There is some inconsistency between how you are using .match() here and above.  Above, you are not assigning anything to the first return value, and you are also guarding against null matches.

I like the above usage better.  For instance, in this case entering "SOMETHING_INVALID" into the jump to line field should probably not affect the cursor location, rather than setting to 0:0.

@@ +759,5 @@
> +    this.openDialog(div, (line) => {
> +      // Handle LINE:COLUMN as well as LINE
> +      let [ match, line, column ] = line.match(RE_JUMP_TO_LINE);
> +      this.setCursor({
> +          line: line > 0 ? line - 1 : 0,

This is protecting against a value <= 0, but not value > lines.length?  Is this already handled by the editor?  If so, would negative numbers also be?

Comment 46

[Adrian Aichner [:anaran]]

Comment on attachment 8427158 [details] [diff] [review]
0001-Bug-1005471-Scratchpad-Jump-to-line-should-preset-in.patch

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

I have addressed all the issues, new patch forthcoming.

What exactly are you referring to about the commit message?

I am still using
git format-patch
and I think the subject line is just split up for mail transport.

If you look at my patch in github it's actually on the Bug1005471WIP branch, which I amend as needed to keep it a single commit.

Just once now today I have seen these mysterious Error: Route error messages.
I'm afraid the three exclamation marks don't really provide a lot of help :-)

The
  info(gScratchpadWindow);
workaround is still needed, then there are no leaks.

Just one time seen: Route error
-------------------------------

0:19.48 INFO TEST-END | chrome://mochitests/content/browser/browser/devtools/scratchpad/test/browser_scratchpad_pprint
0:19.53 TEST-INFO | checking window state
0:19.53 TEST-INFO | unknown test url | must wait for focus
0:20.02
0:20.02 ###!!! [Child][DispatchAsyncMessage] Error: Route error: message sent to unknown actor ID
0:20.02
0:20.02
0:20.02 ###!!! [Child][DispatchAsyncMessage] Error: Route error: message sent to unknown actor ID
0:20.02
0:20.02
0:20.02 ###!!! [Child][DispatchAsyncMessage] Error: Route error: message sent to unknown actor ID
0:20.02
0:20.02
0:20.02 ###!!! [Child][DispatchAsyncMessage] Error: Route error: message sent to unknown actor ID
0:20.02
0:20.20
0:20.20 INFO TEST-START | Shutdown
0:20.20 Browser Chrome Test Summary
0:20.20        Passed: 10
0:20.20        Failed: 0
0:20.20        Todo: 0
0:20.20
0:20.20 *** End BrowserChrome Test Results ***
EST-INFO | Main app process: exit status 0

0:21.38 INFO | runtests.py | Application ran for: 0:00:09.172000
0:21.38 INFO | zombiecheck | Reading PID log: c:\dokume~1\aichne~1\lokale~1\temp\tmp9iw5pjpidlog
0:21.38 Stopping web server
0:21.50 Stopping web socket server
0:21.62 Stopping ssltunnel
0:21.84 WARNING | leakcheck | refcount logging is off, so leaks can't be detected!
0:21.84 runtests.py | Running tests: end.
ichnerAd@KUCKUCK:/c/tmp/git.mozilla.org/gecko-dev$

::: browser/devtools/scratchpad/test/browser_scratchpad_pprint_error_goto_line.js
@@ +52,5 @@
> +    ok(false, "Expecting Invalid regexp flag (3:10)");
> +    finishTest();
> +  }, error => {
> +    testJumpToPrettyPrintError(sp, error, " (Bug 1005471, first time)");
> +    let [ from, to ] = sp.editor.getPosition(0, sp.getText().length);

Removed

@@ +60,5 @@
> +    finishTest();
> +  }, error => {
> +    // Second time verifies bug in earlier implementation fixed.
> +    testJumpToPrettyPrintError(sp, error, " (second time)");
> +    let [ from, to ] = sp.editor.getPosition(0, sp.getText().length);

Removed

::: browser/devtools/scratchpad/test/browser_scratchpad_run_error_goto_line.js
@@ +41,5 @@
> +    // stack are 1-based.
> +    let cursor = sp.editor.getCursor();
> +    is(cursor.line + 1, inputLine, "jumpToLine goto error location (line)");
> +    is(cursor.ch + 1, 1, "jumpToLine goto error location (column)");
> +    finish();

That is correct.
Have you looked at my comments on this highly unpredictable leak in the pprint test?
It is even hard to know whether the problem is still around.
I thought several times I had fixed the leak by some change in the test (not editor.js), and then the leaks came back in latest test runs.
The symptom is that the browser window does not go away immediately after the scratchpad window.
Instead the generic searchbar icon gets replaced by the google search icon in the browser window, then the window goes away.
When that happens, then the two leaks get reported.
I have quoted several bugs in my earlier comments.
At this point I am out of ideas the fix the test leaks, flaky as they are.
The fact that just
info(gScratchpadWindow);
fixes it reliably seems hint enough for me that a SimpleTest expert should look into this as well.

::: browser/devtools/sourceeditor/editor.js
@@ +742,5 @@
>      inp.style.width = "10em";
>      inp.style.MozMarginStart = "1em";
> +    let cm = editors.get(this);
> +    let sel = !this.hasMultipleSelections() ? cm.getSelection() : undefined;
> +    if (sel) {

Done. I also removed line 744 above.

@@ +757,5 @@
>      div.appendChild(inp);
>  
> +    this.openDialog(div, (line) => {
> +      // Handle LINE:COLUMN as well as LINE
> +      let [ match, line, column ] = line.match(RE_JUMP_TO_LINE);

I agree, done.

@@ +759,5 @@
> +    this.openDialog(div, (line) => {
> +      // Handle LINE:COLUMN as well as LINE
> +      let [ match, line, column ] = line.match(RE_JUMP_TO_LINE);
> +      this.setCursor({
> +          line: line > 0 ? line - 1 : 0,

I changed this to just
      if (match) {
        let [ , line, column ] = match;
        this.setCursor({line: line - 1, ch: column - 1 });
      }

Comment 47

[Adrian Aichner [:anaran]]

Comment on attachment 8427158 [details] [diff] [review]
0001-Bug-1005471-Scratchpad-Jump-to-line-should-preset-in.patch

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

Two corrections...

::: browser/devtools/sourceeditor/editor.js
@@ +23,5 @@
>  
> +// Match @Scratchpad/N:LINE[:COLUMN], (LINE[:COLUMN]) anywhere,
> +// or LINE[:COLUMN] just at begin of text selection.
> +const RE_SCRATCHPAD_ERROR = /(?:@Scratchpad\/\d+:|\(|^)(\d+):?(\d+)?/;
> +const RE_JUMP_TO_LINE = /^(\d+)?:?(\d+)?/;

I have changed this regexp to
/^(\d+):?(\d+)?/
to make the line a mandatory match, else
if (match)
would be a mute point:
"".match(/^(\d+)?:?(\d+)?/)
Array [ "", undefined, undefined ]

@@ +759,5 @@
> +    this.openDialog(div, (line) => {
> +      // Handle LINE:COLUMN as well as LINE
> +      let [ match, line, column ] = line.match(RE_JUMP_TO_LINE);
> +      this.setCursor({
> +          line: line > 0 ? line - 1 : 0,

Actually,
        this.setCursor({line: line - 1, ch: column ? column - 1 : 0 });
is needed, since column is an optional match.

scratchpad/test/browser_scratchpad_run_error_goto_line.js kept me honest.
I had forgotten to run that one today.
Now all three test files pass.

Comment 48

[Adrian Aichner [:anaran]]

Attachment: 0001-Bug-1005471-Scratchpad-Jump-to-line-should-preset-in.patch

This should be the proper format now, run through
git-patch-to-hg-patch
of
https://github.com/mozilla/moz-git-tools

It's also to be found at
https://github.com/anaran/gecko-dev/tree/Bug1005471WIP

Comment 49

[Adrian Aichner [:anaran]]

Attachment: 0001-Bug-1005471-Scratchpad-Jump-to-line-should-preset-in.patch

browser/devtools/scratchpad/test/browser_scratchpad_goto_line_ui.js
pointed out another issue:
It passes line as a number
  editor.openDialog = function (text, cb) {
    cb(desiredValue);
  };

  desiredValue = 3;

It makes necessary another one line change to editor.js:
from
      let match = line.match(RE_JUMP_TO_LINE);
to
      let match = line.toString().match(RE_JUMP_TO_LINE);

Now I get all passes for
./mach mochitest-devtools browser/devtools/sourceeditor/test/
and
./mach mochitest-devtools browser/devtools/scratchpad/test/

Comment 50

[Brian Grinstead [:bgrins]]

Comment on attachment 8430417 [details] [diff] [review]
0001-Bug-1005471-Scratchpad-Jump-to-line-should-preset-in.patch

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

> What exactly are you referring to about the commit message?

The commit message is better now, but still needs to be updated.  Get rid of the [PATCH] at the front of it and remove any new lines from the message.

> That is correct.
> Have you looked at my comments on this highly unpredictable leak in the
> pprint test?
> It is even hard to know whether the problem is still around.
> I thought several times I had fixed the leak by some change in the test (not
> editor.js), and then the leaks came back in latest test runs.
> The symptom is that the browser window does not go away immediately after
> the scratchpad window.
> Instead the generic searchbar icon gets replaced by the google search icon
> in the browser window, then the window goes away.
> When that happens, then the two leaks get reported.
> I have quoted several bugs in my earlier comments.
> At this point I am out of ideas the fix the test leaks, flaky as they are.
> The fact that just
> info(gScratchpadWindow);
> fixes it reliably seems hint enough for me that a SimpleTest expert should
> look into this as well.

Yes, I pushed to the test servers both with and without the info() call and interestingly only the patch *with* the info call is causing leaks.

Leaks on debug builds with info call: https://tbpl.mozilla.org/?tree=Try&rev=b4beb1c78355
No leaks without the info call: (I've retriggered some builds to see if any show up): https://tbpl.mozilla.org/?tree=Try&rev=0997309edbb2

Either way, I wouldn't be comfortable landing code that had that line in place as a mechanism for preventing a leak - I'd remove it from the patch.  If you reupload without the info() I will test locally to see if I can reproduce the error.  If I can reproduce locally I will take a look at it.

::: browser/devtools/scratchpad/test/browser_scratchpad_pprint_error_goto_line.js
@@ +22,5 @@
> +  info("will test jumpToLine after prettyPrint error" + remark);
> +    // CodeMirror lines and columns are 0-based, Scratchpad UI and error
> +    // stack are 1-based.
> +    is(/Invalid regexp flag \(3:10\)/.test(error), true, "prettyPrint expects error in editor text:\n" + error);
> +    const [ , errorLine, errorColumn ] = error.match(/\((\d+):(\d+)\)/);

I'm thinking about this regex here.  In this test (and run_error_goto_line.js) since we know the line:col that it is going to fail on, let's hardcode this string instead of relying on a regex.  The test will fail anyway if the regex doesn't work because the inputLine will not be set correctly inside of editor.js

@@ +66,5 @@
> +
> +function finishTest(sw)
> +{
> +  finish();
> +  // calling info on some object: Workaround to eliminate

Remove these comments and the info() call

::: browser/devtools/scratchpad/test/browser_scratchpad_run_error_goto_line.js
@@ +29,5 @@
> +  sp.run().then(() => {
> +    let error = sp.editor.getSelection();
> +    // CodeMirror lines and columns are 0-based, Scratchpad UI and error
> +    // stack are 1-based.
> +    let [ , errorLine ] = error.match(/@Scratchpad\/\d+:(\d+)/);

Same comment about removing this regex and replacing with a string here

::: browser/devtools/sourceeditor/editor.js
@@ +755,5 @@
>      inp.style.MozMarginStart = "1em";
> +    if (!this.hasMultipleSelections()) {
> +      let cm = editors.get(this);
> +      let sel = cm.getSelection();
> +      // Try matching RE_SCRATCHPAD_ERROR in an active text selection,

Please update comment to something like:

// Scratchpad inserts and selects a comment after an error happens:
// "@Scratchpad/1:10:2". Parse this to get the line and column.
// In the string above this is line 10, column 2.

Comment 51

[Adrian Aichner [:anaran]]

Comment on attachment 8430417 [details] [diff] [review]
0001-Bug-1005471-Scratchpad-Jump-to-line-should-preset-in.patch

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

I guess git-patch-to-hg-patch could take care of all the process issues with my commit line part of my patches.

The process of submitting patches seems to be slightly broken in this regard:

https://wiki.mozilla.org/DevTools/Hacking#Making_and_Submitting_a_Patch
and
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#I%27m_all_used_to_git.2C_but_how_can_I_provide_Mercurial-ready_patches_.3F
linked from the former.

I'll take this up with
https://github.com/mozilla/moz-git-tools

::: browser/devtools/scratchpad/test/browser_scratchpad_pprint_error_goto_line.js
@@ +22,5 @@
> +  info("will test jumpToLine after prettyPrint error" + remark);
> +    // CodeMirror lines and columns are 0-based, Scratchpad UI and error
> +    // stack are 1-based.
> +    is(/Invalid regexp flag \(3:10\)/.test(error), true, "prettyPrint expects error in editor text:\n" + error);
> +    const [ , errorLine, errorColumn ] = error.match(/\((\d+):(\d+)\)/);

OK, changed to
    const errorLine = 3, errorColumn = 10;

@@ +66,5 @@
> +
> +function finishTest(sw)
> +{
> +  finish();
> +  // calling info on some object: Workaround to eliminate

Should I keep the finishTest function at all?

::: browser/devtools/scratchpad/test/browser_scratchpad_run_error_goto_line.js
@@ +29,5 @@
> +  sp.run().then(() => {
> +    let error = sp.editor.getSelection();
> +    // CodeMirror lines and columns are 0-based, Scratchpad UI and error
> +    // stack are 1-based.
> +    let [ , errorLine ] = error.match(/@Scratchpad\/\d+:(\d+)/);

Changed to
    let errorLine = 3;

::: browser/devtools/sourceeditor/editor.js
@@ +755,5 @@
>      inp.style.MozMarginStart = "1em";
> +    if (!this.hasMultipleSelections()) {
> +      let cm = editors.get(this);
> +      let sel = cm.getSelection();
> +      // Try matching RE_SCRATCHPAD_ERROR in an active text selection,

Done

Comment 52

[Brian Grinstead [:bgrins]]

> @@ +66,5 @@
> > +
> > +function finishTest(sw)
> > +{
> > +  finish();
> > +  // calling info on some object: Workaround to eliminate
> 
> Should I keep the finishTest function at all?

Probably not. Looks like most scratchpad tests just directly call finish().

Comment 53

[Adrian Aichner [:anaran]]

(In reply to Brian Grinstead [:bgrins] from comment #52)
> > @@ +66,5 @@
> > > +
> > > +function finishTest(sw)
> > > +{
> > > +  finish();
> > > +  // calling info on some object: Workaround to eliminate
> > 
> > Should I keep the finishTest function at all?
> 
> Probably not. Looks like most scratchpad tests just directly call finish().

OK, using just finish(); now.

All sourceeditor and scratchpad tests pass for me now, even without leakage for now.

Comment 54

[Adrian Aichner [:anaran]]

Attachment: 0001-Bug-1005471-Scratchpad-Jump-to-line-should-preset-in.patch

Addresses all review comments raised so far.
Hope the commit line is OK now.
I manually removed the "[PATCH] " prefix.

Comment 55

[Adrian Aichner [:anaran]]

Comment on attachment 8431597 [details] [diff] [review]
0001-Bug-1005471-Scratchpad-Jump-to-line-should-preset-in.patch

Forgot the review flag

Comment 56

[Brian Grinstead [:bgrins]]

Comment on attachment 8431597 [details] [diff] [review]
0001-Bug-1005471-Scratchpad-Jump-to-line-should-preset-in.patch

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

This is looking pretty much ready to go.  Two more notes inline.

Also have a fresh Try push: https://tbpl.mozilla.org/?tree=Try&rev=8d08483af63b

::: browser/devtools/sourceeditor/editor.js
@@ +22,5 @@
>  const MAX_VERTICAL_OFFSET = 3;
>  
> +// Match @Scratchpad/N:LINE[:COLUMN], (LINE[:COLUMN]) anywhere,
> +// or LINE[:COLUMN] just at begin of text selection.
> +const RE_SCRATCHPAD_ERROR = /(?:@Scratchpad\/\d+:|\(|^)(\d+):?(\d+)?/;

I played with this a bit, and realized really the only case where it makes sense to me to prepopulate the input is if this scratchpad pattern matches.  I think it is more likely to confuse people if they happened to have a number highlighted (in Style Editor, for instance) and when they did cmd+j it was prepopulated with that number.  This could also simplify the regex I think, since we would be looking to strictly match: /@Scratchpad\/\d+:(\d+):(\d+)/

@@ +761,5 @@
> +      // In the string above this is line 10, column 2.
> +      let match = sel.match(RE_SCRATCHPAD_ERROR);
> +      if (match) {
> +        let [ , line, column ] = match;
> +        inp.value = column ? line + ":" + column : line;

We should put the selection of the input at the end of the field instead of the front.

Move this if (!this.hasMultipleSelections()) block to below the div.appendChild call, then add this line after setting inp.value:

inp.selectionStart = inp.selectionEnd = inp.value.length;

Comment 57

[Adrian Aichner [:anaran]]

(In reply to Brian Grinstead [:bgrins] from comment #56)
> Comment on attachment 8431597 [details] [diff] [review]
> 0001-Bug-1005471-Scratchpad-Jump-to-line-should-preset-in.patch
> 
> Review of attachment 8431597 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is looking pretty much ready to go.  Two more notes inline.
> 
> Also have a fresh Try push:
> https://tbpl.mozilla.org/?tree=Try&rev=8d08483af63b
> 
> ::: browser/devtools/sourceeditor/editor.js
> @@ +22,5 @@
> >  const MAX_VERTICAL_OFFSET = 3;
> >  
> > +// Match @Scratchpad/N:LINE[:COLUMN], (LINE[:COLUMN]) anywhere,
> > +// or LINE[:COLUMN] just at begin of text selection.
> > +const RE_SCRATCHPAD_ERROR = /(?:@Scratchpad\/\d+:|\(|^)(\d+):?(\d+)?/;
> 
> I played with this a bit, and realized really the only case where it makes
> sense to me to prepopulate the input is if this scratchpad pattern matches. 
> I think it is more likely to confuse people if they happened to have a
> number highlighted (in Style Editor, for instance) and when they did cmd+j
> it was prepopulated with that number.  This could also simplify the regex I
> think, since we would be looking to strictly match:
> /@Scratchpad\/\d+:(\d+):(\d+)/

I see your point about the confusion. That would be bad, yes.

The column match still needs to stay optional though.

Else the runtime error, which only provides a line number, would not provide a match.

It's still very useful to be able to at least go to the line of the error.

> 
> @@ +761,5 @@
> > +      // In the string above this is line 10, column 2.
> > +      let match = sel.match(RE_SCRATCHPAD_ERROR);
> > +      if (match) {
> > +        let [ , line, column ] = match;
> > +        inp.value = column ? line + ":" + column : line;
> 
> We should put the selection of the input at the end of the field instead of
> the front.
> 
> Move this if (!this.hasMultipleSelections()) block to below the
> div.appendChild call, then add this line after setting inp.value:
> 
> inp.selectionStart = inp.selectionEnd = inp.value.length;

I'll amend accordingly.

Comment 58

[Brian Grinstead [:bgrins]]

(In reply to adrian from comment #57)
> (In reply to Brian Grinstead [:bgrins] from comment #56)
> > Comment on attachment 8431597 [details] [diff] [review]
> > 0001-Bug-1005471-Scratchpad-Jump-to-line-should-preset-in.patch
> > 
> > Review of attachment 8431597 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > This is looking pretty much ready to go.  Two more notes inline.
> > 
> > Also have a fresh Try push:
> > https://tbpl.mozilla.org/?tree=Try&rev=8d08483af63b
> > 
> > ::: browser/devtools/sourceeditor/editor.js
> > @@ +22,5 @@
> > >  const MAX_VERTICAL_OFFSET = 3;
> > >  
> > > +// Match @Scratchpad/N:LINE[:COLUMN], (LINE[:COLUMN]) anywhere,
> > > +// or LINE[:COLUMN] just at begin of text selection.
> > > +const RE_SCRATCHPAD_ERROR = /(?:@Scratchpad\/\d+:|\(|^)(\d+):?(\d+)?/;
> > 
> > I played with this a bit, and realized really the only case where it makes
> > sense to me to prepopulate the input is if this scratchpad pattern matches. 
> > I think it is more likely to confuse people if they happened to have a
> > number highlighted (in Style Editor, for instance) and when they did cmd+j
> > it was prepopulated with that number.  This could also simplify the regex I
> > think, since we would be looking to strictly match:
> > /@Scratchpad\/\d+:(\d+):(\d+)/
> 
> I see your point about the confusion. That would be bad, yes.
> 
> The column match still needs to stay optional though.
> 
> Else the runtime error, which only provides a line number, would not provide
> a match.
> 
> It's still very useful to be able to at least go to the line of the error.

It is possible for scratchpad to emit an error message without the column number?  Maybe I just haven't seen this.

Comment 59

[Adrian Aichner [:anaran]]

(In reply to Brian Grinstead [:bgrins] from comment #58)
> (In reply to adrian from comment #57)
> > (In reply to Brian Grinstead [:bgrins] from comment #56)
> > > Comment on attachment 8431597 [details] [diff] [review]
> > > 0001-Bug-1005471-Scratchpad-Jump-to-line-should-preset-in.patch
> > > 
> > > Review of attachment 8431597 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > This is looking pretty much ready to go.  Two more notes inline.
> > > 
> > > Also have a fresh Try push:
> > > https://tbpl.mozilla.org/?tree=Try&rev=8d08483af63b
> > > 
> > > ::: browser/devtools/sourceeditor/editor.js
> > > @@ +22,5 @@
> > > >  const MAX_VERTICAL_OFFSET = 3;
> > > >  
> > > > +// Match @Scratchpad/N:LINE[:COLUMN], (LINE[:COLUMN]) anywhere,
> > > > +// or LINE[:COLUMN] just at begin of text selection.
> > > > +const RE_SCRATCHPAD_ERROR = /(?:@Scratchpad\/\d+:|\(|^)(\d+):?(\d+)?/;
> > > 
> > > I played with this a bit, and realized really the only case where it makes
> > > sense to me to prepopulate the input is if this scratchpad pattern matches. 
> > > I think it is more likely to confuse people if they happened to have a
> > > number highlighted (in Style Editor, for instance) and when they did cmd+j
> > > it was prepopulated with that number.  This could also simplify the regex I
> > > think, since we would be looking to strictly match:
> > > /@Scratchpad\/\d+:(\d+):(\d+)/
> > 
> > I see your point about the confusion. That would be bad, yes.
> > 
> > The column match still needs to stay optional though.
> > 
> > Else the runtime error, which only provides a line number, would not provide
> > a match.
> > 
> > It's still very useful to be able to at least go to the line of the error.
> 
> It is possible for scratchpad to emit an error message without the column
> number?  Maybe I just haven't seen this.

Yes, the very testcase I am using only provides a column on Pretty Print, but not on Run.

I think I should also delete line 30:
    let error = sp.editor.getSelection();
from
browser_scratchpad_run_error_goto_line.js
which is no longer needed.

Comment 60

[Brian Grinstead [:bgrins]]

Comment on attachment 8431597 [details] [diff] [review]
0001-Bug-1005471-Scratchpad-Jump-to-line-should-preset-in.patch

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

This is looking pretty much ready to go.  Two more notes inline.

Also have a fresh Try push: https://tbpl.mozilla.org/?tree=Try&rev=8d08483af63b

::: browser/devtools/scratchpad/test/browser_scratchpad_pprint_error_goto_line.js
@@ +46,5 @@
> +    "// line 2",
> +    "var re = /a bad /regexp/; // line 3 is an obvious syntax error!",
> +    "// line 4",
> +    "// line 5",
> +    ""].join("\n"));

Nit: can you move the ].join("\n")); to a new line here and in the 'run' test as well?

::: browser/devtools/sourceeditor/editor.js
@@ +22,5 @@
>  const MAX_VERTICAL_OFFSET = 3;
>  
> +// Match @Scratchpad/N:LINE[:COLUMN], (LINE[:COLUMN]) anywhere,
> +// or LINE[:COLUMN] just at begin of text selection.
> +const RE_SCRATCHPAD_ERROR = /(?:@Scratchpad\/\d+:|\(|^)(\d+):?(\d+)?/;

I played with this a bit, and realized really the only case where it makes sense to me to prepopulate the input is if this scratchpad pattern matches.  I think it is more likely to confuse people if they happened to have a number highlighted (in Style Editor, for instance) and when they did cmd+j it was prepopulated with that number.  This could also simplify the regex I think, since we would be looking to strictly match: /@Scratchpad\/\d+:(\d+):(\d+)/

@@ +761,5 @@
> +      // In the string above this is line 10, column 2.
> +      let match = sel.match(RE_SCRATCHPAD_ERROR);
> +      if (match) {
> +        let [ , line, column ] = match;
> +        inp.value = column ? line + ":" + column : line;

We should put the selection of the input at the end of the field instead of the front.

Move this if (!this.hasMultipleSelections()) block to below the div.appendChild call, then add this line after setting inp.value:

inp.selectionStart = inp.selectionEnd = inp.value.length;

Comment 61

[Brian Grinstead [:bgrins]]

> Yes, the very testcase I am using only provides a column on Pretty Print,
> but not on Run.

Fair enough - so columns should be optional in the regex. 

> I think I should also delete line 30:
>     let error = sp.editor.getSelection();
> from
> browser_scratchpad_run_error_goto_line.js
> which is no longer needed.

Yes, go ahead and remove any unused lines in the test.

I accidentally resent the whole review, but I meant to just add one more thing:

> +    "// line 2",
> +    "var re = /a bad /regexp/; // line 3 is an obvious syntax error!",
> +    "// line 4",
> +    "// line 5",
> +    ""].join("\n"));

Nit: can you move the ].join("\n")); to a new line here and in the 'run' test as well?

Comment 62

[Adrian Aichner [:anaran]]

While Scratchpad run uses the @Scratchpad/N prefix:
/*
Exception: invalid regular expression flag r
@Scratchpad/2:3
*/

Pretty Print error do not (but often provide better help spotting the error):

/*
Exception: Invalid regexp flag (3:10)
raise@resource://gre/modules/devtools/acorn/acorn.js:231:9
readRegexp@resource://gre/modules/devtools/acorn/acorn.js:771:43
readToken_slash@resource://gre/modules/devtools/acorn/acorn.js:586:38
getTokenFromCode@resource://gre/modules/devtools/acorn/acorn.js:692:7
readToken@resource://gre/modules/devtools/acorn/acorn.js:731:9
getToken@resource://gre/modules/devtools/acorn/acorn.js:146:7
prettyFast@resource://gre/modules/devtools/pretty-fast.js:771:7
self.onmessage@resource://gre/modules/devtools/server/actors/pretty-print-worker.js:39:7

*/

For that reason I propose to use:
const RE_SCRATCHPAD_ERROR = /(?:@Scratchpad\/\d+:|\()(\d+):?(\d+)?(?:\)|\n)/;

So selecting a plain number or numbers separated by colon would not match.
One would have to manually select the left paren before it and either a right paren or newline after it.

Comment 63

[Brian Grinstead [:bgrins]]

(In reply to adrian from comment #62)
> While Scratchpad run uses the @Scratchpad/N prefix:
> /*
> Exception: invalid regular expression flag r
> @Scratchpad/2:3
> */
> 
> Pretty Print error do not (but often provide better help spotting the error):
> 
> /*
> Exception: Invalid regexp flag (3:10)
> raise@resource://gre/modules/devtools/acorn/acorn.js:231:9
> readRegexp@resource://gre/modules/devtools/acorn/acorn.js:771:43
> readToken_slash@resource://gre/modules/devtools/acorn/acorn.js:586:38
> getTokenFromCode@resource://gre/modules/devtools/acorn/acorn.js:692:7
> readToken@resource://gre/modules/devtools/acorn/acorn.js:731:9
> getToken@resource://gre/modules/devtools/acorn/acorn.js:146:7
> prettyFast@resource://gre/modules/devtools/pretty-fast.js:771:7
> self.onmessage@resource://gre/modules/devtools/server/actors/pretty-print-
> worker.js:39:7
> 
> */
> 
> For that reason I propose to use:
> const RE_SCRATCHPAD_ERROR = /(?:@Scratchpad\/\d+:|\()(\d+):?(\d+)?(?:\)|\n)/;
> 
> So selecting a plain number or numbers separated by colon would not match.
> One would have to manually select the left paren before it and either a
> right paren or newline after it.

Dang, I was hoping it would be consistent for both features.  The logic for the suggested regex looks good - but what is the point of the \n at the end of it?  Could we ignore that?

Comment 64

[Adrian Aichner [:anaran]]

(In reply to Brian Grinstead [:bgrins] from comment #63)
> (In reply to adrian from comment #62)
> > While Scratchpad run uses the @Scratchpad/N prefix:
> > /*
> > Exception: invalid regular expression flag r
> > @Scratchpad/2:3
> > */
> > 
> > Pretty Print error do not (but often provide better help spotting the error):
> > 
> > /*
> > Exception: Invalid regexp flag (3:10)
> > raise@resource://gre/modules/devtools/acorn/acorn.js:231:9
> > readRegexp@resource://gre/modules/devtools/acorn/acorn.js:771:43
> > readToken_slash@resource://gre/modules/devtools/acorn/acorn.js:586:38
> > getTokenFromCode@resource://gre/modules/devtools/acorn/acorn.js:692:7
> > readToken@resource://gre/modules/devtools/acorn/acorn.js:731:9
> > getToken@resource://gre/modules/devtools/acorn/acorn.js:146:7
> > prettyFast@resource://gre/modules/devtools/pretty-fast.js:771:7
> > self.onmessage@resource://gre/modules/devtools/server/actors/pretty-print-
> > worker.js:39:7
> > 
> > */
> > 
> > For that reason I propose to use:
> > const RE_SCRATCHPAD_ERROR = /(?:@Scratchpad\/\d+:|\()(\d+):?(\d+)?(?:\)|\n)/;
> > 
> > So selecting a plain number or numbers separated by colon would not match.
> > One would have to manually select the left paren before it and either a
> > right paren or newline after it.
> 
> Dang, I was hoping it would be consistent for both features.  The logic for
> the suggested regex looks good - but what is the point of the \n at the end
> of it?  Could we ignore that?

That ) or \n match at the end is to avoid partial manual matches.

User can still select "(3:10)", but "(3:1" would not match, nor would "@Scratchpad/2:3" in case of
@Scratchpad/2:33
at the end of line.

OK?

Comment 65

[Brian Grinstead [:bgrins]]

> That ) or \n match at the end is to avoid partial manual matches.
> 
> User can still select "(3:10)", but "(3:1" would not match, nor would
> "@Scratchpad/2:3" in case of
> @Scratchpad/2:33
> at the end of line.
> 
> OK?

Matching the paren definitely makes sense, but that @Scratchpad/2:3 string is so specific that no one is likely to have that accidentally selected.  If you manually highlighted that @Scratchpad/2:33 string I would hope that it prepopulates the box, but if needs the newline this would not work.  I don't feel too strongly about it, but I would probably not include the check for the newline.

Comment 66

[Adrian Aichner [:anaran]]

(In reply to Brian Grinstead [:bgrins] from comment #65)
> > That ) or \n match at the end is to avoid partial manual matches.
> > 
> > User can still select "(3:10)", but "(3:1" would not match, nor would
> > "@Scratchpad/2:3" in case of
> > @Scratchpad/2:33
> > at the end of line.
> > 
> > OK?
> 
> Matching the paren definitely makes sense, but that @Scratchpad/2:3 string
> is so specific that no one is likely to have that accidentally selected.  If
> you manually highlighted that @Scratchpad/2:33 string I would hope that it
> prepopulates the box, but if needs the newline this would not work.  I don't
> feel too strongly about it, but I would probably not include the check for
> the newline.

I would still suggest to keep it. It's the equivalence of the ) in the parenthetic case.
I too hope that one day there will be uniform reporting of these errors for pretty print and run.

Comment 67

[Brian Grinstead [:bgrins]]

(In reply to adrian from comment #66)
> (In reply to Brian Grinstead [:bgrins] from comment #65)
> > > That ) or \n match at the end is to avoid partial manual matches.
> > > 
> > > User can still select "(3:10)", but "(3:1" would not match, nor would
> > > "@Scratchpad/2:3" in case of
> > > @Scratchpad/2:33
> > > at the end of line.
> > > 
> > > OK?
> > 
> > Matching the paren definitely makes sense, but that @Scratchpad/2:3 string
> > is so specific that no one is likely to have that accidentally selected.  If
> > you manually highlighted that @Scratchpad/2:33 string I would hope that it
> > prepopulates the box, but if needs the newline this would not work.  I don't
> > feel too strongly about it, but I would probably not include the check for
> > the newline.
> 
> I would still suggest to keep it. It's the equivalence of the ) in the
> parenthetic case.

That's fine

Comment 68

[Adrian Aichner [:anaran]]

Attachment: 0001-Bug-1005471-Scratchpad-Jump-to-line-should-preset-in.patch

Comment 69

[Brian Grinstead [:bgrins]]

(In reply to adrian from comment #68)
> Created attachment 8431793 [details] [diff] [review]
> 0001-Bug-1005471-Scratchpad-Jump-to-line-should-preset-in.patch

Pushed latest patch to try: https://tbpl.mozilla.org/?tree=Try&rev=bd49e45e91d0

Comment 70

[Brian Grinstead [:bgrins]]

Comment on attachment 8431793 [details] [diff] [review]
0001-Bug-1005471-Scratchpad-Jump-to-line-should-preset-in.patch

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

Looking good!  I've left a few minor comments that I'd addressed, but you can reupload the new patch with an r+ after making the changes.

::: browser/devtools/scratchpad/test/browser_scratchpad_run_error_goto_line.js
@@ +1,5 @@
> +/* -*- Mode: js; tab-width: 2; indent-tabs-mode: nil; js-indent-level: 2; fill-column: 80 -*- */
> +/* vim: set ts=2 et sw=2 tw=80: */
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +

The other tests have "use strict" but this one doesn't.  Is there a reason for that?  If not, then I'd say add it

::: browser/devtools/sourceeditor/editor.js
@@ -744,4 @@
>      let div = doc.createElement("div");
>      let inp = doc.createElement("input");
>      let txt = doc.createTextNode(L10N.GetStringFromName("gotoLineCmd.promptTitle"));
> -

Nit: A newline was removed here

@@ +755,5 @@
>      inp.style.MozMarginStart = "1em";
>  
>      div.appendChild(txt);
>      div.appendChild(inp);
> +    if (!this.hasMultipleSelections()) {

Nit: please add a newline before this if

Comment 71

[Adrian Aichner [:anaran]]

Comment on attachment 8431793 [details] [diff] [review]
0001-Bug-1005471-Scratchpad-Jump-to-line-should-preset-in.patch

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

::: browser/devtools/scratchpad/test/browser_scratchpad_run_error_goto_line.js
@@ +1,5 @@
> +/* -*- Mode: js; tab-width: 2; indent-tabs-mode: nil; js-indent-level: 2; fill-column: 80 -*- */
> +/* vim: set ts=2 et sw=2 tw=80: */
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +

Added.

::: browser/devtools/sourceeditor/editor.js
@@ -744,4 @@
>      let div = doc.createElement("div");
>      let inp = doc.createElement("input");
>      let txt = doc.createTextNode(L10N.GetStringFromName("gotoLineCmd.promptTitle"));
> -

Restored

@@ +755,5 @@
>      inp.style.MozMarginStart = "1em";
>  
>      div.appendChild(txt);
>      div.appendChild(inp);
> +    if (!this.hasMultipleSelections()) {

Added

Comment 72

[Adrian Aichner [:anaran]]

Attachment: 0001-Bug-1005471-Scratchpad-Jump-to-line-should-preset-in.patch

Comment 73

[Brian Grinstead [:bgrins]]

Marking checkin-needed.  Relevant try push in Comment 69

Comment 74

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 75

[Carsten Book [:Tomcat]]

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