Closed Bug 772600 Opened 8 years ago Closed 8 years ago

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

Categories

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

defect

Tracking

(Not tracked)

VERIFIED FIXED
Firefox 16

People

(Reporter: Felipe, Assigned: dwalkowski)

References

(Depends on 1 open bug)

Details

(Whiteboard: [qa!], [sec-assigned:curtisk])

Attachments

(2 files, 2 obsolete files)

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
Summary: Create WebAppOSUtils.jsm to host platform-specific webapps code → Create WebappOSUtils.jsm to host platform-specific webapps code
Attached patch PatchSplinter Review
This is the file, let's fill it in
Assignee: nobody → felipc
Status: NEW → ASSIGNED
Attached patch Patch (obsolete) — Splinter Review
Added Windows impl.  This is basically a copy+paste of the patch in bug 763786.
Attachment #640729 - Attachment is obsolete: true
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.
Attachment #640729 - Attachment is obsolete: false
Attachment #640729 - Flags: review?(gavin.sharp)
Summary: Create WebappOSUtils.jsm to host platform-specific webapps code → Create WebappOSUtils.jsm to host platform-specific webapps code and implement native app launch
Attached patch very provisional patch (obsolete) — Splinter Review
this is a provisional, untested patch that incorporates changes for all three OS's.  I am hoping for feedback only at this time.
Assignee: felipc → dwalkowski
Attachment #640729 - Attachment is obsolete: true
Attachment #640735 - Attachment is obsolete: true
Attachment #640729 - Flags: review?(gavin.sharp)
Attachment #640860 - Flags: feedback?(felipc)
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.
Attachment #640860 - Flags: feedback?(felipc) → feedback+
Comment on attachment 640729 [details] [diff] [review]
Patch

(please leave this patch as non-obselete so Gavin can review the file creation for me)
Attachment #640729 - Attachment is obsolete: false
Attachment #640729 - Flags: review?(gavin.sharp)
Priority: -- → P2
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.
Attachment #640860 - Attachment is obsolete: true
Attachment #641252 - Flags: review?(felipc)
Attachment #641252 - Flags: feedback?(tabraldes)
Attachment #641252 - Flags: feedback?(mar.castelluccio)
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 :)
Attachment #640729 - Flags: review?(gavin.sharp) → feedback+
Re-triage flag. Technically this is required for app launching, which is a P1, not a P2.
Priority: P2 → --
Blocks: 745924
Priority: -- → P1
Target Milestone: --- → Firefox 16
(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.
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.
Attachment #641252 - Flags: feedback?(mar.castelluccio) → feedback+
Attachment #641252 - Flags: review?(felipc) → review+
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
Whiteboard: [qa-]
http://hg.mozilla.org/mozilla-central/rev/4cffe2b37d0c
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
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.
Depends on: 774216
Whiteboard: [qa-] → [qa+]
Duplicate of this bug: 763740
Duplicate of this bug: 763789
Duplicate of this bug: 763786
Duplicate of this bug: 745924
Depends on: 773040
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).
Whiteboard: [qa+] → [qa+], [sec-assigned:curtisk]
Blocks: 710062
No longer blocks: 745924
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.
Attachment #641252 - Flags: feedback?(tabraldes) → feedback+
Proposal for testing this is here - https://etherpad.mozilla.org/native-app-launch-testing. Feedback or ideas would be greatly appreciated!
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.
Status: RESOLVED → VERIFIED
Whiteboard: [qa+], [sec-assigned:curtisk] → [qa!], [sec-assigned:curtisk]
Flags: sec-review?(curtisk)
changing the sec-review flag to :mgoodwin as he owns the sec-review bug
Flags: sec-review?(curtisk) → sec-review?(mgoodwin)
Depends on: 935713
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.
No longer depends on: 935713
Flags: sec-review?(mgoodwin)
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.