Serialize <number> to round to at most 6 decimals

Serialize <number> to round to at most 6 decimals

Serialize <number> to round to at most 6 decimals and
don't serialize trailing zeroes.
Rebase test results to reflect this change in behavior.

Behavior matches Firefox.

BUG=453288
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=195702

[rwlbuis, 2015-05-05 02:21:31]

rob.buis@samsung.com changed reviewers:
+ rune@opera.com

[rwlbuis, 2015-05-05 02:21:31]

PTAL

[Erik Dahlström (inactive), 2015-05-05 08:34:11]

ed@opera.com changed reviewers:
+ ed@opera.com

[Erik Dahlström (inactive), 2015-05-05 08:34:12]

I really like this change, it aligns with how svg numbers/lengths are
serialized, which should hopefully mean fewer LayoutTests needing to be changed
if/when svg starts using cssprimitivevalue for representing length values.

Not an owner, but lgtm from me in any case.

https://codereview.chromium.org/1120133004/diff/80001/LayoutTests/imported/cs...
File
LayoutTests/imported/csswg-test/css-shapes-1/shape-outside/values/support/parsing-utils.js
(right):

https://codereview.chromium.org/1120133004/diff/80001/LayoutTests/imported/cs...
LayoutTests/imported/csswg-test/css-shapes-1/shape-outside/values/support/parsing-utils.js:370:
if (number < 1000) {
indentation looks odd. 

Can you explain why this change is necessary?

[rune, 2015-05-05 11:35:50]

rune@opera.com changed reviewers:
+ timloh@chromium.org

[rune, 2015-05-05 11:35:51]

I see that this actually matches Gecko. Did you check their source code to see
that %.6g is actually what they use?

It's not exactly what the CSSOM spec says, I think. Doesn't it say 6 digits
after comma, not in total?

https://codereview.chromium.org/1120133004/diff/80001/LayoutTests/css-parser/...
File LayoutTests/css-parser/scientific-notation.html (right):

https://codereview.chromium.org/1120133004/diff/80001/LayoutTests/css-parser/...
LayoutTests/css-parser/scientific-notation.html:22:
assert_valid_value("opacity", "1e+10", "1e+10");
The CSSOM spec says: "Note: scientific notation is not used." for <number>.

[rwlbuis, 2015-05-05 15:04:25]

On 2015/05/05 11:35:51, rune wrote:
> I see that this actually matches Gecko. Did you check their source code to see
> that %.6g is actually what they use?

I think they use AppendCSSNumber in nsStyleUtil.h, which ends up in
nsTSubstring_CharT::AppendFloat, which ends up using the same dtoa functionality
we use in wtf.

> It's not exactly what the CSSOM spec says, I think. Doesn't it say 6 digits
> after comma, not in total?

It is not really clear to me from the wording in:
http://dev.w3.org/csswg/cssom/#serializing-css-values

So I was going by the Firefox observed behavior:
document.body.style.opacity = 1101.43453454534
1101.43453454534
document.body.style.opacity
"1101.43"

>
https://codereview.chromium.org/1120133004/diff/80001/LayoutTests/css-parser/...
> File LayoutTests/css-parser/scientific-notation.html (right):
> 
>
https://codereview.chromium.org/1120133004/diff/80001/LayoutTests/css-parser/...
> LayoutTests/css-parser/scientific-notation.html:22:
> assert_valid_value("opacity", "1e+10", "1e+10");
> The CSSOM spec says: "Note: scientific notation is not used." for <number>.

I am not sure that is practical for big numbers.

Hmm, maybe we need some spec people weighing in :)

[rwlbuis, 2015-05-05 15:06:48]

On 2015/05/05 08:34:12, Erik Dahlström wrote:
> I really like this change, it aligns with how svg numbers/lengths are
> serialized, which should hopefully mean fewer LayoutTests needing to be
changed
> if/when svg starts using cssprimitivevalue for representing length values.

