Closed Bug 1204072 Opened 9 years ago Closed 9 years ago

Error running a callback in chrome.notifications.create.

Categories

(WebExtensions :: Untriaged, defect)

defect
Not set
normal

Tracking

(firefox43 affected, firefox44 fixed)

RESOLVED FIXED
mozilla44
Tracking Status
firefox43 --- affected
firefox44 --- fixed

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.
I'd like to take this as a first bug if nobody minds. :)
Attachment #8663248 - Flags: review?(wmccloskey)
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+
(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!
Attachment #8663248 - Attachment is obsolete: true
Flags: needinfo?(wmccloskey)
https://hg.mozilla.org/mozilla-central/rev/e4ed64c5feed
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Flags: needinfo?(wmccloskey)
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.