Closed Bug 911785 Opened 6 years ago Closed 6 years ago

Allow installing apps local apps from the app manager UI

Categories

(DevTools :: WebIDE, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 26

People

(Reporter: ochameau, Assigned: ochameau)

References

Details

Attachments

(2 files, 6 obsolete files)

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.
Depends on: 908205
Blocks: appmgr_v1
Duplicate of this bug: 910194
Attached patch Plug packaged app install (obsolete) — Splinter Review
Still need to implement hosted app install.
Duplicate of this bug: 912911
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.
Attached patch Hosted and packaged app install (obsolete) — Splinter Review
Attachment #800270 - Attachment is obsolete: true
Attached patch Hosted and packaged install (obsolete) — Splinter Review
Attachment #801565 - Attachment is obsolete: true
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.
Attachment #801576 - Flags: review?(paul)
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
Attachment #801576 - Flags: review?(paul) → review-
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.
Attachment #801576 - Attachment is obsolete: true
(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".
Attachment #801832 - Flags: review?(paul)
Attachment #801833 - Flags: review?(paul)
Attachment #801833 - Attachment is obsolete: true
Attachment #801833 - Flags: review?(paul)
Attachment #802154 - Flags: review?(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?
Attachment #802154 - Flags: review?(paul) → review-
Comment on attachment 802154 [details] [diff] [review]
UI patch, fix button label localization

Didn't see the first patch :)
Attachment #802154 - Flags: review-
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]
Attachment #802154 - Flags: review?(paul)
Attachment #802154 - Flags: review?(paul) → 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).
Attachment #801832 - Flags: review?(paul) → review+
Depends on: 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'.
Attachment #802154 - Attachment is obsolete: true
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 ...
Attachment #802563 - Flags: review?(paul)
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.
Attachment #802564 - Flags: review+
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.
Attachment #802563 - Flags: review?(paul) → review+
Whiteboard: [land-in-fx-team]
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/f4f0334971ae
https://hg.mozilla.org/mozilla-central/rev/24270cebb090
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 26
Depends on: 915556
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.