I did a quick grep. Did you end up using String::number rather than
String::format?

> Not an owner, but lgtm from me in any case.

Thanks!

>
https://codereview.chromium.org/1120133004/diff/80001/LayoutTests/imported/cs...
> File
>
LayoutTests/imported/csswg-test/css-shapes-1/shape-outside/values/support/parsing-utils.js
> (right):
> 
>
https://codereview.chromium.org/1120133004/diff/80001/LayoutTests/imported/cs...
>
LayoutTests/imported/csswg-test/css-shapes-1/shape-outside/values/support/parsing-utils.js:370:
> if (number < 1000) {
> indentation looks odd. 
> 
> Can you explain why this change is necessary?

Maybe later, since possibly I took the wrong approach in this patch set :)

[Erik Dahlström (inactive), 2015-05-05 15:30:32]

On 2015/05/05 15:06:48, rwlbuis wrote:
> On 2015/05/05 08:34:12, Erik Dahlström wrote:
> > I really like this change, it aligns with how svg numbers/lengths are
> > serialized, which should hopefully mean fewer LayoutTests needing to be
> changed
> > if/when svg starts using cssprimitivevalue for representing length values.
> 
> I did a quick grep. Did you end up using String::number rather than
> String::format?

Ah, right, so it's still not exactly the same :/ Ideally I'd want them to match
precisely, but if that's not possible then at least getting the decimals handled
the same would go a long way.

[rune, 2015-05-05 20:00:09]

The spec author simonp@opera.com is on paternity leave. timloh@ usually have
more opinions than me about css value serializations. Aligning with Gecko sgtm.

[rwlbuis, 2015-05-05 22:02:37]

On 2015/05/05 20:00:09, rune wrote:
> The spec author mailto:simonp@opera.com is on paternity leave. timloh@ usually
have
> more opinions than me about css value serializations. Aligning with Gecko
sgtm.

Thanks Rune. So summarizing the two big questions seem to be:
- do we round with at most 6 digits overall or after '.'
- do we serialize to scientific number format

[Timothy Loh, 2015-05-06 01:55:16]

On 2015/05/05 22:02:37, rwlbuis wrote:
> On 2015/05/05 20:00:09, rune wrote:
> > The spec author mailto:simonp@opera.com is on paternity leave. timloh@
usually
> have
> > more opinions than me about css value serializations. Aligning with Gecko
> sgtm.
> 
> Thanks Rune. So summarizing the two big questions seem to be:
> - do we round with at most 6 digits overall or after '.'
> - do we serialize to scientific number format

Scientific notation should be fine here (we checked with tabatkins@). We should
fix up the tests which are now saying FAIL, but otherwise this seems fine to me.

[zcorpan, 2015-05-06 17:48:41]

On 2015/05/05 22:02:37, rwlbuis wrote:
> - do we round with at most 6 digits overall or after '.'

Should be after the '.' per spec. "rounding the value if necessary to not
produce more than 6 *decimals*"

> - do we serialize to scientific number format

No, per spec. (But that seems like a separate issue.)

Not serializing sci-not is intentional for two, arguably pretty weak, reasons:
1) Really small numbers would show rounding errors from float/double conversion
with sci-not.
2) Legacy parsers don't support sci-not, so it's slightly safer for a serializer
to avoid it.

[rwlbuis, 2015-05-06 21:46:56]

On 2015/05/06 17:48:41, zcorpan wrote:
> On 2015/05/05 22:02:37, rwlbuis wrote:
> > - do we round with at most 6 digits overall or after '.'
> 
> Should be after the '.' per spec. "rounding the value if necessary to not
> produce more than 6 *decimals*"

Thank you, to me that makes more sense than the other option.

