Expose mozAlarms API to installed apps on Firefox desktop

RESOLVED FIXED in mozilla31

Status

()

RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: nsm, Assigned: marco)

Tracking

({dev-doc-needed})

unspecified
mozilla31
x86_64
Linux
dev-doc-needed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Should be a simple matter of flipping the pref and other minor changes.

Gene, would you like to take this as a mentored bug?
Component: DOM → DOM: Apps
That does not belong in DOM:Apps, probably DOM:Devices instead.
Component: DOM: Apps → DOM: Device Interfaces
Hi Nikhil,

Marking you as the mentor if you don't mind. Thank you for addressing this topic!
Whiteboard: [good first bug][mentor=nikhil]
Whiteboard: [good first bug][mentor=nikhil] → [good first bug][mentor=nsm]

Comment 3

5 years ago
Hey I think I can take this on. Where do I start with this?
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.
Assignee: nobody → almasry.mina
(Assignee)

Comment 5

5 years ago
Mina are you still working on this?

Comment 6

5 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

5 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
Assignee: nobody → k

Comment 8

5 years ago
Created attachment 803139 [details] [diff] [review]
webapi.patch

Extremly trivial patch (only 3 lines added) enabling Alarms API in apps started with webappsrt.

Comment 9

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

5 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.
Whiteboard: [good first bug][mentor=nsm]

Comment 11

5 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

5 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

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

5 years ago
Created attachment 8390603 [details] [diff] [review]
Patch
Attachment #803139 - Attachment is obsolete: true
Attachment #8390603 - Flags: review?(nsm.nikhil)
Attachment #8390603 - Flags: review?(myk)
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+
Keywords: dev-doc-needed
(Assignee)

Comment 17

5 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

5 years ago
Attachment #8390603 - Flags: feedback?(jonas)
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

5 years ago
Created attachment 8395039 [details] [diff] [review]
Patch
Assignee: k → mar.castelluccio
Attachment #8390603 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8395039 - Flags: review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/160b4dbded49
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.