Closed Bug 1245877 Opened 4 years ago Closed 4 years ago

Include links to external documentation where possible in JS error messages.

Categories

(Core :: JavaScript: Standard Library, defect)

defect
Not set

Tracking

()

RESOLVED DUPLICATE of bug 1179876
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: mrrrgn, Assigned: mrrrgn)

References

Details

Attachments

(2 files, 2 obsolete files)

When a user sees an error message they don't understand they are forced to hunt down documentation. We should aid users by curating this process; including links to relevant MDN or Mozilla Wiki articles in messages (where possible).
Assignee: nobody → winter2718
Spoke with jlongster on IRC. The plan is to add a .errorId property to all Error objects generated by the JS engine itself.

That alone is great, but not quite enough, because the devtools need to know *before* displaying an error message whether or not to display the link to more info.

So for now, I think we'll put a whitelist into the devtools source code: ids for which we have known-good content at some URL. When the console displays an error message for one of those ids, it'll display a link.
See Also: → 1179876
Attached patch errid.diff (obsolete) — Splinter Review
Attached patch errid.diffSplinter Review
Hmm, so this gets the error ids added and attached to error objects. One thing that troubles me is whether or not this should be attached to the ErrorObject's prototype by default like lineNumber is. For now, it's not.
Attachment #8721709 - Attachment is obsolete: true
Attachment #8721710 - Flags: review?(jorendorff)
Comment on attachment 8721710 [details] [diff] [review]
errid.diff

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

Last we spoke, I was willing to just punch it through and put a nonstandard property on Error objects. But this isn't the done thing, so let's do it the right way: add a Debugger method to expose it. It could be a method on Debugger.Object objects.

Unfortunately this will make it harder to get DOMException working (we can't just throw the same-named property on those), but we'll just have to worry about that later.

::: js/src/js.msg
@@ +47,5 @@
>   */
>  
> +MSG_DEF(JSMSG_NOT_AN_ERROR,             0,  JSEXN_NONE,  "<Error #0 is reserved>", 100)
> +MSG_DEF(JSMSG_NOT_DEFINED,              1,  JSEXN_REFERENCEERR,  "{0} is not defined", 110)
> +MSG_DEF(JSMSG_MORE_ARGS_NEEDED,         3,  JSEXN_TYPEERR,  "{0} requires more than {1} argument{2}", 120)

Continuing to bikeshed on this setup...

It'll eventually get hard to verify by eyeball that these are unique - it's 500+ lines. 
And it's a likely mistake: copy and paste a line, change the message, forget to update the <ID> field.
Is it worth adding some code to check_spidermonkey_style.py (or somewhere) that checks them?

(We already found that <ARGUMENT_COUNT> is very easy to forget about, so in debug builds there's a startup assertion that checks all the argument counts. Maybe that's better than check_spidermonkey_style. 500x500/2 is kind of a lot, but it'll go fast.)

    for (size_t i = 0; i < 

I don't think counting by ten solves the problem you're trying to solve: error messages added at the end of the first section will quickly use up all the numbers between 770 and 780. How about counting by 1s, plus a comment:

    // The next available error id is: 379.  If you use it, remember to update this line.

This way we also wouldn't accidentally reuse an error-id after it gets deleted.
Attachment #8721710 - Flags: review?(jorendorff)
In the next patch I'll be using the message names themselves as unique identifiers, along with a debug build assertion that guarantees uniqueness.
Attached patch errornames.diff (obsolete) — Splinter Review
I've sat and thought about whether or not I should have named the debugger method getErrorId instead. In the end I decided to go with something that seems more accurate.
Attachment #8724295 - Flags: review?(jorendorff)
Comment on attachment 8724295 [details] [diff] [review]
errornames.diff

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

Arg, wrong patch.
Attachment #8724295 - Flags: review?(jorendorff)
Attached patch errornames.diffSplinter Review
Okay, this is my real patch.
Attachment #8724295 - Attachment is obsolete: true
Attachment #8724299 - Flags: review?(jorendorff)
Comment on attachment 8724299 [details] [diff] [review]
errornames.diff

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

::: js/src/jit-test/tests/debug/Object-getErrorMessageName.js
@@ +21,5 @@
> +    } catch (ex) {
> +        assertEq(name, gw.getErrorMessageName(ex));
> +    }
> +}
> +

*doh* I left out a single test.

assertEq(gw.getErrorMessageName(new SyntaxError()), undefined);
Comment on attachment 8724299 [details] [diff] [review]
errornames.diff

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

\o/
Attachment #8724299 - Flags: review?(jorendorff) → review+
https://hg.mozilla.org/mozilla-central/rev/f3281396564c
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
This needs one more patch before completion.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1179876
You need to log in before you can comment on or make changes to this bug.