RESOLVED FIXED 98452
Font::glyphDataAndPageForCharacter doesn't account for text orientation when using systemFallback on a cold cache. 
https://bugs.webkit.org/show_bug.cgi?id=98452
Summary Font::glyphDataAndPageForCharacter doesn't account for text orientation when ...
Enrica Casucci
Reported 2012-10-04 14:06:26 PDT
The text orientation is taken into account only when the glyph is already in the cache. Use the attached test case to reproduce this problem, launching it from a new instance of Safari. The line break occurs in the wrong place and you'll see the difference where reloading the page.
Attachments
Test case (320 bytes, text/html)
2012-10-04 14:07 PDT, Enrica Casucci 		no flags
Details
Patch (13.72 KB, patch)
2012-10-04 16:26 PDT, Enrica Casucci 		mitz: review+
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Enrica Casucci
Comment 1 2012-10-04 14:07:29 PDT
Created attachment 167170 [details] Test case
Enrica Casucci
Comment 2 2012-10-04 16:26:27 PDT
Created attachment 167197 [details] Patch
mitz
Comment 3 2012-10-04 17:02:49 PDT
Comment on attachment 167197 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=167197&action=review > Source/WebCore/platform/graphics/FontFastPath.cpp:50 > +static inline std::pair<GlyphData, GlyphPage*> applyTextOrientationForCharacter(UChar32 c, GlyphData& data, GlyphPage* page, unsigned pageNumber, TextOrientation orientation) Please rename “c” to “character”. The verb “apply” doesn’t fit a function that doesn’t mutate anything. How about glyphDataAndPageForCharacterWithTextOrientation(…), perhaps also moving the TextOrientation parameter to the second spot?
Enrica Casucci
Comment 4 2012-10-04 18:33:20 PDT
(In reply to comment #3) > (From update of attachment 167197 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=167197&action=review > > > Source/WebCore/platform/graphics/FontFastPath.cpp:50 > > +static inline std::pair<GlyphData, GlyphPage*> applyTextOrientationForCharacter(UChar32 c, GlyphData& data, GlyphPage* page, unsigned pageNumber, TextOrientation orientation) > > Please rename “c” to “character”. The verb “apply” doesn’t fit a function that doesn’t mutate anything. How about glyphDataAndPageForCharacterWithTextOrientation(…), perhaps also moving the TextOrientation parameter to the second spot? That was the first name I came up with, but then I thought it was too long. I should have followed my instincts :-) thanks for the review, will do as you suggested.
Enrica Casucci
Comment 5 2012-10-04 19:03:34 PDT
Committed revision 130443.
Note You need to log in before you can comment on or make changes to this bug.