Closed Bug 487059 Opened 11 years ago Closed 9 years ago
User Pref shouldn't throw if a pref doesn't have a user value
The current behavior isn't documented and requires others to 1) make a fragile assumption about the pref's default value, 2) check prefHasUserValue first, or 3) use try-catch, which means a useful exception will be missed if the pref doesn't exist at all. 1) and 3) are harmful; 2) is OK but not widely used, and it shouldn't be necessary either.
Comment on attachment 371260 [details] [diff] [review] patch How is this better than never throwing at all? It seems odd to say that the presence or absence of a default pref would affect an API named "clearUserPref".
Either way is fine by me. I covered only one case, because that was the one I cared about, and I thought it would be a less controversial change. For prefs that are expected to have a default value, that exception seemed useful to me, as it means something went wrong (e.g. mistyped pref name). For prefs that don't have a default value, it's pretty much useless. But one could argue that you shouldn't unconditionally call clearUserPref in that case.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
Is there a reason this never landed on branch? I just got hit by this throwing on branch but not on trunk.
Nobody asked for it to land there. I do not think it should.
Indeed, it's an API change and it came late in the cycle, so it's not really suitable for the branch.
Verified fixed on trunk with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090601 Minefield/3.6a1pre ID:20090601031227.
Status: RESOLVED → VERIFIED
Attachment #371260 - Attachment is obsolete: true
The current behavior is documented: https://developer.mozilla.org/en/Code_snippets/Preferences Note that clearUserPref() will throw an exception unless the pref has a user value; you must test prefHasUserValue(pref) first, then clear the value with clearUserPref(). And this behavior has been around for many years. Have you checked to verify this won't break any extensions? I know it broke mine.
(In reply to comment #8) > The current behavior is documented: > > https://developer.mozilla.org/en/Code_snippets/Preferences You're citing a sentence hidden deep on a code snippets page. I wouldn't call that documentation.
Also, that sentence was added after this patch landed...
As per the discussion in dev-planning (http://groups.google.com/group/mozilla.dev.platform/browse_thread/thread/c083cf990ff2f03f#) this change should be backed out immediately on mozilla-central, mozilla-1.9.2 and the mozilla-1.9.2 beta relbranch.
https://developer.mozilla.org/en/Code_snippets/Preferences page should be backed too, because it was modified 22:20, 27 Oct 2009.
We should do any work on bug 524995. Lets move it over there.
> The current behavior isn't documented nsIPrefBranch.idl says: 218 * @return Other The preference does not exist or have a user set value. as the last sentence of the comment for clearUserPref, and has done so since revision 1.16 (October 2001). How much more documented can you get?
That was half a year ago, but I think I was looking for "throws", so I was probably misreading the idl. Besides, while "The preference does not exist" is quite clear, I still find it questionable whether "have a user set value" here should mean the same as defined for prefHasUserValue, as this makes an explicitly set value matching the default value an exception case here. That's the most painful part that my first patch addressed specifically.
On the relbranch: http://hg.mozilla.org/releases/mozilla-1.9.2/rev/3b49c063bb42
(In reply to comment #16) > On the relbranch: > http://hg.mozilla.org/releases/mozilla-1.9.2/rev/3b49c063bb42 Wrong bug Shawn.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
Status: REOPENED → RESOLVED
Closed: 11 years ago → 9 years ago
Resolution: --- → FIXED
Target Milestone: mozilla1.9.2a1 → mozilla6
(In reply to comment #19) > http://hg.mozilla.org/mozilla-central/rev/56d482ecbbc6 We decided to land this again? Is anybody planning on posting to the add-on compatibility blog about this?
I did, as this was only backed out because it landed in a beta phase. I can communicate the change, but I don't expect significant add-on breakage.
You have no way to predict add-on breakage. This definitely needs to be communicated to the add-on team.
(In reply to comment #22) > You have no way to predict add-on breakage. I surely have. I searched through add-ons, now and in the past, and even without examining every occurrence (there are many of them), you get a good sense of how this is usually used in the wild. I know an add-on of yours was relying on the exception, but this is by no means the common case.
(In reply to comment #23) > I surely have. I searched through add-ons, now and in the past, and even > without examining every occurrence (there are many of them), you get a good > sense of how this is usually used in the wild. I know an add-on of yours was > relying on the exception, but this is by no means the common case. This only applies to add-ons hosted on AMO which are a fraction of all available add-ons. I will CC Jorge and Justin just to make sure we get the right message send out to all necessary channels.
Thank you for the ping. I'll make a note of this for the Firefox 6 upgrade cycle. I assume the milestone is correct and this won't make it to Firefox 5.
Documentation updated: https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIPrefBranch#clearUserPref() And mentioned on Firefox 6 for developers.
You need to log in before you can comment on or make changes to this bug.