Closed Bug 1090949 (simulator-configuration) Opened 10 years ago Closed 9 years ago

Make simulators configurable from WebIDE

Categories

(DevTools Graveyard :: WebIDE, defect)

defect
Not set
normal

Tracking

(firefox40 fixed)

RESOLVED FIXED
Firefox 40
Tracking Status
firefox40 --- fixed

People

(Reporter: janx, Assigned: janx)

References

(Blocks 1 open bug)

Details

Attachments

(9 files, 34 obsolete files)

297.55 KB, image/png
Details
13.05 KB, patch
janx
: review+
janx
: checkin+
Details | Diff | Splinter Review
9.18 KB, patch
janx
: review+
janx
: checkin+
Details | Diff | Splinter Review
17.34 KB, patch
janx
: review+
janx
: feedback+
janx
: checkin+
Details | Diff | Splinter Review
8.90 KB, patch
ochameau
: review+
janx
: checkin+
Details | Diff | Splinter Review
3.98 KB, patch
ochameau
: review+
janx
: checkin+
Details | Diff | Splinter Review
7.71 KB, patch
janx
: review+
Details | Diff | Splinter Review
56.01 KB, patch
janx
: review+
Details | Diff | Splinter Review
34.67 KB, patch
janx
: review+
Details | Diff | Splinter Review
Allow the configuration of simulators in WebIDE, so that developers can:
- Choose the simulator runtime, either a specific simulator addon (already possible) or custom B2G/Gaia.
- Choose different screen sizes

Once these simulator profiles are configurable, more simulation options can be added:
- Fake GPS coordinates
- MNC/MCC codes
- Language
- Time zone
- Hardware/sensors specification
- etc.

(for a rough visual idea, see attached mockup)
WIP, with some UI elements and changes to the SimulatorRuntime class.

- Adding "--screen 600x800" as a start parameter for B2G Desktop allows to set a different screen size (thanks Alex), looks like a good way to customize screen size.
- b2g/simulator/lib/simulator-process.js has logic to run custom B2G + Gaia, however this seems to operate only in addons. Maybe something can be done here.
New WIP with device profiles. Paul and Alex, what do you think of my current approach?

- I'm refactoring (sorry Alex) SimulatorRuntime so that it can use custom B2G + Gaia instead of addon, and have device emulation params.
- I'm adding a WebIDE screen to create or configure a Simulator.
- Its device emulation params (e.g. screen size) can be controlled individually, or using a preset profile (e.g. "Firefox OS Flame").

Paul, I might ask for you help to make the simulator configuration screen look good (I'm bad at that and you'll probably write the CSS in ~30 seconds).

Alex, I'll try to actually launch simulators from custom paths, and make their screen sizes change, so I might need your help with that.
Attachment #8514925 - Attachment is obsolete: true
Attachment #8516919 - Flags: feedback?(poirot.alex)
Attachment #8516919 - Flags: feedback?(paul)
Attached image ugly-screenshot.png (obsolete) —
Comment on attachment 8516919 [details] [diff] [review]
Make simulators configurable from WebIDE.

Review of attachment 8516919 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, there is surely a bunch of work to make it look nice in term of UI, but that goes in the right direction.

