690552 - ScratchPad should display exceptions as a comment, just as it does for results

Status: RESOLVED FIXED

(DevTools Graveyard :: Scratchpad, defect, P2)

Description

[Jim Blandy :jimb]

When I evaluate something in ScratchPad, it's a pain that errors show up somewhere else. It would be nice if it behaved like this instead:

1()/*
Exception: 1 is not a function
@Scratchpad:13*/

Comment 1

[Dave Camp (:dcamp)]

We're doing developer tool prioritization, filter on 'brontozaur'
to ignore the spam.

Comment 3

[Kenny Heaton]

I would like to take this on but had some questions about the expected behavior.

Should errors be output as comments for run, inspect and display or only display? The user is only expecting comment output in the display case.

Should it not send errors to the web console when it is outputting then as a comment?

Should this be optional, add a menu item to toggle this feature?

Comment 4

[:Felipe Gomes (needinfo for replies!)]

Robcee should weigh in, but here is my opinion:
 
> Should errors be output as comments for run, inspect and display or only
> display? The user is only expecting comment output in the display case.

Hm this is a good question.. while it makes sense the user is not expecting output in the run case, an exception is usually _unexpected_ output, and it would make the feature more useful. But I can see arguments for both sides

> 
> Should it not send errors to the web console when it is outputting then as a
> comment?
Yeah, I believe it shouldn't send the errors to the console. If the user wants to inspect the error there they should be able to catch the exception and use console.log

> 
> Should this be optional, add a menu item to toggle this feature?
No need for an option for this

Comment 5

[Kenny Heaton]

