1251084 - Consolidate source abbreviation utilities

Status: RESOLVED FIXED

(DevTools :: Console, defect, P1)

Description

[Jordan Santell [:jsantell] [@jsantell]]

We have one `WebConsoleUtils.abbreviateSourceURL` in devtools/shared/webconsole/utils.js that's mainly used in webconsole, with a few other consumers using only this function from this file (and it's weird to use a "webconsole" utility here). It's not used anywhere else in shared or server, so this can be in client.

The source frame component uses it's own in devtools/client/shared/source-utils.js. They do very similar things, the source-utils does more than just parse a good name as well.

Comment 1

[Jordan Santell [:jsantell] [@jsantell]]

P1 as this blocks source map in console.

Comment 2

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1251084-remove-abbreviate-source-url.patch

https://treeherder.mozilla.org/#/jobs?repo=try&revision=07f3ff11ca3b

Comment 3

[Jordan Santell [:jsantell] [@jsantell]]

This renders unknown sources as "(unknown)" rather than "<unknown>" now, as well -- should remove the l10n corresponding to that, too.

Comment 4

[Lin Clark [:linclark]]

Comment on attachment 8723750 [details] [diff] [review]
1251084-remove-abbreviate-source-url.patch

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

The try push is showing a lot of failing tests. I tried one of them on local and got this:

> INFO Console message: [JavaScript Error: "ReferenceError: getSourceNames is not defined" {file: "resource://devtools/client/shared/widgets/VariablesView.jsm" line: 3751}]

Comment 5

[Jordan Santell [:jsantell] [@jsantell]]

Yeah, didn't include it in that file -- fixed and found real issues of rendering `data:` URIs as source locations does not currently work how we think it does. For example:

Source: `data:text/html,<p>hello%20from%20iframe</p>`
Currently shortened to just `p>`
This patches fixes it to correctly filter out the mime/charset and displays as `data<p>hello%20from%20iframe</p>`.

Unfortunately some test relied on this broken behavior.. *keeps digging*

Comment 6

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1251084-remove-abbreviate-source-url.patch

The waitForMessage was picking up a warning of a missing charset once the data URIs were rendering correctly (looking for a "<p>" in output, which could be found in this warning once rendering correctly) -- added a charset to fix that.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b7bc2cd28ffc

Comment 7

[Jordan Santell [:jsantell] [@jsantell]]

Comment on attachment 8724137 [details] [diff] [review]
1251084-remove-abbreviate-source-url.patch

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

::: devtools/client/webconsole/webconsole.js
@@ +2509,4 @@
>      }
>  
>      filenameNode.className = "filename";
> +    filenameNode.textContent = ` ${filename}`;

getSourceNames handles the fallback "(unknown)" if the name isn't found

Comment 8

[Lin Clark [:linclark]]

Comment on attachment 8724137 [details] [diff] [review]
1251084-remove-abbreviate-source-url.patch

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

It looks like this makes 1 test orange across platforms (browser_memory_allocationStackBreakdown_01.js). The patch is also is failing ESlint validation at the moment.

::: devtools/client/shared/components/frame.js
@@ -9,3 @@
>  const { DOM: dom, createClass, PropTypes } = require("devtools/client/shared/vendor/react");
>  const { getSourceNames } = require("devtools/client/shared/source-utils");
> -const UNKNOWN_SOURCE_STRING = L10N.getStr("frame.unknownSource");

If we're removing this use of the `frame.unknownSource` string, does it make sense to remove the other use of it?

https://dxr.mozilla.org/mozilla-central/source/devtools/client/memory/components/shortest-paths.js#18

::: devtools/client/shared/source-utils.js
@@ +32,5 @@
>  
>  // The cache used in the `nsIURL` function.
>  const gURLStore = new Map();
> +// The cache used in the `getSourceNames` function.
> +const gSourceNamesStore = new Map();

I'm curious about why caching is being added here. Is the getSourceNames function called a lot with the same parameter? Is the calculation heavy?

Comment 9

[Jordan Santell [:jsantell] [@jsantell]]

Comment on attachment 8724137 [details] [diff] [review]
1251084-remove-abbreviate-source-url.patch

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

::: devtools/client/shared/components/frame.js
@@ -9,3 @@
>  const { DOM: dom, createClass, PropTypes } = require("devtools/client/shared/vendor/react");
>  const { getSourceNames } = require("devtools/client/shared/source-utils");
> -const UNKNOWN_SOURCE_STRING = L10N.getStr("frame.unknownSource");

Good call -- removing it from elsewhere (memory) where it's being passed in as a second arg to getSourceNames

::: devtools/client/shared/source-utils.js
@@ +32,5 @@
>  
>  // The cache used in the `nsIURL` function.
>  const gURLStore = new Map();
> +// The cache used in the `getSourceNames` function.
> +const gSourceNamesStore = new Map();

The function is called a lot (especially when constructing the call tree), hence lots of C-style optimizations in this file (yuck). IIRC there was a performance improvement when caching the URL parsing, but this also caches just the name construction, which will skip even more processing anytime we display a URL, as this'll be used for every console.log

Comment 10

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1251084-remove-abbreviate-source-url.patch

The memory failure is from a build 2 weeks ago or so that was subsequently fixed; new try to ensure:  https://treeherder.mozilla.org/#/jobs?repo=try&revision=3c935f8f23f7

None of the eslint failures are from this build or files; most of the affect files are disabled from eslint is my guess as well; is that what you mean? If we're enforcing that for any tools, I'd imagine they should be enabled on try (for fear of constant chasing) -- let me know though, this is admittedly not part of my normal workflow yet

Comment 11

[Lin Clark [:linclark]]

Comment on attachment 8725291 [details] [diff] [review]
1251084-remove-abbreviate-source-url.patch

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

You're right, I wasn't looking at the files that were failing ESLint validation. Those were unrelated to the patch (and it looks like a fix has already been committed).

Just one little nit. We should maybe consider doing a basic test of the cache logic (running it twice with the same parameters), but I leave that up to you.

::: devtools/client/webconsole/test/test-console-output-dom-elements.html
@@ +1,5 @@
>  <!DOCTYPE HTML>
>  <html dir="ltr" lang="en-US">
>  <head>
>    <meta charset="utf-8">
> +  <title>Test the web console output - 04</title>

There's already a test with this title, test-console-output-04.html.

Comment 12

[Jordan Santell [:jsantell] [@jsantell]]

Updated <title> tag; spent some time trying to figure out how to test the cache logic, but can't think of a way to do so without exposing more than I'd like (like the store itself). Any ideas? Can do in a follow up. Thanks!

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/91eedd43f7cf

Comment 14

[Lin Clark [:linclark]]

I was just thinking of a basic test: calling the function with the same parameters and testing the second return. My thinking was less about testing that it's caching properly and more about testing that the caching logic doesn't break anything.

Comment 15

[Jordan Santell [:jsantell] [@jsantell]]

Good call; I'll add that to the getSourceNames changes in bug 1251033

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/91eedd43f7cf