[webkit-reviews] review denied: [Bug 48308] Too precise serialization from floating point number to string for "number" input elements : [Attachment 72653] Patch

[bugzilla-daemon at webkit.org]

Gavin Barraclough <barraclough at apple.com> has denied Dai Mikurube
<dmikurube at google.com>'s request for review:
Bug 48308: Too precise serialization from floating point number to string for
"number" input elements
https://bugs.webkit.org/show_bug.cgi?id=48308

Attachment 72653: Patch
https://bugs.webkit.org/attachment.cgi?id=72653&action=review

------- Additional Comments from Gavin Barraclough <barraclough at apple.com>
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.