1701396 - Convert devtools/client/performance-new/popup/background.jsm.js to a commonjs module

Status: RESOLVED WONTFIX

(DevTools :: Performance Tools (Profiler/Timeline), task, P3)

Description

[Andrey Bienkowski]

According to Bug 1182194 JavaScript code modules should be converted to commonjs. I'll submit a patch to convert devtools/client/performance-new/popup/background.jsm.js

Comment 1

[Andrey Bienkowski]

Attachment: WIP: Bug 1701396 - Convert devtools/client/performance-new/popup/background.jsm.js to a commonjs module. r=jdescottes

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

The Bugbug bot thinks this bug should belong to the 'DevTools::Performance Tools (Profiler/Timeline)' component, and is moving the bug to that component. Please revert this change in case you think the bot is wrong.

Comment 3

[Julien Wajsberg [:julienw]]

Thanks for the patch, I'll try to look at it shortly!

We already had bug 1592907 for this topic, you may want to look at it. Indeed I outlined there some issues to take care when doing this work.

Comment 4

[Andrey Bienkowski]

I have replaced ChromeUtils.import("resource://gre/modules/Services.jsm") with require("Services") and removed ChromeUtils.import("resource://gre/modules/osfile.jsm") as it was apparently unused. Tests still pass.

Comment 5

[Andrey Bienkowski]

Hi, Julien. Given the lack of response (besides yours) to my comment in chat I don't think anyone knows whether require() or ChromeUtils.import() is preferable when importing jsm from commonjs. Let's find out! If we merge this and no one complains - we did good. If someone complains - we fix it. Both methods are currently in use: require(), ChromeUtils().

Pick your poison and I'll brew it for you unless you have a better idea.

Comment 6

[Julien Wajsberg [:julienw]]

I think both are working the same, so let's not overthink this too much :-) Maybe Julian will have some more comments, let's see.

Comment 8

[BugBot [:suhaib / :marco/ :calixte]]

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:hexagonrecursion, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 9

[Julien Wajsberg [:julienw]]

yes, I need to have a final look and land this.
I keep my need info.

Comment 10

[Julian Descottes [:jdescottes]]

Hi Julien, Reminder to finish the review and land

Comment 11

[Julien Wajsberg [:julienw]]

I admit that because this is more a "cleanup" patch it's not in my priority list. But I'll try to spend a few hours on this.

Comment 12

[Andrey Bienkowski]

I'll rebase or redo this, but I might not be able to find time to work on this until Monday. Sorry. I wonder if I could somehow split this into smaller and easier to review chunks.

Comment 13

[Julian Descottes [:jdescottes]]

FWIW, I attempted to rebase and it seems fine (+ had to fix a few new call sites for helpers from the background module). Pushed to try, let's see if we have a green one: https://treeherder.mozilla.org/#/jobs?repo=try&revision=193d42cc8b723007ee8db80011c132fdb2bdd735

And we should still try to land r+'d patches without too much delay, otherwise it takes a lot more time because we need to re-review, rebase etc... :)

Comment 14

[Andrey Bienkowski]

I have redone my patch and submitted it to try. https://treeherder.mozilla.org/jobs?repo=try&revision=2476a0baabf08acfbc8a5d8a65818205921a55bb . Intermittent bugs are making things difficult. For reference: this is a "clean" commit I grabbed from mozilla-central.

Comment 15

[Andrey Bienkowski]

Sorry: forgot to post the link to the clean commit: https://treeherder.mozilla.org/jobs?repo=try&revision=193d42cc8b723007ee8db80011c132fdb2bdd735

I see that try did not like my patch. I will investigate later

Comment 16

[Julian Descottes [:jdescottes]]

