Created attachment 371260 [details] [diff] [review]
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]
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".
Created attachment 371910 [details] [diff] [review]
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.
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.
The current behavior is documented:
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:
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
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:
(In reply to comment #16)
> On the relbranch:
Wrong bug Shawn.
(In reply to comment #17)
> Wrong bug Shawn.
(In reply to comment #19)
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.
And mentioned on Firefox 6 for developers.