1403454 - Migrate browser_webconsole_script_errordoc_urls.js to the new frontend

Status: RESOLVED FIXED

(DevTools :: Console, enhancement, P1)

Comment 1

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Bug 1403454 - Migrate browser_webconsole_script_errordoc_urls.js to the new frontend

Review commit: https://reviewboard.mozilla.org/r/219822/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/219822/

Comment 2

[Julian Descottes [:jdescottes]]

Comment on attachment 8950543 [details]
Bug 1403454 - Migrate browser_webconsole_script_errordoc_urls.js to the new frontend

https://reviewboard.mozilla.org/r/219822/#review225572

Thanks! Test will fail on non-e10s (which we still run on some platforms), plus some comments, let's go for another round of review before landing.

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_script_errordoc_urls.js:30
(Diff revision 1)
> +    expected: 'TypeError: "score" is read-only',
>    },
>    {
>      jsmsg: "JSMSG_STMT_AFTER_RETURN",
>      script: "function a() { return; 1 + 1; };",
>      isException: false,

The "isException" field was used to "expectUncaughtException" which is still needed when the test runs in non-e10s.

We usually handle this as follows:

  let isE10s = Services.appinfo.browserTabsRemoteAutostart; 
  if (!isE10s) {
    expectUncaughtException();
  }
  
Here with the added isException we would have:

  if (testData.isException && !isE10s) {
    expectUncaughtException();
  }
  
(to be added before loading the new document)

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_script_errordoc_urls.js:35
(Diff revision 1)
>      isException: false,
> +    expected: "unreachable code after return statement",
>    }
>  ];
>  
> -add_task(function* () {
> +add_task(async function testErrorDocs() {

nit: main test functions are usually not named.

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_script_errordoc_urls.js:44
(Diff revision 1)
> -  if (testData.isException === true) {
> -    expectUncaughtException();
> +  let browser = gBrowser.selectedBrowser;
> +  BrowserTestUtils.loadURI(browser, makeURIData(testData.script));

can use 
  await loadDocument(makeURIData(testData.script))

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_script_errordoc_urls.js:46
(Diff revision 1)
>  
> -function* testScriptError(hud, testData) {
> -  if (testData.isException === true) {
> -    expectUncaughtException();
> +async function testScriptError(hud, testData) {
> +  let browser = gBrowser.selectedBrowser;
> +  BrowserTestUtils.loadURI(browser, makeURIData(testData.script));
> -  }
>  

Can we have an info here before doing the await (in case this part times out it will make log reading easier).

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_script_errordoc_urls.js:60
(Diff revision 1)
>  
> -  let hrefs = {};
> -  for (let link of hud.jsterm.outputNode.querySelectorAll("a")) {
> -    hrefs[link.href] = true;
> +  // Gather all URLs displayed in the console. [Learn More] links have no href
> +  // but have the URL in the title attribute.
> +  let hrefs = new Set();
> +  for (let link of hud.ui.outputNode.querySelectorAll("a")) {
> +    hrefs.add(link.href || link.title);

According to your comment above, link.href is always falsy here, why keep it?

Comment 3

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8950543 [details]
Bug 1403454 - Migrate browser_webconsole_script_errordoc_urls.js to the new frontend

https://reviewboard.mozilla.org/r/219822/#review225598

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_script_errordoc_urls.js:60
(Diff revision 1)
>  
> -  let hrefs = {};
> -  for (let link of hud.jsterm.outputNode.querySelectorAll("a")) {
> -    hrefs[link.href] = true;
> +  // Gather all URLs displayed in the console. [Learn More] links have no href
> +  // but have the URL in the title attribute.
> +  let hrefs = new Set();
> +  for (let link of hud.ui.outputNode.querySelectorAll("a")) {
> +    hrefs.add(link.href || link.title);

You are right, the links we are interested in here don't have hrefs (although there are other links that do). Removed condition.

Comment 4

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8950543 [details]
Bug 1403454 - Migrate browser_webconsole_script_errordoc_urls.js to the new frontend

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/219822/diff/1-2/

Comment 5

[Julian Descottes [:jdescottes]]

Comment on attachment 8950543 [details]
Bug 1403454 - Migrate browser_webconsole_script_errordoc_urls.js to the new frontend

https://reviewboard.mozilla.org/r/219822/#review225660

Looks good Mike, thanks!

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/17454f8c2ea8
Migrate browser_webconsole_script_errordoc_urls.js to the new frontend r=jdescottes

Comment 7

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/17454f8c2ea8