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

VERIFIED FIXED in Firefox 16

Status

Firefox Graveyard
Web Apps
P1
normal
VERIFIED FIXED
5 years ago
a year ago

People

(Reporter: Felipe, Assigned: Dan Walkowski)

Tracking

(Depends on: 1 bug)

Trunk
Firefox 16
Dependency tree / graph

Details

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

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

5 years ago
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
(Reporter)

Updated

5 years ago
Summary: Create WebAppOSUtils.jsm to host platform-specific webapps code → Create WebappOSUtils.jsm to host platform-specific webapps code
(Reporter)

Comment 1

5 years ago
Created attachment 640729 [details] [diff] [review]
Patch

This is the file, let's fill it in
Assignee: nobody → felipc
Status: NEW → ASSIGNED
Created attachment 640735 [details] [diff] [review]
Patch

Added Windows impl.  This is basically a copy+paste of the patch in bug 763786.
Attachment #640729 - Attachment is obsolete: true
(Reporter)

Comment 3

5 years ago
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)
(Reporter)

Updated

5 years ago
Summary: Create WebappOSUtils.jsm to host platform-specific webapps code → Create WebappOSUtils.jsm to host platform-specific webapps code and implement native app launch
(Assignee)

Comment 4

5 years ago
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.
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)
(Reporter)

Comment 5

5 years ago
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+
(Reporter)

Comment 6

5 years ago
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
(Assignee)

Comment 7

5 years ago
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.
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 → --

Updated

5 years ago
Blocks: 745924
Priority: -- → P1

Updated

5 years ago
Target Milestone: --- → Firefox 16
(Reporter)

Comment 10

5 years ago
(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+
(Reporter)

Updated

5 years ago
Attachment #641252 - Flags: review?(felipc) → review+
(Reporter)

Comment 12

5 years ago
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
https://hg.mozilla.org/integration/mozilla-inbound/rev/20a09c21d3d5
https://hg.mozilla.org/integration/mozilla-inbound/rev/6bfce37d7fa4

Updated

5 years ago
Whiteboard: [qa-]
(Reporter)

Comment 14

5 years ago
http://hg.mozilla.org/mozilla-central/rev/4cffe2b37d0c
Status: ASSIGNED → RESOLVED
Last Resolved: 5 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.
See https://bugzilla.mozilla.org/show_bug.cgi?id=774216

Updated

5 years ago
Depends on: 774216

Updated

5 years ago
Whiteboard: [qa-] → [qa+]

Updated

5 years ago
Duplicate of this bug: 763740

Updated

5 years ago
Duplicate of this bug: 763789

Updated

5 years ago
Duplicate of this bug: 763786

Updated

5 years ago
Duplicate of this bug: 745924

Updated

5 years ago
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).
Keywords: sec-review-needed
Whiteboard: [qa+] → [qa+], [sec-assigned:curtisk]

Updated

5 years ago
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.