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*/
We're doing developer tool prioritization, filter on 'brontozaur' to ignore the spam.
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?
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
(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.
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.
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.
Created attachment 583734 [details] [diff] [review] 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.
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 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
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?
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.
yup, I'm ok with this, though in all likelihood, we'll want to remove the codepath to add the errors to the console.
Created attachment 584146 [details] [diff] [review] 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.
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://email@example.com
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.
Created attachment 584438 [details] [diff] [review] 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 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 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?
I sure will!
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://firstname.lastname@example.org