Closed
Bug 1204072
Opened 9 years ago
Closed 9 years ago
Error running a callback in chrome.notifications.create.
Categories
(WebExtensions :: Untriaged, defect)
WebExtensions
Untriaged
Tracking
(firefox43 affected, firefox44 fixed)
RESOLVED
FIXED
mozilla44
People
(Reporter: bwinton, Assigned: johannh)
Details
Attachments
(1 file, 1 obsolete file)
The console says: runSafe is not defined ext-notifications.js:99:0 The notification seems to show just fine, though.
Assignee | ||
Comment 1•9 years ago
|
||
I'd like to take this as a first bug if nobody minds. :)
Assignee | ||
Comment 2•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Attachment #8663248 -
Flags: review?(wmccloskey)
Reporter | ||
Updated•9 years ago
|
Assignee: nobody → mail
Status: NEW → ASSIGNED
Comment on attachment 8663248 [details] [diff] [review] Fix WebExtensions callbacks failing by importing runSafe correctly Review of attachment 8663248 [details] [diff] [review]: ----------------------------------------------------------------- Thanks very much for fixing this! Do you need any help getting this landed? ::: toolkit/components/extensions/test/extensions/notifications/background.js @@ +1,5 @@ > +browser.test.log("running background script"); > + > +var opts = {title: "Testing Notification", message: "Carry on"}; > + > +browser.notifications.create(5, opts, function(id){ According to the Chrome docs the ID is supposed to be a string. We don't do any checking or coercion, though. To make the test future-proof, can you pass in a string for the ID and check against that?
Attachment #8663248 -
Flags: review?(wmccloskey) → review+
Assignee | ||
Comment 4•9 years ago
|
||
(In reply to Bill McCloskey (:billm) from comment #3) > Comment on attachment 8663248 [details] [diff] [review] > Fix WebExtensions callbacks failing by importing runSafe correctly > > Review of attachment 8663248 [details] [diff] [review]: > ----------------------------------------------------------------- > > Thanks very much for fixing this! > > Do you need any help getting this landed? Yeah, I'll try to ping you on IRC :) > > ::: toolkit/components/extensions/test/extensions/notifications/background.js > @@ +1,5 @@ > > +browser.test.log("running background script"); > > + > > +var opts = {title: "Testing Notification", message: "Carry on"}; > > + > > +browser.notifications.create(5, opts, function(id){ > > According to the Chrome docs the ID is supposed to be a string. We don't do > any checking or coercion, though. To make the test future-proof, can you > pass in a string for the ID and check against that? Good catch! I didn't see that. I'll change it to be a string, although we can definitely improve these tests overall. Thanks for reviewing!
Assignee | ||
Comment 5•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Attachment #8663248 -
Attachment is obsolete: true
Flags: needinfo?(wmccloskey)
Comment 8•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/e4ed64c5feed
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox44:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Assignee | ||
Comment 9•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=05d9b9abc432
Flags: needinfo?(wmccloskey)
Updated•6 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•