Closed
Bug 1353656
Opened 7 years ago
Closed 7 years ago
Stop using sdk/system/child_process/subprocess in WebIDE
Categories
(DevTools Graveyard :: WebIDE, enhancement, P1)
DevTools Graveyard
WebIDE
Tracking
(Performance Impact:low, firefox56 fixed)
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: pbro, Assigned: ochameau)
References
Details
(Whiteboard: [nosdk])
Attachments
(2 files)
We're trying to stop using certain SDK APIs in bug 1350645, including sdk/system/child_process/subprocess which happens to be used in WebIDE. The usages of child_process in WebIDE are for building apps (not important anymore) and running Firefox OS simulators (also not important). So, let's kill these parts of WebIDE so we can stop using this SDK API. The other solution is to kill WebIDE altogether (see bug 1314811), but this is a more complicated thing to do.
Updated•7 years ago
|
Whiteboard: [qf]
Comment 1•7 years ago
|
||
If getting the WebIDE out is not on the short-term horizon (apparently, bug 1212802 is a dependency for removing WebIDE, and is not being worked on yet), then Subprocess.jsm might be the short-term solution to getting off of this SDK module.
Updated•7 years ago
|
Whiteboard: [qf] → [qf:p3]
Reporter | ||
Comment 3•7 years ago
|
||
(In reply to Mike Conley (:mconley) from comment #1) > If getting the WebIDE out is not on the short-term horizon (apparently, bug > 1212802 is a dependency for removing WebIDE, and is not being worked on > yet), then Subprocess.jsm might be the short-term solution to getting off of > this SDK module. Sounds good to me. Thanks. Apparently sdk/system/child_process/subprocess is basically a wrapper around Subprocess.jsm, so that shouldn't be too hard. In WebIDE, the following 2 files use sdk/system/child_process/subprocess: devtools\client\webide\modules\build.js devtools\client\webide\modules\simulator-process.js
(In reply to Patrick Brosset <:pbro> from comment #3) > (In reply to Mike Conley (:mconley) from comment #1) > > If getting the WebIDE out is not on the short-term horizon (apparently, bug > > 1212802 is a dependency for removing WebIDE, and is not being worked on > > yet), then Subprocess.jsm might be the short-term solution to getting off of > > this SDK module. > Sounds good to me. Thanks. > Apparently sdk/system/child_process/subprocess is basically a wrapper around > Subprocess.jsm, so that shouldn't be too hard. > In WebIDE, the following 2 files use sdk/system/child_process/subprocess: > devtools\client\webide\modules\build.js build.js seems reasonable to just remove at this stage. > devtools\client\webide\modules\simulator-process.js This one is probably more trouble to simply remove, so converting it to use Subprocess.jsm seems like the fastest path.
Updated•7 years ago
|
Flags: qe-verify-
Whiteboard: [qf:p3] → [nosdk] [qf:p3]
Assignee | ||
Comment 5•7 years ago
|
||
I'll try to cleanup some things here as I'm the last one of WebIDE era around.
Assignee: nobody → poirot.alex
Updated•7 years ago
|
Status: NEW → ASSIGNED
Priority: P2 → P1
Target Milestone: --- → Firefox 56
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8889522 -
Flags: review?(jryans)
Attachment #8889523 -
Flags: review?(jryans)
Comment 8•7 years ago
|
||
mozreview-review |
Comment on attachment 8889523 [details] Bug 1353656 - Remove WebIDE build step for local apps. https://reviewboard.mozilla.org/r/160550/#review166296 Thanks, looks good overall! :) Just a few more things to remove. ::: devtools/client/webide/content/details.js (Diff revision 1) > } > > AppManager.update("details"); > } > > -function showPrepackageLog() { Please also remove `details_showPrepackageLog_button`[1], the `prePackageLog`[2] button, and line to hide it[3]. [1]: http://searchfox.org/mozilla-central/rev/3a3af33f513071ea829debdfbc628caebcdf6996/devtools/client/locales/en-US/webide.dtd#93 [2]: http://searchfox.org/mozilla-central/rev/3a3af33f513071ea829debdfbc628caebcdf6996/devtools/client/webide/content/details.xhtml#47 [3]: http://searchfox.org/mozilla-central/rev/3a3af33f513071ea829debdfbc628caebcdf6996/devtools/client/webide/content/details.js#43
Attachment #8889523 -
Flags: review?(jryans) → review+
Comment 9•7 years ago
|
||
mozreview-review |
Comment on attachment 8889522 [details] Bug 1353656 - Use SubProcess.jsm from WebIDE instead of SDK equivalent. https://reviewboard.mozilla.org/r/160548/#review166306 Great, thanks for working on this! :) ::: devtools/client/webide/modules/simulator-process.js:92 (Diff revision 1) > > // Spawn a B2G instance. > - this.process = Subprocess.call({ > - command: b2g, > + Subprocess.call({ > + command: b2g.path, > arguments: this.args, > + environmentAppend: true, Hmm, do we really want this `evironmentAppend` mode? Seems like a behavior change, and this file carefully gardens the environment...
Attachment #8889522 -
Flags: review?(jryans) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 12•7 years ago
|
||
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #8) > Comment on attachment 8889523 [details] > Bug 1353656 - Remove WebIDE build step for local apps. > > https://reviewboard.mozilla.org/r/160550/#review166296 > > Thanks, looks good overall! :) Just a few more things to remove. I removed this. (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #9) > > Hmm, do we really want this `evironmentAppend` mode? Seems like a behavior > change, and this file carefully gardens the environment... No idea why, it looks like previous code was defining a precise environment, but it doesn't work without environmentAppend... Simulator doesn't show up and you get a timeout.
(In reply to Alexandre Poirot [:ochameau] from comment #12) > (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #9) > > > > Hmm, do we really want this `evironmentAppend` mode? Seems like a behavior > > change, and this file carefully gardens the environment... > > No idea why, it looks like previous code was defining a precise environment, > but it doesn't work without environmentAppend... Simulator doesn't show up > and you get a timeout. Ah well... sounds like you are doing the right thing then!
Comment 14•7 years ago
|
||
Pushed by apoirot@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/42801a80bc6c Use SubProcess.jsm from WebIDE instead of SDK equivalent. r=jryans https://hg.mozilla.org/integration/autoland/rev/df6fd4e616d9 Remove WebIDE build step for local apps. r=jryans
Comment 15•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/42801a80bc6c https://hg.mozilla.org/mozilla-central/rev/df6fd4e616d9
Updated•6 years ago
|
Product: Firefox → DevTools
Updated•4 years ago
|
Product: DevTools → DevTools Graveyard
Updated•2 years ago
|
Performance Impact: --- → P3
Whiteboard: [nosdk] [qf:p3] → [nosdk]
You need to log in
before you can comment on or make changes to this bug.
Description
•