clearUserPref shouldn't throw if a pref doesn't have a user value

RESOLVED FIXED in Firefox 6

Status

()

Core
Preferences: Backend
RESOLVED FIXED
8 years ago
6 years ago

People

(Reporter: dao, Assigned: dao)

Tracking

({dev-doc-complete, relnote})

Trunk
mozilla6
dev-doc-complete, relnote
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox6+ fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

21.55 KB, patch
bsmedberg
: review+
bsmedberg
: superreview+
Details | Diff | Splinter Review
(Assignee)

Description

8 years ago
Created attachment 371260 [details] [diff] [review]
patch

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.
Attachment #371260 - Flags: superreview?(benjamin)
Attachment #371260 - Flags: review?(benjamin)

Updated

8 years ago
Blocks: 485088
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".
(Assignee)

Comment 2

8 years ago
Created attachment 371910 [details] [diff] [review]
never throw

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.
Attachment #371910 - Flags: superreview?(benjamin)
Attachment #371910 - Flags: review?(benjamin)
Attachment #371910 - Flags: superreview?(benjamin)
Attachment #371910 - Flags: superreview+
Attachment #371910 - Flags: review?(benjamin)
Attachment #371910 - Flags: review+
Attachment #371260 - Flags: superreview?(benjamin)
Attachment #371260 - Flags: review?(benjamin)
Attachment #371260 - Flags: review-
(Assignee)

Updated

8 years ago
Keywords: checkin-needed
(Assignee)

Comment 3

8 years ago
http://hg.mozilla.org/mozilla-central/rev/00e6498d0743
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Keywords: checkin-needed
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

Comment 8

8 years ago
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.
Blocks: 424504
No longer blocks: 424504
(Assignee)

Comment 9

8 years ago
(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.
(Assignee)

Comment 10

8 years ago
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.

Comment 12

8 years ago
https://developer.mozilla.org/en/Code_snippets/Preferences page should be backed too, because it was modified 22:20, 27 Oct 2009.
Blocks: 524995
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?
(Assignee)

Comment 15

8 years ago
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
status1.9.2: --- → beta1-fixed
(In reply to comment #16)
> On the relbranch:
> http://hg.mozilla.org/releases/mozilla-1.9.2/rev/3b49c063bb42

Wrong bug Shawn.
(In reply to comment #17)
> Wrong bug Shawn.
Good catch.
status1.9.2: beta1-fixed → ---
(Assignee)

Updated

8 years ago
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
QA Contact: preferences → preferences-backend
(Assignee)

Comment 19

6 years ago
http://hg.mozilla.org/mozilla-central/rev/56d482ecbbc6
Status: REOPENED → RESOLVED
Last Resolved: 8 years ago6 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?
(Assignee)

Comment 21

6 years ago
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.
(Assignee)

Comment 23

6 years ago
(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.
tracking-firefox6: --- → +
(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.
status-firefox6: --- → fixed

Updated

6 years ago
Keywords: relnote

Updated

6 years ago
Keywords: dev-doc-needed
Documentation updated:

https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIPrefBranch#clearUserPref()

And mentioned on Firefox 6 for developers.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.