Bug 93327 – style->fontMetrics() should be available when setting line-height

• Status: RESOLVED
• Resolution: FIXED
• Component: Canvas

[Kenichi Ishibashi]

See http://crbug.com/137842 and https://bugs.webkit.org/show_bug.cgi?id=66291. Looks like r96122 wasn't enough. We also need to make sure that fontMetrics are available when setting line-height.

[Kenichi Ishibashi]

Created attachment 156843 [details]
Patch

[Kenichi Ishibashi]

Hi Julien,

Could you take a look?

[Julien Chaffraix]

> Could you take a look?

I am not super familiar with the whole logic but can take a look. My main concern is that this is a whack-a-mole game - that I perpetuated - that we are not winning. It should be time to re-think the way we handle fonts and do something saner.

[Darin Adler]

Comment on attachment 156843 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=156843&action=review

> Source/WebCore/ChangeLog:10
> +        Setting line-height assumes the fontMetrics are available for the affected font, but
> +        the fontMetrics won't be available immediately after setting other properties like
> +        font-size. Call styleResolver->updateFont() before setting line-height to update fontMetrics.

What is the performance effect of this second updateFont call? Do we need to make additional changes to keep performance of the canvas text feature acceptable? How do we compare to other browser engines on performance of selecting multiple fonts and drawing them with canvas?

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2019
>      // The updateFont() call below updates the fontMetrics and ensures the proper setting of font-size.

Please update this sentence too, since the words “call” and “updates” should be changed since there are not multiple updateFont calls.

[Kenichi Ishibashi]

Created attachment 157352 [details]
Patch

[Kenichi Ishibashi]

(In reply to comment #3)
> > Could you take a look?
> 
> I am not super familiar with the whole logic but can take a look. My main concern is that this is a whack-a-mole game - that I perpetuated - that we are not winning. It should be time to re-think the way we handle fonts and do something saner.

I agree, but it will take time and this is a crash bug so I'd like to land the patch at this time.

I'm not familiar with CSS implementation so I don't have good idea to handle this problem, but if you (or CSS area reviewers) can give me some advices, I can work on this.

[Kenichi Ishibashi]

Thank you for review, Darin.

(In reply to comment #4)
> What is the performance effect of this second updateFont call? Do we need to make additional changes to keep performance of the canvas text feature acceptable? How do we compare to other browser engines on performance of selecting multiple fonts and drawing them with canvas?

I did quick performance test with the below code:

<canvas id="canvas"></canvas>
<div id="result"></div>
<script>
if (window.testRunner)
    testRunner.dumpAsText();

var ctx = document.getElementById('canvas').getContext('2d');
function test() {
    var start = Date.now();
    for (var i = 0; i < 100000; ++i)
        ctx.font = "G";
    var end = Date.now();
    return end - start;
}

var total = 0;
for (var i = 0; i < 10; ++i)
    total += test();

document.getElementById('result').innerHTML = '' + (total / 10);
</script>

Here is the results of chromium linux port release build DRT on my machine. Results aren't stable so I checked 10 times.

Not applied
398.4
234.4
380.3
243.3
231.2
380.4
377.2
238.1
394.8
351.4
404.7

Applied
408.7
398.5
425.3
380.4
237.1
240.9
240.8
233.7
396.6
241
243.4

I think the performance effect of this patch isn't significant. I also ran the test on Firefox 14 several times. It takes 800+ms every time.

> Please update this sentence too, since the words “call” and “updates” should be changed since there are not multiple updateFont calls.

Done.

[Kenichi Ishibashi]

Hi Darin and Julien,

Could you take another look?

[Kenichi Ishibashi]

(In reply to comment #8)
> Hi Darin and Julien,
> 
> Could you take another look?

Ping agein. Darin, Julien?

[WebKit Review Bot]

Comment on attachment 157352 [details]
Patch

Clearing flags on attachment: 157352

Committed r126959: <http://trac.webkit.org/changeset/126959>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.