Bug 98452

Summary: Font::glyphDataAndPageForCharacter doesn't account for text orientation when using systemFallback on a cold cache.
Product: WebKit Reporter: Enrica Casucci <enrica>
Component: Layout and RenderingAssignee: Enrica Casucci <enrica>
Status: RESOLVED FIXED    
Severity: Normal CC: gyuyoung.kim, mitz, rakuco, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Test case
none
Patch mitz: review+

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.