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

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted 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 41061: stringify CSS units manually to match CSS spec
https://bugs.webkit.org/attachment.cgi?id=41061&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> -FAIL: read 90010000px back as 9.001e+07px, read again as 0px
> +PASS: read 90010000px back as 90010000px, read again as 90010000px

This succeeds only because the large number isn't large enough, right? With a
still larger number we'd have some other problem. I'd like to have a test
showing that we still fail with even larger numbers.

>  This test verifies that reading a floating-point opacity from CSS attributes
gets back a properly-formatted float. Improperly handling locales that cause
decimals to be written as commas might break it.
>  
>  PASS
> +PASS
> +PASS
> +PASS
> +PASS
> +PASS
> +PASS
> +PASS
> +PASS
> +PASS
> +PASS
> +PASS
> +PASS
> +PASS

In the future, you should use the script-tests approach to build tests like
this. This style of tests that write out more about what they're testing and
why they pass that you get automatically from that are superior. A good example
of how to do this sort of test is
LayoutTests/fast/dom/HTMLFormElement/script-tests/elements-not-in-document.js
and you may be able to see from that how this test could have been done that
way.

> From the printf docs %.6g means you show up to 6 significant digits, but then

> somehow it doesn't happen when your number very is close to zero and they
also
> arbitrarily chop off trailing digits if they are 0 even when they are
> significant.	This patch passes all the layout tests, but I had to change the

> precision on one, but I find it hard to care too much about it.

As I understand it, 0.00000123456 is an example with 6 significant digits.
Maybe you misunderstood what it meant. But exactly matching "%.6lg" isn't the
point. Matching what we used to do is a good idea, but matching other browsers
or matching specifications are even more important.

I think at this point the patch is OK. Here are areas for future improvement
and refinement:

    - check on how other browsers handle formatting fractional values in CSS;
how many digits of precision, etc.
    - figure out if we want to stick with the 6-digits-of-precision thing
forever, or if we should have a different formatting rule

    - eliminate the use of snprintf -- as I said, JavaScriptCore has a function
with the relevant code already, JSC::UString::from, so you could just port that
to live in String.cpp

    - change the test to use script-tests for better clarity
    - add quite a few more kinds of numbers to the test cases
    - test all the different types of units -- a test should fail if we made
any mistakes in the switch statements

r=me