Add a remote debugger actor to install apps

RESOLVED FIXED in Firefox 21

Status

Firefox OS
General
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: fabrice, Assigned: fabrice)

Tracking

(Blocks: 2 bugs, {dev-doc-needed})

unspecified
B2G C4 (2jan on)
All
Gonk (Firefox OS)
dev-doc-needed
Dependency tree / graph

Firefox Tracking Flags

(blocking-basecamp:+, firefox19 wontfix, firefox20 wontfix, firefox21 fixed, b2g18 fixed)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
The goal is to let developers install applications on their phones with privileged status without to use a signing service.
(Assignee)

Comment 1

5 years ago
Created attachment 700246 [details] [diff] [review]
patch v1

Panagiotis, can you review the remote debugging part? I also have a xpcshell script to test it if you want to try it.
Assignee: nobody → fabrice
Attachment #700246 - Flags: review?(past)
(Assignee)

Updated

5 years ago
blocking-basecamp: --- → ?
blocking-basecamp: ? → +
Comment on attachment 700246 [details] [diff] [review]
patch v1

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

I have a few comments about making the packet form fit well with the rest of the protocol in some cases, but it looks great overall.

As an aside: have you considered adding this actor to desktop Firefox as well? I'm not sure if it will be of much benefit to a developer, but we could add tests to exercise it and consider supporting it in Firefox Developer Tools.

::: b2g/chrome/content/dbg-webapps-actors.js
@@ +55,5 @@
> +                         type: "webappsEvent",
> +                         event: "InstallDone",
> +                         subject: null,
> +                         data: { status: "ok" }
> +                       });

I see you mostly copied the eventNotification structure verbatim, which essentially mirrors the observer notifications, but you could simplify a bit. I'd go for:
{ from: self.actorID,
  type: "webappsEvent",
  appId: aId }

appId seems useful, otherwise there is no indication which installation this event corresponds to. Note that in the protocol in general we don't return a success status, since the lack of an "error" property denotes success.

@@ +72,5 @@
> +        subject: null,
> +        data: { status: "error",
> +                message: aMsg
> +              }
> +      });

In this case I'd just return:
{ from: this.actorID,
  type: "webappsEvent",
  appId, aId,
  error: "installationFailed",
  message: aMsg }

and make sure that _sendError is called with human-readable descriptive text. aId would have to be provided as a parameter of course.

@@ +139,5 @@
> +
> +          // Move application.zip to the destination directory.
> +          let zipFile = aDir.clone();
> +          zipFile.append("application.zip");
> +          zipFile.moveTo(appDir, "application.zip");

Nit: aDir and appDir were slightly confusing to read. If you care, perhaps consider renaming one to installDir or somesuch.

@@ +178,5 @@
> +
> +  // @param appId : The id of the app we want to install. We will look for the
> +  //                files for the app in $TMP/b2g/$appId
> +  //                For packaged apps: application.zip
> +  //                For hosted apps:   metadata.json and manifest.webapp

A multi-line comment (/* */) would be more consistent, but I won't be picky about it. It would also be cool if the comment described all expected request parameters.

@@ +186,5 @@
> +
> +    let appId = aRequest.appId;
> +    if (!appId) {
> +      return { status: "error",
> +               message: "MISSING_PARAMETER_APPID"}

The standard protocol response in this case is:
{ error: "missingParameter",
  message: "missing parameter appId" }

See: https://wiki.mozilla.org/Remote_Debugging_Protocol#Requests_and_Replies
Feel free to make the message as explanatory or verbose as you want.

@@ +196,5 @@
> +    if (!appDir || !appDir.exists()) {
> +      return { status: "error",
> +               message: "MISSING_DIRECTORY",
> +               detail: appDir.path
> +             }

Same as above, this should return missingParameter if no appDir is specified and badParameterType if the directory is invalid in some way:
{ error: "badParameterType",
  message: "missing directory " + appDir.path }

@@ +217,5 @@
> +      if (missing) {
> +        try {
> +          aDir.remove(true);
> +        } catch(e) {}
> +        return { status: "error", message: "MISSING_HOSTED_APP_FILE" }

Same here:
{ error: "badParameterType",
  message: missing + " hosted app file is missing" }

@@ +223,5 @@
> +
> +      this.installHostedApp(appDir, appId, appType);
> +    }
> +
> +    return { status: "ok", appId: appId, path: appDir.path }

Protocol responses in general are considered successful if they don't contain an error property. In this case you'd just return:
{ appId: appId,
  path: appDir.path }
Attachment #700246 - Flags: review?(past) → review+
Comment on attachment 700246 [details] [diff] [review]
patch v1

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

::: b2g/chrome/content/dbg-webapps-actors.js
@@ +118,5 @@
> +            self._registerApp(app, aId, aDir);
> +          });
> +        } catch(e) {
> +          // If anything goes wrong, just send it back.
> +          self.sendError(e.toString());

Don't know why I didn't notice this before, but this should have been self._sendError... (notice the underscore).

@@ +166,5 @@
> +
> +          self._registerApp(app, aId, aDir);
> +        } catch(e) {
> +          // If anything goes wrong, just send it back.
> +          self.sendError(e.toString());

Ditto.
(Assignee)

Comment 5

5 years ago
followup for the .sendError I didn't caught myself

https://hg.mozilla.org/integration/mozilla-inbound/rev/bd7d549c42ad
Blocks: 815853

Updated

5 years ago
No longer blocks: 815853
status-b2g18: --- → fixed
status-firefox19: --- → wontfix
status-firefox20: --- → wontfix

Updated

5 years ago
Blocks: 815853

Updated

5 years ago
Blocks: 783741
Keywords: dev-doc-needed, verifyme
QA Contact: jsmith
status-firefox21: --- → fixed
Target Milestone: --- → B2G C4 (2jan on)

Updated

5 years ago
Keywords: verifyme

Updated

5 years ago
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.