callback is null in ext-alarms.

RESOLVED FIXED in Firefox 44

Status

RESOLVED FIXED
3 years ago
6 months ago

People

(Reporter: bwinton, Assigned: bwinton)

Tracking

Trunk
mozilla44

Firefox Tracking Flags

(firefox44 fixed)

Details

Attachments

(1 attachment)

So, I'm digging in to why my alarms aren't firing, and I came across this.
It looks like we should be using for…of instead of for…in.
(Assignee)

Comment 1

3 years ago
Created attachment 8674395 [details] [diff] [review]
The first attempt.

(I even added tests!  ;)
Attachment #8674395 - Flags: review?(wmccloskey)
Comment on attachment 8674395 [details] [diff] [review]
The first attempt.

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

Thanks!

::: toolkit/components/extensions/test/mochitest/test_ext_alarms.html
@@ +13,5 @@
> +<script type="application/javascript;version=1.8">
> +
> +add_task(function* test_contentscript() {
> +  function backgroundScript() {
> +    browser.test.log('running alarm script');

Please use double quotes for all string literals.

@@ +22,5 @@
> +    });
> +    chrome.alarms.create('test_ext_alarms', {delayInMinutes: 0.02});
> +    setTimeout(() => {
> +      browser.test.notifyFail('alarms test failed, took too long');
> +    }, 3000);

This doesn't give us a lot of margin for error on slow test machines. Please make this 10 seconds or remove it entirely.
Attachment #8674395 - Flags: review?(wmccloskey) → review+
(Assignee)

Comment 3

3 years ago
Done and done.

I also changed the:
  add_task(function* test_contentscript() {
line to
  add_task(function* test_alarm_fires() {
cause we're not really testing the contentscript here.
(It looks like that line was copied and pasted in a bunch of files, but I don't think it's worth cleaning up in this patch… :)
https://hg.mozilla.org/mozilla-central/rev/b0fc271e4897
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox44: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44

Updated

6 months ago
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.