Closed Bug 1069552 Opened 5 years ago Closed 5 years ago

WebIDE Runtime API

Categories

(DevTools :: WebIDE, defect)

defect
Not set

Tracking

(firefox34 unaffected, firefox35 fixed, firefox36 fixed)

RESOLVED FIXED
Firefox 36
Tracking Status
firefox34 --- unaffected
firefox35 --- fixed
firefox36 --- fixed

People

(Reporter: jryans, Assigned: jryans)

References

Details

Attachments

(2 files, 2 obsolete files)

Add a real API to register new runtimes that may be found via add-ons, etc.

API should be flexible in case various functions are handled by different add-on or code paths, like:

* Listing runtimes
* Making it possible to connect (mapping a port, etc.)
* Connect to the runtime
An update on my plans here:

I'm adding a |Scanner| API to WebIDE's runtimes module, which allows addons or whatever other code to register a new |Scanner|.  Each |Scanner| can produce one or more |Runtime|s, which will then appear in WebIDE's runtime menu for connection.

Paul had described an approach a while ago, which used the word |Driver|, but I believe that is effectively similar to what I am still calling a |Runtime| here.

In my approach, ADB Helper still registers lower-level objects with |Devices| for each ADB device.  However, WebIDE doesn't rely on these anymore like it once did.  Instead, it just enumerates |Runtime|s from the |Scanner|s.  Further, ADB Helper now registers a |Scanner| itself, and produces |Runtime|s that WebIDE can make use of just like any other.

Of course, for features that are ADB-only, WebIDE can also get to the low-level |Device| when needed.

Valence / Fever Dream will also be a |Scanner|, and for ADB devices, it will make use of the |Device| from ADB Helper to port forward and such.
Attached patch Add WebIDE scanner / runtime API (obsolete) — Splinter Review
This adds the scanner / runtime API.  There are API docs in the runtimes.js file, also more design notes above in comment 1.

For ADB Helper, I thought it was best to keep backward compatibility in place for now to avoid potential problems.  So, all 4 combinations of pre-API vs. post-API WebIDE with pre-API vs. post-API ADB Helper are supported for now (and I've tested them locally).  Bug 1085393 is filed to remove the compatibility layer when we are ready to do so (to remove duplication).

My ADB Helper branch using the new API is available[1] which may be useful during the review.  I'll have Alex look at this soon.

For Valence / Fever Dream, I have not currently attempted to maintain compatibility, so post-API WebIDE needs an updated add-on that uses the API.

My V / FD branch with the new API is available[2], which I'll also have reviewed soon.

[1]: https://github.com/jryans/adbhelper/compare/mozilla:master...runtime-api
[2]: https://github.com/jryans/fxdt-adapters/compare/campd:master...runtime-api

Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=e18c3cf347b0
Attachment #8508199 - Flags: review?(paul)
Comment on attachment 8508199 [details] [diff] [review]
Add WebIDE scanner / runtime API

Alex, could you do this review?  If you are too busy, we can try Paul again.

I'd like to get this in for Dev Edition, so I am hoping you can review it soon.  See the comments above for more details and links to corresponding add-on changes.
Attachment #8508199 - Flags: review?(paul) → review?(poirot.alex)
Comment on attachment 8508199 [details] [diff] [review]
Add WebIDE scanner / runtime API

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

Looks good. Feel free to do even more as suggested in last comment.

::: browser/devtools/webide/content/monitor.js
@@ +224,5 @@
> +    if (runtime.device) {
> +      return runtime.device;
> +    }
> +    return Devices.getByName(runtime.id);
> +  },

Instead of putting these workarounds here and there (monitor.js and runtimedetails.js), can't we put it once in DeprecatedUSBRuntime?

::: browser/devtools/webide/modules/app-manager.js
@@ +47,5 @@
> +    this._clearRuntimeList();
> +    this._rebuildRuntimeList = this._rebuildRuntimeList.bind(this);
> +    RuntimeScanners.on("runtime-list-updated", this._rebuildRuntimeList);
> +    RuntimeScanners.enable();
> +    this._rebuildRuntimeList();

