[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





--- Comment #69 from Evan Martin <evan at chromium.org>  2009-10-12 12:21:27 PDT ---
(In reply to comment #67)
> (From update of attachment 41052 [details])
> Looks good.

Thank you a ton for your patience and careful reviews.

> 
> > +    // 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"?

Done.

> 
> > +    const int digits = 6;
> 
> This name is too terse. Maybe it should be digitsAfterDecimalPoint?

Done.

> 
> > +    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.

Ah, I wasn't even aware of this function.

> 
> 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?

It is true.  Here's my thinking.
One of the test cases I'm fixing here is "large-number-round-trip", which is an
old test that predates me discovering this bug.  (I checked it in recently
since I discovered it was related.)  It shows that WebKit currently gets
numbers as small as 90010000 (roughly 10e8) wrong.

If you're willing to assume a long long is 64 bits (call it 10e19), and my
multiply-by-10e6 trick eats 6 of that exponent, I'm making the upper bound
around 10e13, which is at least better than where we were before.  Since we're
talking about CSS units, I don't think that's too bad of a bound; the spec is
silent on the bounds of these values.
  http://www.w3.org/TR/CSS2/syndata.html#value-def-number

Using a plain long is 32 bits, or 10e9, which would bound the values to 10e3
after the rounding trick, which is clearly enough.

> > +    if (rounded == 0) {
> > +      vector.append('0');
> > +      return;
> > +    }
> 
> Need to indent by four spaces here, not two.

This is really embarrassing.  I am normally really finicky about this sort of
thing.  My only excuse is that the other code I work on has 2 space tabs stops.

> 
> > +    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.

Oops, I missed this before I uploaded my new patch.  Will investigate.

> > +    // 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?

Overzealous premature optimization (most numbers only have at most two of the
three mentioned characteristics).  Fixed.

> 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.

The opacity-float test changes try to.  Was there anything else you had in
mind?  Or do you mean testing the failure cases -- when the values exceed the
allowed inputs?


I will upload a patch fixing the snprintf issue once I figure out a proper
solution.  (In my defense, the old code used snprintf for floats.)

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