Thanks to bug 908205, we can implement app install without having to use adb. There is still a substantial work in order to build the app zip, but most of this work can just be uplifted from the simulator codebase.

Still need to implement hosted app install.

Buttons we need: If not installed: refresh + install If installed: refresh + uninstall If running: refresh + stop + restart + debug If not running: refresh + start + debug install should be greyed-out if there are errors.

I tend to think we should end up with the two buttons we ended up with on the simulator: update: same thing than what refresh already does, but also install an updated version to simulation connect: install if not installed, launch if not running, open a toolbox These two buttons are always displayed. Would that work for you? Any input from Darrin on this subject?

ok. What if the user wants to test his app but not start the devtools? I think we should also have start/stop.

(In reply to Paul Rouget [:paul] from comment #6) > ok. > > What if the user wants to test his app but not start the devtools? > I think we should also have start/stop. The update/refresh button launch a new validation check and eventually push to the device if there is no error. So far, in the simulator addon, we weren't starting the app, but may be we should.

Comment on attachment 801576 [details] [diff] [review] Hosted and packaged install Note that all code related to zip package creation is taken from simulator codebase.

Here I just uncomented install and debug buttons, but we should iterate in another bug to simplify the buttons story.

Comment on attachment 801576 [details] [diff] [review] Hosted and packaged install Review of attachment 801576 [details] [diff] [review]: ----------------------------------------------------------------- Can we move the changes in projects.js to a module in /browser/devtools/app-manager/ (not in /browser/devtools/app-manager/content/)? Or maybe even better, in /toolkit/devtools/apps/ ? ::: browser/devtools/app-manager/app-projects.js @@ +8,1 @@ > Nit: be consistent with the previous important (no space). We also don't really care about the 80 column limit here. ::: browser/devtools/app-manager/content/projects.js @@ +19,5 @@ > > +const PR_RDWR = 0x04; > +const PR_CREATE_FILE = 0x08; > +const PR_TRUNCATE = 0x20; > + Can these values be imported from and .IDL? @@ +169,5 @@ > }, > > + install: function(button, location) { > + button.textContent = "Installing..."; > + button.disabled = true; You should create a different button just for this: <button disabled="disabled" class="device-action project-button-installing">&projects.installingApp;</button> @@ +182,5 @@ > + button.disabled = false; > + button.textContent = "Installed!"; > + setTimeout(function() { > + button.textContent = "INSTALL"; > + }, 1500); Why this setTimeout? @@ +187,5 @@ > + }, > + function (res) { > + button.disabled = false; > + alert(res.error + ": " + res.message); > + }); You can keep the alert, but please also use: this.connection.log(res.error + ": " + res.message); @@ +260,5 @@ > + > + _uploadPackage: function (packageFile) { > + let deferred = promise.defer(); > + const CHUNK_SIZE = 10000; > + Move that up in the file. @@ +271,5 @@ > + openFile(client, res.actor); > + }); > + } > + newUpload(this.connection.client, this.listTabsResponse.webappsActor); > + Why do you create a function here? @@ +342,5 @@ > + _installPackaged: function(project) { > + let deferred = promise.defer(); > + let file = FileUtils.File(project.location); > + let tmpZipFile = FileUtils.getDir("TmpD", [], true); > + tmpZipFile.append("application-" + (new Date().getTime()) + ".zip"); What about using createUnique()? https://developer.mozilla.org/en-US/docs/Code_snippets/File_I_O#Creating_temporary_files

So, I moved most of the code to toolkit, in a "front". Not a protocol.js one, as it would require refactoring the whole actor, but just a module with various helper to ease app install. I factorized the code with the existing xpcshell test that was having duplicated code.

(In reply to Paul Rouget [:paul] from comment #12) > Comment on attachment 801576 [details] [diff] [review] > ::: browser/devtools/app-manager/content/projects.js > @@ +19,5 @@ > > > > +const PR_RDWR = 0x04; > > +const PR_CREATE_FILE = 0x08; > > +const PR_TRUNCATE = 0x20; > > + > > Can these values be imported from and .IDL? Unfortunately, no. That's some necko stuff... > > @@ +169,5 @@ > > }, > > > > + install: function(button, location) { > > + button.textContent = "Installing..."; > > + button.disabled = true; > > You should create a different button just for this: > > <button disabled="disabled" class="device-action > project-button-installing">&projects.installingApp;</button> I haven't changed this, as I'm not convinced it is any better. I can easily imagine many improvement but adding a button always disabled just for showing a state sounds weird. Or may be I don't understand the usage of this button? > > @@ +182,5 @@ > > + button.disabled = false; > > + button.textContent = "Installed!"; > > + setTimeout(function() { > > + button.textContent = "INSTALL"; > > + }, 1500); > > Why this setTimeout? To ensure seeing the "Installed" state for at leat 1sec, and get back to the original button label, which is "INSTALL".

Comment on attachment 802154 [details] [diff] [review] UI patch, fix button label localization Did I miss something or you forgot to add actor-front.js?

Comment on attachment 802154 [details] [diff] [review] UI patch, fix button label localization Didn't see the first patch :)

Comment on attachment 802154 [details] [diff] [review] UI patch, fix button label localization There is two patches attached to this bug, see attachment 801832 [details] [diff] [review]

Comment on attachment 801832 [details] [diff] [review] Implement a webapps actor front to ease app install r=paul Review of attachment 801832 [details] [diff] [review]: ----------------------------------------------------------------- r=me if you use moz.build and not Makefile.in (base your patch on top of bug 914110).

Also, instead of using 'devtools/toolkit/apps/actor-front', can you re-use an existing directory? Eventually, this will move to 'devtools/toolkit/server/actors/xxx', so its current location is temporary. So let's not create an new path for that. Maybe you should rename the file 'app-packaging-actor-front.js' and install it in 'modules/devtools'.

Paul, I renamed it from devtools/toolkit/apps/actor-front to devtools/app-actor-front and copy it to gre/modules/devtools/, but unfortunately, there is no loader rule matching this folder, so I still had to modify Loader.jsm ...

Comment on attachment 802564 [details] [diff] [review] UI patch - update actor front require path Carrying the r+ over this one as I only change the require path for the front actor module.

Comment on attachment 802563 [details] [diff] [review] Change require path and renamed module to app-actor-front r=me but I really dislike the fact the you need to update Loader.jsm. But I don't think there's any better way to do for the moment. Let's make sure that we get rid of that in bug 912476.

Wrong changeset in comment 27. https://hg.mozilla.org/integration/fx-team/rev/f4f0334971ae

https://hg.mozilla.org/mozilla-central/rev/f4f0334971ae https://hg.mozilla.org/mozilla-central/rev/24270cebb090