1215130 - Support periodInMinutes key for chrome.alarms.

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect)

Description

[Blake Winton (:bwinton) (:☕️)]

Currently, we pass the periodInMinutes down into the Alarm object, but entirely ignore it when setting the scheduledTime (as shown in https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/ext-alarms.js#23 )  :)

I am a little surprised that the "timer.init(this, delay, Ci.nsITimer.TYPE_ONE_SHOT);" call doesn't complain when getting NaN as the delay…

Johann: Did you want to try fixing this one, or should I give it a shot?

Comment 1

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

The documentation here is really weird. Here's what the Chrome docs say:

"Describes when the alarm should fire. The initial time must be specified by either when or delayInMinutes (but not both). If periodInMinutes is set, the alarm will repeat every periodInMinutes minutes after the initial event. If neither when or delayInMinutes is set for a repeating alarm, periodInMinutes is used as the default for delayInMinutes."

The second sentence implies that one of |when| or |delayInMinutes| must be defined. But then the last sentence suggests that isn't the case. I guess the bug happens if they're both undefined?

Comment 2

[Blake Winton (:bwinton) (:☕️)]

Yeah, that's when the bug happens.  Chrome fires as per the second sentence.  Also, I think the fix will be pretty easy, but I'm not sure how to run multiple tests from the test_ext_alarms.html in bug 1215201.  Do I need to put everything into Promises?

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

No, you can add a separate add_task(...) block. It will run after the first one has finished.

Comment 4

[Blake Winton (:bwinton) (:☕️)]

Attachment: The first attempt.

(In reply to Bill McCloskey (:billm) from comment #3)
> No, you can add a separate add_task(...) block. It will run after the first
> one has finished.

Oh, cool!  Okay, here we go with a little refactoring of the previous test.  Lemme know what should be changed.  Thanks!  :D

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8675005 [details] [diff] [review]
The first attempt.

Review of attachment 8675005 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/extensions/test/mochitest/test_ext_alarms.html
@@ +18,2 @@
>      browser.test.log("running alarm script");
> +    chrome.alarms.clearAll();

This shouldn't be needed. It will only clear the alarms associated with this extension, and there aren't any yet. If you're having problems with alarms firing after the test ends, we should investigate more.

@@ +47,5 @@
> +add_task(function* test_periodic_alarm_fires() {
> +  function backgroundScript() {
> +    var ALARM_NAME = "test_ext_alarms";
> +    browser.test.log("running alarm script");
> +    chrome.alarms.clearAll();

Same here.

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3b987c675553

Comment 7

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/3b987c675553