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).

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.

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

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...

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

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

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.

Forgot to rename lastConnectedRuntimeID to lastConnectedRuntime in webide-prefs.js https://tbpl.mozilla.org/?tree=Try&rev=c52bf2aae49b

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...

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

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?

(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 :/

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

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