790374 - Add ability to retrieve floats from libpref

Status: RESOLVED FIXED

(Core :: Preferences: Backend, defect)

Description

[Scott Johnson (:jwir3)]

Floating point preferences can be stored as strings (e.g. "1.0", "1.9"...) in the backend, but we should add a new GetFloat() API to libpref, so we can retrieve a floating point number from libpref in circumstances where this is desirable. It's possible that, in the future, we could transition to an actual floating point storage mechanism, but, for now, doing automatic translation from string->float is acceptable.

Comment 1

[Scott Johnson (:jwir3)]

Attachment: b790374

Added initial implementation that allows retrieval of string preferences as floating-point numbers.

Comment 2

[Benjamin Smedberg]

Comment on attachment 664156 [details] [diff] [review]
b790374

ISTR that I did another review for something very similar to this where somebody added "AddFloatVarCache". But I can't find the bug# and I don't remember whether I marked r+ or r- on that bug. Could you spend a minute looking for that and coordinating if appropriate?

This looks fine: in the test, please add failure checking: check that getFloatPref throws if the value is "" and "1.4a1" and other values which aren't full floats.

Comment 3

[Scott Johnson (:jwir3)]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #2)
> Comment on attachment 664156 [details] [diff] [review]
> b790374
> 
> ISTR that I did another review for something very similar to this where
> somebody added "AddFloatVarCache". But I can't find the bug# and I don't
> remember whether I marked r+ or r- on that bug. Could you spend a minute
> looking for that and coordinating if appropriate?
> 
I did a number of searches, and spent about twenty minutes looking for this, including searching google for "AddFloat bugzilla", "AddFloatVarCache", and searching bugzilla for attachment data containing the words"AddFloatVarCache" or "AddFloat". Unfortunately, I was unable to find anything. :|

> This looks fine: in the test, please add failure checking: check that
> getFloatPref throws if the value is "" and "1.4a1" and other values which
> aren't full floats.
Sure, I'll add that.

Comment 4

[Scott Johnson (:jwir3)]

Inbound, after requested changes made:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ac9fdc7330d7

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ac9fdc7330d7

Comment 6

[Philip Chee]

> ISTR that I did another review for something very similar to this where somebody added
> "AddFloatVarCache". But I can't find the bug# and I don't remember whether I marked r+
> or r- on that bug. Could you spend a minute looking for that and coordinating if
> appropriate?
That would be Bug 826586 - Add a AddFloatVarCache() API to libpref.

Comment 7

[Benjamin Smedberg]

No actually, it was definitely not that bug because that one is very new. But in any case, it has landed.

Comment 8

[Sar]

I think in nsIPrefBranch.idl the inserting of GetFloatPref procedure between the earlier interface procedures is not correct because it makes the interface NOT backward compatible.
It shifts the offset of procedures by 4, that means different procs will be called in plugins created before this change.
The proc should be appended at the end, or a new interface created. It gives hard time to the developers. By the way it makes almost impossible to be backward compatible with the earlier browsers.

Comment 9

[Benjamin Smedberg]

Binary compatibility of interfaces is *not* a goal in Firefox. It is expected that if you are using binary extensions, they will need to be recompiled for each release. Thus use of binary XPCOM is strongly discouraged.