> > - do we serialize to scientific number format
> 
> No, per spec. (But that seems like a separate issue.)
> 
> Not serializing sci-not is intentional for two, arguably pretty weak, reasons:
> 1) Really small numbers would show rounding errors from float/double
conversion
> with sci-not.
> 2) Legacy parsers don't support sci-not, so it's slightly safer for a
serializer
> to avoid it.

Ok.

[rwlbuis, 2015-05-06 21:56:13]

@timloh please have a look at patchset #6, it implements zcorpan's suggestion
(note that it does not match FireFox so I removed that from the commit message).

[rwlbuis, 2015-05-07 18:42:29]

On 2015/05/06 21:56:13, rwlbuis wrote:
> @timloh please have a look at patchset #6, it implements zcorpan's suggestion
> (note that it does not match FireFox so I removed that from the commit
message).

I tested with new IE(Microsoft Edge) and it matches Firefox, meaning rounding
with at most 6 digits overall. So for compatibility that may be the best option
for us too.

[Timothy Loh, 2015-05-11 00:03:56]

On 2015/05/07 18:42:29, rwlbuis wrote:
> On 2015/05/06 21:56:13, rwlbuis wrote:
> > @timloh please have a look at patchset #6, it implements zcorpan's
suggestion
> > (note that it does not match FireFox so I removed that from the commit
> message).
> 
> I tested with new IE(Microsoft Edge) and it matches Firefox, meaning rounding
> with at most 6 digits overall. So for compatibility that may be the best
option
> for us too.