RuntimeScanners.enable is very likely goind to emit a runtime-list-updated and call rebuildRuntimeList.

@@ -59,5 @@
> -      custom: [gRemoteRuntime]
> -    };
> -    if (Services.prefs.getBoolPref("devtools.webide.enableLocalRuntime")) {
> -      this.runtimeList.custom.push(gLocalRuntime);
> -    }

Shouldn't we remove this pref from the whole codebase if we are no longer using it?

@@ +611,1 @@
>      this.update("runtimelist");

1/ I'm not sure it is still worth doing this convertion, we may expose whatever exposes RuntimeScanners,
2/ webide.js may just listen/query directly RuntimeScanners and app-manager would be just simplier.

::: browser/devtools/webide/modules/runtimes.js
@@ +22,5 @@
> + * by registering additional |Scanner|s that emit them.
> + *
> + * Each |Scanner| must support the following API:
> + *
> + * * enable()

nit: `*` but bullet points are not easily readable.

@@ +49,5 @@
> + *   @return Iterable
> + *
> + * Each |Runtime| must support the following API:
> + *
> + * * type field / getter

nit: would be more readable by quoting the name of each field:
 - `type` field (getter)

@@ +69,5 @@
> + *   @param  Connection connection
> + *           A |Connection| object from the DevTools |ConnectionManager|.
> + *   @return Promise
> + *           Resolved once you've called the |connection|'s connect() method.
> + */

Thanks for the doc!!

@@ +88,5 @@
> +      // Enable any scanner added while globally enabled
> +      this._enableScanner(scanner);
> +    }
> +    this._scanners.add(scanner);
> +    this._emitUpdated();

Except StaticScanner, all scanners emit updated on enable,
so isn't that redundant to call _emitUpdated if the RuntimeScanners is already enabled?

@@ +147,5 @@
> +  enable() {
> +    if (this._enabledCount++ !== 0) {
> +      // Already enabled scanners during a previous call
> +      return;
> +    }

Is that any useful?
I can see only one RuntimeScanners.enable() in the whole patch.
It is not introducing that much complexity... but it also looks useless.
We could live with just a `this.enabled` flag instead of a count.
Attachment #8508199 - Flags: review?(poirot.alex) → review+
(In reply to Alexandre Poirot [:ochameau] from comment #4)
> ::: browser/devtools/webide/content/monitor.js
> @@ +224,5 @@
> > +    if (runtime.device) {
> > +      return runtime.device;
> > +    }
> > +    return Devices.getByName(runtime.id);
> > +  },
> 
> Instead of putting these workarounds here and there (monitor.js and
> runtimedetails.js), can't we put it once in DeprecatedUSBRuntime?

You're right, I have moved this to the runtime.

> ::: browser/devtools/webide/modules/app-manager.js
> @@ +47,5 @@
> > +    this._clearRuntimeList();
> > +    this._rebuildRuntimeList = this._rebuildRuntimeList.bind(this);
> > +    RuntimeScanners.on("runtime-list-updated", this._rebuildRuntimeList);
> > +    RuntimeScanners.enable();
> > +    this._rebuildRuntimeList();
> 
> RuntimeScanners.enable is very likely goind to emit a runtime-list-updated
> and call rebuildRuntimeList.

It is likely, but setting up the list immediately like this is useful for tests, which can get access to the local runtime right after page load.

> @@ -59,5 @@
> > -      custom: [gRemoteRuntime]
> > -    };
> > -    if (Services.prefs.getBoolPref("devtools.webide.enableLocalRuntime")) {
> > -      this.runtimeList.custom.push(gLocalRuntime);
> > -    }
> 
> Shouldn't we remove this pref from the whole codebase if we are no longer
> using it?

Sounds fine to me, I've removed all the references.

