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.
Created attachment 156843 [details] Patch
Hi Julien, Could you take a look?
> 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 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.
Created attachment 157352 [details] Patch
(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.
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.
Hi Darin and Julien, Could you take another look?
(In reply to comment #8) > Hi Darin and Julien, > > Could you take another look? Ping agein. Darin, Julien?
Comment on attachment 157352 [details] Patch Clearing flags on attachment: 157352 Committed r126959: <http://trac.webkit.org/changeset/126959>
All reviewed patches have been landed. Closing bug.