Closed Bug 1251084 Opened 4 years ago Closed 4 years ago

Consolidate source abbreviation utilities

Categories

(DevTools :: Console, defect, P1)

defect

Tracking

(firefox47 fixed)

RESOLVED FIXED
Firefox 47
Tracking Status
firefox47 --- fixed

People

(Reporter: jsantell, Assigned: jsantell)

References

Details

(Whiteboard: [btpp-fix-now])

Attachments

(1 file, 2 obsolete files)

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.
P1 as this blocks source map in console.
Priority: -- → P1
Whiteboard: [btpp-fix-now]
This renders unknown sources as "(unknown)" rather than "<unknown>" now, as well -- should remove the l10n corresponding to that, too.
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}]
Attachment #8723750 - Flags: review?(lclark) → review-
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*
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
Attachment #8723750 - Attachment is obsolete: true
Attachment #8724137 - Flags: review?(lclark)
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 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?
Attachment #8724137 - Flags: review?(lclark) → review-
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
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
Attachment #8724137 - Attachment is obsolete: true
Attachment #8725291 - Flags: review?(lclark)
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.
Attachment #8725291 - Flags: review?(lclark) → review+
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!
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.
Good call; I'll add that to the getSourceNames changes in bug 1251033
https://hg.mozilla.org/mozilla-central/rev/91eedd43f7cf
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.