Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Support periodInMinutes key for chrome.alarms.

RESOLVED FIXED in Firefox 44

Status

()

Toolkit
WebExtensions: Untriaged
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: bwinton, Assigned: bwinton)

Tracking

Trunk
mozilla44
Points:
---
Bug Flags:
blocking-webextensions +

Firefox Tracking Flags

(firefox44 fixed)

Details

(Whiteboard: [alarms])

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
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?

Updated

2 years ago
Whiteboard: [alarms]
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?
(Assignee)

Comment 2

2 years ago
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?
No, you can add a separate add_task(...) block. It will run after the first one has finished.
(Assignee)

Comment 4

2 years ago
Created attachment 8675005 [details] [diff] [review]
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
Assignee: nobody → bwinton
Status: NEW → ASSIGNED
Attachment #8675005 - Flags: review?(wmccloskey)

Updated

2 years ago
Blocks: 1214433
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.
Attachment #8675005 - Flags: review?(wmccloskey) → review+

Comment 6

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b987c675553
https://hg.mozilla.org/mozilla-central/rev/3b987c675553
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox44: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44

Updated

2 years ago
Flags: blocking-webextensions+
You need to log in before you can comment on or make changes to this bug.