1016924 - [appmgr v2] support non-device runtimes

Status: RESOLVED FIXED

(DevTools Graveyard :: WebIDE, defect)

Comment 1

[Paul Rouget [:paul]]

Attachment: v1

Comment 2

[Alexandre Poirot [:ochameau]]

Comment on attachment 8429996 [details] [diff] [review]
v1

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

Does that mean we can have adb helper devices AND connecting to custom ip:port ?!
yes => \o/

Otherwise, I'm not sure it is relevant to expose connecting to firefox itself.
What about pref it off or something?

::: browser/devtools/webide/modules/app-manager.js
@@ +41,5 @@
>      this.onWebAppsStoreready = this.onWebAppsStoreready.bind(this);
>      this.webAppsStore = new WebappsStore(this.connection);
>      this.webAppsStore.on("store-ready", this.onWebAppsStoreready);
>  
> +    this.runtimeList = {usb: [], simulator: [], custom: [gLocalRuntime, gRemoteRuntime]};

Are you sure we are ready to expose local runtime access by default?
It looks like something useful for us for testing but may be misleading for most people discovering webide.
What about keeping it behind a pref?

@@ +280,5 @@
> +    try {
> +      this.selectedRuntime.connect(this.connection).then(
> +        () => {},
> +        () => {deferred.reject()});
> +    } catch(e) {}

Shouldn't we reject the promise in case of exception?

::: browser/devtools/webide/modules/runtimes.js
@@ +19,5 @@
> +  connect: function(connection) {
> +    let device = Devices.getByName(this.id);
> +    if (!device) {
> +      console.error("Can't find device: " + id);
> +      return promise.reject();

Shouldn't we reject the promise with this message?
Shouldn't we show an error in UI instead of console.error?

::: toolkit/devtools/server/actors/webapps.js
@@ +798,5 @@
>  
>      let deferred = promise.defer();
>  
> +    if (Services.appinfo.ID != "{3c2e2abc-06d4-11e1-ac3b-374f68613e61}") {
> +      return { error: "notB2G",

s/notB2G/notSupported/

Comment 3

[Paul Rouget [:paul]]

(In reply to Alexandre Poirot (:ochameau) from comment #2)
> Comment on attachment 8429996 [details] [diff] [review]
> v1
> 
> Review of attachment 8429996 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Does that mean we can have adb helper devices AND connecting to custom
> ip:port ?!
> yes => \o/

Yep.

> Otherwise, I'm not sure it is relevant to expose connecting to firefox
> itself.
> What about pref it off or something?

Ok.

> ::: browser/devtools/webide/modules/app-manager.js
> @@ +41,5 @@
> >      this.onWebAppsStoreready = this.onWebAppsStoreready.bind(this);
> >      this.webAppsStore = new WebappsStore(this.connection);
> >      this.webAppsStore.on("store-ready", this.onWebAppsStoreready);
> >  
> > +    this.runtimeList = {usb: [], simulator: [], custom: [gLocalRuntime, gRemoteRuntime]};
> 
> Are you sure we are ready to expose local runtime access by default?
> It looks like something useful for us for testing but may be misleading for
> most people discovering webide.
> What about keeping it behind a pref?

Ok.

> @@ +280,5 @@
> > +    try {
> > +      this.selectedRuntime.connect(this.connection).then(
> > +        () => {},
> > +        () => {deferred.reject()});
> > +    } catch(e) {}
> 
> Shouldn't we reject the promise in case of exception?

Done.

> ::: browser/devtools/webide/modules/runtimes.js
> @@ +19,5 @@
> > +  connect: function(connection) {
> > +    let device = Devices.getByName(this.id);
> > +    if (!device) {
> > +      console.error("Can't find device: " + id);
> > +      return promise.reject();
> 
> Shouldn't we reject the promise with this message?
> Shouldn't we show an error in UI instead of console.error?

Done.

> ::: toolkit/devtools/server/actors/webapps.js
> @@ +798,5 @@
> >  
> >      let deferred = promise.defer();
> >  
> > +    if (Services.appinfo.ID != "{3c2e2abc-06d4-11e1-ac3b-374f68613e61}") {
> > +      return { error: "notB2G",
> 
> s/notB2G/notSupported/

Done.

Comment 4

[Paul Rouget [:paul]]

Attachment: v1.1

Addressed comments.

Comment 5

[Paul Rouget [:paul]]

https://hg.mozilla.org/integration/fx-team/rev/2fb11b6f1dfe

Comment 6

[Paul Rouget [:paul]]

https://hg.mozilla.org/integration/fx-team/rev/350f078f92a3

Comment 7

[Paul Rouget [:paul]]

Attachment: v1.2

Fixes xpcshell failure.

https://tbpl.mozilla.org/?tree=Try&rev=afcd957f12e9

Comment 8

[Paul Rouget [:paul]]

https://hg.mozilla.org/integration/fx-team/rev/64d5937a15d0

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/64d5937a15d0