|
|
|
In order to match Windows font metrics on Linux with Chromium, several additional font metrics needed to be extracted from the TrueType files.
This patch is upstreaming those changes.
Patch Set 1 #Patch Set 2 : ... #
Total comments: 17
Patch Set 3 : ... #
Total messages: 3
|
agl
|
16 years, 9 months ago (2009年03月30日 20:21:35 UTC) #1 | |||||||||||||||||||||||||||||||||||||||||||||||
In general I think the additions are fine, but its hard to know for sure w/o seeing how the new fields are used. Since this represents both an addition to the public API and an addition to the porting layer requirements, its important that the additions be necessary, minimal, and clear (to future backend implementors). http://codereview.appspot.com/28145/diff/7/1006 File include/core/SkPaint.h (right): http://codereview.appspot.com/28145/diff/7/1006#newcode616 Line 616: SkScalar fLeading; //!< The recommended distance to add between lines of text (will be >= 0) Does fHeight == fDescent - fAscent + fLeading? If so, do we need it? If not, how do we explain it? http://codereview.appspot.com/28145/diff/7/1006#newcode618 Line 618: SkScalar fAvgCharWidth; //!< the average charactor width (>= 0) Not sure how this is useful if we don't know the xmin and/or xmax. If we are going to add this data, I think I'd prefer fXMin and fXMax (or better names since we have H and V versions of this struct. http://codereview.appspot.com/28145/diff/7/1006#newcode624 Line 624: These last three are a little wacky, since they may not reflect what our font scaler actually computes (since they were built off of the windows rasterizer). Are they really necessary to add? http://codereview.appspot.com/28145/diff/7/1007 File src/ports/SkFontHost_FreeType.cpp (right): http://codereview.appspot.com/28145/diff/7/1007#newcode319 Line 319: case kNormal_Hints: Good addition. This should have been here from the beginning. http://codereview.appspot.com/28145/diff/7/1007#newcode721 Line 721: // ----------------------------------------------------------------------------- Has this file name changed? http://codereview.appspot.com/28145/diff/7/1007#newcode724 Line 724: // ----------------------------------------------------------------------------- Should we create a formal header file for this function? http://codereview.appspot.com/28145/diff/7/1007#newcode806 Line 806: ys[4] = leading; How is this height different from ascent+descent+leading? If it is different, that would be weird. http://codereview.appspot.com/28145/diff/7/1007#newcode810 Line 810: SkScalar x_height; why aren't we using another ys[] slot for xheight? http://codereview.appspot.com/28145/diff/7/1010 File src/ports/SkFontHost_TrueType_Tables.cpp (right): http://codereview.appspot.com/28145/diff/7/1010#newcode44 Line 44: // ----------------------------------------------------------------------------- Are we in danger of collisions with another class by this name? perhaps we should uglify the name (e.g. TrueTypeTableBufferUtil) or use some local namespace to reduce that chance.
http://codereview.appspot.com/28145/diff/7/1006 File include/core/SkPaint.h (right): http://codereview.appspot.com/28145/diff/7/1006#newcode616 Line 616: SkScalar fLeading; //!< The recommended distance to add between lines of text (will be >= 0) On 2009年04月07日 16:40:21, reed wrote: > Does fHeight == fDescent - fAscent + fLeading? If so, do we need it? If not, how > do we explain it? I've removed it. Leading was previously wrong as explained below. http://codereview.appspot.com/28145/diff/7/1006#newcode618 Line 618: SkScalar fAvgCharWidth; //!< the average charactor width (>= 0) On 2009年04月07日 16:40:21, reed wrote: > Not sure how this is useful if we don't know the xmin and/or xmax. If we are > going to add this data, I think I'd prefer fXMin and fXMax (or better names > since we have H and V versions of this struct. I've switched to XMin and XMax (we already have fTop and fBottom). The average width is needed to calculate the width of text boxes in WebKit. http://codereview.appspot.com/28145/diff/7/1007 File src/ports/SkFontHost_FreeType.cpp (right): http://codereview.appspot.com/28145/diff/7/1007#newcode721 Line 721: // ----------------------------------------------------------------------------- On 2009年04月07日 16:40:21, reed wrote: > Has this file name changed? Yes, fixed. thanks. http://codereview.appspot.com/28145/diff/7/1007#newcode724 Line 724: // ----------------------------------------------------------------------------- On 2009年04月07日 16:40:21, reed wrote: > Should we create a formal header file for this function? It seems like overkill for a single function. Esp if we're going to be moving this into our WebKit port anyway. http://codereview.appspot.com/28145/diff/7/1007#newcode793 Line 793: int leading = face->height - face->ascender + face->descender; The reason that I added fHeight was because I couldn't get it to work otherwise. I suspect the problem is missing brackets here. This line is doing leading = (height - ascent) + descent. Fixed and removed fHeight. http://codereview.appspot.com/28145/diff/7/1007#newcode806 Line 806: ys[4] = leading; On 2009年04月07日 16:40:21, reed wrote: > How is this height different from ascent+descent+leading? If it is different, > that would be weird. Removed. See comment on line 793. http://codereview.appspot.com/28145/diff/7/1007#newcode810 Line 810: SkScalar x_height; On 2009年04月07日 16:40:21, reed wrote: > why aren't we using another ys[] slot for xheight? It doesn't work when the font doesn't have xHeight in the OS/2 table, right? We don't want to double translate the result from Get_CBox I believe. http://codereview.appspot.com/28145/diff/7/1010 File src/ports/SkFontHost_TrueType_Tables.cpp (right): http://codereview.appspot.com/28145/diff/7/1010#newcode44 Line 44: // ----------------------------------------------------------------------------- On 2009年04月07日 16:40:21, reed wrote: > Are we in danger of collisions with another class by this name? perhaps we > should uglify the name (e.g. TrueTypeTableBufferUtil) or use some local > namespace to reduce that chance. It's local to the file and we don't have any C++ includes, so I think we're safe.