Can we just do that then? Serializing 1e-10 to 0 seems bad :(

[rwlbuis, 2015-05-11 20:39:03]

On 2015/05/11 00:03:56, Timothy Loh wrote:
> On 2015/05/07 18:42:29, rwlbuis wrote:
> > On 2015/05/06 21:56:13, rwlbuis wrote:
> > > @timloh please have a look at patchset #6, it implements zcorpan's
> suggestion
> > > (note that it does not match FireFox so I removed that from the commit
> > message).
> > 
> > I tested with new IE(Microsoft Edge) and it matches Firefox, meaning
rounding
> > with at most 6 digits overall. So for compatibility that may be the best
> option
> > for us too.
> 
> Can we just do that then? Serializing 1e-10 to 0 seems bad :(

Sure, patch set 7 implements this.

[Timothy Loh, 2015-05-18 04:32:02]

Sorry for the delay :(

This is fine aside from a couple of tests.

https://codereview.chromium.org/1120133004/diff/120001/LayoutTests/fast/css/l...
File LayoutTests/fast/css/large-numbers-expected.txt (right):

https://codereview.chromium.org/1120133004/diff/120001/LayoutTests/fast/css/l...
LayoutTests/fast/css/large-numbers-expected.txt:9: FAIL element.width =
100000000px, returns offsetWidth 33554428, rect.width 33554428 and computed
width 33554400, expected 33554428.
Should say PASS if this is working as intended

https://codereview.chromium.org/1120133004/diff/120001/LayoutTests/imported/c...
File
LayoutTests/imported/csswg-test/css-shapes-1/shape-outside/values/support/parsing-utils.js
(right):

https://codereview.chromium.org/1120133004/diff/120001/LayoutTests/imported/c...
LayoutTests/imported/csswg-test/css-shapes-1/shape-outside/values/support/parsing-utils.js:370:
if (number < 1000) {
We shouldn't be changing this file, see LayoutTests/imported/README

[rwlbuis, 2015-05-18 20:01:27]

Patchset #8 (id:140001) has been deleted

[rwlbuis, 2015-05-18 23:19:11]

On 2015/05/18 04:32:02, Timothy Loh wrote:
> Sorry for the delay :(
> 
> This is fine aside from a couple of tests.
> 
>
https://codereview.chromium.org/1120133004/diff/120001/LayoutTests/fast/css/l...
> File LayoutTests/fast/css/large-numbers-expected.txt (right):
> 
>
https://codereview.chromium.org/1120133004/diff/120001/LayoutTests/fast/css/l...
> LayoutTests/fast/css/large-numbers-expected.txt:9: FAIL element.width =
> 100000000px, returns offsetWidth 33554428, rect.width 33554428 and computed
> width 33554400, expected 33554428.
> Should say PASS if this is working as intended

I fixed it by splitting up the tests into normal and computed widths.

>
https://codereview.chromium.org/1120133004/diff/120001/LayoutTests/imported/c...
> File
>
LayoutTests/imported/csswg-test/css-shapes-1/shape-outside/values/support/parsing-utils.js
> (right):
> 
>
https://codereview.chromium.org/1120133004/diff/120001/LayoutTests/imported/c...
>
LayoutTests/imported/csswg-test/css-shapes-1/shape-outside/values/support/parsing-utils.js:370:
> if (number < 1000) {
> We shouldn't be changing this file, see LayoutTests/imported/README

Sure, I added the expected (fail) result to the repo.

[Timothy Loh, 2015-05-19 00:26:18]

lgtm

https://codereview.chromium.org/1120133004/diff/160001/LayoutTests/imported/c...
File
LayoutTests/imported/csswg-test/css-shapes-1/shape-outside/values/shape-margin-002.html
(right):

https://codereview.chromium.org/1120133004/diff/160001/LayoutTests/imported/c...
LayoutTests/imported/csswg-test/css-shapes-1/shape-outside/values/shape-margin-002.html:20:
"expected_inline": "10.1235px",
This file looks imported too, I guess we shouldn't change it either

[rwlbuis, 2015-05-19 13:39:54]

The CQ bit was checked by rob.buis@samsung.com

[rwlbuis, 2015-05-19 13:39:55]

The patchset sent to the CQ was uploaded after l-g-t-m from ed@opera.com,
timloh@chromium.org
Link to the patchset: https://codereview.chromium.org/1120133004/#ps180001
(title: "Patch for landing")

[commit-bot: I haz the power, 2015-05-19 13:40:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1120133004/180001

[commit-bot: I haz the power, 2015-05-19 16:14:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-05-19 16:14:44]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/63069)

[rwlbuis, 2015-05-19 16:20:01]

The CQ bit was checked by rob.buis@samsung.com

[commit-bot: I haz the power, 2015-05-19 16:20:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1120133004/180001

[rwlbuis, 2015-05-19 18:58:33]

The CQ bit was checked by rob.buis@samsung.com

[rwlbuis, 2015-05-19 18:58:34]

The patchset sent to the CQ was uploaded after l-g-t-m from timloh@chromium.org,
ed@opera.com
Link to the patchset: https://codereview.chromium.org/1120133004/#ps200001
(title: "Fix win results")

[commit-bot: I haz the power, 2015-05-19 18:58:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1120133004/200001

[commit-bot: I haz the power, 2015-05-19 20:12:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-05-19 20:12:39]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/6...)

[rwlbuis, 2015-05-19 21:23:17]

Patchset #10 (id:200001) has been deleted

[rwlbuis, 2015-05-19 21:56:21]

The CQ bit was checked by rob.buis@samsung.com

[rwlbuis, 2015-05-19 21:56:22]

The patchset sent to the CQ was uploaded after l-g-t-m from timloh@chromium.org,
ed@opera.com
Link to the patchset: https://codereview.chromium.org/1120133004/#ps220001
(title: "Fix win+linux tests")

[commit-bot: I haz the power, 2015-05-19 21:56:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1120133004/220001

[commit-bot: I haz the power, 2015-05-19 22:04:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-05-19 22:04:54]

Try jobs failed on following builders:
  blink_presubmit on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/3...)

[rwlbuis, 2015-05-19 22:13:40]

Patchset #10 (id:220001) has been deleted

[rwlbuis, 2015-05-19 22:14:03]

The CQ bit was checked by rob.buis@samsung.com

[rwlbuis, 2015-05-19 22:14:04]

The patchset sent to the CQ was uploaded after l-g-t-m from timloh@chromium.org,
ed@opera.com
Link to the patchset: https://codereview.chromium.org/1120133004/#ps240001
(title: "Fix win + linux test")

[commit-bot: I haz the power, 2015-05-19 22:15:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1120133004/240001

[rwlbuis, 2015-05-20 19:29:46]

The CQ bit was checked by rob.buis@samsung.com

[rwlbuis, 2015-05-20 19:29:47]

The patchset sent to the CQ was uploaded after l-g-t-m from timloh@chromium.org,
ed@opera.com
Link to the patchset: https://codereview.chromium.org/1120133004/#ps260001
(title: "Patch for landing II")

[commit-bot: I haz the power, 2015-05-20 19:30:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1120133004/260001

[commit-bot: I haz the power, 2015-05-20 20:16:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-05-20 20:16:29]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/6...)

[rwlbuis, 2015-05-20 20:50:30]

The CQ bit was checked by rob.buis@samsung.com

[commit-bot: I haz the power, 2015-05-20 20:50:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1120133004/260001

[commit-bot: I haz the power, 2015-05-20 21:21:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-05-20 21:21:57]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/6...)

[rwlbuis, 2015-05-20 21:32:49]

The CQ bit was checked by rob.buis@samsung.com

[commit-bot: I haz the power, 2015-05-20 21:33:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1120133004/260001

[commit-bot: I haz the power, 2015-05-20 23:02:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-05-20 23:02:54]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/6...)

[rwlbuis, 2015-05-21 01:15:09]

On 2015/05/20 23:02:54, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   linux_blink_rel on tryserver.blink (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/6...)

I have no idea how to fix this mess:

- win needs a special result
- after adding that result, linux will pick it up since it prefers win over no
result
- if I add a platform/linux result I run into a presubmit rule:
* The following files are passing testharness results without console error
messages, they should be removed:

/home/rwlbuis/work/src/third_party/WebKit/LayoutTests/platform/linux/css-parser/scientific-notation-expected.txt
- adding a generic result has no use since the linux case will still prefer the
win result

Maybe I am missing something and somebody with more experience here knows how to
fix it. So far I suspect the presubmit check is at error, it should warn me, not
block me.

[rwlbuis, 2015-05-21 15:26:12]

The CQ bit was checked by rob.buis@samsung.com

[rwlbuis, 2015-05-21 15:26:13]

The patchset sent to the CQ was uploaded after l-g-t-m from timloh@chromium.org,
ed@opera.com
Link to the patchset: https://codereview.chromium.org/1120133004/#ps280001
(title: "Try to fix win")

[commit-bot: I haz the power, 2015-05-21 15:26:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1120133004/280001

[rwlbuis, 2015-05-21 15:26:44]

On 2015/05/21 01:15:09, rwlbuis wrote:
> On 2015/05/20 23:02:54, I haz the power (commit-bot) wrote:
> > Try jobs failed on following builders:
> >   linux_blink_rel on tryserver.blink (JOB_FAILED,
> >
>
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/6...)
> 
> I have no idea how to fix this mess:
> 
> - win needs a special result
> - after adding that result, linux will pick it up since it prefers win over no
> result
> - if I add a platform/linux result I run into a presubmit rule:
> * The following files are passing testharness results without console error
> messages, they should be removed:
> 
>
/home/rwlbuis/work/src/third_party/WebKit/LayoutTests/platform/linux/css-parser/scientific-notation-expected.txt
> - adding a generic result has no use since the linux case will still prefer
the
> win result
> 
> Maybe I am missing something and somebody with more experience here knows how
to
> fix it. So far I suspect the presubmit check is at error, it should warn me,
not
> block me.

Nm, turned out Windows problem could be fixed.

[commit-bot: I haz the power, 2015-05-21 16:54:36]

Committed patchset #12 (id:280001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=195702