Closed Bug 503971 Opened 13 years ago Closed 13 years ago

nsIContentPrefService methods should throw when passed a null setting name

Categories

(Toolkit :: Preferences, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.3a1

People

(Reporter: myk, Assigned: darktrojan)

Details

Attachments

(1 file, 2 obsolete files)

nsIContentPrefService methods that take a setting name should throw when passed a null name, since it doesn't make sense for a preference to have a null setting name, and the setting.name column has a NOT NULL constraint that prevents it from happening.

Currently, the methods this affects are getPref, setPref, hasPref, and removePref; once bug 458299 is fixed, it will include getPrefsByName and removePrefsByName.
Attached patch patch and tests (obsolete) — Splinter Review
Attachment #390951 - Flags: review?(myk)
On second thoughts, I should be using NS_ERROR_ILLEGAL_VALUE instead of NS_ERROR_NULL_POINTER, shouldn't I?
Comment on attachment 390951 [details] [diff] [review]
patch and tests

(In reply to comment #2)
> On second thoughts, I should be using NS_ERROR_ILLEGAL_VALUE instead of
> NS_ERROR_NULL_POINTER, shouldn't I?

Yes, I think you're right.

Otherwise this looks great, so r=myk with that fix.

I do wonder if the code should be checking specifically for a null value rather than truth.  I think the current approach is ok, since the only valid string that would evaluate to false is the empty string, and we probably don't want consumers to set preferences whose name is the empty string.

But maybe then we should update the error message to say "aName can't be null or the empty string", and maybe it makes the most sense to explicitly check for these things rather than relying on truth as an implicit check.

Gavin: do you have an opinion on that?
Attachment #390951 - Flags: review?(myk)
Attachment #390951 - Flags: review?(gavin.sharp)
Attachment #390951 - Flags: review+
Attached patch patch v2 (obsolete) — Splinter Review
(In reply to comment #3)
> I do wonder if the code should be checking specifically for a null value
> rather than truth.  I think the current approach is ok, since the only valid
> string that would evaluate to false is the empty string, and we probably 
> don't want consumers to set preferences whose name is the empty string.
> 
> But maybe then we should update the error message to say "aName can't be null
> or the empty string", and maybe it makes the most sense to explicitly check 
> for these things rather than relying on truth as an implicit check.

I hadn't thought of the empty string, but we don't want it anyway. I've added some tests to cover it and changed the message.

Myk: I've also refactored the tests so have a quick look over that and check it's still ok.
Attachment #390951 - Attachment is obsolete: true
Attachment #391848 - Flags: review?(gavin.sharp)
Attachment #390951 - Flags: review?(gavin.sharp)
Comment on attachment 391848 [details] [diff] [review]
patch v2

This refactoring looks great.
Attachment #391848 - Flags: review+
Gavin: any thoughts on this?
Comment on attachment 391848 [details] [diff] [review]
patch v2

This doesn't need review from Gavin, although I thought it would be useful.  It might need super-review, if this is considered an API addition (although I think of it more as an API change).  Requesting that from mconnor.
Attachment #391848 - Flags: review?(gavin.sharp) → superreview?(mconnor)
Comment on attachment 391848 [details] [diff] [review]
patch v2

SR is required either way (changes or additions)

Please add the @throws details to the IDL definitions.  sr=me with that.
Attachment #391848 - Flags: superreview?(mconnor) → superreview+
with @throws, which I had been meaning to add ...
Attachment #391848 - Attachment is obsolete: true
Assignee: nobody → geoff
Status: NEW → ASSIGNED
Keywords: checkin-needed
Attachment #395709 - Attachment description: final patch → final patch [Checkin: Comment 10]
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
Version: unspecified → Trunk
Flags: wanted1.9.2?
Flags: wanted1.9.2?
You need to log in before you can comment on or make changes to this bug.