Last Comment Bug 772600 - Create WebappOSUtils.jsm to host platform-specific webapps code and implement native app launch
: Create WebappOSUtils.jsm to host platform-specific webapps code and implement...
Status: VERIFIED FIXED
[qa!], [sec-assigned:curtisk]
:
Product: Firefox Graveyard
Classification: Graveyard
Component: Web Apps (show other bugs)
: Trunk
: All All
: P1 normal
: Firefox 16
Assigned To: Dan Walkowski
: Jason Smith [:jsmith]
:
Mentors:
: 745924 763740 763786 763789 (view as bug list)
Depends on: 773040 774216
Blocks: 710062
  Show dependency treegraph
 
Reported: 2012-07-10 12:28 PDT by :Felipe Gomes (needinfo me!)
Modified: 2016-02-04 15:00 PST (History)
13 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch (1.64 KB, patch)
2012-07-10 12:49 PDT, :Felipe Gomes (needinfo me!)
gavin.sharp: feedback+
Details | Diff | Splinter Review
Patch (2.95 KB, patch)
2012-07-10 13:13 PDT, Tim Abraldes [:TimAbraldes] [:tabraldes]
no flags Details | Diff | Splinter Review
very provisional patch (6.86 KB, patch)
2012-07-10 16:56 PDT, Dan Walkowski
felipc: feedback+
Details | Diff | Splinter Review
completed patch. needs testing on Windows and Linux. (6.79 KB, patch)
2012-07-11 16:28 PDT, Dan Walkowski
felipc: review+
timabraldes: feedback+
mcastelluccio: feedback+
Details | Diff | Splinter Review

Description :Felipe Gomes (needinfo me!) 2012-07-10 12:28:53 PDT
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 me!) 2012-07-10 12:49:08 PDT
Created attachment 640729 [details] [diff] [review]
Patch

This is the file, let's fill it in
Comment 2 Tim Abraldes [:TimAbraldes] [:tabraldes] 2012-07-10 13:13:44 PDT
Created attachment 640735 [details] [diff] [review]
Patch

Added Windows impl.  This is basically a copy+paste of the patch in bug 763786.
Comment 3 :Felipe Gomes (needinfo me!) 2012-07-10 13:27:34 PDT
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 2012-07-10 16:56:16 PDT
Created attachment 640860 [details] [diff] [review]
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 me!) 2012-07-11 01:58:35 PDT
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 me!) 2012-07-11 02:01:05 PDT
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 2012-07-11 16:28:59 PDT
Created attachment 641252 [details] [diff] [review]
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: gavin@gavinsharp.com] 2012-07-11 17:07:58 PDT
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] 2012-07-12 11:00:21 PDT
Re-triage flag. Technically this is required for app launching, which is a P1, not a P2.
Comment 10 :Felipe Gomes (needinfo me!) 2012-07-12 14:17:47 PDT
(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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: gavin@gavinsharp.com] 2012-07-13 01:14:39 PDT
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 me!) 2012-07-14 01:16:16 PDT
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 14 :Felipe Gomes (needinfo me!) 2012-07-16 00:08:15 PDT
http://hg.mozilla.org/mozilla-central/rev/4cffe2b37d0c
Comment 15 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2012-07-16 03:19:23 PDT
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) 2012-07-16 03:28:09 PDT
See https://bugzilla.mozilla.org/show_bug.cgi?id=774216
Comment 17 Jason Smith [:jsmith] 2012-07-17 21:16:05 PDT
*** Bug 763740 has been marked as a duplicate of this bug. ***
Comment 18 Jason Smith [:jsmith] 2012-07-17 21:16:35 PDT
*** Bug 763789 has been marked as a duplicate of this bug. ***
Comment 19 Jason Smith [:jsmith] 2012-07-17 21:16:50 PDT
*** Bug 763786 has been marked as a duplicate of this bug. ***
Comment 20 Jason Smith [:jsmith] 2012-07-17 21:17:11 PDT
*** Bug 745924 has been marked as a duplicate of this bug. ***
Comment 21 Jason Smith [:jsmith] 2012-07-17 21:19:39 PDT
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] 2012-07-17 21:47:03 PDT
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] 2012-07-23 17:38:33 PDT
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] 2012-07-24 17:34:34 PDT
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]] 2013-06-13 09:31:10 PDT
changing the sec-review flag to :mgoodwin as he owns the sec-review bug
Comment 26 Mark Goodwin [:mgoodwin] 2013-11-15 03:39:27 PST
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.

Note You need to log in before you can comment on or make changes to this bug.