Stop using devtools/shared/system to retrieve AppConstants

RESOLVED FIXED in Firefox 61

Status

enhancement
P3
normal
RESOLVED FIXED
Last year
Last year

People

(Reporter: jdescottes, Assigned: jdescottes)

Tracking

unspecified
Firefox 61

Firefox Tracking Flags

(firefox61 fixed)

Details

Attachments

(2 attachments)

devtools/shared/system.js is complex and not regularly maintained (still mentions b2g). It is imported by several files only to get access to resource://gre/modules/AppConstants.jsm.

The main complexity of the file is mostly relevant to the current performance panel and it might make the dependency relationship clearer if other files directly relied on AppConstants.jsm.
Came across the while reviewing a b2g cleanup, and was surprised to see it used so widely in DevTools. Let me know if this makes sense in your opinion.
s/the while/the file while
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Comment on attachment 8960366 [details]
Bug 1447154 - Remove unused getOSCPU method from devtools/shared/system;

https://reviewboard.mozilla.org/r/229122/#review234954

Thanks for doing this cleanup! :)
Attachment #8960366 - Flags: review?(jryans) → review+
Attachment #8960367 - Flags: review?(nchevobbe)
Comment on attachment 8960367 [details]
Bug 1447154 - Use AppConstants.jsm instead of devtools/shared/system to get constants;

https://reviewboard.mozilla.org/r/229124/#review234956

Overall, I think this looks fine in general from an m-c perspective.  We don't really gain anything by indirecting the constants through an extra module, so it seems fine to undo that.

I noticed that the perf UI imports[1] `system` as a global, but doesn't seem to actually use it...?  Can this reference also be removed?  (Perf server code does use it, but this client reference seems removable.)

I am less clear on the impact this might have to Launchpad and other GitHub-based workflows.  I see you fixed a console stub here related to this, but are there any other projects somewhere on GitHub that need similar treatment?  I added :nchevobbe to review that aspect.

[1]: https://searchfox.org/mozilla-central/rev/877c99c523a054419ec964d4dfb3f0cadac9d497/devtools/client/performance/performance-controller.js#50
Attachment #8960367 - Flags: review?(jryans) → review+
Comment on attachment 8960367 [details]
Bug 1447154 - Use AppConstants.jsm instead of devtools/shared/system to get constants;

https://reviewboard.mozilla.org/r/229124/#review235002

This looks good to me, thanks Ryan. 
I only have one small comment but this is good to go with a green try

::: commit-message-d4187:1
(Diff revision 2)
> +Bug 1447154 - Use AppConstants.jsm instead of devtools/shared/system to get constants;r=jryans

Could we explain briefly why we do that ?
Attachment #8960367 - Flags: review?(nchevobbe) → review+
Oops,, I meant thanks Julian :) And yes, the console part seems fine.
Thanks for the reviews! Regarding Launchpad projects, I only found the Console to be impacted, so we should be good on that front.
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d5f46438881b
Remove unused getOSCPU method from devtools/shared/system;r=jryans
https://hg.mozilla.org/integration/autoland/rev/0500c3c58714
Use AppConstants.jsm instead of devtools/shared/system to get constants;r=jryans,nchevobbe
https://hg.mozilla.org/mozilla-central/rev/d5f46438881b
https://hg.mozilla.org/mozilla-central/rev/0500c3c58714
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.