Bug 216601

<rdar://problem/69022887>

Created attachment 414781 [details] Patch

Created attachment 414782 [details] Patch

Created attachment 414914 [details] Patch

Thanks for the review, Simon! Just rebased the patch, will mark cq? when the tests are green again.

Comment on attachment 414914 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=414914&action=review > LayoutTests/imported/w3c/web-platform-tests/css/css-fonts/parsing/font-computed-expected.txt:10 > -FAIL Property font value 'normal normal xx-large/1.2 cursive' assert_equals: expected "32px / 38px cursive" but got "normal normal normal normal 32px/38px cursive" > +FAIL Property font value 'normal normal xx-large/1.2 cursive' assert_equals: expected "32px / 38.400001525878906px cursive" but got "normal normal normal normal 32px/38.400001525878906px cursive" We appear to be failing these WPT tests harder than before?

(In reply to Antti Koivisto from comment #6) > Comment on attachment 414914 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=414914&action=review > > > LayoutTests/imported/w3c/web-platform-tests/css/css-fonts/parsing/font-computed-expected.txt:10 > > -FAIL Property font value 'normal normal xx-large/1.2 cursive' assert_equals: expected "32px / 38px cursive" but got "normal normal normal normal 32px/38px cursive" > > +FAIL Property font value 'normal normal xx-large/1.2 cursive' assert_equals: expected "32px / 38.400001525878906px cursive" but got "normal normal normal normal 32px/38.400001525878906px cursive" > > We appear to be failing these WPT tests harder than before? How so? With this patch, both the expectations and results for line-height values have been updated to be fractional. Both Gecko and Chromium also serialize these fractionally for this test. Maybe I'm missing your point, though -- can you elaborate? wpt.fyi: https://wpt.fyi/results/css/css-fonts/parsing/font-computed.html?label=master&label=experimental&aligned&q=css%2Fcss-fonts Test source: https://github.com/web-platform-tests/wpt/blob/master/css/css-fonts/parsing/font-computed.html

Committed r270248: <https://trac.webkit.org/changeset/270248> All reviewed patches have been landed. Closing bug and clearing flags on attachment 414914 [details].

> How so? With this patch, both the expectations and results for line-height > values have been updated to be fractional. Both Gecko and Chromium also > serialize these fractionally for this test. Maybe I'm missing your point, > though -- can you elaborate? Based on the output the expected value in the test is 32px/38px which is what we were producing before this patch. Are you saying that both Chrome and Gecko now fail the same way as we do? Or that our version of the test is out of date? If the latter, you should update it.

(In reply to Antti Koivisto from comment #9) > Based on the output the expected value in the test is 32px/38px which is what we were producing before this patch. The expected value in this test is not hardcoded, it is generated by a "reference" style vs. the "under-test" style. So it's not like this: assert_equals('...32px / 38px...', 'actual value') and is instead like this (pseudocode): assert_equals(getComputedStyle(reference)['font'], getComputedStyle(actual)['font]) So by changing line-height to serialize as a double instead of an int, we change both the reference-expectation and the actual value to a fractional value. > Are you saying that both Chrome and Gecko now fail the same way as we do? Using the case quoted above as an example: WebKit before this patch: 32px / 38px WebKit after this patch: 32px / 38.400001525878906px Gecko: 32px / 38.4px Chromium: 32px / 38.4px Point being, we now represent line-height fractionally as others do. Chromium passes this case, and Gecko fails for unrelated reasons. > Or that our version of the test is out of date? Our version of the test is indeed the latest, so no concerns there. --- Does this clear it up, or am I still missing your point?

> Does this clear it up, or am I still missing your point? Yep, that makes sense. Thanks for explaining.