(In reply to Felipe Gomes (:felipe) from comment #4)
> Robcee should weigh in, but here is my opinion:
>  
> > Should errors be output as comments for run, inspect and display or only
> > display? The user is only expecting comment output in the display case.
> 
> Hm this is a good question.. while it makes sense the user is not expecting
> output in the run case, an exception is usually _unexpected_ output, and it
> would make the feature more useful. But I can see arguments for both sides

The more I think about this the more I'm not sure about outputting the errors as comments in the run scenario. Maybe 'expecting' was the wrong word and its about where the user's focus is. Here is an example, I've used the scratch pad before to test scripts on the page. I write some code, press Ctrl + R and look for the results in the browser and repeat. If in this case the error was output in the scratch pad I might not notice right away because my focus is on the browser, and I now have comments in my code I might need to clean up (especially if I repeat several times and keep getting exceptions).

In the same way if I press Ctrl + L (display) my focus is on the scratch pad and what it's about to output as a comment. So to have the error pop up in the console is a pain and why the bug was opened.

Just my thoughts.

Comment 6

[Panos Astithas (he/him) [:past] (please ni?)]

In my opinion, the comment should only appear in the display case. In general users wouldn't expect the editor contents to be modified when running the code, but the display option is an exception. It's sole purpose is to turn the scratchpad into a console of sorts. For the same reason I think it would make sense to not involve the web console when an exception is generated using the display option. The user's focus will be in the scratchpad as Kenny mentions.

Comment 7

[:Felipe Gomes (needinfo for replies!)]

Sounds right!

Kenny, do you know how to begin working on it? The scratchpad code lives in http://mxr.mozilla.org/mozilla-central/source/browser/devtools/scratchpad/scratchpad.js

The display() function has the logic used to insert the result as a comment, and it first calls the run() function that is the same used when the Run option is selected.

You'll probably need to refactor a bit the evalInContentSandbox/evalInChromeSandbox to also return the exception code instead of always reporting it to the webconsole.

Comment 8

[Kenny Heaton]

Attachment: Patch for bug and test code

I have the error output as a comment in the display case only and refactored a bit to prevent the error from going to the console.
In the test I make sure the error is a comment and not to the console for display. And that it still goes to the console for run.
I did not explicitly test Inspect.
Let me know if I need to make any changes.

Comment 9

[Rob Campbell [:rc] (:robcee)]

hey guys. Nice to see some activity in this bug!

Felipe, past and Kenny: Typically in a Smalltalk Workspace (which Scratchpads are based on), you do see exceptions appear in the editor whenever a user encounters one regardless of execution mode. These are exceptional events and the user should be notified of them in a consistent way.

Also, opening the web console on an error feels like a funny side-effect to me. The distance between my error and my editor seems very far, but it was a suitable stop-gap solution at the time.

Comment 10

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 583734 [details] [diff] [review]
Patch for bug and test code

Thanks for the patch, it looks really good. I have some comments to improve it further before going for review.

- nice job on moving the main parts of run() to execute(), that's exactly what I wanted to do too.

- Usually we avoid boolean parameters which do not have any clear mapping with the function name and with yes/no. So execute(true) and execute(false) ends up being very unclear. To get rid of that, you could make evalIn* to always return the error code, and move the error reporting code to a  separate function.  That way,  run() and inspect() can call that function, while display() writes out the comment instead.  It will also simplify things since you won't need to pass in the extra parameter

- The code style on this file has opening braces on the same line as the if statements. Please also match the style from the file to any of new code you're adding

- Usually on if/else branches the main use case (the non-exception) is put in the if part, and the else is used for the shorter one

- Thanks for providing a test for it! You probably need to clean-up the text and log entries generated by the test, otherwise you may break a test that runs after yours

- we need to consider robcee's explanation to decide on when to display the error as comment. This should be easy to change afterwards too so we could probably experiment and see what works best

Comment 11

[Kenny Heaton]

Robcee's explanation makes sense to me. I'm not familiar with the Samlltalk Workspace but if the Scratchpad is modeled after it, it makes sense to follow the Workspace example.

As I apply your feedback should I keep the code so it can do either one and easily switch back and forth between errors as comments or sending them to the console. Or I can just take the console code out and have it always output as a comment?

Comment 12

[:Felipe Gomes (needinfo for replies!)]

I say we should keep that in a function that takes the error and the context type (using the consts from the top of the file).
If Rob prefers it removed then we can do it easily before landing the patch.

Comment 13

[Rob Campbell [:rc] (:robcee)]

yup, I'm ok with this, though in all likelihood, we'll want to remove the codepath to add the errors to the console.

Comment 14

[Kenny Heaton]

Attachment: Apply feeback to earlyer patch

I refacteored as your suggested and removed the need to the boolean. Your right it makes much more sense this way.
I updated the if/else braces to match the rest of the file. It confused my at first that function open on the next line and if statements open on the same line.
Put the most comment condition in the if and the exception in the else.
Cleaned up the best I could. head.js cleans up the window and tab for me and I think cleaning up the tab sets the gScratchpad to undefined because it throws an error in my clean up function if I try calling any functions on it. I am now reseting the console service.
Errors output for all cases but I left the code in that will let us switch to reporting errors to the consoles with the reportError* functions. These as it is are dead code and can easily be removed.

Comment 15

[Mozilla RelEng Bot]

Try run for c47f92c23ac1 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=c47f92c23ac1
Results (out of 208 total builds):
    success: 178
    warnings: 29
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-c47f92c23ac1

Comment 16

[:Felipe Gomes (needinfo for replies!)]

The test failed on debug builds only. From the logs it looks like some JS strict warnings from orion.js are appearing on the console and tricking the test.

Comment 17

[Kenny Heaton]

Attachment: Updated Patch

I removed the check on the console. It seems to be unreliable and the only reason I had it in the first place was to validate that I didn't regress the run case. But now that run will also be outputting to a comment I don't know that it's really needed.

Comment 18

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 584438 [details] [diff] [review]
Updated Patch

>+  writeAsComment: function SP_writeAdComment(aValue)
small typo

After a fresh look I realized it'd make no sense in landing the functions that will be unused. If we need them in the future they can be easily retrieved from this bug again. No need to upload a new patch, I can remove them for you before pushing.

r+ from me. But I'm not a peer for this module so we'll also need a review from a peer; I'm tagging robcee for it.

If no other changes are required I'll land the patch for you. Thanks for working on this, Kenny!

Comment 19

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 584438 [details] [diff] [review]
Updated Patch

This looks great. Agree that we should pull out the unused methods before pushing.

Felipe, do you want to take care of the landing?

Comment 20

[:Felipe Gomes (needinfo for replies!)]

I sure will!

Comment 21

[:Felipe Gomes (needinfo for replies!)]

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

Comment 22

[Mozilla RelEng Bot]

Try run for 6fc88745b0d7 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=6fc88745b0d7
Results (out of 168 total builds):
    success: 150
    warnings: 18
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-6fc88745b0d7

Comment 23

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/8c43976e73a4