Closed Bug 881499 Opened 7 years ago Closed 7 years ago

Modify webapps actor to receive a receipt

Categories

(DevTools :: General, defect)

x86
macOS
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 24

People

(Reporter: nick, Assigned: nick)

Details

Attachments

(1 file, 3 obsolete files)

Attached patch patch v1 (obsolete) — Splinter Review
This is would be helpful for the FxOS Simulator when pushing an app to device to also push the receipt, to test paid apps.

https://github.com/mozilla/r2d2b2g/issues/496
Attachment #760609 - Flags: review?(fabrice)
Assignee: dclarke → nobody
Component: Infrastructure → Developer Tools
Product: Web Apps → Firefox
Version: unspecified → Trunk
Assignee: nobody → ndesaulniers
Component: Developer Tools → Infrastructure
Product: Firefox → Web Apps
Version: Trunk → unspecified
Attachment #760609 - Attachment is patch: true
Component: Infrastructure → Developer Tools
Product: Web Apps → Firefox
Version: unspecified → Trunk
Comment on attachment 760609 [details] [diff] [review]
patch v1

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

Also, can you create your patches with 8 lines of context (see https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F)? thanks!

::: toolkit/devtools/server/actors/webapps.js
@@ +107,4 @@
>      return type;
>    },
>  
> +  installHostedApp: function wa_actorInstallHosted({ aDir, aId, aReceipts }) {

There's no reason to change the signature style here.

@@ +176,4 @@
>                                         Ci.nsIThread.DISPATCH_NORMAL);
>    },
>  
> +  installPackagedApp: function wa_actorInstallPackaged({ aDir, aId, aReceipts }) {

idem.
Attachment #760609 - Flags: review?(fabrice) → review-
Attached patch patch v2 (obsolete) — Splinter Review
Attachment #760609 - Attachment is obsolete: true
Attachment #760649 - Flags: review?(fabrice)
Comment on attachment 760649 [details] [diff] [review]
patch v2

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

::: toolkit/devtools/server/actors/webapps.js
@@ +270,5 @@
>  
>      let testFile = appDir.clone();
>      testFile.append("application.zip");
>  
> +    let receipts = aRequest.receipt ? [aRequest.receipt] : null;

That will go bad if aRequest.receipt is actually an array.
Attachment #760649 - Flags: review?(fabrice) → review-
Attached patch patch v3 (obsolete) — Splinter Review
Attachment #760649 - Attachment is obsolete: true
Attachment #760996 - Flags: review?(fabrice)
Comment on attachment 760996 [details] [diff] [review]
patch v3

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

::: toolkit/devtools/server/actors/webapps.js
@@ +270,5 @@
>  
>      let testFile = appDir.clone();
>      testFile.append("application.zip");
>  
> +    let receipts = typeof aRequest.receipt === "string" ? [aRequest.receipt] : null;

We do it differently in the DOM API: https://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.js#134

I'd like both to stay in sync as much as possible, so let me know if you have a good reason to not follow the DOM api pattern.
Attachment #760996 - Flags: review?(fabrice)
The DOM api expects an array of strings, the simulator only sends a single string or null.
(In reply to Nick Desaulniers [:\n] from comment #6)
> The DOM api expects an array of strings, the simulator only sends a single
> string or null.

That sounds like a bug in the simulator. I don't see any reason why we shouldn't support multiple receipts.
The point is not was the simulator does, it's what the public sideloading API should accept. I would argue that it has to support an API as close as possible to the DOM API.
Attached patch patch v4Splinter Review
Attachment #760996 - Attachment is obsolete: true
Attachment #761629 - Flags: review?(fabrice)
Attachment #761629 - Flags: review?(fabrice) → review+
https://hg.mozilla.org/mozilla-central/rev/a0e3d36a4acb
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 24
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.