Add more functions to the webapps actor

RESOLVED FIXED

Status

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: cjones, Assigned: ochameau)

Tracking

(Depends on 1 bug)

unspecified
x86_64
Linux
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 7 obsolete attachments)

One use case goes something like, "I want to launch and kill app X 100 times to test some perf issue."

Another use case is, "I'm going 'shift-reload' development and want to redeploy and launch my app Y on each 'reload' cycle."

Android provides an |am| helper program to do this, so you can do something like
 $ am launch Foo
(oversimplifying nasty java goo).

We can easily do this with marionette, but we don't have any helpers for that that are "convenient" enough to satisfy the cases above.
We could add a make target for this in Gaia:

make launch Foo

is that about what you had in mind?  

and/or a simple shell script; either would just front-end Marionette.

Of course this makes this feature dependent on a Marionette-enabled build; I'm not sure if that's a bad thing or not.
It would be nice if this was decoupled from the gaia build system.

It would also be nice if this was decoupled from remote debugging, but in practice that's the only way to enable adb in production, so that's less of a goal.
We could add some helper scripts in B2G/scripts, perhaps.
I think we have a couple of options:
- add a launch() command to the debugger actor we use to sideload apps. I think we need that anyway if we want to provide nice devtools.
- use a command line handler. We already have one in b2g/chrome/content/runapp.js but it's disabled on device (also, it's not a real CLH so we probably need to rewrite that).
runapp.js doesn't even work on b2g desktop builds right now (see bug 840268), but that probably wouldn't be hard for someone to fix.
(In reply to Fabrice Desré [:fabrice] from comment #4)
> I think we have a couple of options:
> - add a launch() command to the debugger actor we use to sideload apps. I
> think we need that anyway if we want to provide nice devtools.

Sounds like the right mechanism.

You're right that we need the install helper tools for a sane developer flow, so folding that into those sgtm.
Assignee: nobody → fabrice
Posted patch wip patch (obsolete) — Splinter Review
With this patch we can launch an app, knowing its manifest URL and start point if any. I'd like to add a close() call also.
Alex, I think that what I started there is relevant to your needs for the devtools. Feel free to steal!
Assignee

Comment 9

6 years ago
Yes, I'm interested in getting your patch landed!
I can spend some time on this next week.
But it's not really clear what is missing here...
It looks like the missing piece would be the app manager devtool?
Or do we suggest using b2gremote addon in the meantime?
If that's the plan, I can provide a pull request to get it working with this new command.
In this bug, I'd like to add not only a launch() command, but also close(), uninstall() and getAll() so that we can build a first usable management tool.

Building the host side devtool itself belongs to another bug. I would rather not add more to the b2gremote addon and work toward getting what we need in the platform.
Summary: Add a way to launch an app (create activity) from the command line → Add more functions to the webapps actor
Assignee

Updated

6 years ago
Assignee: fabrice → poirot.alex
Assignee

Comment 11

6 years ago
What do you think about always using mozApps API instead of faking Webapps.js calls or using any Webapps.jsm internal?
It would be less effective as it will involve the IPC communication stuff, but also way more stable against any cleanup being done in webapps.jsm.
And eventually, if we clean it up, we may be able to call a stable API that isn't deeply mixed with DOMRequest/message manager stuffs.
Assignee

Updated

6 years ago
Blocks: 865340
Assignee

Updated

6 years ago
No longer blocks: 865340
Depends on: 830376, 865340
Assignee

Updated

6 years ago
Attachment #717429 - Attachment is obsolete: true
Assignee

Updated

6 years ago
Attachment #741404 - Attachment is obsolete: true
Assignee

Comment 15

6 years ago
Comment on attachment 743634 [details] [diff] [review]
Add more functions to the webapps actor

New patch that drop the dependency to toplevel xul window (used to get a mozApps object). Removing it allows to write xpcshell tests against the actor and also be more efficient.
I had to tweak Webapps.jsm in order to factor out IPC code in getAll, launch and uninstall. So that we can use these methods from the actor without having to add mock or IPC-specific fields before calling these functions.

This patch comes with an xpcshell test that do sanity check over all actor requests. I already have a followup patch for bug 865207 that move the actor to devtools. The xpcshell test already pass on Desktop.

https://tbpl.mozilla.org/?tree=Try&rev=e84efda154cb
Assignee

Updated

6 years ago
Attachment #743634 - Attachment is obsolete: true
Assignee

Comment 17

6 years ago
Comment on attachment 744592 [details] [diff] [review]
Add more functions to the webapps actor

New patch where tests actually pass!
  https://tbpl.mozilla.org/?tree=Try&rev=fa7753488203

See comment 15 where I describe this patch.
Note that this work depends on gaia modification already r+ in bug 865340.
Attachment #744592 - Flags: review?(fabrice)
Comment on attachment 744592 [details] [diff] [review]
Add more functions to the webapps actor

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

r- for using the origin instead of the manifest URL in webapps-close. All other comments are mostly nits.

::: b2g/chrome/content/shell.js
@@ +929,5 @@
>          break;
> +      case "webapps-close":
> +        shell.sendChromeEvent({
> +          "type": "webapps-close",
> +          "origin": json.origin

you really want the manifest URL here, not the origin.

::: dom/apps/src/Webapps.jsm
@@ +814,5 @@
> +            // Fall-through, fails to uninstall the desired app because:
> +            //   - we cannot find the app to be uninstalled.
> +            //   - the app to be uninstalled is not removable.
> +            mm.sendAsyncMessage("Webapps:Uninstall:Return:KO", msg);
> +          });

I would prefer to not have that code here in the switch(). Can you move it to a doUninstall(). Same for launch() and getAll().

::: dom/apps/tests/unit/test_webappsActor.js
@@ +47,5 @@
> +
> +// Install a test app
> +add_test(function testInstallPackaged() {
> +  // Copy our test webapp to tmp folder, where the actor retrieves it
> +  let zip = do_get_file("data/app.zip");

did you forget to add the zip to this patch?
Assignee

Updated

6 years ago
Attachment #744592 - Attachment is obsolete: true
Attachment #744592 - Flags: review?(fabrice)
Assignee

Comment 20

6 years ago
I addressed your comments in this patch.
app.zip was already included in the patch?
Otherwise I'm bit lost on gaia side, as most WindowManager methods use origin.
I'll try to point that out during the review of the gaia patch.

I tested it manually on device and submitted a new try run:
  https://tbpl.mozilla.org/?tree=Try&rev=2894e5c7cffb
Assignee

Updated

6 years ago
Attachment #744789 - Flags: review?(fabrice)
Comment on attachment 744789 [details] [diff] [review]
Add more functions to the webapps actor

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

::: b2g/chrome/content/dbg-webapps-actors.js
@@ +56,5 @@
>      reg._readManifests([{ id: aId }], function(aResult) {
>        let manifest = aResult[0].manifest;
>        aApp.name = manifest.name;
> +      if ("_registerSystemMessages" in reg)
> +        reg._registerSystemMessages(manifest, aApp);

nit: { } even for single line if's

@@ +58,5 @@
>        aApp.name = manifest.name;
> +      if ("_registerSystemMessages" in reg)
> +        reg._registerSystemMessages(manifest, aApp);
> +      if ("_registerActivities" in reg)
> +        reg._registerActivities(manifest, aApp, true);

idem.
Attachment #744789 - Flags: review?(fabrice) → review+
Assignee

Updated

6 years ago
Attachment #744789 - Attachment is obsolete: true
Assignee

Comment 23

6 years ago
Comment on attachment 745835 [details] [diff] [review]
Add more functions to the webapps actor

Nits fixed.
Attachment #745835 - Flags: review+
Assignee

Updated

6 years ago
Keywords: checkin-needed
Doesn't apply cleanly to birch. Please rebase.
Keywords: checkin-needed
Assignee

Comment 26

6 years ago
Comment on attachment 745842 [details] [diff] [review]
Add more functions to the webapps actor

Rebased.
Attachment #745842 - Flags: review+
Assignee

Comment 27

6 years ago
Thanks Ryan for your responsiveness :)
Keywords: checkin-needed
Attachment #745835 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/c50f597b1e6a
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Assignee

Comment 31

6 years ago
Comment on attachment 771323 [details] [diff] [review]
b2g18 - Add more functions to the webapps actor

Patch rebased for b2g18.
Attachment #771323 - Attachment description: Add more functions to the webapps actor → b2g18 - Add more functions to the webapps actor
Assignee

Updated

6 years ago
Attachment #771323 - Attachment is obsolete: true
Assignee

Updated

6 years ago
Attachment #771345 - Attachment description: Add more functions to the webapps actor → b2g18 - Add more functions to the webapps actor
Depends on: 912164
You need to log in before you can comment on or make changes to this bug.