[webkit-reviews] review denied: [Bug 18994] LANG/LC_ALL influences the result of element.style.opacity : [Attachment 41052] stringify CSS units manually to match CSS spec

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Evan Martin <evan at chromium.org>'s
request for review:
Bug 18994: LANG/LC_ALL influences the result of element.style.opacity
https://bugs.webkit.org/show_bug.cgi?id=18994

Attachment 41052: stringify CSS units manually to match CSS spec
https://bugs.webkit.org/attachment.cgi?id=41052&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.