Closed
Bug 884686
Opened 11 years ago
Closed 11 years ago
<setting type="menulist"> throws exception on change if no pref is specified
Categories
(Toolkit :: Add-ons Manager, defect)
Tracking
()
RESOLVED
FIXED
mozilla24
People
(Reporter: Mook, Assigned: Mook)
Details
Attachments
(1 file, 1 obsolete file)
747 bytes,
patch
|
darktrojan
:
review+
|
Details | Diff | 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)
Updated•11 years ago
|
Assignee: nobody → mook.moz+mozbz
Status: NEW → ASSIGNED
Comment 1•11 years ago
|
||
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 2•11 years ago
|
||
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-
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 4•11 years ago
|
||
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 5•11 years ago
|
||
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...)
Comment 7•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/78372eb6707f
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
You need to log in
before you can comment on or make changes to this bug.
Description
•