772600 - Create WebappOSUtils.jsm to host platform-specific webapps code and implement native app launch

Status: VERIFIED FIXED

(Firefox Graveyard :: Web Apps, defect, P1)

Description

[:Felipe Gomes (needinfo for replies!)]

We want to host isLaunchable, launch(), the naming scheme, and other future functions to be easily accessible by browser, the webapp runtime, and the dom code

Comment 1

[:Felipe Gomes (needinfo for replies!)]

Attachment: Patch

This is the file, let's fill it in

Comment 2

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Patch

Added Windows impl.  This is basically a copy+paste of the patch in bug 763786.

Comment 3

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 640729 [details] [diff] [review]
Patch

Asking for review on the file structure and placement.

Gavin, we need to create this file to host platform-specific code from webapps that need to be called from the dom code. The idea is to have the ifdefs implemented here for each function so that everything is easily reachable in a single place.

Also I'm creating this folder, toolkit/webapps/, to host other similar cases like this. For example we'll need to move WebappsInstaller.jsm out of browser when bsmedberg change the way browser packaging works (and thus make the files under browser/modules non-acessible by the webapp runtime), so I'm wondering if toolkit/webapps/ is the right place to put these files.

Comment 4

[Dan Walkowski]

Attachment: very provisional patch

this is a provisional, untested patch that incorporates changes for all three OS's.  I am hoping for feedback only at this time.

Comment 5

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 640860 [details] [diff] [review]
very provisional patch

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

This looks good. I'll review more carefully soon, but my observations:

- The notification shouldn't be used as an error case, it should be just the #else case for platforms which do not have a native implementation. And each implementation returns true or false indicating success.

- The function definition in the idl needs a comment.

- The test for the macWebAppUtils's new function can't be left as is, because it will leave TextEdit open. We need to find a way (fine to be hacky) to check if it was actually opened (so the test has some value) and then close it, or leave that test out.

Comment 6

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 640729 [details] [diff] [review]
Patch

(please leave this patch as non-obselete so Gavin can review the file creation for me)

Comment 7

[Dan Walkowski]

Attachment: completed patch. needs testing on Windows and Linux.

This includes the launching code for all three platforms; the patches from 763740, 763786, and 763789.  The Mac patch works correctly as before, and since the Windows and Linux code was added in, they should get retested.

Comment 8

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 640729 [details] [diff] [review]
Patch

toolkit/webapps for this code (and the other code you mention moving here) seems fine. I guess toolkit/ is the new toolkit/components :)

Comment 9

[Jason Smith [:jsmith]]

Re-triage flag. Technically this is required for app launching, which is a P1, not a P2.

Comment 10

[:Felipe Gomes (needinfo for replies!)]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #8)
> I guess toolkit/ is the new toolkit/components :)

I used toolkit/ because nothing else in toolkit/components/ was a .jsm, they were all proper components. Perhaps this should be the first one?

I created this as a .jsm instead of a component because they are so much nicer and easier to write and use.

Comment 11

[:Gavin Sharp [email: [e-mail]]]

Lots of people seem to assume that the "components" in {browser|toolkit}/components refers to XPCOM components, but I've never really seen it that way - I just see them as "components" in the more general sense (agglomerations of related code). e.g. toolkit/components/aboutmemory and toolkit/components/viewconfig aren't XPCOM components. But it doesn't really matter much either way, I think putting it directly under toolkit is fine.

Comment 12

[:Felipe Gomes (needinfo for replies!)]

Thanks for the patches, all. I landed it with minor style changes, and added return true/false in the success/error cases. I don't think I needed a new review on that.

https://hg.mozilla.org/integration/mozilla-inbound/rev/58d5f1d1443e

Comment 13

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/20a09c21d3d5
https://hg.mozilla.org/integration/mozilla-inbound/rev/6bfce37d7fa4

Comment 14

[:Felipe Gomes (needinfo for replies!)]

http://hg.mozilla.org/mozilla-central/rev/4cffe2b37d0c

Comment 15

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

This code breaks B2G since it is considered as a XP_UNIX in this case. I don't understand why the notifyObserver code has not been left as if. Firefox will have been able to listen for this message and use the .jsm file.

Comment 16

[Etienne Segonzac (:etienne)]

See https://bugzilla.mozilla.org/show_bug.cgi?id=774216

Comment 21

[Jason Smith [:jsmith]]

Moving the sec flags over to this bug from bug 745924, so that we don't have 5 bugs lieing around for the main core bug (that's here).

Comment 22

[Jason Smith [:jsmith]]

Initial tests show this is working quite well on windows 7. I'll draft a summarized test plan for this and run each test to make sure everything is okay.

Comment 23

[Jason Smith [:jsmith]]

Proposal for testing this is here - https://etherpad.mozilla.org/native-app-launch-testing. Feedback or ideas would be greatly appreciated!

Comment 24

[Jason Smith [:jsmith]]

Verified across multiple operating systems. No major issues detected. Any minor issues picked up have been filed in bugs accordingly.

See https://docs.google.com/spreadsheet/ccc?key=0AiZoGR-iOAlUdFRNcmhYNFRJcTdwVHk4NG1CV25mWkE#gid=0 for the test results.

Comment 25

[Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]]

changing the sec-review flag to :mgoodwin as he owns the sec-review bug

Comment 26

[Mark Goodwin [:mgoodwin]]

Secreview is being handled in bug 935713. Clearing secreview flag (and dependancy on secreview bug) as the code has changed significantly in the time since this landed. See the secreview bug for any further info.