Open Bug 1701396 Opened 1 year ago Updated 3 months ago

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

Categories

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

task

Tracking

(Not tracked)

UNCONFIRMED

People

(Reporter: hexagonrecursion, Assigned: hexagonrecursion, NeedInfo)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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

Assignee: nobody → hexagonrecursion
Blocks: 1182194

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.

Component: General → Performance Tools (Profiler/Timeline)

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.

See Also: → 1592907
See Also: → 1592914
Severity: -- → S4
Priority: -- → P3

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.

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.

Flags: needinfo?(felash)

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.

Flags: needinfo?(felash)
Duplicate of this bug: 1592907

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.

Flags: needinfo?(hexagonrecursion)
Flags: needinfo?(felash)

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

Flags: needinfo?(hexagonrecursion)

Hi Julien, Reminder to finish the review and land

Flags: needinfo?(felash)
Flags: needinfo?(felash)

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.

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.

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... :)

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.

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

(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.

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.

(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!

  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

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

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

  1. 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

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

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.

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.

You need to log in before you can comment on or make changes to this bug.