908205 - Allows to install an app via the webapp actor without having to push file with adb

Status: RESOLVED FIXED

(DevTools Graveyard :: WebIDE, defect)

Description

[Alexandre Poirot [:ochameau]]

For now, we need to push the app package to /tmp/b2g/$(app-id)/application.zip
That introduce a hard dependency on adb and requires to find adb and run it on multiple platform. That's not an easy task. We should allow file upload via the remote debugger protocol in order to not depend on adb and eventually offer other communication channels for the remote debugger protocol, like wifi.

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: Allows to install an app via the webapp actor without having to push file with adb

Comment 2

[Alexandre Poirot [:ochameau]]

I'm waiting for bug 908198 before asking for review for this patch as it depends on it, but this patch is ready. I renable one webapps xpcshell test in order to cover this new feature.

Comment 3

[Alexandre Poirot [:ochameau]]

Attachment: Allows to install an app via the webapp actor without having to push file with adb

Now with hosted app support.

Comment 4

[Alexandre Poirot [:ochameau]]

Attachment: Allows to install an app via the webapp actor without having to push file with adb

Comment 5

[Alexandre Poirot [:ochameau]]

Attachment: Moved FileUploadActor to webapps actor and updated test to use OS.File

I'm still using FileUtils, mostly to keep synchronous code when creating empty unique temporary file and automatically mkdir temporary folder.

https://tbpl.mozilla.org/?tree=Try&rev=432644fbedf6

Comment 6

[Alexandre Poirot [:ochameau]]

Panos, Given the previous feedback on bug 908198, I opened the two followups: bug 912475 and bug 912476

Comment 7

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 799462 [details] [diff] [review]
Moved FileUploadActor to webapps actor and updated test to use OS.File

Review of attachment 799462 [details] [diff] [review]:
-----------------------------------------------------------------

This is much simpler now, cool! I have a few comments, but only the prefix/constructor name is a somewhat important one.

::: toolkit/devtools/apps/tests/unit/test_webappsActor.js
@@ +37,5 @@
>  
>    // The install request is asynchronous and send back an event to tell
>    // if the installation succeed or failed
> +  gClient.addListener("webappsEvent", function listener(aState, aType, aPacket) {
> +    gClient.removeListener("webappsEvent", listener);

You could just use addOneTimeListener here and avoid having to remove it manually.

@@ +191,5 @@
> +      .then(function (bytes) {
> +        let content = new TextDecoder("utf-8").decode(bytes);
> +        // Encode the typed array as a string as JSON stringify
> +        // translate them as objects
> +        content = String.fromCharCode.apply(null, bytes);

I'm not familiar with the encoding API, but are you sure that you need to specify UTF-8 (isn't it the default?) and then use charFromCode to get a string? The documentation seems to imply that just decode() should suffice, but I haven't actually tried it.

In any case the comment is confusing and needs some rewording.

@@ +220,5 @@
> +    webappActorRequest(request, function (aResponse) {
> +      do_check_eq(aResponse.appId, gAppId);
> +    });
> +    gClient.addListener("webappsEvent", function listener(aState, aType, aPacket) {
> +      gClient.removeListener("webappsEvent", listener);

You could use addOneTimeListener here too, if you want.

@@ +266,5 @@
> +
> +  // The install request is asynchronous and send back an event to tell
> +  // if the installation succeed or failed
> +  gClient.addListener("webappsEvent", function listener(aState, aType, aPacket) {
> +    gClient.removeListener("webappsEvent", listener);

Same here.

::: toolkit/devtools/server/actors/webapps.js
@@ +27,5 @@
> +  this.size = 0;
> +}
> +
> +UploadActor.prototype = {
> +  actorPrefix: "packageUploadActor",

It's usually a good idea to have the actor prefix match the constructor, otherwise a new actor might appear that would conflict with one but not the other. Registration would fail if constructors conflicted, but not if prefixes did IIRC. In any case, PackageUploadActor sounds nice.

@@ +213,5 @@
>    },
>  
> +  uploadPackage: function () {
> +    debug("uploadPackage\n");
> +    let tmpDir = FileUtils.getDir("TmpD", ["file-upload"], true, false);

I don't believe you need FileUtils for the temporary directory, there is tmpDir in OS.Constants.Path:

https://developer.mozilla.org/docs/JavaScript_OS.File/OS.Path

@@ +430,5 @@
>  
>      if (!appDir || !appDir.exists()) {
> +      if (aRequest.upload) {
> +        // Ensure creating the directory (recursively)
> +        appDir = FileUtils.getDir("TmpD", ["b2g", appId], true, false);

Same here.

@@ +440,5 @@
> +        }
> +        let appFile = FileUtils.File(actor.getFilePath());
> +        if (!appFile.exists()) {
> +          return { error: "badParameter",
> +                   message: "The uploaded file doesn't exists on device" };

Typo: exist.

Comment 8

[Alexandre Poirot [:ochameau]]

Attachment: Address last comments

Comment 9

[Alexandre Poirot [:ochameau]]

Comment on attachment 800081 [details] [diff] [review]
Address last comments

Review of attachment 800081 [details] [diff] [review]:
-----------------------------------------------------------------

https://tbpl.mozilla.org/?tree=Try&rev=783b6b144ed6

Comment 10

[Alexandre Poirot [:ochameau]]

Wrong try build... this one is going to run xpcshell tests:
  https://tbpl.mozilla.org/?tree=Try&rev=b4f4ae787b1b
(With this patch, we are going to re-enable webapp actor xpcshell test!)

Comment 11

[Alexandre Poirot [:ochameau]]

Attachment: Fix xpcshell failure because of strict exceptions in test_add_actor

https://tbpl.mozilla.org/?tree=Try&rev=067be4bbcdf8

Comment 12

[Alexandre Poirot [:ochameau]]

Comment on attachment 800131 [details] [diff] [review]
Fix xpcshell failure because of strict exceptions in test_add_actor

Review of attachment 800131 [details] [diff] [review]:
-----------------------------------------------------------------

Carrying r+ after some fixes in xpcshell tests.

Comment 13

[Paul Rouget [:paul]]

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

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/45ad3919056e