911785 - Allow installing apps local apps from the app manager UI

Status: RESOLVED FIXED

(DevTools Graveyard :: WebIDE, defect)

Description

[Alexandre Poirot [:ochameau]]

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.

Comment 2

[Alexandre Poirot [:ochameau]]

Attachment: Plug packaged app install

Still need to implement hosted app install.

Comment 4

[Paul Rouget [:paul]]

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.

Comment 5

[Alexandre Poirot [:ochameau]]

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?

Comment 6

[Paul Rouget [:paul]]

ok.

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

Comment 7

[Alexandre Poirot [:ochameau]]

(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 8

[Alexandre Poirot [:ochameau]]

Attachment: Hosted and packaged app install

Comment 9

[Alexandre Poirot [:ochameau]]

Attachment: Hosted and packaged install

Comment 10

[Alexandre Poirot [:ochameau]]

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.

Comment 11

[Alexandre Poirot [:ochameau]]

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

Comment 12

[Paul Rouget [:paul]]

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

Comment 13

[Alexandre Poirot [:ochameau]]

Attachment: Implement a webapps actor front to ease app install r=paul

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.

Comment 14

[Alexandre Poirot [:ochameau]]

Attachment: Allow installing local apps from the app manager UI r=paul

Comment 15

[Alexandre Poirot [:ochameau]]

(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 16

[Alexandre Poirot [:ochameau]]

Attachment: UI patch, fix button label localization

Comment 17

[Paul Rouget [:paul]]

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 18

[Paul Rouget [:paul]]

Comment on attachment 802154 [details] [diff] [review]
UI patch, fix button label localization

Didn't see the first patch :)

Comment 19

[Alexandre Poirot [:ochameau]]

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 20

[Paul Rouget [:paul]]

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

Comment 21

[Paul Rouget [:paul]]

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

Comment 22

[Alexandre Poirot [:ochameau]]

Attachment: Change require path and renamed module to app-actor-front

Comment 23

[Alexandre Poirot [:ochameau]]

Attachment: UI patch - update actor front require path

Comment 24

[Alexandre Poirot [:ochameau]]

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 25

[Alexandre Poirot [:ochameau]]

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 26

[Paul Rouget [:paul]]

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.

Comment 27

[Paul Rouget [:paul]]

https://hg.mozilla.org/integration/fx-team/rev/24270cebb090

Comment 28

[Paul Rouget [:paul]]

Wrong changeset in comment 27.

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

Comment 29

[Ryan VanderMeulen [:RyanVM]]

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