::: browser/devtools/webide/content/simulator.js
@@ +9,5 @@
> +const {DeviceProfiles} = Cu.import("resource:///modules/devtools/DeviceProfiles.jsm", {})
> +const Strings = Services.strings.createBundle("chrome://browser/locale/devtools/webide.properties");
> +
> +window.addEventListener("load", function onLoad() {
> +  document.querySelector("#close").onclick = e => {

addEventListener?

@@ +26,5 @@
> +      form.addon.appendChild(option);
> +    }
> +    // TODO select latest stable addon
> +  }, (e) => {
> +    // Oops.

Is it intentional? Do you really want to ignore errors?

@@ +31,5 @@
> +  });
> +
> +  ["phones", "tablets", "televisions", "watches"].forEach(group => {
> +    let optgroup = document.createElement("optgroup");
> +    optgroup.setAttribute("label", DeviceProfiles._strings[group]);

It looks wrong to use an attribute prefixed by `_`, which usually means it is private.
I think you want to expose `_strings` more explicitely from DeviceProfiles.

::: browser/devtools/webide/modules/runtimes.js
@@ +189,5 @@
>  
>    enable() {
> +    this._registerSimulatorAddon = this._registerSimulatorAddon.bind(this);
> +    Simulator.on("register", this._registerSimulatorAddon);
> +    // TODO on "Add simulator" save, update or create

We are no longer unregistering simulators?

@@ +493,5 @@
>    type: RuntimeTypes.SIMULATOR,
>    connect: function(connection) {
> +    if (this.version == null) {
> +      // TODO launch custom runtime with this.b2g and this.gaia.
> +      // Maybe look at simulator-process.js

Surely look at simulator-process.js!
Even just grab it!
Attachment #8516919 - Flags: feedback?(poirot.alex) → feedback+
Thanks Alex!
Comment on attachment 8516919 [details] [diff] [review]
Make simulators configurable from WebIDE.

Marking myself to look this over as well.
Attachment #8516919 - Flags: feedback?(jryans)
Yes please! :)

Paul and Ryan, please keep in mind that it's a rough work-in-progress, and I'm interested in high-level feedback about the approach, not typos / uncaught errors. Thanks!
Comment on attachment 8516919 [details] [diff] [review]
Make simulators configurable from WebIDE.

Review of attachment 8516919 [details] [diff] [review]:
-----------------------------------------------------------------

Seems reasonable at a high level!  I glanced at it very quickly. :)
Attachment #8516919 - Flags: feedback?(jryans) → feedback+
Comment on attachment 8516919 [details] [diff] [review]
Make simulators configurable from WebIDE.

Review of attachment 8516919 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me. I'd prefer if we could avoid to have to hardcode the category names in simulator.js (Phones,Tables,Watches,...). I'll help you for the UI as soon as the patches stabilizes, but here is a couple of comments about the ux: splitting the simulator page in 2 sections "software" and "hardware" will help the user understand what's going on. "Add Simulator" is a confusing label. Call that "configure new simulator". In the page, start with the name of the simulator.
Attachment #8516919 - Flags: feedback?(paul) → feedback+
Blocks: 996619
As a first step to refactor Simulators in WebIDE, I'd like to register them by name and not by version.

This will allow to have several different simulators using the same runtime version (addon or soon custom B2G). Alex, please have a look.
Attachment #8516919 - Attachment is obsolete: true
Updated WIP.
Attachment #8516925 - Attachment is obsolete: true
Attachment #8513463 - Attachment description: simulator-menus.png → simulator configuration (mockup)
Alex, please see comment 10. Next step will be to refactor simulator start-up, to allow for start parameters and custom b2g/gaia.
Attachment #8527930 - Attachment is obsolete: true
Comment on attachment 8528516 [details] [diff] [review]
In WebIDE, register simulators by name not by version.

(looks like `git-bz` doesn't set r? flags anymore)
Attachment #8528516 - Flags: review?(poirot.alex)
Do these changes break compatibility with existing releases of the simulator (because you change some APIs in the simulator itself)?
Comment on attachment 8528516 [details] [diff] [review]
In WebIDE, register simulators by name not by version.

Review of attachment 8528516 [details] [diff] [review]:
-----------------------------------------------------------------

While you are at modifying Simulator.jsm, could you remove it from browser/devtools/webide/modules/addons.jsm, it is imported without being used!

It looks like it doesn't really change the API itself, at least not the methods used by simulator addon (register/unregister), it just renames "label" with "name" and then internally consider name as key instead of the FxOS version pulled out of the label.

Do not forget the green try ;)

::: toolkit/devtools/apps/Simulator.jsm
@@ +15,2 @@
>      // simulators register themselves as "Firefox OS X.Y"
> +    simulator.name = name;

Is this used anywhere?
Attachment #8528516 - Flags: review?(poirot.alex) → review+
Thanks for the quick review! I'll address your comments.

> While you are at modifying Simulator.jsm, could you remove it from
> browser/devtools/webide/modules/addons.jsm, it is imported without being
> used!

Yes, I noticed it as well and fixed it in the next patch. I'll do the fix in this patch instead.

>> Do these changes break compatibility with existing releases of the simulator
>> (because you change some APIs in the simulator itself)?
> 
> It looks like it doesn't really change the API itself, at least not the
> methods used by simulator addon (register/unregister), it just renames
> "label" with "name" and then internally consider name as key instead of the
> FxOS version pulled out of the label.

What Alex says is true, but "appinfo" is removed from the simulator objects being registered. It makes latest firefox work with older addons, but older firefox could wrongly expect an "appinfo" in what newer simulators register. Is that a problem?

> Do not forget the green try ;)

Will be with next patch.

> ::: toolkit/devtools/apps/Simulator.jsm
> @@ +15,2 @@
> >      // simulators register themselves as "Firefox OS X.Y"
> > +    simulator.name = name;
> 
> Is this used anywhere?

I thought it would be, but as you noticed it's not in this patch. I'll remove it, and add it back later if needed.
Comment on attachment 8528866 [details] [diff] [review]
In WebIDE, register simulators by name not by version.

Looks about green. Alex, what about the updated patch?
Attachment #8528866 - Flags: review?(poirot.alex)
(In reply to Jan Keromnes [:janx] from comment #16)
> >> Do these changes break compatibility with existing releases of the simulator
> >> (because you change some APIs in the simulator itself)?
> > 
> > It looks like it doesn't really change the API itself, at least not the
> > methods used by simulator addon (register/unregister), it just renames
> > "label" with "name" and then internally consider name as key instead of the
> > FxOS version pulled out of the label.
> 
> What Alex says is true, but "appinfo" is removed from the simulator objects
> being registered. It makes latest firefox work with older addons, but older
> firefox could wrongly expect an "appinfo" in what newer simulators register.
> Is that a problem?

Ideally developers use a firefox more recent than simulators,
but if we can easily prevent unecessary breakage that's better...
Can we just keep the appinfo object? It looks like it would prevent any compatibility issue?
Attachment #8528866 - Flags: review?(poirot.alex) → review+
Thanks Alex!

> Ideally developers use a firefox more recent than simulators,
> but if we can easily prevent unecessary breakage that's better...
> Can we just keep the appinfo object? It looks like it would prevent any
> compatibility issue?

Agreed, that makes more sense.
Comment on attachment 8529418 [details] [diff] [review]
In WebIDE, register simulators by name not by version.

(r+ actually carried over)
Attachment #8529418 - Flags: review+
Looks like try has given up completely, but previous try was green enough:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3f6ed36cf6dd

Please check in "In WebIDE, register simulators by name not by version". Thanks!
Comment on attachment 8530597 [details] [diff] [review]
Refactor Firefox OS Simulator launch.

Alex, this refactor puts the simulator-process logic into Simulator.jsm, allowing WebIDE to launch custom B2G with custom Gaia (and other start params like "--screen") as well as addons.

What are your thoughts on this? I'm not exactly sure how the addon interacts with firefox (e.g. `require("addon")`, the `id`, `uri` etc). Please tell me if you find anything strange, especially if it may cause problems with backward-compatibility.
Attachment #8530597 - Flags: review?(poirot.alex)
Attachment #8529418 - Flags: checkin+
Attachment #8532623 - Flags: review?(poirot.alex)
Comment on attachment 8532623 [details] [diff] [review]
Make Firefox OS Simulator accept launch arguments.

Review of attachment 8532623 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.

::: b2g/simulator/lib/main.js
@@ +51,5 @@
> +  args.runtimePath = path;
> +  console.log("simulator path:", args.runtimePath);
> +
> +  // Compute Gaia profile path.
> +  if (!args.profilePath) {

Note that here you allow launch's caller to overload profilePath,
but you don't allow overloading runtimePath.

::: b2g/simulator/lib/simulator-process.js
@@ -10,5 @@
>  Cu.import("resource://gre/modules/Services.jsm");
>  
> -const { EventTarget } = require("sdk/event/target");
> -const { emit, off } = require("sdk/event/core");
> -const { Class } = require("sdk/core/heritage");

\o/

@@ +27,5 @@
>    function(s) console.debug("subprocess: " + s.trim())
>  );
>  
> +function SimulatorProcess(args) {
> +  this.args = args;

nit: I would have called this attribute `options` rather than `args` but that's not really important...

@@ +129,1 @@
>    },

I'm wondering if b2gFilename and runtimeName is really useful.
It is only used in a console.log on `done`...
It may just be easier to get rid of it.
Attachment #8532623 - Flags: review?(poirot.alex) → review+
Thanks for the review! Rebased, carried over.

(In reply to Alexandre Poirot [:ochameau] from comment #31)
> Note that here you allow launch's caller to overload profilePath,
> but you don't allow overloading runtimePath.

That's on purpose. An addon is a specific runtime, for which you can overload a custom Gaia profile if you want.

For custom B2G runtimes, we'll create a separate SimulatorProcess altogether (in up-coming patches).

> > +function SimulatorProcess(args) {
> > +  this.args = args;
> 
> nit: I would have called this attribute `options` rather than `args` but
> that's not really important...

You're right, they don't map directly to command-line arguments so let's rename them to `options`.

> I'm wondering if b2gFilename and runtimeName is really useful.
> It is only used in a console.log on `done`...
> It may just be easier to get rid of it.

I thought the string might be used in an addon's configuration UI, and didn't grep for uses. Removed.
Attachment #8532623 - Attachment is obsolete: true
Comment on attachment 8535059 [details] [diff] [review]
Make Firefox OS Simulator accept launch options.

Actually carried over.
Attachment #8535059 - Flags: review+
2nd patch can land, about 3 more to go.
Keywords: checkin-needed
Shouldn't we dupe bug 1020668?
I thought that was a weird dashboard status tracking bug, Paul what do you think?
Flags: needinfo?(paul)
(In reply to Jan Keromnes [:janx] from comment #36)
> I thought that was a weird dashboard status tracking bug, Paul what do you
> think?

There's no need for two bugs.  Let's just dupe and mark this one in the whiteboard.
Flags: needinfo?(paul)
Whiteboard: [status:inflight]
The other bugs has many dependencies that this one does not.  Is the other one a larger meta bug then?
Flags: needinfo?(janx)
Comment on attachment 8535059 [details] [diff] [review]
Make Firefox OS Simulator accept launch options.

2nd patch has landed.
Attachment #8535059 - Flags: checkin+
Those weren't actually dependencies but blocking relations. Fixing this bug will enable/fix many things.
Flags: needinfo?(janx)
In the future, we will add more and more start options to the instantiation of the simulator process.
As of now, older simulator addons will continue to work in a recent firefox, but since they insantiate their own process, they won't take into account new options.
Taking that insantiation out of the addon and into firefox, and uplifting this patch and the previous one, allows older simulators to use newer options (e.g. screen size) as we add them.
Comment on attachment 8536163 [details] [diff] [review]
Move simulator process creation out of addon.

Alex, please have a look.
Attachment #8536163 - Flags: review?(poirot.alex)
Comment on attachment 8536163 [details] [diff] [review]
Move simulator process creation out of addon.

Review of attachment 8536163 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good but it looks like the changeset misses `git mv b2g/simulator/lib/simulator-process.js toolkit/devtools/apps/SimulatorProcess.jsm`.

Also I'm wondering if it wouldn't simplify things a lot if new simulators were exposing launch method options through Simulator.register. So that webide would call SimulatorProcess itself and have a better control. The addon would just be a package telling where are the binary and profile.
But it may also just complexify the whole code given that we have to support the old addons...

# browser/devtools/webide/modules/runtimes.js
SimulatorRuntime.prototype = {
  type: RuntimeTypes.SIMULATOR,
  connect: function(connection) {
    ...
    let simulator = Simulator.getByName(this.name);
    ...
    let p;
    if (simulator.options)
      // Here you can tweak options from webide.
      simulator.process = SimulatorProcess(simulator.options);
      simulator.process.run();
      p = promise.resolve();
    else if (simulator.launch) {
      p = simulator.launch({port: port});
    else
      error...
     p.then(() => { ...connect... });
(I modified runtimes.js, but we can tweak the launch code from somewhere else as well.)
Attachment #8536163 - Flags: review?(poirot.alex) → feedback+
(In reply to Jan Keromnes [:janx] from comment #44)
> Created attachment 8536163 [details] [diff] [review]
> Move simulator process creation out of addon.
> 
> In the future, we will add more and more start options to the instantiation
> of the simulator process.
> As of now, older simulator addons will continue to work in a recent firefox,
> but since they insantiate their own process, they won't take into account
> new options.
> Taking that insantiation out of the addon and into firefox, and uplifting
> this patch and the previous one, allows older simulators to use newer
> options (e.g. screen size) as we add them.

Maybe I am missing something, but I am not sure I follow how this works...  If a new option is added, doesn't an old simulator need to be patched and rebuilt so that the b2g binary, etc. understands the new option?  Will we actually do that?

If we won't, does it matter that this file is in the add-on?  What do we gain by moving it out of the add-on?

I am not making a judgement on the actual patch or approach for the moment, as I am just trying to understand the plan here.
Most options are going to be new in b2g, and we don't really plan to uplift any of them (screen ratio, sdcard and next ones). We may consider rebuilding older simulators to support custom gaia profile and simple screen size support but we would have to uplift patches to mozilla-central, which is difficult.

Otherwise as we move options to webide, I think it makes sense to move the logic to webide and let simulator addons be just some package that just ship binary and profiles. It should also allow us to have less regressions if some deps of simulator code gets broken in newer firefoxes. The less code the addon has the more stable it will be.
Thanks Alex and Ryan for the feedback!

The easily upliftable patches are mostly to support --screen with older simulators: I believe this launch option has been around for a while. It won't help with future options, because uplifting these might be too tricky.

(In reply to J. Ryan Stinnett [:jryans] from comment #47)
> If we won't, does it matter that this file is in the add-on?  What do we
> gain by moving it out of the add-on?

Uplifting this gains us --screen support in older simulators.

There is another benefit to taking the file out of the addon: WebIDE will be able to use it to create its own SimulatorProcesses, for entirely custom runtimes (no addon, just paths to a B2G binary and Gaia profile). Support for this will be added in a follow-up patch.

(In reply to Alexandre Poirot [:ochameau] from comment #46)
> Looks good but it looks like the changeset misses `git mv
> b2g/simulator/lib/simulator-process.js
> toolkit/devtools/apps/SimulatorProcess.jsm`.

More exciting git/hg patch issues! I'll see if I can negotiate with git-bz, else I'll edit diffs by hand again.

> Also I'm wondering if it wouldn't simplify things a lot if new simulators
> were exposing launch method options through Simulator.register. So that
> webide would call SimulatorProcess itself and have a better control. The
> addon would just be a package telling where are the binary and profile.
> But it may also just complexify the whole code given that we have to support
> the old addons...

Making simulator addons register only their unique options instead of a launch method is a good idea. I did consider it, but here is why I think my approach is better:
- It already gives WebIDE better control, because WebIDE creates the options object sent to addon.launch(), and gets it back in the SimulatorProcess() constructor, essentially controlling everything that's not addon-specific.
- Simulator addons have two prefs ("extensions.<addonID>.customRuntime" and "extensions.<addonID>.gaiaProfile") that are looked up during start() calls. WebIDE could do the lookup on every launch if the runtime is an addon, but I feel this makes more sense inside an addon launch method.
- Older simulators will always register with a "launch" method anyway.

Note: For custom simulators not tied to an addon but to a B2G binary, WebIDE will create the SimulatorProcess() itself.
New patch, rebased and edited by hand to reflect the file move.
Attachment #8536163 - Attachment is obsolete: true
Attachment #8538476 - Flags: review?(poirot.alex)
Actually, my edited patch looks exactly the same to the previous, and clearly says:

> diff --git a/b2g/simulator/lib/simulator-process.js b/toolkit/devtools/apps/SimulatorProcess.jsm
> similarity index 86%
> rename from b2g/simulator/lib/simulator-process.js
> rename to toolkit/devtools/apps/SimulatorProcess.jsm
> index d2d5155..f77d5de 100644
> --- a/b2g/simulator/lib/simulator-process.js
> +++ b/toolkit/devtools/apps/SimulatorProcess.jsm

Why did you feel that a `git mv` was missing?
Flags: needinfo?(poirot.alex)
(In reply to Jan Keromnes [:janx] from comment #51)
> Actually, my edited patch looks exactly the same to the previous, and
> clearly says:
> 
> > diff --git a/b2g/simulator/lib/simulator-process.js b/toolkit/devtools/apps/SimulatorProcess.jsm
> > similarity index 86%
> > rename from b2g/simulator/lib/simulator-process.js
> > rename to toolkit/devtools/apps/SimulatorProcess.jsm
> > index d2d5155..f77d5de 100644
> > --- a/b2g/simulator/lib/simulator-process.js
> > +++ b/toolkit/devtools/apps/SimulatorProcess.jsm
> 
> Why did you feel that a `git mv` was missing?

I agree with Jan, it looks like the original patch was right, and splinter's patch view is just terrible as usual.  Use MozReview in the future! :)
Comment on attachment 8538476 [details] [diff] [review]
Move simulator process creation out of addon.

Review of attachment 8538476 [details] [diff] [review]:
-----------------------------------------------------------------

The old guard does use splinter and `patch -p1 <` and doesn't like fancy git diff :-o
Attachment #8538476 - Flags: review?(poirot.alex) → review+
Comment on attachment 8538476 [details] [diff] [review]
Move simulator process creation out of addon.

I think we should make a few tweaks here first.  I am writing more comments as we speak...
Flags: needinfo?(poirot.alex)
Attachment #8538476 - Flags: review-
Comment on attachment 8538476 [details] [diff] [review]
Move simulator process creation out of addon.

Review of attachment 8538476 [details] [diff] [review]:
-----------------------------------------------------------------

r- mainly to make sure I had time to write all my thoughts before something lands.  Let's at least fix the name and location as I mention below.  Other pieces can be addressed in future patches.

I may have something wrong or misunderstand what you're trying to do, and if so, let's be sure to talk about it further!

(In reply to Jan Keromnes [:janx] from comment #49)
> Thanks Alex and Ryan for the feedback!
> 
> The easily upliftable patches are mostly to support --screen with older
> simulators: I believe this launch option has been around for a while. It
> won't help with future options, because uplifting these might be too tricky.

When I read "uplift" in this context, I am thinking of "apply patch to older simulator add-on and rebuild it".

I think you are using "uplift" to mean "extract patch from the add-on (b2g/simulator) into toolkit (part of Firefox)", so let's each be specific about what we really mean.

> (In reply to J. Ryan Stinnett [:jryans] from comment #47)
> > If we won't, does it matter that this file is in the add-on?  What do we
> > gain by moving it out of the add-on?
> 
> Uplifting this gains us --screen support in older simulators.

Again, I am assuming you mean "move SimulatorProcess from add-on to Firefox".

SimulatorProcess does not seem to do anything with --screen at the moment, so I guess that's a future patch?  But yes, I see what you mean.  SimulatorProcess (after the move to Firefox) can be updated to add the --screen option, and this would then be applied for all simulators.

> There is another benefit to taking the file out of the addon: WebIDE will be
> able to use it to create its own SimulatorProcesses, for entirely custom
> runtimes (no addon, just paths to a B2G binary and Gaia profile). Support
> for this will be added in a follow-up patch.

Okay, I agree this is a useful advantage.

> (In reply to Alexandre Poirot [:ochameau] from comment #46)
> > Looks good but it looks like the changeset misses `git mv
> > b2g/simulator/lib/simulator-process.js
> > toolkit/devtools/apps/SimulatorProcess.jsm`.

About the file location and name:  I don't think it belongs in toolkit, as I have trouble imagining non-Firefox programs starting simulators.  If it's in toolkit, it would ship on Fennec for example, which seems undesired.  Put this in browser/devtools/webide/modules.  Also, why did you morph it into a .jsm instead of .js module?  Generally, new things should use .js and require, instead of the .jsm approach, so let's leave this as simulator-process.js.

Please fix this before landing.

> > Also I'm wondering if it wouldn't simplify things a lot if new simulators
> > were exposing launch method options through Simulator.register. So that
> > webide would call SimulatorProcess itself and have a better control. The
> > addon would just be a package telling where are the binary and profile.
> > But it may also just complexify the whole code given that we have to support
> > the old addons...
> 
> Making simulator addons register only their unique options instead of a
> launch method is a good idea. I did consider it, but here is why I think my
> approach is better:
> - It already gives WebIDE better control, because WebIDE creates the options
> object sent to addon.launch(), and gets it back in the SimulatorProcess()
> constructor, essentially controlling everything that's not addon-specific.

I think the current approach with this patch of: 

1. WebIDE calls launch(options) (registered by add-on)
2. Add-on augments options with runtime and profile
3. Add-on creates SimulatorProcess (lives in Firefox) and runs it

is an odd control flow to have.  We're already agreeing to move most of the work out of the add-on in this patch, but let's go further by making the add-on fully declarative by adding the runtime and profile to the registration for WebIDE to invoke, as Alex mentions.

Yes, it doesn't work for old simulators, but it moves all of the control flow to WebIDE, which is a great win for the future, since updating Firefox once is vastly simpler than updating all the add-ons.

Really, the main reason that the simulators are add-ons at all is that it's a convenient way to distribute things without reinventing the wheel.  So, we should aim to let WebIDE keep as much control as we can, while the add-on is as "dumb" as possible.

This can be done as follow up work after this patch if you prefer, but let's do it soon!

Taking this idea further, I think we should add even more data to the simulator's registration object about what options it supports.  So, just like we have traits in the DevTools server that tells what features are supported, the simulator can say something like "screen: true" etc. to say it does support the --screen option.  I think you'll really want to have this when building your profile configuration UI, so you'll know what CLI options will actually do something for a given simulator and you can hide unsupported ones.

You'd have to stub that out for earlier simulators, but again, I think it's data we want for the future.  

This idea is definitely follow up, but I think it will help you quite a bit!

> - Simulator addons have two prefs ("extensions.<addonID>.customRuntime" and
> "extensions.<addonID>.gaiaProfile") that are looked up during start() calls.
> WebIDE could do the lookup on every launch if the runtime is an addon, but I
> feel this makes more sense inside an addon launch method.

Are these prefs useful in the future?  I was imagining that once your profile UX exists in WebIDE, people would create a custom simulator profile for this use case.  So, we could then just ignore these prefs, and use the values the user specified in place of the declarative values I am suggesting the add-on would register.

> - Older simulators will always register with a "launch" method anyway.

Right, we'll still need to support them, but I think that's okay.

If we feel extra ambitious later, maybe we consider putting some of these changes in older simulators too, but it's not required.

::: toolkit/devtools/apps/SimulatorProcess.jsm
@@ +23,5 @@
>  // for all consumers of the API.  We trim the messages because they sometimes
>  // have trailing newlines.  And note that registerLogHandler actually registers
>  // an error handler, despite its name.
>  Subprocess.registerLogHandler(
> +  function(s) dump("subprocess: " + s.trim() + "\n")

Why are you changing to dump?  console.* calls allowed these messages to appear in the Browser Console, which was nice for bug reports, as many users will have an easier time access that to include logs (especially on Windows).

Also, we were previously able to control whether these messages appeared via the logging level for the add-on.  I assume this control is now lost outside of the add-on?  If so, we should add a pref (default off) to control this.
Thanks for the r+/-!

(In reply to J. Ryan Stinnett [:jryans] from comment #55)
> r- mainly to make sure I had time to write all my thoughts before something
> lands.  Let's at least fix the name and location as I mention below.  Other
> pieces can be addressed in future patches.

Sure, no rush, let's make sure we all agree on this.

> When I read "uplift" in this context, I am thinking of "apply patch to older
> simulator add-on and rebuild it".
> 
> I think you are using "uplift" to mean "extract patch from the add-on
> (b2g/simulator) into toolkit (part of Firefox)", so let's each be specific
> about what we really mean.

No by "uplift" I actually meant "uplift". This patch and the previous (already landed) achieve two things:

1. Take SimulatorProcess out of the addon and into firefox (which is not an uplift), giving firefox control over the process creation, but only in future simulators.

2. Be trivial enough to be easily "upliftable" to earlier simulators, so that it's at least possible (if we so wish) to apply these two patches to older branches, to rebuild and repackage older simulators. This will allow support for starting an older simulator with `--screen` argument, once upcoming versions of firefox run simulators with that option.

> > Uplifting this gains us --screen support in older simulators.
> 
> Again, I am assuming you mean "move SimulatorProcess from add-on to Firefox".

Well, I'm talking of uplifting the "move SimulatorProcess from add-on to Firefox" patch to earlier simulators.

> SimulatorProcess does not seem to do anything with --screen at the moment,
> so I guess that's a future patch?

Correct. My next patches will be:

- "Allow custom simulator runtimes in WebIDE": de-coupling the list of simulators from list of simulator addons
- "Make simulators configurable from WebIDE": using --screen, among other things

> SimulatorProcess (after the move to Firefox) can be updated to add the
> --screen option, and this would then be applied for all simulators.

To all future simulators yes.

If no patch is uplifted at all, older simulators will continue to use their own "SimulatorProcess", which doesn't support --screen. That's why we may want to make older simulators use firefox's "SimulatorProcess" at some point.

> > There is another benefit to taking the file out of the addon: WebIDE will be
> > able to use it to create its own SimulatorProcesses, for entirely custom
> > runtimes (no addon, just paths to a B2G binary and Gaia profile). Support
> > for this will be added in a follow-up patch.
> 
> Okay, I agree this is a useful advantage.

:)

> About the file location and name:  I don't think it belongs in toolkit, as I
> have trouble imagining non-Firefox programs starting simulators.  If it's in
> toolkit, it would ship on Fennec for example, which seems undesired.

That's a good point, I didn't put much thought into the location other than "I want this accessible in both addons and WebIDE".

> Put this in browser/devtools/webide/modules.  Also, why did you morph it into a
> .jsm instead of .js module?  Generally, new things should use .js and
> require, instead of the .jsm approach, so let's leave this as
> simulator-process.js.

I tried to make it a .js module, but didn't find a way to expose it to addons. The only .js modules I see being used in addons are sdk/ modules that do some funny require-path-alias business in Loader.jsm, which didn't work out for me.

Making it a .jsm made things much simpler and just worked.

> I think the current approach with this patch of: 
> 
> 1. WebIDE calls launch(options) (registered by add-on)
> 2. Add-on augments options with runtime and profile
> 3. Add-on creates SimulatorProcess (lives in Firefox) and runs it

Correct.

> is an odd control flow to have.  We're already agreeing to move most of the
> work out of the add-on in this patch, but let's go further by making the
> add-on fully declarative by adding the runtime and profile to the
> registration for WebIDE to invoke, as Alex mentions.

The "problem" with that, as I mentioned at the end of comment 49, is that upon a start() call, an addon will check the prefs "extensions.<addonID>.customRuntime" and "extensions.<addonID>.gaiaProfile" to see if it wants to overwrite its own binary and profile paths for that particular start.

It feels awkward to have this addon-specific code in a start() method in WebIDE, especially if that method is also used by non-addon simulators. Why not leave that addon-specific control flow in the addon? Maybe future addons (e.g. TVs) will also need to do addon-specific startup things that are not declarative, and tweak the "options" object in their own ways before sending it to SimulatorProcess().

> Yes, it doesn't work for old simulators, but it moves all of the control
> flow to WebIDE, which is a great win for the future, since updating Firefox
> once is vastly simpler than updating all the add-ons.

I don't see how moving addon-specific control flow to WebIDE is a great win, and my approach makes sure that you only ever need to update Firefox, not all add-ons:

Once addons' start() method becomes only a transit point for the "options" object, WebIDE can put whatever it wants into this "options" object, the addon will tweak what it knows about / wants to tweak, and then back to WebIDE's SimulatorProcess which will do the rest.

> Really, the main reason that the simulators are add-ons at all is that it's
> a convenient way to distribute things without reinventing the wheel.  So, we
> should aim to let WebIDE keep as much control as we can, while the add-on is
> as "dumb" as possible.

... with the occasional addon-specific logic, I would argue.

> Taking this idea further, I think we should add even more data to the
> simulator's registration object about what options it supports.  So, just
> like we have traits in the DevTools server that tells what features are
> supported, the simulator can say something like "screen: true" etc. to say
> it does support the --screen option.  I think you'll really want to have
> this when building your profile configuration UI, so you'll know what CLI
> options will actually do something for a given simulator and you can hide
> unsupported ones.

Independently from the "declarative addon" vs "addon with start() method" argument, I like this idea.

In a Simulator's configuration screen in WebIDE, greying out which features aren't supported by the currently selected runtime (you'll be able to switch between all simulator addons and custom runtimes) is an interesting visual hint, though I'm not sure how it will work for things that don't register themselves (e.g. you select a path to a custom B2G binary on your computer).

> You'd have to stub that out for earlier simulators, but again, I think it's
> data we want for the future.  
> 
> This idea is definitely follow up, but I think it will help you quite a bit!

Yes definitely a potentially interesting follow up!

It also relates to how the Device Emulation toolbox panel will interact with different targets (e.g. simulator runtime or firefox tab with responsive design view). My current thinking is that a Device Emulation actor inside the target will specify which emulation features are available. That should work for simulator addons, custom runtimes and tabs alike.

> Are these prefs useful in the future?  I was imagining that once your
> profile UX exists in WebIDE, people would create a custom simulator profile
> for this use case.  So, we could then just ignore these prefs, and use the
> values the user specified in place of the declarative values I am suggesting
> the add-on would register.

I wouldn't rush to dropping support for these. In my suggested design it's easy to continue supporting them in older addons, but maybe at some point we'll stop using them in future addons. 

> Why are you changing to dump?

Simply because console was undefined in my .jsm, but maybe there is a way to continue using console? (import console in the .jsm or make it a .js module that works in addons).

> console.* calls allowed these messages to
> appear in the Browser Console, which was nice for bug reports, as many users
> will have an easier time access that to include logs (especially on Windows).

Assuming dump() messages don't appear in the Browser Console, then this advantage is lost with the switch to dump(). Users would have to look at firefox log messages, e.g. by running it from the console.

> Also, we were previously able to control whether these messages appeared via
> the logging level for the add-on.  I assume this control is now lost outside
> of the add-on?  If so, we should add a pref (default off) to control this.

It seems easy enough to have a pref-controlled log function in this file, or something like "debug()". I'll leave that to a follow-up.

I didn't know about the addon logging level, so for me the result is the same: all simulator log messages show up in the console I run firefox from.
(In reply to Alexandre Poirot [:ochameau] from comment #53)
> The old guard does use splinter and `patch -p1 <` and doesn't like fancy git
> diff :-o

Splinter has its failures, and "fancy git diff" === "diff -up", the good old-fashioned way Linus Torvalds does patches. The rename in my patch works perfectly with "patch -p1 <" :)
(In reply to Jan Keromnes [:janx] from comment #56)
> > Put this in browser/devtools/webide/modules.  Also, why did you morph it into a
> > .jsm instead of .js module?  Generally, new things should use .js and
> > require, instead of the .jsm approach, so let's leave this as
> > simulator-process.js.
> 
> I tried to make it a .js module, but didn't find a way to expose it to
> addons. The only .js modules I see being used in addons are sdk/ modules
> that do some funny require-path-alias business in Loader.jsm, which didn't
> work out for me.
> 
> Making it a .jsm made things much simpler and just worked.

Let's stick with a .js file.  You should import the devtools loader in the add-on and then require it, as in:

const { devtools } = Cu.import("resource://gre/modules/devtools/Loader.jsm", {})
const { SimulatorProcess } = devtools.require("devtools/webide/simulator-process");

However, there is another complication of this move we haven't discussed yet (and this applies to both .jsm and .js).  By removing it entirely from the add-on, any new build of any add-on now requires Fx 37 / Nightly to run, since only that version contains SimulatorProcess.

I believe you should:

1. Leave SimulatorProcess in the add-on for now, but also copy it to WebIDE as discussed
2. In the add-on, use the code above in a try block
3. If it fails, fall back to the copy of SimulatorProcess still inside the add-on
4. File a bug to remove SimulatorProcess from the add-on once 37 has reached release channel

> > is an odd control flow to have.  We're already agreeing to move most of the
> > work out of the add-on in this patch, but let's go further by making the
> > add-on fully declarative by adding the runtime and profile to the
> > registration for WebIDE to invoke, as Alex mentions.
> 
> The "problem" with that, as I mentioned at the end of comment 49, is that
> upon a start() call, an addon will check the prefs
> "extensions.<addonID>.customRuntime" and "extensions.<addonID>.gaiaProfile"
> to see if it wants to overwrite its own binary and profile paths for that
> particular start.
> 
> It feels awkward to have this addon-specific code in a start() method in
> WebIDE, especially if that method is also used by non-addon simulators. Why
> not leave that addon-specific control flow in the addon? Maybe future addons
> (e.g. TVs) will also need to do addon-specific startup things that are not
> declarative, and tweak the "options" object in their own ways before sending
> it to SimulatorProcess().

I think I failed to fully express what I was imagining.  Specifically, I was *not* expecting you would examine the add-on prefs at simulator registration time.  Instead, the add-on simply declares the default location of the runtime and profile at registration.

When you say "add-on specific code", I believe you mean "code to read the runtime / profile prefs of a specific simulator add-on".  WebIDE would not contain any such add-on specific code for simulators using this new style (the format we are discussing now where the locations are registered).

You might also intend "add-on specific code" to mean "a branch in what WebIDE does for different types of simulators".  If so, I am not too worried about this.  We have a chance to clean up some things, so let's clean them up!  Let's make the future better. :) We can easily contain such compatibility code to one object.  It does not need to strewn about WebIDE all over the place.

New-style simulators would state their official runtime and profile locations.  No prefs involved.  If someone configures a custom location for either in your new profile UX, then WebIDE provides those modified locations in the options object to SimulatorProcess, just like any other option you would offer.

For simulators using the existing registration format, WebIDE can see that they did not state their runtime and profile at registration time, so to modify it, it needs to muck with prefs.  We're trying to improve things, and that happens to be the cost of backwards compatibility here.  Or else we uplift this work to all simulators and there is only one path.

To be honest, I am not convinced that the custom runtime / custom profile feature is even used much at all.  At most, it's probably on used by some Gaia developers, and then only for the more recent simulator versions that are close to master.  But I digress...

It is quite possible to support this feature for both simulator styles if we wish, so the feature is not dropped in any way.

> > Really, the main reason that the simulators are add-ons at all is that it's
> > a convenient way to distribute things without reinventing the wheel.  So, we
> > should aim to let WebIDE keep as much control as we can, while the add-on is
> > as "dumb" as possible.
> 
> ... with the occasional addon-specific logic, I would argue.

The only thing I can thing of that you are referring to is reading / writing prefs of the add-on to support the custom location for add-ons that are not using the new-style format.

If you mean a branch appears in WebIDE's code, it can be contained.

> > Are these prefs useful in the future?  I was imagining that once your
> > profile UX exists in WebIDE, people would create a custom simulator profile
> > for this use case.  So, we could then just ignore these prefs, and use the
> > values the user specified in place of the declarative values I am suggesting
> > the add-on would register.
> 
> I wouldn't rush to dropping support for these. In my suggested design it's
> easy to continue supporting them in older addons, but maybe at some point
> we'll stop using them in future addons. 

It seems clear that you are okay with setting the prefs if needed.

For new-style add-ons, there would be branch to instead read the defaults and override the options object.  Again it can be contained.

Are you convinced yet? :)

> > Why are you changing to dump?
> 
> Simply because console was undefined in my .jsm, but maybe there is a way to
> continue using console? (import console in the .jsm or make it a .js module
> that works in addons).

By using the devtools loader as I suggest above, console should be defined again.  If it is not, we should keep trying things it it becomes defined again.

> > console.* calls allowed these messages to
> > appear in the Browser Console, which was nice for bug reports, as many users
> > will have an easier time access that to include logs (especially on Windows).
> 
> Assuming dump() messages don't appear in the Browser Console, then this
> advantage is lost with the switch to dump(). Users would have to look at
> firefox log messages, e.g. by running it from the console.

I think that is insufficient for debugging simulator issues, so we'll need to get the console back one way or another.
One further comment...  The main reason I like the idea of this "new-style" simulator is that it means *all* options are set through an object, instead of *most* of them plus two prefs sometimes.  

In my opinion, the prefs only exist today as a super cheap UI (if it can even be called that) to control these two paths precisely because the feature you are now building did not exist.  So, since you are now building a real UI, let's get rid of the prefs if we can.  They are a legacy, historical choice.

That's really the main advantage.  Doesn't that sound better overall?
Nits aside (see below the dashes) I think our disagreements stem from us having different goals.

If I understood your view correctly:
- You want a clear break from the old way simulators work, making their registration declarative, allowing us to "clean up things".
- That means dropping "extensions.<addonID>.customRuntime" and ".gaiaProfile" startup-time prefs in favor of a new UI in WebIDE.
- In fact, WebIDE takes control of almost everything, addons are just "dumb" containers with a set of declarative, *registration-time* options.
- To support new features in older simulators, we'll have a somewhat-complex compatibility layer to concile old way with new features.

My view:
- I just want to slightly improve the way simulators currently work, so that it works well for older simulators but also allows for cool new things in the future.
- This involves continuing to have a *startup-time* configuration object ("options" or "props" object) passed from WebIDE to addon to simulator process.
- This also gives WebIDE control of almost everything, but gives addons the opportunity to tweak a few things just before startup (e.g. the binary path inside the addon).
- Older simulators still work the same way (so no need for a compatibility layer), but there are two very small patches that can be easily uplifted to give WebIDE more control over the process and support certain features like --screen.

We both want to give WebIDE more power, but I feel an involved redesign doesn't bring that much value, will be harder to uplift, and your compatibility code sounds unnecessarily complex. I really think there is a way to concile the current design with all the new features we want to add.

---

> About the file location and name:  I don't think it belongs in toolkit, as I
> have trouble imagining non-Firefox programs starting simulators.  If it's in
> toolkit, it would ship on Fennec for example, which seems undesired.

Actually, I originally put SimulatorProcess.jsm in /toolkit/devtools/apps/ because it seems related to the other modules there (e.g. Simulator.jsm). But none of these files make sense in Fennec either, should they all be moved to /browser/devtools/webide/modules/?

> Let's stick with a .js file.  You should import the devtools loader in the
> add-on and then require it, as in:
> 
> const { devtools } = Cu.import("resource://gre/modules/devtools/Loader.jsm", {})
> const { SimulatorProcess } = devtools.require("devtools/webide/simulator-process");

I remember trying that. But I'll try harder.

> However, there is another complication of this move we haven't discussed yet
> (and this applies to both .jsm and .js).  By removing it entirely from the
> add-on, any new build of any add-on now requires Fx 37 / Nightly to run,
> since only that version contains SimulatorProcess.
> 
> I believe you should:
> 
> 1. Leave SimulatorProcess in the add-on for now, but also copy it to WebIDE as discussed
> 2. In the add-on, use the code above in a try block
> 3. If it fails, fall back to the copy of SimulatorProcess still inside the add-on
> 4. File a bug to remove SimulatorProcess from the add-on once 37 has reached release channel

Right, I did think of that but then I forgot. Thanks for reminding me.

> When you say "add-on specific code", I believe you mean "code to read the
> runtime / profile prefs of a specific simulator add-on".  WebIDE would not
> contain any such add-on specific code for simulators using this new style
> (the format we are discussing now where the locations are registered).
> 
> You might also intend "add-on specific code" to mean "a branch in what
> WebIDE does for different types of simulators".  If so, I am not too worried
> about this.

What I call addon-specific code is this block: http://dxr.mozilla.org/mozilla-central/source/b2g/simulator/lib/main.js?from=b2g/simulator/lib/main.js#28-57

At startup-time, it finds the location of the executable file inside the addon, and reads the addon-specific prefs "extensions.<addonID>.whatever".

You're suggesting that new addons should compute the executable file location at registration-time instead of startup-time, and that we drop the startup-time prefs. However:
- Dropping the prefs in this patch will prevent us from uplifting it to older simulators without breaking their prefs.
- I've seen people use these prefs, and in any case we should keep supporting them at least until we land a functional configuration screen in WebIDE.

So we are stuck with a startup-time pref lookup. I think it makes more sense inside the addon's start() method, do you think it makes more sense in a branch of WebIDE's simulator startup code?

And I'm not sure what your idea for compatibility is. None of these options seem right to me:
- Use older addons' start() method. New simulation features won't be supported, unless we uplift, but uplifting "new-style" seems unrealistic.
- Guess where the older addons store their binaries and what their prefs are called to read/write them (are we sure they're the same across all versions?)

> New-style simulators would state their official runtime and profile
> locations.  No prefs involved.  If someone configures a custom location for
> either in your new profile UX, then WebIDE provides those modified locations
> in the options object to SimulatorProcess, just like any other option you
> would offer.

I see why this design looks right to you. But...

> For simulators using the existing registration format, WebIDE can see that
> they did not state their runtime and profile at registration time, so to
> modify it, it needs to muck with prefs.

What do you mean by "muck with prefs"? I don't see a good solution here (see above comments about compatibility). In my mind there is no good solution here, making your "new-style" simulators break backwards compatibility.

> We're trying to improve things, and
> that happens to be the cost of backwards compatibility here.  Or else we
> uplift this work to all simulators and there is only one path.

No good solution here either:
- We really shouldn't break backwards compatibility, because many app developers are still targeting old versions like 1.0, 1.1 and 1.3.
- Uplifting your suggested "new-style" is much more complex and involved. I think it's unrealistic.

> To be honest, I am not convinced that the custom runtime / custom profile
> feature is even used much at all.  At most, it's probably on used by some
> Gaia developers, and then only for the more recent simulator versions that
> are close to master.  But I digress...

We advertize these prefs in our MDN pages and in our mailing lists. I believe they're being used at least by some contributors and some people working on TV builds. Transitioning these addon prefs to a WebIDE configuration screen needs to be done very carefully, and I believe they'll stay around for a while.

> > ... with the occasional addon-specific logic, I would argue.
> 
> The only thing I can thing of that you are referring to is reading / writing
> prefs of the add-on to support the custom location for add-ons that are not
> using the new-style format.

Writing? Oh, so you suggest preserving compatibility of "custom profile configured in WebIDE" with older addons by calling their "start()" method but overwriting their "extensions.<id>.gaiaProfile" pref beforehand. Maybe tying these together is a good idea (in my current code the WebIDE configuration takes precedence over the addon pref), but that might erase whatever custom pref the user had set before, and only ensures compatibility of the profile option (it doesn't solve things like adding a "--screen" launch argument, for that we'd need to uplift a patch that makes addon.start() forward an options object to SimulatorProcess, and a patch that takes SimulatorProcess out of the addon).

> It seems clear that you are okay with setting the prefs if needed.

Well, I hadn't considered it, and I'm still not sure about overwriting addon-specific prefs from WebIDE.

> > Assuming dump() messages don't appear in the Browser Console, then this
> > advantage is lost with the switch to dump(). Users would have to look at
> > firefox log messages, e.g. by running it from the console.
> 
> I think that is insufficient for debugging simulator issues, so we'll need
> to get the console back one way or another.

I'll try and get the console back, but I'm not convinced that dump() is insufficient.
Flags: needinfo?(jryans)
It seems I have failed to convince you. :P

At this point, I think we are wasting time arguing about it.  For some reason, I continued to write my perspective again anyway.

However, I do want to see the right file location, .js extension, and console.log changes for sure.

(In reply to Jan Keromnes [:janx] from comment #60)
> Nits aside (see below the dashes) I think our disagreements stem from us
> having different goals.
> 
> If I understood your view correctly:
> - You want a clear break from the old way simulators work, making their
> registration declarative, allowing us to "clean up things".

Right, since we are already messing with the simulator <-> WebIDE interface, it seemed like a good time to push a little bit further so they become entirely dumb, instead of just a little bit dumber.

I guess it feels like a missed opportunity if we don't just go "all the way" here, since we're unlikely to tweak the simulator <-> WebIDE interface again for a while.

> - That means dropping "extensions.<addonID>.customRuntime" and
> ".gaiaProfile" startup-time prefs in favor of a new UI in WebIDE.

Correct, at least for the addons as far back as we would uplift these changes to.

However, the feature they represent comes back once there is a simulator profile UI to control it.

> - In fact, WebIDE takes control of almost everything, addons are just "dumb"
> containers with a set of declarative, *registration-time* options.

Yes.

> - To support new features in older simulators, we'll have a somewhat-complex
> compatibility layer to concile old way with new features.

I believe it would be only a few ifs and ||s.  Or alternatively, separate |SimulatorRuntime| objects could be made, one for each of the styles.  Either way, I don't think it's too bad.

> My view:
> - I just want to slightly improve the way simulators currently work, so that
> it works well for older simulators but also allows for cool new things in
> the future.

I don't feel the changes I am suggesting are vastly more complex than ones you are already making anyway.

> - This involves continuing to have a *startup-time* configuration object
> ("options" or "props" object) passed from WebIDE to addon to simulator
> process.

Yes, which seems unnecessary, as it is odd to have the logic of some options in the addon and others in WebIDE for no reason other than history.

> - This also gives WebIDE control of almost everything, but gives addons the
> opportunity to tweak a few things just before startup (e.g. the binary path
> inside the addon).

Right, the part about letting the add-ons do a tiny piece is the part that is strange to me.  I guess I find more strange after you move simulator-process.js out of the add-on than it is today.

> - Older simulators still work the same way (so no need for a compatibility
> layer), but there are two very small patches that can be easily uplifted to
> give WebIDE more control over the process and support certain features like
> --screen.

main.js and simulator-process.js are largely unchanged all the way back to Simulator 1.2.  We could make large, sweeping changes if we wished and still have about the same success uplifting them.

> We both want to give WebIDE more power, but I feel an involved redesign
> doesn't bring that much value, will be harder to uplift, and your
> compatibility code sounds unnecessarily complex. I really think there is a
> way to concile the current design with all the new features we want to add.
> 
> ---
> 
> > About the file location and name:  I don't think it belongs in toolkit, as I
> > have trouble imagining non-Firefox programs starting simulators.  If it's in
> > toolkit, it would ship on Fennec for example, which seems undesired.
> 
> Actually, I originally put SimulatorProcess.jsm in /toolkit/devtools/apps/
> because it seems related to the other modules there (e.g. Simulator.jsm).
> But none of these files make sense in Fennec either, should they all be
> moved to /browser/devtools/webide/modules/?

Well, if you're up for it.  It would mean another path change inside the add-on that you'd need to try-catch since it would vary across Firefox channels.  But you're already doing one...  Up to you.

At the very least, I think we should be good citizens and change the toolkit moz.build file to only include Simulator.jsm for Firefox if you do not move it.

> > When you say "add-on specific code", I believe you mean "code to read the
> > runtime / profile prefs of a specific simulator add-on".  WebIDE would not
> > contain any such add-on specific code for simulators using this new style
> > (the format we are discussing now where the locations are registered).
> > 
> > You might also intend "add-on specific code" to mean "a branch in what
> > WebIDE does for different types of simulators".  If so, I am not too worried
> > about this.
> 
> What I call addon-specific code is this block:
> http://dxr.mozilla.org/mozilla-central/source/b2g/simulator/lib/main.
> js?from=b2g/simulator/lib/main.js#28-57
> 
> At startup-time, it finds the location of the executable file inside the
> addon, and reads the addon-specific prefs "extensions.<addonID>.whatever".
> 
> You're suggesting that new addons should compute the executable file
> location at registration-time instead of startup-time, and that we drop the
> startup-time prefs. However:
> - Dropping the prefs in this patch will prevent us from uplifting it to
> older simulators without breaking their prefs.
> - I've seen people use these prefs, and in any case we should keep
> supporting them at least until we land a functional configuration screen in
> WebIDE.

Well, for most people they don't really go away until we make an official release of a new simulator build, as most aren't using Nightly simulators.  But yes, it could be tricky until the UI exists.

> So we are stuck with a startup-time pref lookup. I think it makes more sense
> inside the addon's start() method, do you think it makes more sense in a
> branch of WebIDE's simulator startup code?

You could compromise and read the pref add-on startup / registration, instead simulator launch time.  So, the add-on would have to be toggled to re-read the pref.  No, I don't think it makes sense in WebIDE.

> And I'm not sure what your idea for compatibility is. None of these options
> seem right to me:
> - Use older addons' start() method. New simulation features won't be
> supported, unless we uplift, but uplifting "new-style" seems unrealistic.

You still have to uplift your current work to move out SimulatorProcess.  I believe the uplift is identical, only the number of patches might be different.  It does not seem harder to me.

> > New-style simulators would state their official runtime and profile
> > locations.  No prefs involved.  If someone configures a custom location for
> > either in your new profile UX, then WebIDE provides those modified locations
> > in the options object to SimulatorProcess, just like any other option you
> > would offer.
> 
> I see why this design looks right to you. But...
> 
> > For simulators using the existing registration format, WebIDE can see that
> > they did not state their runtime and profile at registration time, so to
> > modify it, it needs to muck with prefs.
> 
> What do you mean by "muck with prefs"? I don't see a good solution here (see
> above comments about compatibility). In my mind there is no good solution
> here, making your "new-style" simulators break backwards compatibility.

When I wrote this, I was assuming you would set a custom runtime path that was entered via your new UI by setting the pref that still exists in the add-on.

However, I guess you really intend to overwrite this piece of the options object in further SimulatorProcess changes that read from your new UI profile or something.

> > We're trying to improve things, and
> > that happens to be the cost of backwards compatibility here.  Or else we
> > uplift this work to all simulators and there is only one path.
> 
> No good solution here either:
> - We really shouldn't break backwards compatibility, because many app
> developers are still targeting old versions like 1.0, 1.1 and 1.3.

Sure, we both agree.  The old simulators (back to 1.2) should keep working.

> - Uplifting your suggested "new-style" is much more complex and involved. I
> think it's unrealistic.

No, see above.

> > To be honest, I am not convinced that the custom runtime / custom profile
> > feature is even used much at all.  At most, it's probably on used by some
> > Gaia developers, and then only for the more recent simulator versions that
> > are close to master.  But I digress...
> 
> We advertize these prefs in our MDN pages and in our mailing lists. I
> believe they're being used at least by some contributors and some people
> working on TV builds. Transitioning these addon prefs to a WebIDE
> configuration screen needs to be done very carefully, and I believe they'll
> stay around for a while.

Okay.  One way to transition is for the UI to exist first, so there is no time window in which it is impossible to set these custom paths.

> > > ... with the occasional addon-specific logic, I would argue.
> > 
> > The only thing I can thing of that you are referring to is reading / writing
> > prefs of the add-on to support the custom location for add-ons that are not
> > using the new-style format.
> 
> Writing? Oh, so you suggest preserving compatibility of "custom profile
> configured in WebIDE" with older addons by calling their "start()" method
> but overwriting their "extensions.<id>.gaiaProfile" pref beforehand. Maybe
> tying these together is a good idea (in my current code the WebIDE
> configuration takes precedence over the addon pref), but that might erase
> whatever custom pref the user had set before, and only ensures compatibility
> of the profile option (it doesn't solve things like adding a "--screen"
> launch argument, for that we'd need to uplift a patch that makes
> addon.start() forward an options object to SimulatorProcess, and a patch
> that takes SimulatorProcess out of the addon).

See above for why I mention writing.  I misunderstood your intent I think.

Of course you need to move SimulatorProcess to add future CLI flags and such...  I was never trying to argue against that!

> > It seems clear that you are okay with setting the prefs if needed.
> 
> Well, I hadn't considered it, and I'm still not sure about overwriting
> addon-specific prefs from WebIDE.
> 
> > > Assuming dump() messages don't appear in the Browser Console, then this
> > > advantage is lost with the switch to dump(). Users would have to look at
> > > firefox log messages, e.g. by running it from the console.
> > 
> > I think that is insufficient for debugging simulator issues, so we'll need
> > to get the console back one way or another.
> 
> I'll try and get the console back, but I'm not convinced that dump() is
> insufficient.

I consider the main user of these logs to be not Mozilla developers, but end users of the Simulator when trying to post logs for help.  I have found it much simpler to say "open the Browser Console" than to say "start Firefox from the terminal" especially for Windows users.
Flags: needinfo?(jryans)
There's no need to reply to all of my comments once again unless you really feel like it... :) Proceed with your plan if you wish, assuming the key items I mentioned are addressed:

file location, .js extension, and console.log changes.
Alias: emulation-profiles
New approach.
Attachment #8538476 - Attachment is obsolete: true
Once the WebIDE-Simulator-control patch has landed and rolled to stable, we'll
be able to land this clean-up patch in Nightly Simulator, and upstream them to
older Simulators. Essentially, this removes all logic from the Simulator, since
WebIDE will be in full control of everything.
Once Simulator addons have been cleaned up of unnecessary code, they will no
longer try to import "Simulator.jsm", which will be pointless because WebIDE now
controls the simulators. We'll then be able to remove this file. Note that older
simulators to which we won't have been able to uplift the clean-up patch will
start logging errors (because they'll still try to register to Simulator.jsm)
but will still continue to work.
Comment on attachment 8562019 [details] [diff] [review]
Remove deprecated files in Firefox OS Simulator.

Don't land until "Give WebIDE full control over Simulator addons" has made its way to stable WebIDE.
Attachment #8562019 - Flags: review-
Comment on attachment 8562020 [details] [diff] [review]
Remove deprecated Simulator.jsm file.

Don't land until "Remove deprecated files in Firefox OS Simulator" has landed in Nightly Simulator, and been uplifted at least a couple versions back to older simulators.
Attachment #8562020 - Flags: review-
Attachment #8562018 - Attachment is obsolete: true
Comment on attachment 8562022 [details] [diff] [review]
Give WebIDE full control over Simulator addons.

Alex, please review my new approach. Ryan, your feedback is also welcome.

My new approach:
- Allows (in upcoming patches) to configure simulators (e.g. screen size) and change their runtime (installed simulator addon or local B2G binary).
- Simulator addons no longer register themselves. Instead, WebIDE listens for their installation, and handles the instantiation/kill of their process.
- The new "simulators.js" module maintains a list of "Simulator" objects. For now, they're strictly tied to a simulator addon, but in later patches they will become configurable: e.g. switch their runtime to a different simulator addon, or to a custom binary, define custom Gaia profiles, modify device-emulation features, etc.

Note that I managed to avoid all the caveats we previously thought this approach would bring:
- Addon prefs for customRuntime and gaiaProfile are still supported (we can still decide to drop them later).
- You won't need to toggle the addon for changes to these prefs to be taken into account.
- The compatibility layer probably won't be so big.
Attachment #8562022 - Flags: review?(poirot.alex)
Attachment #8562022 - Flags: feedback?(jryans)
Comment on attachment 8562022 [details] [diff] [review]
Give WebIDE full control over Simulator addons.

Review of attachment 8562022 [details] [diff] [review]:
-----------------------------------------------------------------

A few inline precisions:

::: browser/devtools/webide/modules/simulator-process.js
@@ +30,5 @@
> +// Log subprocess error and debug messages to the console.  This logs messages
> +// for all consumers of the API.  We trim the messages because they sometimes
> +// have trailing newlines.  And note that registerLogHandler actually registers
> +// an error handler, despite its name.
> +//Subprocess.registerLogHandler(s => console.error("subprocess: " + s.trim()));

Many `console.{log,debug,error}` are left over from the original "simulator-process.js" file, but they generate a lot of output. I don't think we should keep them.

@@ +127,5 @@
> +EventEmitter.decorate(SimulatorProcess.prototype);
> +
> +
> +function CustomSimulatorProcess(options) {
> +  this.options = options;

`CustomSimulatorProcess` is not used yet. It will be used in "simulators.js" once a Simulator can be configured to use a custom B2G binary.

::: browser/devtools/webide/modules/simulators.js
@@ +15,5 @@
> +  _simulators: [],
> +
> +  // TODO (Bug 1090949) Don't generate this list from installed simulator
> +  // addons, but instead implement a persistent list of user-configured
> +  // simulators.

This TODO describes my next patch.

@@ +45,5 @@
> +});
> +
> +
> +function Simulator(addon) {
> +  this.addon = addon;

This is temporary. Future `Simulator` objects will be able to use addons and custom binaries interchangeably.
Alias: emulation-profiles → simulator-profiles
Comment on attachment 8562022 [details] [diff] [review]
Give WebIDE full control over Simulator addons.

Review of attachment 8562022 [details] [diff] [review]:
-----------------------------------------------------------------

I have some comments, but LGTM.

::: browser/devtools/webide/modules/simulator-process.js
@@ +30,5 @@
> +// Log subprocess error and debug messages to the console.  This logs messages
> +// for all consumers of the API.  We trim the messages because they sometimes
> +// have trailing newlines.  And note that registerLogHandler actually registers
> +// an error handler, despite its name.
> +//Subprocess.registerLogHandler(s => console.error("subprocess: " + s.trim()));

There is a lot of noise added by Subprocess, but b2g stdout can be useful. We can see various gecko log messages in it, actor exceptions for ex, ...
I would like to at least keep stdout, without keep subprocess additional noise.

@@ +131,5 @@
> +  this.options = options;
> +}
> +CustomSimulatorProcess.prototype = Object.create(SimulatorProcess.prototype);
> +CustomSimulatorProcess.prototype.getB2GBinary = function() {
> +

nit: this space looks useless.

@@ +156,5 @@
> +  //this.on("stderr", (e, data) => { console.error(data.trim()) });
> +}
> +AddonSimulatorProcess.prototype = Object.create(SimulatorProcess.prototype);
> +AddonSimulatorProcess.prototype.getB2GBinary = function() {
> +

nit: this space looks useless.

@@ +179,5 @@
> +
> +  return file;
> +};
> +AddonSimulatorProcess.prototype.getGaiaProfile = function() {
> +

nit: this space looks useless.

@@ +205,5 @@
> +  //this.on("stderr", (e, data) => { console.error(data.trim()) });
> +}
> +OldAddonSimulatorProcess.prototype = Object.create(AddonSimulatorProcess.prototype);
> +OldAddonSimulatorProcess.prototype.getB2GBinary = function() {
> +

nit: this space looks useless.

@@ +232,5 @@
> +  }
> +  return file;
> +};
> +Object.defineProperty(OldAddonSimulatorProcess.prototype, "args", {
> +

nit: this space looks useless.

::: browser/devtools/webide/modules/simulators.js
@@ +5,5 @@
> +const {Cu} = require("chrome");
> +const {AddonManager} = Cu.import("resource://gre/modules/AddonManager.jsm");
> +const {EventEmitter} = Cu.import("resource://gre/modules/devtools/event-emitter.js");
> +const {ConnectionManager} = require("devtools/client/connection-manager");
> +const {AddonSimulatorProcess,OldAddonSimulatorProcess} = require("devtools/webide/simulator-process");

nit: add a space between `,` and `OldAddonSimulatorProcess`

@@ +11,5 @@
> +
> +const SimulatorRegExp = new RegExp(Services.prefs.getCharPref("devtools.webide.simulatorAddonRegExp"));
> +
> +let Simulators = {
> +  _simulators: [],

It looks like you are never reusing this variable so that you can keep it a variable of getAll()

@@ +25,5 @@
> +          this._simulators.push(new Simulator(addon));
> +        }
> +      }
> +      // Sort simulators alphabetically by name.
> +      this._simulators.sort((a, b) => { return (a.name > b.name) - (a.name < b.name) });

Rather than using magical types coercion, what about using localeCompare, or a dumb but readable code?

@@ +26,5 @@
> +        }
> +      }
> +      // Sort simulators alphabetically by name.
> +      this._simulators.sort((a, b) => { return (a.name > b.name) - (a.name < b.name) });
> +      callback(this._simulators);

What about using a promise instead?
Attachment #8562022 - Flags: review?(poirot.alex) → review+
(In reply to Jan Keromnes [:janx] from comment #66)
> Comment on attachment 8562019 [details] [diff] [review]
> Remove deprecated files in Firefox OS Simulator.
> 
> Don't land until "Give WebIDE full control over Simulator addons" has made
> its way to stable WebIDE.

Probably best for these deprecation patches to live on a separate bug just for that purpose.  This one is already quite long-lived, and we want to track the deprecation step separately.
Comment on attachment 8562022 [details] [diff] [review]
Give WebIDE full control over Simulator addons.

Review of attachment 8562022 [details] [diff] [review]:
-----------------------------------------------------------------

Yay, I think it makes sense overall! :D

::: browser/devtools/webide/modules/simulator-process.js
@@ +30,5 @@
> +// Log subprocess error and debug messages to the console.  This logs messages
> +// for all consumers of the API.  We trim the messages because they sometimes
> +// have trailing newlines.  And note that registerLogHandler actually registers
> +// an error handler, despite its name.
> +//Subprocess.registerLogHandler(s => console.error("subprocess: " + s.trim()));

I agree that I don't want the subprocess noise enabled.

As with Alex, I *do* want the b2g stdout / stderr.

@@ +137,5 @@
> +  let file = Cc['@mozilla.org/file/local;1'].createInstance(Ci.nsILocalFile);
> +  file.initWithPath(this.options.b2gBinary);
> +  return file;
> +}
> +CustomSimulatorProcess.prototype.getGaiaProfile = function() {

Maybe use "real" getters?  You can easily put it on the prototype:

Object.assign(CustomSimulatorProcess.prototype, {
  get gaiaProfile() {

  }
});

Up to you!

@@ +151,5 @@
> +function AddonSimulatorProcess(addon, options) {
> +  this.addon = addon;
> +  this.options = options;
> +
> +  //this.on("stdout", (e, data) => { console.log(data.trim()) });

Please restore these.  When we were running inside the add-on, it was possible to control these with a logging pref for the SDK for that add-on.  We don't have that anymore (I am pretty sure, but check first!) since this code inside the browser now, so please add a new pref (default off) to enable this.

::: browser/devtools/webide/modules/simulators.js
@@ +17,5 @@
> +  // TODO (Bug 1090949) Don't generate this list from installed simulator
> +  // addons, but instead implement a persistent list of user-configured
> +  // simulators.
> +  getAll(callback) {
> +    AddonManager.getAllAddons(addons => {

Does this allow even random nightly simulators (non-official XPI) to still work?

For example, I have a 3.0 simulator that WebIDE does not know about yet.  Can I use it still?
Attachment #8562022 - Flags: feedback?(jryans) → feedback+
(In reply to Alexandre Poirot [:ochameau] from comment #71)
> I have some comments, but LGTM.

(In reply to J. Ryan Stinnett [:jryans] from comment #73)
> Yay, I think it makes sense overall! :D

Cool! \o/

(In reply to J. Ryan Stinnett [:jryans] from comment #73)
> Does this allow even random nightly simulators (non-official XPI) to still
> work?

As long as their addon ID is not too distorted, yes.

> For example, I have a 3.0 simulator that WebIDE does not know about yet. 
> Can I use it still?

Yes you can! Courtesy of the simulatorAddonRegExp.

I'll now address your various nits and comments.
(In reply to J. Ryan Stinnett [:jryans] from comment #73)
> ::: browser/devtools/webide/modules/simulator-process.js
> @@ +137,5 @@
> > +  let file = Cc['@mozilla.org/file/local;1'].createInstance(Ci.nsILocalFile);
> > +  file.initWithPath(this.options.b2gBinary);
> > +  return file;
> > +}
> > +CustomSimulatorProcess.prototype.getGaiaProfile = function() {
> 
> Maybe use "real" getters?  You can easily put it on the prototype:
> 
> Object.assign(CustomSimulatorProcess.prototype, {
>   get gaiaProfile() {
> 
>   }
> });
> 
> Up to you!

Well, it seems that `Object.assign` is enumerating over its second parameter, hence it's not copying the getter itself, but the getter's return value, into `CustomSimulatorProcess.prototype`.

However, at some point before uploading the patch, I was using defineProperty:

Object.defineProperty(CustomSimulatorProcess.prototype, "gaiaProfile", {
  get: function() {

  }
});

It's verbose, but I'll go back to that and use real getters.
New patch addressing nits, still needs a simulator-log-pref and follow-up cleaning bugs.

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e0ffdb2072c2
Attachment #8562022 - Attachment is obsolete: true
Attachment #8562919 - Flags: feedback+
Comment on attachment 8562919 [details] [diff] [review]
Give WebIDE full control over simulator addons.

(Carrying over ochameau's r+ manually because "git bz" forgot about it.)
Attachment #8562919 - Flags: review+
Depends on: 1132448
Blocks: 1132452
Comment on attachment 8562019 [details] [diff] [review]
Remove deprecated files in Firefox OS Simulator.

Moved patch to follow-up bug 1132452.
Attachment #8562019 - Attachment is obsolete: true
Comment on attachment 8562020 [details] [diff] [review]
Remove deprecated Simulator.jsm file.

Moved patch to follow-up bug 1132453.
Attachment #8562020 - Attachment is obsolete: true
Comment on attachment 8563456 [details] [diff] [review]
Give WebIDE full control over simulator addons.

(ochameau's r+)
Attachment #8563456 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/632df219369f

Does this need the leave-open anymore?
Keywords: checkin-needed
Whiteboard: [status:inflight] → [fixed-in-fx-team]
Thanks Ryan!

Yes it still needs the leave-open, but the next patch should do it!
Attachment #8563456 - Flags: checkin+
Depends on: 1132669
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #82)
> https://hg.mozilla.org/integration/fx-team/rev/632df219369f

Backed out at Jan's request.
https://hg.mozilla.org/integration/fx-team/rev/2c97566c3cb7
Whiteboard: [fixed-in-fx-team]
Comment on attachment 8563456 [details] [diff] [review]
Give WebIDE full control over simulator addons.

Review of attachment 8563456 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks! The patch actually made simulators > 1.3 unusable on Mac OS.

::: browser/devtools/webide/modules/simulator-process.js
@@ +187,5 @@
> +        linux64: "b2g-bin",
> +      };
> +      file = this.addon.getResourceURI().QueryInterface(Ci.nsIFileURL).file;
> +      file.append("b2g");
> +      file.append(binaries[OS]);

`file.append()` doesn't work for multiple levels, so if `OS === "mac64"`, `file.append("B2G.app/Contents/MacOS/b2g-bin")` breaks, causing bug 1132669.
Attachment #8563456 - Flags: review-
Attachment #8563456 - Flags: review+
Attachment #8563456 - Flags: feedback+
Attachment #8563456 - Flags: checkin+
Fix MacOS simulator start.
Attachment #8563456 - Attachment is obsolete: true
Comment on attachment 8564077 [details] [diff] [review]
Give WebIDE full control over simulator addons. r=ochameau f=jryans

Ryan, could you please try this patch on a Mac build and see if you can start both:
- a 1.2 or 1.3 simulator
- a >=1.4 simulator (can be nightly)
? Many thanks!
Flags: needinfo?(jryans)
Attachment #8564077 - Flags: review+
Attachment #8564077 - Flags: feedback+
(In reply to Jan Keromnes [:janx] from comment #87)
> Comment on attachment 8564077 [details] [diff] [review]
> Give WebIDE full control over simulator addons. r=ochameau f=jryans
> 
> Ryan, could you please try this patch on a Mac build and see if you can
> start both:
> - a 1.2 or 1.3 simulator
> - a >=1.4 simulator (can be nightly)
> ? Many thanks!

Yes, this version works fine on Mac for me!  I tried 1.3, 2.2, and 3.0.
Flags: needinfo?(jryans)
Cool, thanks for verifying!

Re `checkin-needed` for "Give WebIDE full control over simulator addons".

Previous try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e0ffdb2072c2
Keywords: checkin-needed
Attachment #8564077 - Flags: checkin+
Depends on: 1133927
WIP.
Depends on: 1135018
Phew, getting there. Please let me know what you think. Pre-review because of remaining TODOs.

Patch summary:
- New "Simulator Options" screen in WebIDE (looks like https://thefiletree.com/jan/shots/fxosyarrr.png)
- "Gear" icons next to Simulators in the "Runtime" dropdown, to configure them.
- Test: ./mach mochitest-chrome browser/devtools/webide/test/test_simulators.js (not 100% finished but works).
- Depends on bug 1135018 (which edits a few files in WebIDE) for new devices list and test file.
- Several TODOs and FIXMEs left (some I'll address, some I'll leave to follow up)
- Still no persistence (follow-up?)
- Fixed test: browser/devtools/webide/test/test_addons.js and stub XPIs fixed to not use Simulator.js.
- Drive-by style and order fix in browser/devtools/webide/content/prefs.xhtml
Attachment #8569182 - Attachment is obsolete: true
Attachment #8574066 - Flags: feedback?(poirot.alex)
Attachment #8574066 - Flags: feedback?(jryans)
Erratum: "test_addons.js and stub XPIs fixed to not use Simulator.js" should read "Simulator.jsm" instead.

The test does not use the deprecated toolkit/devtools/apps/Simulator.jsm anymore, but the more recent browser/devtools/webide/modules/simulators.js .
Comment on attachment 8574066 [details] [diff] [review]
Make simulators configurable from WebIDE.

Review of attachment 8574066 [details] [diff] [review]:
-----------------------------------------------------------------

I see various code being misleading, but overall it looks good to me.

::: browser/devtools/webide/content/simulator.js
@@ +44,5 @@
> +        opt(form.version, addon.id, addon.name);
> +      });
> +      opt(form.version, "custom", "");
> +      opt(form.version, "pick", Strings.GetStringFromName("simulator_custom_binary"));
> +      deferred.resolve();

I'm lost here. How does findSimulatorAddons competes with GetDevices?
Both looks async, but findSimulatorAddons resolve the promise.
Can't GetDevices() be resolved after findSimulatorAddons() and then introduce a race?

@@ +56,5 @@
> +    opt(form.profile, "pick", Strings.GetStringFromName("simulator_custom_profile"));
> +
> +    // Generate example devices list.
> +    form.device.innerHTML = "";
> +    form.device.classList.remove("custom");

nit: the various innertHTML="" and classList.remove seems to better match clear() method than init() one.

@@ +98,5 @@
> +
> +    this.clear();
> +    this.init().then(() => {
> +
> +      this._simulator = simulator;

Is there any particular reason to wait for init() completiong before setting _simulator?

@@ +124,5 @@
> +  clear() {
> +    this._simulator = null;
> +  },
> +
> +  // Select an available option, or set the "custom" option.

This comment doesn't seem to reflect the magic behind this method.
What about something like:
Select "custom" option for the given <select> and update its text to the given `value` text.

@@ +181,5 @@
> +  // DEVICE. Can be an existing `device.name` or "custom".
> +
> +  get device() {
> +    // TODO Search for a `device.name` matching current simulator options.
> +    return "custom";

We should translate that.

@@ +276,5 @@
> +  document.querySelector("#reset").onclick = e=> {
> +    SimulatorEditor.resetToDefaults();
> +  };
> +
> +  let form = SimulatorEditor._form = document.querySelector("#simulator-editor");

That looks wrong to set _form, a private attribute outside of the component implementation.
I would have expect init() to receive it as parameter or something.

@@ +284,5 @@
> +    SimulatorEditor.edit(simulator);
> +  });
> +
> +  // We just loaded, so we probably missed the first configure request.
> +  SimulatorEditor.edit(Simulators._lastConfiguredSimulator);

I would have put all or most of onLoad content in a method on SimulatorEditor.
It feels disconnected to have all this here in a load listener.

::: browser/devtools/webide/content/simulator.xhtml
@@ +71,5 @@
> +            <span>&simulator_pixelRatio;</span>
> +            <input name="pixelRatio" placeholder="ratio" type="number" step="0.05"/>
> +          </label>
> +        </li>
> +      </ul>

Regarding the UI it would be great to align all the labels and the form elements.
Like aligning on the right all labels and then having all form elements starting at the same horizontal position.

::: browser/devtools/webide/content/webide.js
@@ +112,5 @@
>  
>      gDevToolsBrowser.isWebIDEInitialized.resolve();
> +
> +    Simulators.on("configure", (e, simulator) => {
> +      this.hidePanels();

You shoudln't have to call hidePanels as selectDeckPanel is already doing it.

::: browser/devtools/webide/modules/simulators.js
@@ +25,3 @@
>      let deferred = promise.defer();
> +    // TODO Load a persistent list of configured simulators first.
> +    this._simulators = [];

The usage of this _simulators private is misleading and introduces unnecessary complexity over the whole module. (`if (!_simulators) findAll().then(myFun) else ...` everywhere)

I see two ways to address that:
- make Simulators be lazy itself, so that any user of it would need to fetch it as a promise, similarely to GetDevices().
Something like `GetSimulators().then(Simulators => ... )`.
- stop using _simulators everywhere in this module and instead, make it so that findAll is the only one to use it internally to memoize it. All other callsites are going to replace
  let simulators = this._simulators;
by
  let simulators = yield this.findAll();
findAll may sound wrong with such practice and propagate promise usage. Tasks may help you handle that (up to you as you tend to use raw promises so far).

@@ +27,5 @@
> +    this._simulators = [];
> +
> +    GetDevices().then(devices => {
> +      // Use the first device (featured) as reference for emulation options.
> +      this._defaultOptions = devices[devices.TYPES[0]][0];

hum I don't feel confident with such practice.
It looks too much implicit and greatly increase the risk of breaking something with the remote devices ressource.
Also regarding security I feel very bad as we are passing some arguments from a remote source to some code running processes.

Can't we used hardcoded defaults here ?

@@ +66,5 @@
> +
> +  // Detect simulator addons, including "unofficial" ones
> +  isSimulatorAddon(addon) {
> +    return SimulatorRegExp.exec(addon.id);
> +  },

nit: it looks like an internal to this component. _isSimulatorAddon?
Same comment applies to most other method: add.* and remove.*

@@ +75,5 @@
> +    let simulators = this._simulators;
> +    if (!simulators) {
> +      return this.findAll().then(this.add.bind(this, simulator));
> +    }
> +    // TODO Ensure `simulator.name` is unique by appending to it if necessary.

Note that the general practice, not enforced everytime, is to open a followup bug and refer to the bug number for TODOs.

@@ +76,5 @@
> +    if (!simulators) {
> +      return this.findAll().then(this.add.bind(this, simulator));
> +    }
> +    // TODO Ensure `simulator.name` is unique by appending to it if necessary.
> +    let concat = simulators.concat([simulator]).sort(LocaleCompare);

simulators.push(simulator) ??

@@ +126,5 @@
> +    this.emit("updated");
> +  },
> +
> +  onConfigure(e, simulator) {
> +    this._lastConfiguredSimulator = simulator;

That looks like a workaround the fact that selectDeckPanel doesn't accept any option to pass to simulator.xhtml. (unecessary state)
It would be great if we could somehow pass to simulator.xhtml some data in order to figure out which simulator to load.

@@ +165,3 @@
>  
> +  // Fill `this.options` with default values where needed.
> +  let defaults = Simulators._defaultOptions;

What about passing it as an argument instead of using another component private?
  new Simulator({name}, addon, this._defaultOptions) ?

@@ +165,5 @@
>  
> +  // Fill `this.options` with default values where needed.
> +  let defaults = Simulators._defaultOptions;
> +  for (let option in defaults) {
> +    if (this.options[option] == null) {

nit: if (!(option in this.options))  ?
Attachment #8574066 - Flags: feedback?(poirot.alex) → feedback+
Thanks for the feedback Alex! I'll apply most of your suggestions.

> > +    form.device.innerHTML = "";
> > +    form.device.classList.remove("custom");
> 
> nit: the various innertHTML="" and classList.remove seem to better match
> clear() method than init() one.

I made the "init()" function safe to call several times. If you move the `innerHTML=""` somewhere else, you can find yourself with options duplicated several times.

Also, maybe "clear()" is not the best name, because it's rather the opposite of "edit(simulator)", and it's only supposed to "unlink" the editor from its simulator (maybe "close" or "unedit" or "detach" or "stop"...).

> > +    this.init().then(() => {
> > +
> > +      this._simulator = simulator;
> 
> Is there any particular reason to wait for init() completion before setting
> _simulator?

The later the better. It's just out of paranoia, to be sure that whatever happens to the form during "init()" phase doesn't trigger changes to the simulator.

> > +  // Select an available option, or set the "custom" option.
> 
> This comment doesn't seem to reflect the magic behind this method.
> What about something like:
> Select "custom" option for the given <select> and update its text to the
> given `value` text.

I think it's a clear and concise way of explaining what happens. It doesn't hide magic, because you can infer that if an option is not available, a special "custom" option is set somehow. To know exactly what that means, it's quick to read the code.

Also, your suggestion doesn't explain what happens when an option is in fact available. I could however add a comment next to the "custom" setting code, would that help?

> > +  get device() {
> > +    // TODO Search for a `device.name` matching current simulator options.
> > +    return "custom";
> 
> We should translate that.

I'm not sure what you mean. "custom" is not visible to the user, but is a special <option> value, and that <option> already contains a localized string.

> The usage of this _simulators private is misleading and introduces
> unnecessary complexity over the whole module. (`if (!_simulators)
> findAll().then(myFun) else ...` everywhere)

I'll fix that.

> nit: it looks like an internal to this component. _isSimulatorAddon?
> Same comment applies to most other methods: add.* and remove.*

They do look private now, but `add()` will be used by the Simulator panel to configure new simulators, and `remove()` will be used by a "Delete" button in that panel. As for `isSimulatorAddon` I thought it might become useful elsewhere some day, but ok I'll underscore it.

> > +  onConfigure(e, simulator) {
> > +    this._lastConfiguredSimulator = simulator;
> 
> That looks like a workaround to the fact that selectDeckPanel doesn't accept
> any option to pass to simulator.xhtml. (unecessary state)
> It would be great if we could somehow pass to simulator.xhtml some data in
> order to figure out which simulator to load.

You're right, it's a workaround. I'll see if I can make it better.

> > +  for (let option in defaults) {
> > +    if (this.options[option] == null) {
> 
> nit: if (!(option in this.options))  ?

I would prefer that options like { width: null } or { height: undefined } be overridden by a default value.
Comment on attachment 8574066 [details] [diff] [review]
Make simulators configurable from WebIDE.

New version is coming up soon.
Attachment #8574066 - Flags: feedback?(jryans)
Alex, please have a look. It works really great now, and the code is completely covered by tests.

I will probably punt the remaining TODOs to follow-up bugs, do you agree?

To try it out, you need the latest devices.js catalog from bug 1135018.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d442958cb6cf
Attachment #8574066 - Attachment is obsolete: true
Comment on attachment 8579563 [details] [diff] [review]
Make simulators configurable from WebIDE.

(looks like `git bz` randomly forgets about `Review:` instructions…)
Attachment #8579563 - Flags: review?(poirot.alex)
Comment on attachment 8579563 [details] [diff] [review]
Make simulators configurable from WebIDE.

Review of attachment 8579563 [details] [diff] [review]:
-----------------------------------------------------------------

I have quite a bunch of comments, but looks good overall.
Do not hesitate to split into multiple patches in one bug, that can help reviewing!

::: browser/devtools/webide/content/prefs.xhtml
@@ +98,5 @@
> +            <option value="emacs">Emacs</option>
> +            <option value="sublime">Sublime</option>
> +          </select>
> +        </label>
> +      </li>

I'm not really strict about having a focused and precise scope for patches, but it really feels like you are modifying very unrelated stuff in the same patch/bug. How is that related to simulators ?
Please at least move that in a dedicated patch with a meaningful and related title.

::: browser/devtools/webide/content/simulator.js
@@ +17,5 @@
> +let SimulatorEditor = {
> +  _addons: {},
> +  _devices: {},
> +  _form: null,
> +  _simulator: null,

nit: it would be helpful to document very briefly these fields, especialy _addons and _devices, just to know what are the keys/values.

@@ +54,5 @@
> +        opt(form.version, addon.id, addon.name);
> +      });
> +      opt(form.version, "custom", "");
> +      opt(form.version, "pick", Strings.GetStringFromName("simulator_custom_binary"));
> +      return promise.resolve();

Promises handlers don't have to return promises. You can remove this.

@@ +73,5 @@
> +      devices.TYPES.forEach(type => {
> +        let success = false;
> +        let optgroup = document.createElement("optgroup");
> +        optgroup.label = GetDeviceString(type);
> +        devices[type].forEach(device => {

`success` variable name is poor...
what about hadDevice?

Or filter devices first and then skip or do something:
  let b2gDevices = devices[type].filter(d => d.firefoxOS);
  if (b2gDevices.length ==0) return;
  let optgroup = ...

@@ +88,5 @@
> +      });
> +      return promise.resolve();
> +    }));
> +
> +    promise.all(promises).then(deferred.resolve, deferred.reject);

Isn't it equivalent to `return promise.all(promise)` ??

@@ +116,5 @@
> +      this.updateDeviceSelector();
> +      this.updateDeviceFields();
> +
> +      deferred.resolve();
> +    });

Same thing, you don't need intermediate promise here,
just do `return this.init.then( ... );`

@@ +133,5 @@
> +  deleteSimulator() {
> +    let simulator = this._simulator;
> +    this.clear();
> +    Simulators.remove(simulator);
> +    window.parent.UI.openProject(); // Close the panel.

nit: you can factorize that with #close event handler.

@@ +161,5 @@
> +
> +  set version(value) {
> +    let f = this._form;
> +    let s = this._simulator;
> +    let v = s.version;

nit: personal opinion, I would prefer short names rather than 1 char variable names.

@@ +170,5 @@
> +    } else {
> +      // `value` is a custom binary path.
> +      s.addon = null;
> +      s.options.b2gBinary = value;
> +      // TODO Indicate that a custom profile is required.

please fill bugs for the followups in order to not forget them

@@ +173,5 @@
> +      s.options.b2gBinary = value;
> +      // TODO Indicate that a custom profile is required.
> +    }
> +    // If `form.name` contained the old version, update its last occurrence.
> +    let name = f.name.value.replace(new RegExp("(.*)" + v), "$1" + s.version);

I don't get it, v seems equal to s.version as we did `let v = s.version;` and nor v nor s changed in between ??

@@ +211,5 @@
> +    let f = this._form;
> +    let s = this._simulator;
> +    s.options.width = f.width.value = device.width || null;
> +    s.options.height = f.height.value = device.height || null;
> +    s.options.pixelRatio = f.pixelRatio.value = device.pixelRatio || null;

Sorry for my paranoia, but could you ensure that all fields of GetDevices are correct.
Here we pass our remote resource from device.* to simulator.options.*.
simulator.options is later used to craft a command line to run the simulator
and I'm not sure subprocess is really safe against command line injection in parameter.
So I would be more confident if we were doing something like:
  s.options.width = f.width.value = parseInt(device.width) || null;
  ...
You can do it here or in SimulatorProcess as you prefer.

@@ +273,5 @@
> +            this.profile = input[input.selectedIndex].textContent;
> +            break;
> +          default:
> +            this.profile = input.value;
> +        }

It looks like you miss a break here.

::: browser/devtools/webide/content/simulator.xhtml
@@ +39,5 @@
> +        </li>
> +        <li>
> +          <label>
> +            <span class="label">&simulator_version;</span>
> +            <select name="version"/>

I would make this select larger to be able to display default name completely.

::: browser/devtools/webide/content/webide.xul
@@ +182,5 @@
>          <vbox flex="1" id="runtime-actions">
>            <toolbarbutton class="panel-item" id="runtime-details" command="cmd_showRuntimeDetails"/>
>            <toolbarbutton class="panel-item" id="runtime-permissions" command="cmd_showPermissionsTable"/>
>            <toolbarbutton class="panel-item" id="runtime-preferences" command="cmd_showDevicePrefs"/>
> +          <toolbarbutton class="panel-item" id="runtime-settings" command="cmd_showSettings"/>

When the patch grows, it becomes handy to pull out obvious unrelated changes like this in a distinct patch so that we read/r+ it only once.
(if that helps you can merge the patches as you want before checking it in)

::: browser/devtools/webide/modules/addons.js
@@ +8,3 @@
>  const {Services} = Cu.import("resource://gre/modules/Services.jsm");
>  const {GetAddonsJSON} = require("devtools/webide/remote-resources");
> +const EventEmitter = require("devtools/toolkit/event-emitter");

Same thing (distinct patch when the original patch grows)

::: browser/devtools/webide/modules/runtimes.js
@@ +207,5 @@
>      this.emit("runtime-list-updated");
>    },
>  
>    _updateRuntimes() {
> +    Simulators.findAll().then(simulators => {

With the Simulator init weirdness we end up calling _updateRuntimes `x` times, `x` being the number of simulator you have registered!
BTW, do you plan to offer a way to create new simulator?
Right now, the only way to create new one is to install a new xpi.

::: browser/devtools/webide/modules/simulators.js
@@ +39,5 @@
> +  findAll() {
> +    if (!this._loaded) {
> +      return this._load();
> +    }
> +    return promise.resolve(this._simulators);

Please merge _load into findAll, or, findAll() can also be just a promise:

let Simulators = {
 get simulators() {
   if (this._simulators) {
     return this._simulators;
   }
   return this._simulators = this._load();
 },
 ..
};

The `this._loaded` logic shared between these two methods is unecessary hard to follow.

This proposal introduces back some async code... but this findAll / _simulators story is still a big misleading.
By default _simulators is an empty array. We can start populating it by calling Simulators.add or equivalent, but at some point we do call findAll from runtimes.js, but we call it "at some point". We dont try to ensure to call it early or on startup, or it's not really explicit.
It feels like we meant or want to call a init/load method early.

May be it would be clearer if we expose a load or init method and call during startup (SimulatorScanner.enable??)

::: browser/devtools/webide/test/test_simulators.html
@@ +116,5 @@
> +
> +          is(find("#deck").selectedPanel, simulatorPanel, "Simulator deck panel is selected");
> +
> +          yield lazyIframeIsLoaded(simulatorPanel);
> +          yield nextTick();

All users of lazyIframeIsLoaded call nextTick... may be we should move resolve lazyIframeIsLoaded only after nextTick!

@@ +134,5 @@
> +          MockFilePicker.init(simulatorPanel.contentWindow);
> +
> +          // Test `name`.
> +
> +          is(form.name.value, findAll(".runtime-panel-item-simulator")[0].label, "Original simulator name");

Here and everywhere you are using findAll("...")[0], please replace by find("...").

::: browser/devtools/webide/themes/deck.css
@@ +76,5 @@
> +}
> +
> +input[type="number"] {
> +  width: 60px;
> +}

I'm not sure we want such arbitrary size in deck.css,
it looks more something specific to simulators.css.
Attachment #8579563 - Flags: review?(poirot.alex)
Depends on: 1146097
Blocks: 1146519
Blocks: 1146521
Blocks: 1146531
Blocks: 1146862
Attachment #8579563 - Attachment is obsolete: true
Attachment #8582366 - Flags: review?(poirot.alex)
Attachment #8582367 - Flags: review?(poirot.alex)
Comment on attachment 8582368 [details] [diff] [review]
Transition test_addons.html to the new Simulator architecture.

This makes test_addons.html stop using the deprecated `Simulator` and instead uses the new `Simulators` module.

Also, I removed all simulator-registration code from the test addon XPIs.
Attachment #8582368 - Flags: review?(poirot.alex)
Comment on attachment 8582370 [details] [diff] [review]
Add way to make WebIDE runtimes configurable.

This reworks the runtime list a little. TL;DR - If a runtime has a "configure()" method, then a settings icon is added to the right of its runtime button.
Attachment #8582370 - Flags: review?(poirot.alex)
Comment on attachment 8582371 [details] [diff] [review]
Make WebIDE's Firefox OS Simulators configurable.

Alex, thanks for the review! Please have a look at the newer, exploded patches.

I addressed all your nits, except when otherwise stated below.

(In reply to Alexandre Poirot [:ochameau] from comment #101)
> > +    // If `form.name` contained the old version, update its last occurrence.
> > +    let name = f.name.value.replace(new RegExp("(.*)" + v), "$1" + s.version);
> 
> I don't get it, v seems equal to s.version as we did `let v = s.version;`
> and nor v nor s changed in between ??

You didn't see that `simulator.version` is a magical getter that checks its runtime (which changes above) to figure out its version.

I've rewritten the code to make it clearer (and fix a unique name corner-case), is that better?

> > +    s.options.width = f.width.value = device.width || null;
> > +    s.options.height = f.height.value = device.height || null;
> > +    s.options.pixelRatio = f.pixelRatio.value = device.pixelRatio || null;
> 
> Sorry for my paranoia, but could you ensure that all fields of GetDevices
> are correct.
> Here we pass our remote resource from device.* to simulator.options.*.
> simulator.options is later used to craft a command line to run the simulator
> and I'm not sure subprocess is really safe against command line injection in
> parameter.
> So I would be more confident if we were doing something like:
>   s.options.width = f.width.value = parseInt(device.width) || null;
>   ...
> You can do it here or in SimulatorProcess as you prefer.

Your paranoia is justified, thanks for catching that! I edited SimulatorProcess to `parseInt` or `nsIFile` all the fields.

> > +          default:
> > +            this.profile = input.value;
> > +        }
> 
> It looks like you miss a break here.

I don't think a `break` statement is very useful here.

> > +            <span class="label">&simulator_version;</span>
> > +            <select name="version"/>
> 
> I would make this select larger to be able to display default name
> completely.

The select was large enough on my screen, but as discussed it looks like sometimes different font sizes are used for inputs. I've changed the styling to make it a little larger, and use root-font-tied sizes `rem`. Please let me know if it looks better for you.

> >      this.emit("runtime-list-updated");
> >    },
> >  
> >    _updateRuntimes() {
> > +    Simulators.findAll().then(simulators => {
> 
> With the Simulator init weirdness we end up calling _updateRuntimes `x`
> times, `x` being the number of simulator you have registered!

Yes, because simulators are added one by one, mutating the simulator list every time. I didn't think it's a problem to call `updateRuntimes` several times on startup. Do you think it is?

> BTW, do you plan to offer a way to create new simulator?
> Right now, the only way to create new one is to install a new xpi.

Yes, eventually. There is already a code path for it (in content/simulator.js, when `edit()` is called with no parameter) and I filed bug 1146862 for it.

> This proposal introduces back some async code... but this findAll /
> _simulators story is still a big misleading.
> By default _simulators is an empty array. We can start populating it by
> calling Simulators.add or equivalent, but at some point we do call findAll
> from runtimes.js, but we call it "at some point". We dont try to ensure to
> call it early or on startup, or it's not really explicit.
> It feels like we meant or want to call a init/load method early.
> 
> May be it would be clearer if we expose a load or init method and call
> during startup (SimulatorScanner.enable??)

I would like `this._simulators` to be an array early on, while the list of persisted simulator configurations (and default configurations for each unused simulator addon) is being loaded lazily. That way new simulator configurations can be added early on (e.g. by an addon). I've removed the `_load()` function, but a `this._loaded` boolean still remains. Please tell me if you're still unhappy about that code.

> Here and everywhere you are using findAll("...")[0], please replace by
> find("...").

Done, although I thought that the `findAll("...")[i]` form would make it more explicit that we're grabbing element number `#i` in a list of `#N` elements.
Attachment #8582371 - Flags: review?(poirot.alex)
Attachment #8582366 - Flags: review?(poirot.alex) → review+
Comment on attachment 8582367 [details] [diff] [review]
Add `nextTick()` to `lazyIframeIsLoaded()` in WebIDE tests.

Review of attachment 8582367 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!
Attachment #8582367 - Flags: review?(poirot.alex) → review+
Attachment #8582368 - Flags: review?(poirot.alex) → review+
Attachment #8582370 - Flags: review?(poirot.alex) → review+
Comment on attachment 8582366 [details] [diff] [review]
Refactor WebIDE preferences panel style.

Please check this patch in.
Attachment #8582366 - Flags: checkin?
Comment on attachment 8582367 [details] [diff] [review]
Add `nextTick()` to `lazyIframeIsLoaded()` in WebIDE tests.

Please check this patch in.
Attachment #8582367 - Flags: checkin?
Thanks for the reviews, Alex!

Sheriffs, please land the first two patches (marked with the "checkin?" flag).
Here is a try for them: https://treeherder.mozilla.org/#/jobs?repo=try&revision=118d0bbb643d
Comment on attachment 8582371 [details] [diff] [review]
Make WebIDE's Firefox OS Simulators configurable.

Review of attachment 8582371 [details] [diff] [review]:
-----------------------------------------------------------------

Patch looks good now but:
- the UI fails when I have bad connection (rejected SSL due to wrong date/time).
- I'm not sure we want to expose this feature in nightly until we have persitent data...
We should either hide the feature (may be hiding the config button is enough??) or wait for persistency.
- Using a custom gaia profile doesn't work unless I also use a custom b2g binary!
- Deleting a simulator from the simulator options is weird as it disappear and reappear when you relaunch firefox.

Or may be I mis-applied the patches as I had some rejects?

::: browser/devtools/webide/content/simulator.js
@@ +155,5 @@
> +  // VERSION: Can be an installed `addon.id` or a custom binary path.
> +
> +  get version() {
> +    let simulator = this._simulator;
> +    return simulator.options.b2gBinary || simulator.addon.id;

nit: I'm not sure the `let simulator = ` shortchut is really valuable here...

::: browser/devtools/webide/content/simulator.xhtml
@@ +70,5 @@
> +        </li>
> +        <li>
> +          <label>
> +            <span class="label">&simulator_pixelRatio;</span>
> +            <input name="pixelRatio" data-device="" type="number" step="0.05"/>

We should hide the ratio, or just not land it until we start using it!

::: browser/devtools/webide/modules/simulators.js
@@ +7,5 @@
>  const { ConnectionManager } = require("devtools/client/connection-manager");
> +const { AddonSimulatorProcess,
> +        OldAddonSimulatorProcess,
> +        CustomSimulatorProcess } = require("devtools/webide/simulator-process");
> +const { GetDevices } = require("devtools/shared/devices");

You are no longer using GetDevices in this module.
Attachment #8582371 - Flags: review?(poirot.alex) → review-
:ochameau's r+.
Attachment #8582368 - Attachment is obsolete: true
Alex, you already r+'d this but I put it behind a pref (default off).
Attachment #8582370 - Attachment is obsolete: true
Attachment #8583020 - Flags: review+
Comment on attachment 8583021 [details] [diff] [review]
Add way to make WebIDE runtimes configurable.

(see comment 116)
Attachment #8583021 - Flags: review?(poirot.alex)
(In reply to Alexandre Poirot [:ochameau] from comment #114)

New nits addressed.

> - the UI fails when I have bad connection (rejected SSL due to wrong
> date/time).

I couldn't reproduce this. In any case, when you really don't have a network connection, all the remote resources (templates, addons and devices) will break unless you have a cached version. This problem is probably out of scope.

> - I'm not sure we want to expose this feature in nightly until we have
> persitent data...
> We should either hide the feature (may be hiding the config button is
> enough??) or wait for persistency.

The patch brings a desired functionality, even though it's not yet as practical as it will be once the list is persisted. But I've hidden the feature behind a WebIDE pref for now.

> - Using a custom gaia profile doesn't work unless I also use a custom b2g
> binary!

Right, I forgot about the custom Gaia profile in AddonSimulatorProcess. Fixed with added test.

> - Deleting a simulator from the simulator options is weird as it disappear
> and reappear when you relaunch firefox.

I agree it's weird. What do you suggest to fix this?
- Remove the "Delete Simulator" option?
- Uninstall the addon? (Only if the Simulator was the only one to use it)
- Only generate the default configurations for installed addons when the persistent list is empty?

> Or may be I mis-applied the patches as I had some rejects?

Please let me know about future rejects in more details. The patches should apply cleanly, as long as you apply them from earliest to latest.
Attachment #8582371 - Attachment is obsolete: true
Attachment #8583023 - Flags: review?(poirot.alex)
Comment on attachment 8583021 [details] [diff] [review]
Add way to make WebIDE runtimes configurable.

Review of attachment 8583021 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/webide/content/prefs.xhtml
@@ +51,5 @@
> +          <input type="checkbox" data-pref="devtools.webide.enableRuntimeConfiguration"/>
> +          <span>&prefs_options_runtimeconfiguration;</span>
> +        </label>
> +      </li>
> +      <li>

For such prototype there is no need to make it visible.
Like devtools.webide.sidebars for new project listing feature.
Once it is ready we will just get rid of the pref completely.

::: browser/locales/en-US/chrome/browser/devtools/webide.dtd
@@ +121,5 @@
>  <!ENTITY prefs_options_autoconnectruntime_tooltip "Reconnect to previous runtime when WebIDE starts">
>  <!ENTITY prefs_options_rememberlastproject "Remember last project">
>  <!ENTITY prefs_options_rememberlastproject_tooltip "Restore previous project when WebIDE starts">
> +<!ENTITY prefs_options_runtimeconfiguration "Enable runtime configuration">
> +<!ENTITY prefs_options_runtimeconfiguration_tooltip "Show a configure button next to runtimes that can be configured">

Do not forget to also remove that.
Attachment #8583021 - Flags: review?(poirot.alex) → review+
Comment on attachment 8583023 [details] [diff] [review]
Make WebIDE's Firefox OS Simulators configurable.

Review of attachment 8583023 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Jan Keromnes [:janx] from comment #118) 
> I couldn't reproduce this. In any case, when you really don't have a network
> connection, all the remote resources (templates, addons and devices) will
> break unless you have a cached version. This problem is probably out of
> scope.

You are right, It doesn't work out of an empty profile.
But it does work correctly once you opened WebIDE one time with a valid connection.
That's all we need.

> 
> > - I'm not sure we want to expose this feature in nightly until we have
> > persitent data...
> > We should either hide the feature (may be hiding the config button is
> > enough??) or wait for persistency.
> 
> The patch brings a desired functionality, even though it's not yet as
> practical as it will be once the list is persisted. But I've hidden the
> feature behind a WebIDE pref for now.

May be but that's also really disturbing to see half baked feature like this.
Nightly is used by too many people now...
This is really common to work like that for big features.

> > - Deleting a simulator from the simulator options is weird as it disappear
> > and reappear when you relaunch firefox.
> 
> I agree it's weird. What do you suggest to fix this?
> - Remove the "Delete Simulator" option?
> - Uninstall the addon? (Only if the Simulator was the only one to use it)
> - Only generate the default configurations for installed addons when the
> persistent list is empty?

I would say we can just get rid of the delete link for now.
Otherwise I think it would make sense to uninstall the addon if we remove the last one,
but that doesn't sound like an important feature.
You can just remove this link and figure out something better in a followup if you consider this important.

> Please let me know about future rejects in more details. The patches should
> apply cleanly, as long as you apply them from earliest to latest.

It's just that I'm working on top of a big patch queue, so I'm used to resolve conflicts...
But sometimes I fail resolving correctly some of them.

::: browser/devtools/webide/content/simulator.js
@@ +290,5 @@
> +            this.profile = input[input.selectedIndex].textContent;
> +            break;
> +          default:
> +            this.profile = input.value;
> +        }

As for `case "version":` you miss a `break;` here. otherwise, when we select a gaia profile we are also going to execute `device` case!
Attachment #8583023 - Flags: review?(poirot.alex) → review+
Attachment #8582366 - Flags: checkin? → checkin+
Attachment #8582367 - Flags: checkin? → checkin+
:ochameau's r+.
Attachment #8583021 - Attachment is obsolete: true
Attachment #8583023 - Attachment is obsolete: true
Attachment #8583020 - Attachment is obsolete: true
Attachment #8583848 - Flags: review+
Attachment #8583849 - Flags: review+
Attachment #8583850 - Flags: review+
Sheriffs, please land the 3 remaining patches on fx-team.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9486213c3251

Relevant tests pass locally. The try is just to be safe, but please disregard any unrelated WebRTC failures caused by bug 1147853 (hence the "CLOSED TREE").

Note: The try push includes two additional patches that already landed in fx-team, but haven't rolled to master yet.
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
Looks like I broke M(oth) and maybe M(dt4) as well.
Keywords: checkin-needed
I find these failures surprising, and can't reproduce them locally.

Let's try again: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1b004061f054
Blocks: 1149214
No longer depends on: 1149214
Blocks: 1149221
No longer depends on: 1149221
Depends on: 1151796
Attachment #8583848 - Attachment is obsolete: true
Attachment #8583849 - Attachment is obsolete: true
Attachment #8583850 - Attachment is obsolete: true
Attachment #8589049 - Flags: review+
Attachment #8589052 - Flags: review+
Attachment #8589054 - Flags: review+
Potential race condition and window leak are real. I'll have to spend more time on this.
Alias: simulator-profiles → simulator-configuration
Attachment #8589049 - Attachment is obsolete: true
Attachment #8589052 - Attachment is obsolete: true
Attachment #8589902 - Flags: superreview+
Attachment #8589900 - Flags: review+
Attachment #8589902 - Flags: superreview+ → review+
Attachment #8589904 - Flags: review+
Let's call it green! Sheriff-in-charge, please land the last three patches.
https://hg.mozilla.org/mozilla-central/rev/c271c5217374
https://hg.mozilla.org/mozilla-central/rev/3b8b0b7881a7
https://hg.mozilla.org/mozilla-central/rev/7e2d53892c7f
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 40
Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/7f40865ab25abad3e28077f5500b00a590278a5f
Bug 1090949 - Make WebIDE's Firefox OS Simulators configurable. r=ochameau
Blocks: 1156834
Blocks: 1212352
No longer blocks: 1212352
No longer blocks: 1030565
Product: Firefox → DevTools
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.