[Webkit-unassigned] [Bug 48308] Too precise serialization from floating point number to string for "number" input elements

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=48308


Gavin Barraclough <barraclough at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #72653|review?                     |review-
               Flag|                            |




--- Comment #29 from Gavin Barraclough <barraclough at apple.com>  2010-11-03 01:18:12 PST ---
(From update of attachment 72653)
Hi Dai,

Sorry for taking the day to get back to you, but took while to get my head around the fp math (particularly convince myself that allowing rounding all the way up to the 16 DP limit was okay & wouldn't be introducing significant inaccuracies at that point).  But all looks like it should work fine to me.

I think this patch looks awesome – I think this will be a really great resolution to the bug.
Couple of really trivial issues with the code.

> WebCore/html/HTMLInputElement.cpp:320
> +    if (newValue - m_inputType->minimum() < -acceptableError) {

You've changed the logic here to allow a margin of error on this check.  I see why that makes sense, but it seems this could lead to the internal value being lower than the permitted minimum (albeit by a very small amount).  We could maybe catch this with a second check? - leave the error check as is with the tolerance, but below it follow up with a quick:...

+  if (newValue < m_inputType->minimum())
+      newValue = m_inputType->minimum();

... to make sure the internal value is never below the minimum, by snapping back up to the minimum if it was close but below.
What do you think, worth doing anything to fix this case up?
Also, I guess the same is true for the maximum check, that could go slightly over, may need the value nudging down to ensure it's in range.

> WebCore/html/HTMLInputElement.cpp:329
> +    baseDecimalPlaces = min(baseDecimalPlaces, (unsigned) 16);
> +    if (newValue < pow((double) 10, (double) 21)) {
> +        double scale = pow((double) 10, (double) max(stepDecimalPlaces, baseDecimalPlaces));

WebKit coding guidelines require c++ casts – personally I prefer C style casts for numeric code, but this should be sticking to the rules.
Also, in this case, we could probably ditch most of these casts anyway – we normally write (double)10 as 10.0 etc, and (unsigned)16 can just be 16u.
These casts should really be fixed, so r- I'm afraid.


One last thought looking at this code, just wondering if this is another bug in applyStep (this would be a bug in the existing code too, not an issue specific to your patch).

We're doing the minimum/maximum bounds checks before we adjust to ensure we're at an integer multiple of steps.  Could this be an error?
I'm thinking, could we end up in a situation something like:
  base = 0;
  value = 8.7;
  step = 1;
  maximum = 9.9
(I assume this is already in an invalid state, since value is not equal to base + step * INTEGER, but I'm also assuming it is possible to get into such states, since otherwise I don't think we'd need the code to snap back to a multiple of steps).

If we called stepUp from this state we'd increment value, giving a new value of 9.7.  This would pass the maximum bounds check, so no error would be generated.  We'd then adjust to snap to an integer multiple of steps, and round off new value to 10, above the maximum, and not catch this.

Think there's any danger this could be a real error? – I'm thinking maybe it would be safer to sink the min/max checks to below the code to snap to multiples on step, to prevent any risk of this.
What do you think?

cheers,
G.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.