Closed Bug 884686 Opened 6 years ago Closed 6 years ago

<setting type="menulist"> throws exception on change if no pref is specified

Categories

(Toolkit :: Add-ons Manager, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: Mook, Assigned: Mook)

Details

Attachments

(1 file, 1 obsolete file)

Attached patch Do nothing if not using prefs (obsolete) — Splinter Review
When using inline settings, given a <setting type="menulist">, changing the setting causes an exception in the error console.

This is because the binding has a command event listener that calls valueToPreference unconditionally, even if there is no pref= attribute.

The attached patch adds a if (this.usePref) check in valueToPreference.

It might be better to change the event listener to an actual function that checks + calls valueToPreference instead (so random people can still call valueToPreference while ignoring usePref, similar to the other types of settings).  I'm happy to write that patch too; this was just the smaller change.
Attachment #764580 - Flags: review?(bmcbride)
Assignee: nobody → mook.moz+mozbz
Status: NEW → ASSIGNED
Comment on attachment 764580 [details] [diff] [review]
Do nothing if not using prefs

(Bounce back to me if need be)
Attachment #764580 - Flags: review?(bmcbride) → review?(geoff)
Comment on attachment 764580 [details] [diff] [review]
Do nothing if not using prefs

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

I'd rather the event listener in the constructor was changed to call inputChanged instead of valueToPreference. Same result, plus a bonus oninputchanged event fired.
Attachment #764580 - Flags: review?(geoff) → review-
Attached patch use inputChangedSplinter Review
Thanks for the speedy review! I wasn't sure if firing that event was wanted, but if that's actually a positive... :D
Attachment #764580 - Attachment is obsolete: true
Attachment #765760 - Flags: review?(geoff)
Comment on attachment 765760 [details] [diff] [review]
use inputChanged

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

I think it was just an oversight on my part when I first did that bit.

My r+ doesn't carry any weight so back to Blair for a rubber stamp.
Attachment #765760 - Flags: review?(geoff)
Attachment #765760 - Flags: review?(bmcbride)
Attachment #765760 - Flags: review+
Comment on attachment 765760 [details] [diff] [review]
use inputChanged

(In reply to Geoff Lankow (:darktrojan) from comment #4)
> My r+ doesn't carry any weight so back to Blair for a rubber stamp.

It does if I say it does ;)
Attachment #765760 - Flags: review?(bmcbride)
https://hg.mozilla.org/integration/mozilla-inbound/rev/78372eb6707f
(Oops, I forgot about the post-into-bugzilla step for a few hours...)
https://hg.mozilla.org/mozilla-central/rev/78372eb6707f
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
You need to log in before you can comment on or make changes to this bug.