Closed
Bug 876980
Opened 11 years ago
Closed 11 years ago
Expose mozAlarms API to installed apps on Firefox desktop
Categories
(Core :: DOM: Device Interfaces, defect)
Tracking
()
RESOLVED
FIXED
mozilla31
People
(Reporter: nsm, Assigned: marco)
References
Details
(Keywords: dev-doc-needed)
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
marco
:
review+
|
Details | Diff | Splinter Review |
Should be a simple matter of flipping the pref and other minor changes.
Gene, would you like to take this as a mentored bug?
Reporter | ||
Updated•11 years ago
|
Component: DOM → DOM: Apps
Comment 1•11 years ago
|
||
That does not belong in DOM:Apps, probably DOM:Devices instead.
Component: DOM: Apps → DOM: Device Interfaces
Comment 2•11 years ago
|
||
Hi Nikhil,
Marking you as the mentor if you don't mind. Thank you for addressing this topic!
Whiteboard: [good first bug][mentor=nikhil]
Reporter | ||
Updated•11 years ago
|
Whiteboard: [good first bug][mentor=nikhil] → [good first bug][mentor=nsm]
Comment 3•11 years ago
|
||
Hey I think I can take this on. Where do I start with this?
Reporter | ||
Comment 4•11 years ago
|
||
Mina,
You'll want to edit the browser preferences in browser/app/profile/firefox.js similar to b2g/app/b2g.js so that alarms can be enabled. After that you need to write (or use one on the internet) a simple application that can be installed as an app on firefox desktop to check if alarms are working.
The code for alarms is in dom/alarm while that of system messages (which are used to notify apps when an alarm fires) is in dom/messages.
Reporter | ||
Updated•11 years ago
|
Assignee: nobody → almasry.mina
Assignee | ||
Comment 5•11 years ago
|
||
Mina are you still working on this?
Comment 6•11 years ago
|
||
I'm planning on working on it more in a week or so when I have less on my plate, but if you'd like to work on it yourself feel free to take it.
Comment 7•11 years ago
|
||
Actually, I'll just reassign it to myself when I start working on it, if it's still available.
Assignee: almasry.mina → nobody
Reporter | ||
Updated•11 years ago
|
Assignee: nobody → k
Extremly trivial patch (only 3 lines added) enabling Alarms API in apps started with webappsrt.
There are two problems with above patch that should be addresed:
1. Alarms are not preserved between webapp restarts (they are lost after user closes the app). I'm currently checking how to address this issue. It is posibly related to the next one.
2. There is no way an app can be started by alarm after it's closed. AlarmService is attached to webapprt process and this process is killed when user closes the app. Alternative solutions:
2.1 Attach AlarmService to main browser window which will become something like "master" for all the web apps. In this case apps wont be started if user closes main browser or when they are started standalone (and they are supposed to be working this way).
2.2 Create a dedicated daemon process that is responsible for scheduling alarms.
2.3 Attach AlarmService to all firefox windows, if at least one window is alive, it should be resposible for scheduling alarms and starting apps.
All propositions are nontrivial so I need some comments on what would be the best approach for this.
Assignee | ||
Comment 10•11 years ago
|
||
I think this is quite complicated and not a good first bug. I think the proper way to support this API (and other APIs, like the Push API, that need to work while apps aren't running) is to create a deamon.
Reporter | ||
Updated•11 years ago
|
Whiteboard: [good first bug][mentor=nsm]
Comment 11•11 years ago
|
||
After further investigation, it seems I was wrong about first issue. Alarms are properly stored in database and preserved between restarts. The only issue is the 2nd one.
Assignee | ||
Comment 12•11 years ago
|
||
If you want we can start to support this basic case and then do a proper implementation in another bug.
Flags: needinfo?(nsm.nikhil)
Comment 13•11 years ago
|
||
I believe this is good idea since creating proper solution may take a lot of time - the deamon have to be started by operating system so this is quite a big change and since many users wont be running webapps anyways, not all distributions/users will be willing to enable it by default. Since it is possible to run different versions of Firefox concurently in the system, it should be possible to run multiple versions of such daemon too. And update mechanism should be able to force restarting such daemon when needed which is another concern.. And there's probably even more problems with this.
Providing partial solution means that at least some apps may work unchanged in both desktop and b2g.
Reporter | ||
Comment 14•11 years ago
|
||
I agree, the daemon thing is something that'll have to first pass muster from various module owners since that is a pretty invasive system change. Let's land this so that webapps can at least use alarms while they are running. This will also allow alarms to eventually be used by pages/services.
Flags: needinfo?(nsm.nikhil)
Assignee | ||
Comment 15•11 years ago
|
||
Attachment #803139 -
Attachment is obsolete: true
Attachment #8390603 -
Flags: review?(nsm.nikhil)
Attachment #8390603 -
Flags: review?(myk)
Reporter | ||
Comment 16•11 years ago
|
||
Comment on attachment 8390603 [details] [diff] [review]
Patch
Review of attachment 8390603 [details] [diff] [review]:
-----------------------------------------------------------------
This looks good. Please post a try run before landing though.
::: webapprt/prefs.js
@@ +52,5 @@
> +// System messages
> +pref("dom.sysmsg.enabled", true);
> +
> +// Alarm API
> +pref("dom.mozAlarms.enabled", true);
Both of these will need review by a DOM peer [1]
Our new policy is not to have prefixed APIs exposed to webpages. I'm not sure about the policy for apps. I guess it is acceptable. Please ask :sicking or someone.
[1]: https://wiki.mozilla.org/Modules/Core
::: webapprt/test/chrome/alarm.html
@@ +4,5 @@
> + <meta charset="utf-8">
> + </head>
> + <body>
> + <script>
> + navigator.mozSetMessageHandler("alarm", function (message) {
Nit: Trailing whitespace.
Attachment #8390603 -
Flags: review?(nsm.nikhil) → review+
Reporter | ||
Updated•11 years ago
|
Keywords: dev-doc-needed
Assignee | ||
Comment 17•11 years ago
|
||
(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #16)
> This looks good. Please post a try run before landing though.
It's sadly useless at the moment, because we aren't running webapprt tests on try :(
Assignee | ||
Updated•11 years ago
|
Attachment #8390603 -
Flags: feedback?(jonas)
Comment 18•11 years ago
|
||
Comment on attachment 8390603 [details] [diff] [review]
Patch
Review of attachment 8390603 [details] [diff] [review]:
-----------------------------------------------------------------
::: webapprt/prefs.js
@@ +52,5 @@
> +// System messages
> +pref("dom.sysmsg.enabled", true);
> +
> +// Alarm API
> +pref("dom.mozAlarms.enabled", true);
It should be acceptable for apps, to which we already expose these APIs on other runtimes.
Attachment #8390603 -
Flags: review?(myk) → review+
Comment on attachment 8390603 [details] [diff] [review]
Patch
I don't think I need to look at this as long as it's properly reviewed.
Attachment #8390603 -
Flags: feedback?(jonas)
Assignee | ||
Comment 20•11 years ago
|
||
Assignee: k → mar.castelluccio
Attachment #8390603 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8395039 -
Flags: review+
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Keywords: checkin-needed
Comment 22•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in
before you can comment on or make changes to this bug.
Description
•