[Webkit-unassigned] [Bug 18994] LANG/LC_ALL influences the result of element.style.opacity

[bugzilla-daemon at webkit.org]

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


Darin Adler <darin at apple.com> changed:

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




--- Comment #67 from Darin Adler <darin at apple.com>  2009-10-12 11:58:35 PDT ---
(From update of attachment 41052)
Looks good.

> +    // For compatibility with existing layout tests, we target 6 digits of precision.

This seems strange to me, even though we've been discussing this one back and
forth. I hope that eventually we can make a choice based on something other
than "layout tests". Perhaps the best way to word the comment would be to say
"compatibility with what was returned by older versions of WebKit"?

> +    const int digits = 6;

This name is too terse. Maybe it should be digitsAfterDecimalPoint?

> +    long long rounded = round(fabs(value) * 1000000.0);

The best function to use to round a double and put the result in a long long
would be llround, not round.

What makes long long big enough? If you are trying to avoid overflow, long long
is not sufficient for that. There are definitely values that fit in a double
that are too large to fit in a long long. How does this handle super-large
numbers? Do we have test cases that cover that?

> +    if (rounded == 0) {
> +      vector.append('0');
> +      return;
> +    }

Need to indent by four spaces here, not two.

> +    char buf[80];
> +    int length = snprintf(buf, sizeof(buf), "%lld", rounded);

Since snprintf has rather poor performance, it would be better to use some
other code here to convert the integer to a string. Functions like
UString::from in JavaScriptCore do this job, and I'd like to see this use a
similar function in the String class rather than relying on snprintf.

> +    // Reserve an estimate of space for the number of digits we anticipate
> +    // along with a minus sign/initial zero/decimal point.

That sounds like length + 3.

> +    vector.reserveCapacity(vector.size() + 2 + length);

And this says 2 + length. Why doesn't the code match the comment?

I'd like to see test cases for the edge cases of this code. I worry that this
will do the wrong thing for extremely large and extremely small numbers -- test
cases could make it clear that the behavior hasn't changed, or that it has
improved.

I'm going to say review- because I'd at least like you to rename "digits", but
please consider the rest of the comments too.

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