The App Manager should offer to install simulators without pointing the user to a web page. The App Manager should detect the presence of the ADB Addon Helper and offer the user to install it if it's not installed. And maybe auto-install it as well.

I'm a bit messy with my patches, here is my patch queue order: Bug 1010271 - remove active attribute when toolbox iframe gets destroyed Bug 1010174 - [appmgr v2] update runningApps list on uninstall. r=jryans Bug 1010387 - [appmgr v2] write tests and make good use of promises & tasks. r=jryans Bug 1007057 - build appmgr v2 (keep it preffed-off). r=mshal Bug 1007218 - Ability to execute App Manager V2 commands from shell Bug 1010712 - [appmgr v2] remove panels animation. r=jryans Bug 1007059 - App Manager v2 (pref it on) Bug 1000415 - Enable async animations in B2G desktop Bug 1011530 - [appmgr v2] Remove the logs from the main UI. r=jryans Bug 1012760 - [appmgr v2] build a webide-specific addon manager

Comment on attachment 8424954 [details] [diff] [review] [appmgr v2] build a webide-specific addon manager Review of attachment 8424954 [details] [diff] [review]: ----------------------------------------------------------------- (Unreviewed yet, please see comment in bug 1011530.)

Alex, before we go through a formal review, can you take a quick look at this? The 3 things I'd like you to focus on: - the mechanism that installs/uninstalls addons. I want to make sure I'm doing it the right way. - the ADB addon auto-install. How do you feel about this? - The static urls in webide-prefs.js. Is that solid enough?