(In reply to Andrey Bienkowski from comment #15)

Sorry: forgot to post the link to the clean commit: https://treeherder.mozilla.org/jobs?repo=try&revision=193d42cc8b723007ee8db80011c132fdb2bdd735

I see that try did not like my patch. I will investigate later

The link above is my try push (which had some mistakes apparently), not to a clean central commit. But yes, we have existing intermittents.

Comment 17

[Andrey Bienkowski]

Good catch! I posted the wrong link https://treeherder.mozilla.org/jobs?repo=try&revision=a47bf8ba596f346029005b459ab95bdf77c55c64 - 12 intermittents

Comment 18

[Julien Wajsberg [:julienw]]

Thanks!
I retriggered a few of the intermittents that seemed the most concerning to me (about devtools/client/performance-new/test/browser/browser_devtools-record-capture.js). Let's see how this goes.

Comment 19

[Julien Wajsberg [:julienw]]

(In reply to Julien Wajsberg [:julienw] from comment #18)

Thanks!
I retriggered a few of the intermittents that seemed the most concerning to me (about devtools/client/performance-new/test/browser/browser_devtools-record-capture.js). Let's see how this goes.

This looks good, these are green now!

Comment 20

[Andrey Bienkowski]

1. My commit fails to lint -- fixed
2. As helpfully pointed out by typescript (source-test-node-devtools-tests) resource://devtools/shared/Loader.jsm is now resource://devtools/shared/loader/Loader.jsm -- fixed

A new try push: https://treeherder.mozilla.org/jobs?repo=try&revision=b8c0a455f0dafa606d5fcb3c5630a5aa6c72a424

Comment 21

[Andrey Bienkowski]

3. In devtools/client/performance-new/test/browser/head.js I used require before it is defined -- fixed

Comment 22

[Andrey Bienkowski]

Now I get:
Mozilla crash reason: Missing chrome or resource URLs: resource://devtools/client/performance-new/popup/background.js

I have figured it out. The above error is from my local tests. It turns out that I am supposed to rerun mach build after changing a moz.build file.

There are still errors:
https://treeherder.mozilla.org/jobs?repo=try&revision=3a64c656c8a769e0a47c4cd19661c421c7dfd355

Comment 23

[Andrey Bienkowski]

4. The following hack that was required to make typescript work with JSM was breaking commonjs exports:

// Provide a fake module.exports for the JSM to be properly read by TypeScript.
/** @type {any} */ (this).module = { exports: {} };

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

Comment 24

[Andrey Bienkowski]

Fixed: lint errors
Fixed: ReferenceError: require is not defined at /builds/worker/workspace/build/tests/xpcshell/tests/devtools/client/performance-new/test/xpcshell/head.js:12

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

Comment 25

[Andrey Bienkowski]

There are some failures:

• TEST-UNEXPECTED-FAIL | devtools/client/performance-new/test/browser/browser_devtools-record-capture.js | Uncaught exception in test - at chrome://mochitests/content/browser/devtools/client/performance-new/test/browser/helpers.js:405 - Error: The fake frontend indicated that there was an error injecting the profile.
• TEST-UNEXPECTED-FAIL | devtools/client/storage/test/browser_storage_indexeddb_duplicate_names.js | There is correct number of rows in indexedDB > https://test1.example.org - Got 5, expected 6
• TEST-UNEXPECTED-FAIL | devtools/client/performance/test/browser_perf-calltree-js-categories.js | A category node with the text Gecko is displayed in the tree. -
• TEST-UNEXPECTED-FAIL | devtools/client/debugger/test/mochitest/browser_dbg-quick-open.js | Test timed out -
• TEST-UNEXPECTED-FAIL | devtools/client/inspector/animation/test/browser_animation_pause-resume-button_end-time.js | The playState of animation [0] should be running - Got "paused", expected "running"
• TEST-UNEXPECTED-FAIL | devtools/client/webconsole/test/browser/browser_webconsole_scroll.js | The console stayed scrolled to the top - Got 2543, expected +0
• TEST-UNEXPECTED-FAIL | devtools/client/aboutdebugging/test/browser/browser_aboutdebugging_devtoolstoolbox_navigate_reload_button.js | leaked 2 window(s) until shutdown [url = about:devtools-toolbox?id=376&type=tab]
• TEST-UNEXPECTED-FAIL | devtools/client/aboutdebugging/test/browser/browser_aboutdebugging_devtoolstoolbox_navigate_reload_button.js | leaked 2 window(s) until shutdown [url = about:devtools-toolbox?id=376&type=tab]
• TEST-UNEXPECTED-FAIL | devtools/client/netmonitor/test/browser_net_timing-division.js | Test timed out -
• TEST-UNEXPECTED-FAIL | devtools/client/webconsole/test/browser/browser_webconsole_warning_group_multiples.js | Test timed out -

I suspect they might be intermittents, but something is broken and I don't have permission to repeat jobs.

Comment 26

[Andrey Bienkowski]

I have found a bug (race condition) at https://searchfox.org/mozilla-central/source/devtools/client/performance-new/test/browser/helpers.js#394. This was making it difficult to reproduce one of the failures above locally. I'll report this as a separate bug.

Comment 27

[Nancie]

Many thanks to all of you for sharing your knowledge with us to prevent any bug errors while compiling the code of two languages together. I'm a coding tutor working with world-class coding mentors in the world. I've been teaching students for over a decade by providing them with the right solutions for their college projects and assignments.

I met many students who had a heavy workload to solve their coding problems with Javascript to get the code to compile correctly. I have wide experience in solving the doubts of students regarding any type of coding problem within the deadline for submission of their assignments. I would recommend that you take a look at my blog which is most informative. Visit at https://universityhomeworkhelp.com/javascript-homework-help/

Comment 28

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.

Comment 29

[Julien Wajsberg [:julienw]]

I had a look at this yesterday. I rebased the patch but didn't finish it yet. Indeed I realized we probably don't want to do this now, instead we'll want to convert it to an ESM directly when we'll convert the rest of the codebase.

Also the file is now used in other places and changing this file would make it necessary to load the devtools' Loader.sys.mjs as well, which can be possibly costly and brings some distraction in the other codebase.

In this case I believe the drawbacks outweight the advantages, so I'm gonna close this bug.