> @@ +611,1 @@
> >      this.update("runtimelist");
> 
> 1/ I'm not sure it is still worth doing this convertion, we may expose
> whatever exposes RuntimeScanners,
> 2/ webide.js may just listen/query directly RuntimeScanners and app-manager
> would be just simplier.

I agree with you, but I will do this further work in a follow up: bug 1082123.

> ::: browser/devtools/webide/modules/runtimes.js
> @@ +22,5 @@
> > + * by registering additional |Scanner|s that emit them.
> > + *
> > + * Each |Scanner| must support the following API:
> > + *
> > + * * enable()
> 
> nit: `*` but bullet points are not easily readable.

Removed and using indentation now.

> @@ +88,5 @@
> > +      // Enable any scanner added while globally enabled
> > +      this._enableScanner(scanner);
> > +    }
> > +    this._scanners.add(scanner);
> > +    this._emitUpdated();
> 
> Except StaticScanner, all scanners emit updated on enable,
> so isn't that redundant to call _emitUpdated if the RuntimeScanners is
> already enabled?

I suppose it's redundant for some, but I think it's best to be complete and cover very simple scanners too, without adding more complex API requirements of having to emit during |enable| or something.

> @@ +147,5 @@
> > +  enable() {
> > +    if (this._enabledCount++ !== 0) {
> > +      // Already enabled scanners during a previous call
> > +      return;
> > +    }
> 
> Is that any useful?
> I can see only one RuntimeScanners.enable() in the whole patch.
> It is not introducing that much complexity... but it also looks useless.
> We could live with just a `this.enabled` flag instead of a count.

I used a count in case there are future consumers other than WebIDE that want to make use of the runtime list.  This way, the scanner would only be disabled once all consumers have called disable.  Maybe it won't happen, but like you say, the count is not too complex either.

Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=5d506a541e76
Attachment #8508199 - Attachment is obsolete: true
Attachment #8509779 - Flags: review+
Comment on attachment 8509779 [details] [diff] [review]
Add WebIDE scanner / runtime API (v2, ochameau: r+)

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

::: browser/devtools/webide/content/runtimedetails.js
@@ +126,5 @@
>  
>  }
>  
>  function EnableCertApps() {
> +  let device = GetDevice();

It looks like GetDevice isn't defined anywhere?
(In reply to Alexandre Poirot [:ochameau] from comment #6)
> Comment on attachment 8509779 [details] [diff] [review]
> Add WebIDE scanner / runtime API (v2, ochameau: r+)
> 
> Review of attachment 8509779 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/webide/content/runtimedetails.js
> @@ +126,5 @@
> >  
> >  }
> >  
> >  function EnableCertApps() {
> > +  let device = GetDevice();
> 
> It looks like GetDevice isn't defined anywhere?

Yikes, thanks for catching this!
Fixed up |GetDevice| references.  Tested re-enabling unrestricted access locally.

Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=469901d22c0e
Attachment #8509779 - Attachment is obsolete: true
Attachment #8510368 - Flags: review+
https://hg.mozilla.org/integration/fx-team/rev/dfff6e9b01f6
Keywords: checkin-needed
Whiteboard: [status:inflight] → [status:inflight][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/dfff6e9b01f6
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [status:inflight][fixed-in-fx-team] → [status:inflight]
Target Milestone: --- → Firefox 36
Comment on attachment 8510368 [details] [diff] [review]
Add WebIDE scanner / runtime API (v3, ochameau: r+)

Approval Request Comment
[Feature/regressing bug #]: Required for Dev Edition
[User impact if declined]: Fever Dream / Valence uses the API and would be broken won't work with WebIDE without it
[Describe test coverage new/current, TBPL]: On m-c
[Risks and why]: New feature for WebIDE, but risk is contained to WebIDE
[String/UUID change made/needed]: None
Attachment #8510368 - Flags: approval-mozilla-aurora?
Whiteboard: [status:inflight] → [status:landedon]
(In reply to J. Ryan Stinnett [:jryans] from comment #11)
> Comment on attachment 8510368 [details] [diff] [review]
> Add WebIDE scanner / runtime API (v3, ochameau: r+)

> [String/UUID change made/needed]: None

Err, this seems to be not true.

Also, why did this land on gum without aurora approval?
(In reply to Axel Hecht [:Pike] from comment #12)
> (In reply to J. Ryan Stinnett [:jryans] from comment #11)
> > Comment on attachment 8510368 [details] [diff] [review]
> > Add WebIDE scanner / runtime API (v3, ochameau: r+)
> 
> > [String/UUID change made/needed]: None
> 
> Err, this seems to be not true.

Ah, you are right, sorry for missing this.  I think I can craft a version without string changes.  I'll remove the aurora request for now.

> Also, why did this land on gum without aurora approval?

Hmm, I did not place it on gum...  I see that Panos did, but I am not sure why or what his selection criteria were.
Comment on attachment 8510368 [details] [diff] [review]
Add WebIDE scanner / runtime API (v3, ochameau: r+)

Clearing Aurora request for now.
Attachment #8510368 - Flags: approval-mozilla-aurora?
(In reply to J. Ryan Stinnett [:jryans] from comment #13)
> (In reply to Axel Hecht [:Pike] from comment #12)
> > Also, why did this land on gum without aurora approval?
> 
> Hmm, I did not place it on gum...  I see that Panos did, but I am not sure
> why or what his selection criteria were.

Gum is just our very own try branch for having a build with the latest and greatest so that people can test and report bugs. Landing something on gum is not in any way official or anything.
(In reply to Panos Astithas [:past] (overloaded, please needinfo) from comment #15)
> Gum is just our very own try branch for having a build with the latest and
> greatest so that people can test and report bugs. Landing something on gum
> is not in any way official or anything.

So you're not planning to merge Gum back to Aurora later?
I agree that string freeze is important for Aurora, but the changes in this patch are pretty inconsequential. At any rate, we could either keep the removed keys around in 35 or just land a patch there that doesn't make any string changes, but that really seems heavy-handed to me.
(In reply to Francesco Lodolo [:flod] from comment #16)
> So you're not planning to merge Gum back to Aurora later?

No, since we are already tracking every change in its own bug, we will follow the natural landing process, if only in a more expedited way. The delta between aurora and gum should be shrinking with time and today we made a major leap towards that by "rebasing" gum on top of aurora.
(In reply to Panos Astithas [:past] (overloaded, please needinfo) from comment #17)
> I agree that string freeze is important for Aurora, but the changes in this
> patch are pretty inconsequential. At any rate, we could either keep the
> removed keys around in 35 or just land a patch there that doesn't make any
> string changes, but that really seems heavy-handed to me.

Please land a patch that doesn't touch strings on aurora, it avoids creating unnecessary noise for tools.

Right now I have two main differences between gum and aurora: one is this bug, the other is the pref switch (not sure why something already landed on Gum, since the patch wasn't approved yet).
(In reply to Francesco Lodolo [:flod] from comment #19)
> (not sure why something already landed on
> Gum, since the patch wasn't approved yet).

Like I said above, we often land stuff on gum that haven't even been reviewed yet. Look at gum builds the same way you would look at a try build.
Approval Request Comment
[Feature/regressing bug #]: Required for Dev Edition
[User impact if declined]: Fever Dream / Valence uses the API and would be broken won't work with WebIDE without it
[Describe test coverage new/current, TBPL]: On m-c
[Risks and why]: New feature for WebIDE, but risk is contained to WebIDE
[String/UUID change made/needed]: None

Here's a version for aurora without string changes.

This requires the patch from bug 1081093 (currently waiting for aurora approval) to be applied first.
Attachment #8511268 - Flags: review+
Attachment #8511268 - Flags: approval-mozilla-aurora?
Attachment #8511268 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.