HTML specs moved from single precision to double precision for mostly everything: http://html5.org/tools/web-apps-tracker?from=5372&to=5373 And the rules for parsing floating points values have recently changed to be double-precision: http://html5.org/tools/web-apps-tracker?from=5914&to=5915 For 32 bits platforms, this is going to increase the size of nsAttrValue of 32 bits (MiscContainer will be 32 bits bigger). On 64 bits platforms, nothing will change. A work around could be to save a pointer to the double value instead of the double value. It would take another 32 bits more space when a double would be used but it wouldn't change anything in other cases.

First of all, this change won't make nsAttrValue bigger. It'll just make MiscContainer bigger. So as long as your attr value is a string, or an integer in our min-to-max range, there is no change. If we really care, we could try to rework this whole thing to use something like NaN-boxing (esp. on 32-bit), I suppose...

(In reply to comment #1) > First of all, this change won't make nsAttrValue bigger. It'll just make > MiscContainer bigger. So as long as your attr value is a string, or an integer > in our min-to-max range, there is no change. Oups, I thought MiscContainer was used by nsAttrValue. > If we really care, we could try to rework this whole thing to use something > like NaN-boxing (esp. on 32-bit), I suppose... What do you mean?

> Oups, I thought MiscContainer was used by nsAttrValue. It is, in some cases. Not in all case. > What do you mean? http://blog.mozilla.com/rob-sayre/2010/08/02/mozillas-new-javascript-value-representation/

Does PR_strtod give the results that the spec calls for, btw? Or does the spec not specify how doubles should be stringified here?

Comment on attachment 515112 [details] [diff] [review] Part 1 - Add ToDouble to nsTString Are we keeping ToFloat just so we won't have to rewrite existing code? Could we make it an inline accessor, and just have ToDouble be the one impl? We probably ought to have a way of testing this...

(In reply to comment #5) > Does PR_strtod give the results that the spec calls for, btw? Or does the spec > not specify how doubles should be stringified here? HTML rules for parsing floating point numbers are: http://www.whatwg.org/specs/web-apps/current-work/multipage/common-microsyntaxes.html#rules-for-parsing-floating-point-number-values The only difference between parsing single-precision and double-precision is in step 14: the allowed values are all double-precision float as defined by IEEE 754 instead of the single-precision ones. Given that PR_strtod isn't really readable, I can't tell if what it does match with what is requested but I would say that if it was matching for single-precision floats, it might very likely still match. (In reply to comment #6) > Comment on attachment 515112 [details] [diff] [review] > Part 1 - Add ToDouble to nsTString > > Are we keeping ToFloat just so we won't have to rewrite existing code? If consumers of the API want a float, ToFloat seems better than (float)ToDouble given that we could imagine that ToFloat might be optimized someday (like ignoring characters after the first x with x being the maximum precision single-precision floats have). > Could we > make it an inline accessor, and just have ToDouble be the one impl? Will do that. > We probably ought to have a way of testing this... And that too.

Some calls haven't been changed because they end up to really want a float (like passing the result to a method taking a float in parameter).

I have some patches that are going to implicitly test that (progress.{value,max,position} and input.valueAsNumber). Boris, let me know if you think I should rename GetFloatAttr, SetFloatAttr, GetFloatValue and ParseFloatValue.

Comment on attachment 515484 [details] [diff] [review] Part 3 - nsAttrValue::MiscContainer is now storing a double instead of a float Should I ask for a super-review for this patch?

> but I would say that if it was matching for single-precision floats, it might > very likely still match. It could do whatever the heck it wants for the low 20 bits of the mantissa and _still_ match for floats, if you think about it. I seriously doubt it matches in edge cases, fwiw. We'd have to test....

(In reply to comment #10) > Created attachment 515484 [details] [diff] [review] > Part 3 - nsAttrValue::MiscContainer is now storing a double instead of a float > > I have some patches that are going to implicitly test that > (progress.{value,max,position} and input.valueAsNumber). > > Boris, let me know if you think I should rename GetFloatAttr, SetFloatAttr, > GetFloatValue and ParseFloatValue. I'm not Boris, but I'd say yes :)

Comment on attachment 515478 [details] [diff] [review] Part 1 - Add ToDouble to nsTString ToFloat should not be NS_COM now that it is inline.

Boris, to summarize our IRC conversation, PR_strtod should be fine as long as the HTML specs fix a few bugs in the parsing algorithm? I guess we can take this patch and assume HTML specs will change given that they will have to change to match what JS specs say, at least. For people interested, this is the bug against the specs: http://www.w3.org/Bugs/Public/show_bug.cgi?id=12220

> PR_strtod should be fine as long as the HTML specs fix a few bugs in the > parsing algorithm? _Probably_, yes. I'd have to read the result carefully...

I wrote a small test using the progress element to know how the double attributes are managed in the different browsers. Firefox trunk passes all tests. Opera 11.10 alpha fails one test: '42foo' is parsed as '42'. Webkit trunk and Chrome 11 (dev channel) fail one test: '+1' isn't parsed as 1 (it fails and fallback to the default value of progress.value which is 0). I would say that we can land these patches and update our behavior later to fix the edge cases if needed.

> Boris, let me know if you think I should rename GetFloatAttr, SetFloatAttr, > GetFloatValue and ParseFloatValue. I think it's a good idea, but followup is fine for that. And have sicking review that?

(In reply to comment #19) > > Boris, let me know if you think I should rename GetFloatAttr, SetFloatAttr, > > GetFloatValue and ParseFloatValue. > > I think it's a good idea, but followup is fine for that. And have sicking > review that? Hmmm, that's part 4... Do you want me to move it to another bug? or ask Jonas to review it?

Comment on attachment 515484 [details] [diff] [review] Part 3 - nsAttrValue::MiscContainer is now storing a double instead of a float r=me

Comment on attachment 521556 [details] [diff] [review] Part 4 - renaming methods and variables Ah, indeed. r=me, but should get sicking's sr.

Comment on attachment 521556 [details] [diff] [review] Part 4 - renaming methods and variables Looks great! Was there any particular part that you wanted me to look at?

No, just the general idea.

Pushed: http://hg.mozilla.org/mozilla-central/rev/9f5a4c820913 http://hg.mozilla.org/mozilla-central/rev/6e2cb996ff53 http://hg.mozilla.org/mozilla-central/rev/f3c80cecaf9a http://hg.mozilla.org/mozilla-central/rev/3b1b22862652

FWIW, the specs have been changed and the two points that we have raised have been fixed.