1447154 - Stop using devtools/shared/system to retrieve AppConstants

Status: RESOLVED FIXED

(DevTools :: General, enhancement, P3)

Description

[Julian Descottes [:jdescottes]]

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.

Comment 1

[Julian Descottes [:jdescottes]]

Attachment: Bug 1447154 - Remove unused getOSCPU method from devtools/shared/system;

Review commit: https://reviewboard.mozilla.org/r/229122/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/229122/

Comment 2

[Julian Descottes [:jdescottes]]

Attachment: Bug 1447154 - Use AppConstants.jsm instead of devtools/shared/system to get constants;

Review commit: https://reviewboard.mozilla.org/r/229124/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/229124/

Comment 3

[Julian Descottes [:jdescottes]]

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.

Comment 4

[Julian Descottes [:jdescottes]]

s/the while/the file while

Comment 5

[Julian Descottes [:jdescottes]]

Comment on attachment 8960367 [details]
Bug 1447154 - Use AppConstants.jsm instead of devtools/shared/system to get constants;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/229124/diff/1-2/

Comment 6

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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

Comment 7

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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

Comment 8

[Nicolas Chevobbe [: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/#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 ?

Comment 9

[Nicolas Chevobbe [:nchevobbe]]

Oops,, I meant thanks Julian :) And yes, the console part seems fine.

Comment 10

[Julian Descottes [:jdescottes]]

Thanks for the reviews! Regarding Launchpad projects, I only found the Console to be impacted, so we should be good on that front.

Comment 11

[Julian Descottes [:jdescottes]]

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

Comment 12

[Julian Descottes [:jdescottes]]

Comment on attachment 8960367 [details]
Bug 1447154 - Use AppConstants.jsm instead of devtools/shared/system to get constants;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/229124/diff/2-3/

Comment 13

[Julian Descottes [:jdescottes]]

Comment on attachment 8960367 [details]
Bug 1447154 - Use AppConstants.jsm instead of devtools/shared/system to get constants;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/229124/diff/3-4/

Comment 14

[Pulsebot]

Pushed by [e-mail]:
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

Comment 15

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/d5f46438881b
https://hg.mozilla.org/mozilla-central/rev/0500c3c58714