503971 - nsIContentPrefService methods should throw when passed a null setting name

Status: RESOLVED FIXED

(Toolkit :: Preferences, defect)

Description

[Myk Melez [:myk] [@mykmelez]]

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.

Comment 1

[Geoff Lankow (:darktrojan)]

Attachment: patch and tests

Comment 2

[Geoff Lankow (:darktrojan)]

On second thoughts, I should be using NS_ERROR_ILLEGAL_VALUE instead of NS_ERROR_NULL_POINTER, shouldn't I?

Comment 3

[Myk Melez [:myk] [@mykmelez]]

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?

Comment 4

[Geoff Lankow (:darktrojan)]

Attachment: patch v2

(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.

Comment 5

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 391848 [details] [diff] [review]
patch v2

This refactoring looks great.

Comment 6

[Myk Melez [:myk] [@mykmelez]]

Gavin: any thoughts on this?

Comment 7

[Myk Melez [:myk] [@mykmelez]]

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.

Comment 8

[Mike Connor [: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.

Comment 9

[Geoff Lankow (:darktrojan)]

Attachment: final patch [Checkin: Comment 10]

with @throws, which I had been meaning to add ...

Comment 10

[Serge Gautherie (:sgautherie)]

Comment on attachment 395709 [details] [diff] [review]
final patch
[Checkin: Comment 10]


http://hg.mozilla.org/mozilla-central/rev/5e61c13575e7