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

Status: UNCONFIRMED

(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: Bug 1701396 - Convert devtools/client/performance-new/popup/background.jsm.js to a commonjs module. r=julienw

Comment 2

[Release mgmt bot [: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

[Release mgmt bot [: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.