Closed Bug 1259563 Opened 4 years ago Closed 4 years ago

Display links to error message documentation alongside relevant errors.

Categories

(DevTools :: Console, defect, P3)

defect

Tracking

(firefox48 fixed)

RESOLVED FIXED
Firefox 48
Tracking Status
firefox48 --- fixed

People

(Reporter: mrrrgn, Assigned: mrrrgn)

References

Details

(Whiteboard: [btpp-backlog])

Attachments

(2 files, 2 obsolete files)

This should work for script errors and js terminal errors.
Blocks: 1179876
Assignee: nobody → winter2718
No longer blocks: 1179876
This tweaks nsIScriptError, to allow access to error message names, so that we can find error doc urls. The error doc links are generated via an existing mechanism for creating [Learn More] links.
Attachment #8734499 - Flags: review?(bgrinstead)
Comment on attachment 8734499 [details] [diff] [review]
errordocs-1of2.diff

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

Looks good to me!  Fits in with the existing 'learn more' functionality well

::: devtools/client/webconsole/test/browser_webconsole_script_errordoc_urls.js
@@ +8,5 @@
> +
> +"use strict";
> +
> +const ErrorDocs = require("devtools/server/actors/errordocs");
> +

Nit: extra blank line

@@ +16,5 @@
> +}
> +
> +const TestData = [
> +  {
> +    jsmsg: "JSMSG_READ_ONLY",

So I think we can have a smaller set of tests here so that when a new URL is added to errordocs.js people can add a case only to test_page_errors.html b/c otherwise I could see this one falling out of sync.  We probably need just a couple cases here plus a link to the other test

@@ +72,5 @@
> +  if (testData.isException) {
> +    expectUncaughtException();
> +  }
> +
> +  content.location = makeURIData(testData.script);

Please use BrowserTestUtils.loadURI(gBrowser.selectedBrowser, testData.script); so we don't access CPOW

@@ +91,5 @@
> +  for (let link of hud.jsterm.outputNode.querySelectorAll("a")) {
> +    hrefs[link.href] = true;
> +  }
> +
> +  ok(url in hrefs);

Add a second param here with a message in case it fails
Attachment #8734499 - Flags: review?(bgrinstead) → review+
Priority: -- → P3
Whiteboard: [btpp-backlog]
Depends on: 1261904
Attached patch errordocs-2of2.diff (obsolete) — Splinter Review
The "getErrorMessageName" method on Debugger.object was replaced with a property "errorMessageName" (a better API) which allows us to avoid relying on dereference.

This patch adds support for error doc links alongside errors created for some statement entered into the web console itself.
Attachment #8738931 - Flags: review?(bgrinstead)
Comment on attachment 8738931 [details] [diff] [review]
errordocs-2of2.diff

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

This looks good to me, thanks!

::: devtools/client/webconsole/jsterm.js
@@ +319,5 @@
> +      errorDocLink.title = errorDocURL;
> +      errorDocLink.href = "#";
> +      errorDocLink.draggable = false;
> +      errorDocLink.addEventListener("click", () => {
> +        this.hud.owner.openLink(errorDocURL);

We should also preventDefault inside this handler

::: devtools/server/actors/webconsole.js
@@ +889,5 @@
>                                  error.unsafeDereference();
>          errorMessage = unsafeDereference && unsafeDereference.toString
>            ? unsafeDereference.toString()
>            : String(error);
> +        errorDocURL = ErrorDocs.GetURL(error.errorMessageName);

We should be defensive when accessing properties on `error` here because of some security wrapper issue in workers (Bug 1224073).

Something like this should work

errorDocURL = ErrorDocs.GetURL(error && error.errorMessageName)
Attachment #8738931 - Flags: review?(bgrinstead) → review+
Release Note Request (optional, but appreciated)
[Why is this notable]: Webconsole will provide links to documentation about common JS errors that happen on the page
[Suggested wording]:
[Links (documentation, blog post, etc)]:
relnote-firefox: --- → ?
(In reply to Wes Kocher (:KWierso) from comment #7)
> I had to back this out because it apparently caused some webconsole tests to
> fail like
> https://treeherder.mozilla.org/logviewer.html#?job_id=25440266&repo=mozilla-
> inbound
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/767a6c8090a7

Oy vey, investigating. Will try landing again Monday with some changes to effected tests.
Flags: needinfo?(winter2718)
If it helps the investigation, these failures are in m(oth), not the m(dt) tests...
Attached patch errordocs-2of2.diff (obsolete) — Splinter Review
Apologies for the re-review. One test caused CheckedUnwrap to fail, raising a permission error. To deal with this, I really just needed to wrap error.errorMessageName in a try statement. If a permission check fails now the user simply won't see an error doc link.

+
+        let errorMessageName;
+
+        // It is possible that we won't have permission to unwrap an
+        // object and retrieve its errorMessageName.
+        if (error) {
+          try {
+            errorMessageName = error.errorMessageName;
+          } catch (ex) {}
+        }
+        errorDocURL = ErrorDocs.GetURL(errorMessageName);
Attachment #8738931 - Attachment is obsolete: true
Attachment #8740046 - Flags: review?(bgrinstead)
Comment on attachment 8740046 [details] [diff] [review]
errordocs-2of2.diff

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

::: devtools/server/actors/webconsole.js
@@ +899,5 @@
> +          try {
> +            errorMessageName = error.errorMessageName;
> +          } catch (ex) {}
> +        }
> +        errorDocURL = ErrorDocs.GetURL(errorMessageName);

This should be inside the if statement. Changed on my end.
Thought for a moment about the tradeoffs of wrapping ErrorDocs.GetURL in a try statement along with error.errorMessageName. I think it's fair considering that GetURL is extremely simple |(errorName) => errorDocs[errorName];| and doing so makes the code more readable.
Attachment #8740046 - Attachment is obsolete: true
Attachment #8740046 - Flags: review?(bgrinstead)
Attachment #8740056 - Flags: review?(bgrinstead)
Comment on attachment 8740056 [details] [diff] [review]
errordocs-2of2.diff

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

Seems fine
Attachment #8740056 - Flags: review?(bgrinstead) → review+
Component: Developer Tools → Developer Tools: Console
https://hg.mozilla.org/mozilla-central/rev/8defde6e5e29
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
(In reply to Brian Grinstead [:bgrins] from comment #5)
> Release Note Request (optional, but appreciated)
> [Why is this notable]: Webconsole will provide links to documentation about
> common JS errors that happen on the page
> [Suggested wording]:
> [Links (documentation, blog post, etc)]:

I suggest to wait with announcing this more broadly until we actually document more errors on MDN. Currently, we provide links for a handful of errors only. I don't know to which release this will be tied to exactly. 48 is shipping in August (beta in June). I am finishing up some other work and will go on vacation, but I hope to then focus on this a lot for the rest of the quarter. Also, once we have the telemetry from bug 1255133, we'll have data for focusing on the most common errors here, so that we feed real users needs. I think there is a good chance that we will have docs out there until August, but we need to see if/how to uplift MDN links to the tree(s). That said, this might only be most visible in 49 or 50. I'm all excited nevertheless!
relnote-firefox: ? → ---
(In reply to Florian Scholz [:fscholz] (MDN) from comment #16)
> (In reply to Brian Grinstead [:bgrins] from comment #5)
> > Release Note Request (optional, but appreciated)
> > [Why is this notable]: Webconsole will provide links to documentation about
> > common JS errors that happen on the page
> > [Suggested wording]:
> > [Links (documentation, blog post, etc)]:
> 
> I suggest to wait with announcing this more broadly until we actually
> document more errors on MDN. Currently, we provide links for a handful of
> errors only. I don't know to which release this will be tied to exactly. 48
> is shipping in August (beta in June). I am finishing up some other work and
> will go on vacation, but I hope to then focus on this a lot for the rest of
> the quarter. Also, once we have the telemetry from bug 1255133, we'll have
> data for focusing on the most common errors here, so that we feed real users
> needs. I think there is a good chance that we will have docs out there until
> August, but we need to see if/how to uplift MDN links to the tree(s). That
> said, this might only be most visible in 49 or 50. I'm all excited
> nevertheless!

OK, works for me as long as we don't forget to announce it once everything is in place (looking forward to it)
(In reply to (Unavailable until April 25) Brian Grinstead [:bgrins] from comment #17)
> (In reply to Florian Scholz [:fscholz] (MDN) from comment #16)
> > (In reply to Brian Grinstead [:bgrins] from comment #5)
> > > Release Note Request (optional, but appreciated)
> > > [Why is this notable]: Webconsole will provide links to documentation about
> > > common JS errors that happen on the page
> > > [Suggested wording]:
> > > [Links (documentation, blog post, etc)]:
> > 
> > I suggest to wait with announcing this more broadly until we actually
> > document more errors on MDN. Currently, we provide links for a handful of
> > errors only. I don't know to which release this will be tied to exactly. 48
> > is shipping in August (beta in June). I am finishing up some other work and
> > will go on vacation, but I hope to then focus on this a lot for the rest of
> > the quarter. Also, once we have the telemetry from bug 1255133, we'll have
> > data for focusing on the most common errors here, so that we feed real users
> > needs. I think there is a good chance that we will have docs out there until
> > August, but we need to see if/how to uplift MDN links to the tree(s). That
> > said, this might only be most visible in 49 or 50. I'm all excited
> > nevertheless!
> 
> OK, works for me as long as we don't forget to announce it once everything
> is in place (looking forward to it)

Then I'm removing ddn from this one, and adding it to bug 1179876.
Keywords: dev-doc-needed
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.