Paul Rouget [:paul] Reporter
	Description • 10 years ago

Developers usually always work with the same runtime and debug the same app.

We can probably help the user by auto-selecting the latest runtime and try to connect to it, and in the case of Runtime Apps, auto select the latest app (package/hosted apps are already auto-selected).

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)
	Comment 1 • 10 years ago

We have bug 1055666 to focus on the "re-select last project if it's a runtime app" portion.

Let's re-focus this to just the "auto-select last used runtime" step.

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)
	Comment 2 • 10 years ago

Attaching Alex's patch from bug 1055279 as initial work.

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)
	Comment 3 • 10 years ago

Comment on attachment 8487327 [details] [diff] [review]
Initial Attempt

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

I do think these bits would make more sense it webide.js instead of app-manager.js.

I think of app-manager as the model object, with the current state of the apps and runtimes, and emits events when these things change.

webide.js is more the view & controller object.  It asks app-manager for the current state, tells it to perform actions and make changes, and listens for events from app-manager to hear when the state changes.

So, overall it seems good, but let's move to webide.js.

::: browser/devtools/webide/modules/app-manager.js
@@ +104,5 @@
>      this.emit("app-manager-update", what, details);
> +
> +    // Automatically reconnect to the previously runtime,
> +    // if available and has an ID
> +    if (what == "runtimelist" && this.selectedRuntime == null &&

Moving this to webide.js as a new method called after "runtimelist" seems good.

With the type name as well (as I suggest below), you should know where to look too.

@@ +139,5 @@
>    onConnectionChanged: function() {
>      if (this.connection.status == Connection.Status.DISCONNECTED) {
> +      if (typeof(this.selectedRuntime.getID) === "function") {
> +        this.lastConnectedRuntimeID = this.selectedRuntime.getID();
> +        Services.prefs.setCharPref("devtools.webide.lastConnectedRuntimeID",

Rather than storing this at disconnection time, it seems clearer to me to store it right after a successful connection.

In webide.js, I'd suggest adding a new method that's called on the "runtime" event, and you'd store it then.

Also, I think we should store the type of runtime along with the ID.  In bug 916804, I added string type names to the runtimes.  That would useful here if that patch ever lands...

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)
	Comment 4 • 10 years ago

Alex, I've added some comments above.  If you don't have time for this anymore, let me know.

	Alexandre Poirot [:ochameau] Assignee
	Comment 5 • 10 years ago

Part of patch already r+ by paul in bug 916804.
Hopefully, this isn't the part of the patch that makes TBPL intermittently orange!!

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

	Alexandre Poirot [:ochameau] Assignee
	Comment 6 • 10 years ago

This patch depend on you work adding `type` on runtimes,
so I just pulled that and submitted it to eventually land it
if this patch lands before bug 916804.

Otherwise I tried to address all the feedback comments
and also provided a test.

	Alexandre Poirot [:ochameau] Assignee
	Comment 7 • 10 years ago

Forgot to rename lastConnectedRuntimeID to lastConnectedRuntime in webide-prefs.js

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

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)
	Comment 8 • 10 years ago

Comment on attachment 8491013 [details] [diff] [review]
patch v3

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

A few renames and such, but overall it looks good.  Thanks!

::: browser/devtools/webide/content/webide.js
@@ +346,5 @@
>        }
>      }
>    },
>  
> +  autoSelectedRuntime: function () {

Either "autoSelectRuntime" or "autoConnectToRuntime"

@@ +347,5 @@
>      }
>    },
>  
> +  autoSelectedRuntime: function () {
> +    // Automatically reconnect to the previously runtime,

previously selected

@@ +362,5 @@
> +      type = "custom";
> +    }
> +
> +    // We support most runtimes except simulator,
> +    // that needs to be manually launched

Nit: Wrap to 80 chars

@@ +368,5 @@
> +      for (let runtime of AppManager.runtimeList[type]) {
> +        // Some runtimes do not expose getID function and don't support
> +        // autoconnect (like remote connection)
> +        if (typeof(runtime.getID) == "function" && runtime.getID() == id) {
> +          AppManager.connectToRuntime(runtime);

Use |this.connectToRuntime| (the webide.js UI function), so that the busy indicator is displayed.

@@ +390,5 @@
>        labelNode.setAttribute("value", name);
>      }
>    },
>  
> +  saveLastSelectedRuntime: function () {

The pref is "connected" and this function is "selected", pick your favorite and use it for both.

::: browser/devtools/webide/modules/app-manager.js
@@ +605,5 @@
>        this.runtimeList.usb.push(r);
>        r.updateNameFromADB().then(
> +        () => {
> +          this.update("runtimelist");
> +          // Also update the runtime button label,

Nit: Feel free to wrap to 80 chars like usual

::: browser/devtools/webide/test/test_autoconnect_runtime.html
@@ +75,5 @@
> +          is(Object.keys(DebuggerServer._connections).length, 1, "Automatically reconnected");
> +
> +          yield win.Cmds.disconnectRuntime();
> +
> +          yield closeWebIDE(win);

Move close and server destroy to a cleanup function so they still happen if the test blows up.

::: browser/devtools/webide/webide-prefs.js
@@ +15,5 @@
>  pref("devtools.webide.adbAddonURL", "https://ftp.mozilla.org/pub/mozilla.org/labs/fxos-simulator/adb-helper/#OS#/adbhelper-#OS#-latest.xpi");
>  pref("devtools.webide.adbAddonID", "adbhelper@mozilla.org");
>  pref("devtools.webide.monitorWebSocketURL", "ws://localhost:9000");
> +pref("devtools.webide.lastConnectedRuntime", "");
> +pref("devtools.webide.lastSelectedProject", "");

This project pref belongs in the other bug I think...

	Alexandre Poirot [:ochameau] Assignee
	Comment 9 • 10 years ago

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

	Alexandre Poirot [:ochameau] Assignee
	Comment 10 • 10 years ago

Unfortunately, the usage of registerCleanupfunction fails...
I have no idea why but very few tests are doing this
and the only usage in webide uses synchronous calls.
Here we have some async function to call.
But the cleanup function doesn't seem to be called at all.

Here is a new patch without cleanup function...
https://tbpl.mozilla.org/?tree=Try&rev=d7fe90600344

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)
	Comment 11 • 10 years ago

Comment on attachment 8491460 [details] [diff] [review]
patch v4

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

::: browser/devtools/webide/test/test_autoconnect_runtime.html
@@ +25,5 @@
> +          DebuggerServer.addBrowserActors();
> +
> +          let win = yield openWebIDE();
> +
> +          SimpleTest.registerCleanupFunction(function* () {

Looks like |registerCleanupFunction| doesn't understand generators, maybe that was the issue?

	Alexandre Poirot [:ochameau] Assignee
	Comment 12 • 10 years ago

(In reply to J. Ryan Stinnett [:jryans] from comment #11)
> > +          SimpleTest.registerCleanupFunction(function* () {
> 
> Looks like |registerCleanupFunction| doesn't understand generators, maybe
> that was the issue?

It was also failing with a regular function. It doesn't seem to be called at all :/

	Ryan VanderMeulen [:RyanVM]
	Comment 13 • 10 years ago

https://hg.mozilla.org/integration/fx-team/rev/c10eea79e559
https://hg.mozilla.org/integration/fx-team/rev/9119c622fdf3

	Ryan VanderMeulen [:RyanVM]
	Comment 14 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/c10eea79e559
https://hg.mozilla.org/mozilla-central/rev/9119c622fdf3