1259563 - Display links to error message documentation alongside relevant errors.

Status: RESOLVED FIXED

(DevTools :: Console, defect, P3)

Description

[Morgan Phillips [:mrrrgn]]

This should work for script errors and js terminal errors.

Comment 1

[Morgan Phillips [:mrrrgn]]

Attachment: errordocs-1of2.diff

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.

Comment 2

[Brian Grinstead [:bgrins]]

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

Comment 3

[Morgan Phillips [:mrrrgn]]

Attachment: errordocs-2of2.diff

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.

Comment 4

[Brian Grinstead [:bgrins]]

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)

Comment 5

[Brian Grinstead [:bgrins]]

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)]:

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f71740ef8656

Comment 7

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

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

Comment 8

[Morgan Phillips [:mrrrgn]]

(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.

Comment 9

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

If it helps the investigation, these failures are in m(oth), not the m(dt) tests...

Comment 10

[Morgan Phillips [:mrrrgn]]

Attachment: errordocs-2of2.diff

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);

Comment 11

[Morgan Phillips [:mrrrgn]]

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.

Comment 12

[Morgan Phillips [:mrrrgn]]

Attachment: errordocs-2of2.diff

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.

Comment 13

[Brian Grinstead [:bgrins]]

Comment on attachment 8740056 [details] [diff] [review]
errordocs-2of2.diff

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

Seems fine

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8defde6e5e29

Comment 15

[Carsten Book [:Tomcat]]

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

Comment 16

[Florian Scholz (Open Web Docs)]

(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!

Comment 17

[Brian Grinstead [:bgrins]]

(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)

Comment 18

[Will Bamberg [:wbamberg]]

(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.