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)

enhancement

Tracking

(Performance Impact:low, firefox56 fixed)

RESOLVED FIXED
Firefox 56
Performance Impact low
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.
Whiteboard: [qf]
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.
Whiteboard: [qf] → [qf:p3]
Depends on: 1361311
WebIDE triage. Filter on TRIAGE-JD201705
Priority: -- → P2
(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.
No longer blocks: 1350645
Flags: qe-verify-
Whiteboard: [qf:p3] → [nosdk] [qf:p3]
I'll try to cleanup some things here as I'm the last one of WebIDE era around.
Assignee: nobody → poirot.alex
Status: NEW → ASSIGNED
Priority: P2 → P1
Target Milestone: --- → Firefox 56
Attachment #8889522 - Flags: review?(jryans)
Attachment #8889523 - Flags: review?(jryans)
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 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+
(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!
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
https://hg.mozilla.org/mozilla-central/rev/42801a80bc6c
https://hg.mozilla.org/mozilla-central/rev/df6fd4e616d9
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Product: Firefox → DevTools
Product: DevTools → DevTools Graveyard
Performance Impact: --- → P3
Whiteboard: [nosdk] [qf:p3] → [nosdk]
You need to log in before you can comment on or make changes to this bug.