(In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from comment #4) > Created attachment 8434891 [details] [diff] [review] > v1 > > Alex, before we go through a formal review, can you take a quick look at > this? > > The 3 things I'd like you to focus on: > - the mechanism that installs/uninstalls addons. I want to make sure I'm > doing it the right way. That's great. The only issue I'm seeing is that it seems to behave badly if you happen to disable an addon. It will appear as uinstalled and may try to reinstall it and even it is tries, I think it will stay disabled. (i.e. Devices.helperAddonInstalled or Devices.available() are for installed *and* enabled addon. I think we should handle disabled state and use AddonManager APIs instead.) > - the ADB addon auto-install. How do you feel about this? Ease installing is something we should have been doing from day 1. But I'm not sure automagically installing it behind the scene is the best option. I would be more confident doing something in a first launch screen or something. Or a one click install when not available, ... > - The static urls in webide-prefs.js. Is that solid enough? When writing the server side JSON I thought about not only providing addon versions being available, but also href or path to the related xpis.

(In reply to Alexandre Poirot (:ochameau) from comment #5) > (In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from > comment #4) > > Created attachment 8434891 [details] [diff] [review] > > v1 > > > > Alex, before we go through a formal review, can you take a quick look at > > this? > > > > The 3 things I'd like you to focus on: > > - the mechanism that installs/uninstalls addons. I want to make sure I'm > > doing it the right way. > > That's great. The only issue I'm seeing is that it seems to behave badly if > you happen to disable an addon. It will appear as uinstalled and may try to > reinstall it and even it is tries, I think it will stay disabled. You're right, it will re-install it. But at the end, it does: > addon.userDisabled = false; > (i.e. Devices.helperAddonInstalled or Devices.available() are for installed > *and* enabled addon. I think we should handle disabled state and use > AddonManager APIs instead.) Right. > > - the ADB addon auto-install. How do you feel about this? > > Ease installing is something we should have been doing from day 1. But I'm > not sure automagically installing it behind the scene is the best option. I > would be more confident doing something in a first launch screen or > something. Or a one click install when not available, ... Why? I agree that we're being aggressive here, but I think it's best. If the user really doesn't want it, he can uninstall it (and then, we won't redownload it (which is not part of my code yet)) > > - The static urls in webide-prefs.js. Is that solid enough? > > When writing the server side JSON I thought about not only providing addon > versions being available, but also href or path to the related xpis. How would that look?

https://tbpl.mozilla.org/?tree=Try&rev=9aa3979d7372 I kept the autoinstall. It autoinstalls only once. If the user uninstall the helper, it won't re-install it. I'm still not 100% sure we want to do that. This will be discussed before we land this code (I asked some Firefox Desktop folks). I still use the Simulator/Device API to know if the addon is installed. But I make sure that we enable the addon if it was disabled when addon.install() is called.

Not to self: is the import in the CSS file really needed?

We will turn off adb autoinstall until we get approval from legal that it's a thing we can do.

Comment on attachment 8439087 [details] [diff] [review] v2 Review of attachment 8439087 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, thanks for this awesome UX improvement! ::: browser/devtools/webide/content/addons.js @@ +17,5 @@ > + window.parent.UI.closeLastDeckPanel(); > + } > + GetAvailableAddons().then(BuildUI, (e) => { > + console.error(e); > + window.alert(e); We should be more helpful in the alert() call. Here we get "Network error: [object ProgressEvent]" displayed. We should explain that we can't fetch the addon list and display e.message. @@ +30,5 @@ > +} > + > +function BuildItem(addon, isADB) { > + > + function onAddonUpdate(event, ...args) { nit: what is the point of using ...args if you only use args[0]? ::: browser/devtools/webide/modules/addons.js @@ +7,5 @@ > +const {EventEmitter} = Cu.import("resource://gre/modules/devtools/event-emitter.js"); > +const {Simulator} = Cu.import("resource://gre/modules/devtools/Simulator.jsm"); > +const {Devices} = Cu.import("resource://gre/modules/devtools/Devices.jsm"); > +const {Services} = Cu.import("resource://gre/modules/Services.jsm"); > +const {GetAddonsJSON} = require("devtools/webide/remote-resources"); Could you explicitely load promise? (to not rely on the magic of it being injected as global of all modules) @@ +14,5 @@ > +let ADB_LINK = Services.prefs.getCharPref("devtools.webide.adbAddonURL"); > +let SIMULATOR_ADDON_ID = Services.prefs.getCharPref("devtools.webide.simulatorAddonID"); > +let ADB_ADDON_ID = Services.prefs.getCharPref("devtools.webide.adbAddonID"); > + > +let platform = Services.appShell.hiddenDOMWindow.navigator.platform; humm hidden window... what about using Services.appinfo.OS instead? @@ +73,5 @@ > + } > + addons.adb = new ADBAddon(); > + deferred.resolve(addons); > + }, e => { > + deferred.reject(e); If we tried to fetch the list while network is off/bad, we will cache this rejected promise and never try to do the request again. We should do something to retry the request on error: GetAvailableAddons_promise = null; here? @@ +163,5 @@ > + this.version = version; > + this.xpiLink = SIMULATOR_LINK.replace(/#OS#/g, OS) > + .replace(/#VERSION#/g, version) > + .replace(/#SLASHED_VERSION#/g, version.replace(/\./g, "_")); > + this.addonID = SIMULATOR_ADDON_ID.replace(/#SLASHED_VERSION#/g, version.replace(/\./g, "_")); I'm fine with that, but we can also tweak index.json to include urls to xpis: { "adb": { url: "https://ftp.mozilla.org/pub/mozilla.org/labs/fxos-simulator/adb-helper/#OS#/adbhelper-#OS#-latest.xpi", addonId: "adbhelper@mozilla.org" }, "stable": [ { version: "1.3", url: "https://ftp.mozilla.org/pub/mozilla.org/labs/fxos-simulator/1.3/#OS#/fxos_1_3_simulator-#OS#-latest.xpi", addonId: "fxos_1_3_simulator@mozilla.org"} { version: "1.4", url: "https://ftp.mozilla.org/pub/mozilla.org/labs/fxos-simulator/1.4/#OS#/fxos_1_4_simulator-#OS#-latest.xpi", addonId: "fxos_1_4_simulator@mozilla.org"} ], "unstable": [ { version: "2.0", url: "https://ftp.mozilla.org/pub/mozilla.org/labs/fxos-simulator/2.0/#OS#/fxos_2_0_simulator-#OS#-latest.xpi", addonId: "fxos_2_0_simulator@mozilla.org" } ] } (tell me I can update the index.json right away) @@ +171,5 @@ > +SimulatorAddon.prototype = Object.create(Addon.prototype, { > + updateInstallStatus: { > + enumerable: true, > + value: function() { > + if (Simulator.availableVersions().indexOf("Firefox OS " + this.version) > -1) { This is making assumptions on Simulator.jsm behavior, I would better make them within the jsm. We already have something close `getByVersion` but that only works with the full version. What about exposing getByShortVersion ? Also the regexp you used in Simulator.jsm (fullVersion.match(/(\d+\.\d+)/)[0]) looks safier to do the match you do here. In register(), you can compute the short version once for all and put it on simulator object -or- just use the short version as keys? ::: browser/devtools/webide/test/test_addons.html @@ +30,5 @@ > + > + function onSimulatorInstalled(version) { > + let deferred = promise.defer(); > + Simulator.on("register", function onUpdate() { > + if (Simulator.getByVersion("Firefox OS " + version)) { Can't you get the short version from register event? (3rd argument) @@ +52,5 @@ > + let li = doc.querySelector('[status="uninstalled"][addon="simulator-' + version + '"]'); > + if (li) { > + Simulator.off("unregister", onUpdate); > + deferred.resolve(); > + } Shouldn't you reject if you can't find the uninstalled state? @@ +68,5 @@ > + let li = doc.querySelector('[status="uninstalled"][addon="adb"]'); > + if (li) { > + Devices.off("addon-status-updated", onUpdate); > + deferred.resolve(); > + } Same here? ::: browser/devtools/webide/themes/addons.css @@ +111,5 @@ > +li[status="installed"] > .uninstall-button, > +li[status="uninstalled"] > .install-button, > +li[status="preparing"] > progress, > +li[status="downloading"] > progress, > +li[status="installing"] > progress { There is something weird when installing a simulator addon. It starts with a indeterminate progress bar, then switch to an empty progress bar and on my connection, it takes some time before we can see any progression as the xpi is quite big. So given that we have no text, no % of progress and just the progress bar, it looks stuck/buggy until we can finally see some progress.

(In reply to Alexandre Poirot (:ochameau) from comment #10) > @@ +14,5 @@ > > +let ADB_LINK = Services.prefs.getCharPref("devtools.webide.adbAddonURL"); > > +let SIMULATOR_ADDON_ID = Services.prefs.getCharPref("devtools.webide.simulatorAddonID"); > > +let ADB_ADDON_ID = Services.prefs.getCharPref("devtools.webide.adbAddonID"); > > + > > +let platform = Services.appShell.hiddenDOMWindow.navigator.platform; > > humm hidden window... what about using Services.appinfo.OS instead? I know you don't like hiddenDOMWindow :) But I'd like to keep the same logic we find on the FTP server and in the AppManager. > @@ +163,5 @@ > > + this.version = version; > > + this.xpiLink = SIMULATOR_LINK.replace(/#OS#/g, OS) > > + .replace(/#VERSION#/g, version) > > + .replace(/#SLASHED_VERSION#/g, version.replace(/\./g, "_")); > > + this.addonID = SIMULATOR_ADDON_ID.replace(/#SLASHED_VERSION#/g, version.replace(/\./g, "_")); > > I'm fine with that, but we can also tweak index.json to include urls to xpis I like to have all the URLs in the same place. Let's keep it that way. > There is something weird when installing a simulator addon. > It starts with a indeterminate progress bar, then switch to an empty > progress bar > and on my connection, it takes some time before we can see any progression > as the xpi is quite big. > So given that we have no text, no % of progress and just the progress bar, > it looks stuck/buggy until we can finally see some progress. I agree, this can be improved. Followup.

Addressed Alex' comments.

Comment on attachment 8441257 [details] [diff] [review] use simulator version as key Review of attachment 8441257 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/devtools/apps/Simulator.jsm @@ +18,5 @@ > + register: function (label, simulator) { > + // simulators register themselves as "Firefox OS X.Y" > + if (!("label" in simulator)) { > + simulator.label = label; > + } Note that simulator object already has label in simulator.appinfo.label

This patch now uses `appinfo.label`. I also updated the test (xpis was not exposing appinfo). https://tbpl.mozilla.org/?tree=Try&rev=ddf39aa9b68e

New try, without opening the panel (we know it leads to timeouts) https://tbpl.mozilla.org/?tree=Try&rev=d812d573bcf9

remove call to showPanelRuntime