Closed Bug 908205 Opened 7 years ago Closed 7 years ago

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

Categories

(DevTools Graveyard :: WebIDE, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 26

People

(Reporter: ochameau, Assigned: ochameau)

References

Details

Attachments

(1 file, 5 obsolete files)

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.
Depends on: 908198
Assignee: nobody → poirot.alex
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.
Blocks: 911785
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
Attachment #794039 - Attachment is obsolete: true
Attachment #796558 - Attachment is obsolete: true
Attachment #796607 - Attachment is obsolete: true
Attachment #799462 - Flags: review?(past)
Panos, Given the previous feedback on bug 908198, I opened the two followups: bug 912475 and bug 912476
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.
Attachment #799462 - Flags: review?(past) → review+
Blocks: appmgr_v1
Attached patch Address last comments (obsolete) — Splinter Review
Attachment #799462 - Attachment is obsolete: true
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
Attachment #800081 - Flags: review+
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 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.
Attachment #800131 - Flags: review+
Keywords: checkin-needed
Whiteboard: [land-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/45ad3919056e
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
Product: Firefox → DevTools
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.