Bug 93327 - style->fontMetrics() should be available when setting line-height
Summary: style->fontMetrics() should be available when setting line-height
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Canvas (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kenichi Ishibashi
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-08-06 19:41 PDT by Kenichi Ishibashi
Modified: 2012-08-28 22:10 PDT (History)
3 users (show)

See Also:


Attachments
Patch (3.87 KB, patch)
2012-08-06 19:54 PDT, Kenichi Ishibashi
no flags Details | Formatted Diff | Diff
Patch (4.06 KB, patch)
2012-08-08 17:45 PDT, Kenichi Ishibashi
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kenichi Ishibashi 2012-08-06 19:41:58 PDT
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.
Comment 1 Kenichi Ishibashi 2012-08-06 19:54:19 PDT
Created attachment 156843 [details]
Patch
Comment 2 Kenichi Ishibashi 2012-08-06 19:56:21 PDT
Hi Julien,

Could you take a look?
Comment 3 Julien Chaffraix 2012-08-07 11:27:34 PDT
> 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.
Comment 4 Darin Adler 2012-08-07 14:24:01 PDT
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.
Comment 5 Kenichi Ishibashi 2012-08-08 17:45:45 PDT
Created attachment 157352 [details]
Patch
Comment 6 Kenichi Ishibashi 2012-08-08 17:50:07 PDT
(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.
Comment 7 Kenichi Ishibashi 2012-08-08 17:51:48 PDT
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.
Comment 8 Kenichi Ishibashi 2012-08-12 17:00:51 PDT
Hi Darin and Julien,

Could you take another look?
Comment 9 Kenichi Ishibashi 2012-08-28 18:48:56 PDT
(In reply to comment #8)
> Hi Darin and Julien,
> 
> Could you take another look?

Ping agein. Darin, Julien?
Comment 10 WebKit Review Bot 2012-08-28 22:10:44 PDT
Comment on attachment 157352 [details]
Patch

Clearing flags on attachment: 157352

Committed r126959: <http://trac.webkit.org/changeset/126959>
Comment 11 WebKit Review Bot 2012-08-28 22:10:47 PDT
All reviewed patches have been landed.  Closing bug.