Issue 4174041: Provide windows font host implementation needed to support TrueType text in p... (Closed)
|
Created:
13 years, 4 months ago by Chris Guillory Modified:
13 years, 4 months ago CC:
skia-review_googlegroups.com Base URL:
http://skia.googlecode.com/svn/trunk/ Visibility:
Public. |
DescriptionProvide windows font host implementation needed to support TrueType text in pdf backend.
- Move AdvanceMetric template functions into new file SkAdvancedTypefaceMetrics.cpp
Committed as revision 789.
http://code.google.com/p/skia/source/detail?r=789
Patch Set 1 #
Total comments: 4
Patch Set 2 : '' #Patch Set 3 : Updates from comments. Moved template functions into new file #
Total comments: 23
Patch Set 4 : Some cleanup. Updated with comments from Steve. #
Total comments: 23
Patch Set 5 : Updated with more comments from Steve. #
Total comments: 12
Patch Set 6 : Updated with more comments from Steve. #
Total comments: 7
Patch Set 7 : Minor cleanup. Updated with nit comments from Steve. #
Total comments: 14
Patch Set 8 : Updated with comments from Mike. #
MessagesTotal messages: 16
Sign in to reply to this message.
http://codereview.appspot.com/4174041/diff/1/src/ports/SkFontHost_win.cpp File src/ports/SkFontHost_win.cpp (right): http://codereview.appspot.com/4174041/diff/1/src/ports/SkFontHost_win.cpp#new... src/ports/SkFontHost_win.cpp:505: if (type == SkAdvancedTypefaceMetrics::AdvanceMetric<Data>::kRange) needs { } for the if and else http://codereview.appspot.com/4174041/diff/1/src/ports/SkFontHost_win.cpp#new... src/ports/SkFontHost_win.cpp:560: info->fStyle |= SkAdvancedTypefaceMetrics::kSerif_Style; this series should have { } around the ifs
Sign in to reply to this message.
http://codereview.appspot.com/4174041/diff/1/src/ports/SkFontHost_win.cpp File src/ports/SkFontHost_win.cpp (right): http://codereview.appspot.com/4174041/diff/1/src/ports/SkFontHost_win.cpp#new... src/ports/SkFontHost_win.cpp:505: if (type == SkAdvancedTypefaceMetrics::AdvanceMetric<Data>::kRange) On 2011/02/09 22:27:33, reed1 wrote: > needs { } for the if and else Done. http://codereview.appspot.com/4174041/diff/1/src/ports/SkFontHost_win.cpp#new... src/ports/SkFontHost_win.cpp:560: info->fStyle |= SkAdvancedTypefaceMetrics::kSerif_Style; On 2011/02/09 22:27:33, reed1 wrote: > this series should have { } around the ifs Done.
Sign in to reply to this message.
Sign in to reply to this message.
http://codereview.appspot.com/4174041/diff/3002/include/core/SkAdvancedTypefa... File include/core/SkAdvancedTypefaceMetrics.h (right): http://codereview.appspot.com/4174041/diff/3002/include/core/SkAdvancedTypefa... include/core/SkAdvancedTypefaceMetrics.h:110: template <typename Data> Maybe these should go in a namespace? SkAdvancedTypefaceMetricsUtils? Not sure. http://codereview.appspot.com/4174041/diff/3002/src/ports/SkFontHost_win.cpp File src/ports/SkFontHost_win.cpp (right): http://codereview.appspot.com/4174041/diff/3002/src/ports/SkFontHost_win.cpp#... src/ports/SkFontHost_win.cpp:25: #include <algorithm> Still needed? http://codereview.appspot.com/4174041/diff/3002/src/ports/SkFontHost_win.cpp#... src/ports/SkFontHost_win.cpp:473: // To request design units, create a logical font whose height is specified as unitsPerEm. nit: 80 columns. http://codereview.appspot.com/4174041/diff/3002/src/ports/SkFontHost_win.cpp#... src/ports/SkFontHost_win.cpp:494: delete [] familyName; nit: extra space http://codereview.appspot.com/4174041/diff/3002/src/ports/SkFontHost_win.cpp#... src/ports/SkFontHost_win.cpp:518: // The main italic angle of the font, in tenths of a degree counterclockwise from vertical. nit: 80 columns http://codereview.appspot.com/4174041/diff/3002/src/ports/SkFontHost_win.cpp#... src/ports/SkFontHost_win.cpp:522: // MSDN says otmsCapEmHeight is not support but it is returning a value on my Win7 box. and here http://codereview.appspot.com/4174041/diff/3002/src/ports/SkFontHost_win.cpp#... src/ports/SkFontHost_win.cpp:525: otm.otmrcFontBox.right, otm.otmrcFontBox.bottom); and here http://codereview.appspot.com/4174041/diff/3002/src/ports/SkFontHost_win.cpp#... src/ports/SkFontHost_win.cpp:543: if (otm.otmfsType & 0x1) Does this account for bitmap only embedding? What about a font with only bitmap representations? http://codereview.appspot.com/4174041/diff/3002/src/ports/SkFontHost_win.cpp#... src/ports/SkFontHost_win.cpp:547: SkAdvancedTypefaceMetrics::AdvanceMetric<int16_t>* curRange; How hard would it be to make getAdvanceData() generic and share it? http://codereview.appspot.com/4174041/diff/3002/src/ports/SkFontHost_win.cpp#... src/ports/SkFontHost_win.cpp:559: if(GDI_ERROR == GetGlyphOutline(hdc, gId, GGO_METRICS | GGO_GLYPH_INDEX, &gm, 0, NULL, &mat2)) { nit: 80 columns http://codereview.appspot.com/4174041/diff/3002/src/ports/SkFontHost_win.cpp#... src/ports/SkFontHost_win.cpp:624: void* buffer = sk_malloc_throw(bufferSize); Instead of an explicit malloc here, why not new SkMemoryStream(bufferSize) and GetFontData(... stream.getMemoryBase()...) ?
Sign in to reply to this message.
http://codereview.appspot.com/4174041/diff/3002/include/core/SkAdvancedTypefa... File include/core/SkAdvancedTypefaceMetrics.h (right): http://codereview.appspot.com/4174041/diff/3002/include/core/SkAdvancedTypefa... include/core/SkAdvancedTypefaceMetrics.h:110: template <typename Data> On 2011/02/10 23:22:18, Steve VanDeBogart wrote: > Maybe these should go in a namespace? SkAdvancedTypefaceMetricsUtils? Not sure. Done. skia_advanced_typeface_metrics_utils. http://codereview.appspot.com/4174041/diff/3002/src/ports/SkFontHost_win.cpp File src/ports/SkFontHost_win.cpp (right): http://codereview.appspot.com/4174041/diff/3002/src/ports/SkFontHost_win.cpp#... src/ports/SkFontHost_win.cpp:25: #include <algorithm> On 2011/02/10 23:22:18, Steve VanDeBogart wrote: > Still needed? Nope. Removed. http://codereview.appspot.com/4174041/diff/3002/src/ports/SkFontHost_win.cpp#... src/ports/SkFontHost_win.cpp:473: // To request design units, create a logical font whose height is specified as unitsPerEm. On 2011/02/10 23:22:18, Steve VanDeBogart wrote: > nit: 80 columns. Done. http://codereview.appspot.com/4174041/diff/3002/src/ports/SkFontHost_win.cpp#... src/ports/SkFontHost_win.cpp:494: delete [] familyName; On 2011/02/10 23:22:18, Steve VanDeBogart wrote: > nit: extra space Done. http://codereview.appspot.com/4174041/diff/3002/src/ports/SkFontHost_win.cpp#... src/ports/SkFontHost_win.cpp:518: // The main italic angle of the font, in tenths of a degree counterclockwise from vertical. On 2011/02/10 23:22:18, Steve VanDeBogart wrote: > nit: 80 columns Done. http://codereview.appspot.com/4174041/diff/3002/src/ports/SkFontHost_win.cpp#... src/ports/SkFontHost_win.cpp:522: // MSDN says otmsCapEmHeight is not support but it is returning a value on my Win7 box. On 2011/02/10 23:22:18, Steve VanDeBogart wrote: > and here Done. http://codereview.appspot.com/4174041/diff/3002/src/ports/SkFontHost_win.cpp#... src/ports/SkFontHost_win.cpp:525: otm.otmrcFontBox.right, otm.otmrcFontBox.bottom); On 2011/02/10 23:22:18, Steve VanDeBogart wrote: > and here Done. http://codereview.appspot.com/4174041/diff/3002/src/ports/SkFontHost_win.cpp#... src/ports/SkFontHost_win.cpp:543: if (otm.otmfsType & 0x1) On 2011/02/10 23:22:18, Steve VanDeBogart wrote: > Does this account for bitmap only embedding? What about a font with only bitmap > representations? Unsure. Function documentation doesn't mention bitmap embedding. http://codereview.appspot.com/4174041/diff/3002/src/ports/SkFontHost_win.cpp#... src/ports/SkFontHost_win.cpp:547: SkAdvancedTypefaceMetrics::AdvanceMetric<int16_t>* curRange; On 2011/02/10 23:22:18, Steve VanDeBogart wrote: > How hard would it be to make getAdvanceData() generic and share it? Made it generic. Compiles but need to verify on linux. http://codereview.appspot.com/4174041/diff/3002/src/ports/SkFontHost_win.cpp#... src/ports/SkFontHost_win.cpp:559: if(GDI_ERROR == GetGlyphOutline(hdc, gId, GGO_METRICS | GGO_GLYPH_INDEX, &gm, 0, NULL, &mat2)) { On 2011/02/10 23:22:18, Steve VanDeBogart wrote: > nit: 80 columns Done. http://codereview.appspot.com/4174041/diff/3002/src/ports/SkFontHost_win.cpp#... src/ports/SkFontHost_win.cpp:624: void* buffer = sk_malloc_throw(bufferSize); On 2011/02/10 23:22:18, Steve VanDeBogart wrote: > Instead of an explicit malloc here, why not new SkMemoryStream(bufferSize) and > GetFontData(... stream.getMemoryBase()...) ? Looks like getMemoryBase() returns const void*.
Sign in to reply to this message.
http://codereview.appspot.com/4174041/diff/3002/src/ports/SkFontHost_win.cpp File src/ports/SkFontHost_win.cpp (right): http://codereview.appspot.com/4174041/diff/3002/src/ports/SkFontHost_win.cpp#... src/ports/SkFontHost_win.cpp:624: void* buffer = sk_malloc_throw(bufferSize); On 2011/02/11 01:27:10, Chris Guillory wrote: > On 2011/02/10 23:22:18, Steve VanDeBogart wrote: > > Instead of an explicit malloc here, why not new SkMemoryStream(bufferSize) and > > GetFontData(... stream.getMemoryBase()...) ? > Looks like getMemoryBase() returns const void*. Yea, but this is what the comments in the header suggestion, just const cast it. http://codereview.appspot.com/4174041/diff/13001/include/core/SkAdvancedTypef... File include/core/SkAdvancedTypefaceMetrics.h (right): http://codereview.appspot.com/4174041/diff/13001/include/core/SkAdvancedTypef... include/core/SkAdvancedTypefaceMetrics.h:130: void* param, int num_glyphs, Data (*getAdvance)(void* param, int gId)); void* params aren't great. This is already templatetized, can we just make it another template parameter? http://codereview.appspot.com/4174041/diff/13001/src/core/SkAdvancedTypefaceM... File src/core/SkAdvancedTypefaceMetrics.cpp (right): http://codereview.appspot.com/4174041/diff/13001/src/core/SkAdvancedTypefaceM... src/core/SkAdvancedTypefaceMetrics.cpp:71: Data lastAdvance = -1; Negative advances are possible in some fonts, so a different sentinel value would be better. http://codereview.appspot.com/4174041/diff/13001/src/core/SkAdvancedTypefaceM... src/core/SkAdvancedTypefaceMetrics.cpp:115: // int16_t. We haven't used it yet, but should probably do this for VerticalMetric as well. http://codereview.appspot.com/4174041/diff/13001/src/core/SkAdvancedTypefaceM... src/core/SkAdvancedTypefaceMetrics.cpp:117: template AdvanceMetric* getAdvanceData(void* param, int num_glyphs, This can be platform specific. http://codereview.appspot.com/4174041/diff/13001/src/ports/SkFontHost_win.cpp File src/ports/SkFontHost_win.cpp (right): http://codereview.appspot.com/4174041/diff/13001/src/ports/SkFontHost_win.cpp... src/ports/SkFontHost_win.cpp:471: return -1; A different sentinel here as well. Or you can change the signature: bool getWidthAdvance(HDC, gId, int16_t* advance) http://codereview.appspot.com/4174041/diff/13001/src/ports/SkFontHost_win.cpp... src/ports/SkFontHost_win.cpp:491: SkAssertResult(GetOutlineTextMetrics(hdc, sizeof(otm), &otm)); In release mode SkAsserts don't check anything. Would it be better to return NULL if these fail? http://codereview.appspot.com/4174041/diff/13001/src/ports/SkFontHost_win.cpp... src/ports/SkFontHost_win.cpp:511: info->fFontName.set(lf.lfFaceName); Is this the name of the font or the name of the requested font? http://codereview.appspot.com/4174041/diff/13001/src/ports/SkFontHost_win.cpp... src/ports/SkFontHost_win.cpp:514: SkASSERT(otm.otmTextMetrics.tmPitchAndFamily & TMPF_TRUETYPE); This should be a conditional, setting info->fType to other if the font isn't truetype (for now). http://codereview.appspot.com/4174041/diff/13001/src/ports/SkFontHost_win.cpp... src/ports/SkFontHost_win.cpp:549: char stem_chars[] = {'i', 'I', '!', '1'}; Should init info->fStemV to 0 here. Also a bug in the freetype implementation. http://codereview.appspot.com/4174041/diff/13001/src/ports/SkFontHost_win.cpp... src/ports/SkFontHost_win.cpp:561: // If bit 1 is set, the font may not be embedded in a document. This is kind of strange because TT has more bits than this. I wonder if there's an alternate method that will give you all the bits (see http://freetype.sourceforge.net/freetype2/docs/reference/ft2-base_interface.h... )
Sign in to reply to this message.
http://codereview.appspot.com/4174041/diff/13001/include/core/SkAdvancedTypef... File include/core/SkAdvancedTypefaceMetrics.h (right): http://codereview.appspot.com/4174041/diff/13001/include/core/SkAdvancedTypef... include/core/SkAdvancedTypefaceMetrics.h:130: void* param, int num_glyphs, Data (*getAdvance)(void* param, int gId)); On 2011/02/11 19:01:15, Steve VanDeBogart wrote: > void* params aren't great. This is already templatetized, can we just make it > another template parameter? Done. http://codereview.appspot.com/4174041/diff/13001/src/core/SkAdvancedTypefaceM... File src/core/SkAdvancedTypefaceMetrics.cpp (right): http://codereview.appspot.com/4174041/diff/13001/src/core/SkAdvancedTypefaceM... src/core/SkAdvancedTypefaceMetrics.cpp:71: Data lastAdvance = -1; On 2011/02/11 19:01:15, Steve VanDeBogart wrote: > Negative advances are possible in some fonts, so a different sentinel value > would be better. Using SHRT_MIN as was done previously. http://codereview.appspot.com/4174041/diff/13001/src/core/SkAdvancedTypefaceM... src/core/SkAdvancedTypefaceMetrics.cpp:115: // int16_t. On 2011/02/11 19:01:15, Steve VanDeBogart wrote: > We haven't used it yet, but should probably do this for VerticalMetric as well. You mean move the helpers into this file? http://codereview.appspot.com/4174041/diff/13001/src/core/SkAdvancedTypefaceM... src/core/SkAdvancedTypefaceMetrics.cpp:117: template AdvanceMetric* getAdvanceData(void* param, int num_glyphs, On 2011/02/11 19:01:15, Steve VanDeBogart wrote: > This can be platform specific. Done. http://codereview.appspot.com/4174041/diff/13001/src/ports/SkFontHost_win.cpp File src/ports/SkFontHost_win.cpp (right): http://codereview.appspot.com/4174041/diff/13001/src/ports/SkFontHost_win.cpp... src/ports/SkFontHost_win.cpp:471: return -1; On 2011/02/11 19:01:15, Steve VanDeBogart wrote: > A different sentinel here as well. Or you can change the signature: bool > getWidthAdvance(HDC, gId, int16_t* advance) Done. http://codereview.appspot.com/4174041/diff/13001/src/ports/SkFontHost_win.cpp... src/ports/SkFontHost_win.cpp:491: SkAssertResult(GetOutlineTextMetrics(hdc, sizeof(otm), &otm)); On 2011/02/11 19:01:15, Steve VanDeBogart wrote: > In release mode SkAsserts don't check anything. Would it be better to return > NULL if these fail? Returning NULL on failure. http://codereview.appspot.com/4174041/diff/13001/src/ports/SkFontHost_win.cpp... src/ports/SkFontHost_win.cpp:511: info->fFontName.set(lf.lfFaceName); On 2011/02/11 19:01:15, Steve VanDeBogart wrote: > Is this the name of the font or the name of the requested font? This is the name of the font realized into the device context. http://codereview.appspot.com/4174041/diff/13001/src/ports/SkFontHost_win.cpp... src/ports/SkFontHost_win.cpp:514: SkASSERT(otm.otmTextMetrics.tmPitchAndFamily & TMPF_TRUETYPE); On 2011/02/11 19:01:15, Steve VanDeBogart wrote: > This should be a conditional, setting info->fType to other if the font isn't > truetype (for now). Done. http://codereview.appspot.com/4174041/diff/13001/src/ports/SkFontHost_win.cpp... src/ports/SkFontHost_win.cpp:549: char stem_chars[] = {'i', 'I', '!', '1'}; On 2011/02/11 19:01:15, Steve VanDeBogart wrote: > Should init info->fStemV to 0 here. Also a bug in the freetype implementation. Done. Changed in freetype implementation also. http://codereview.appspot.com/4174041/diff/13001/src/ports/SkFontHost_win.cpp... src/ports/SkFontHost_win.cpp:561: // If bit 1 is set, the font may not be embedded in a document. On 2011/02/11 19:01:15, Steve VanDeBogart wrote: > This is kind of strange because TT has more bits than this. I wonder if there's > an alternate method that will give you all the bits (see > http://freetype.sourceforge.net/freetype2/docs/reference/ft2-base_interface.h... > ) TTGetEmbeddingType. I don't see a mapping for subsetting or bitmap embedding though.
Sign in to reply to this message.
http://codereview.appspot.com/4174041/diff/13001/src/core/SkAdvancedTypefaceM... File src/core/SkAdvancedTypefaceMetrics.cpp (right): http://codereview.appspot.com/4174041/diff/13001/src/core/SkAdvancedTypefaceM... src/core/SkAdvancedTypefaceMetrics.cpp:115: // int16_t. On 2011/02/11 21:27:25, Chris Guillory wrote: > On 2011/02/11 19:01:15, Steve VanDeBogart wrote: > > We haven't used it yet, but should probably do this for VerticalMetric as > well. > You mean move the helpers into this file? No, I meant also declare the helpers for vertical advances... i.e. template void resetRange(SkAdvancedTypefaceMetrics::VerticalAdvanceRane* range, int startId); etc. http://codereview.appspot.com/4174041/diff/13001/src/core/SkAdvancedTypefaceM... src/core/SkAdvancedTypefaceMetrics.cpp:117: template AdvanceMetric* getAdvanceData(void* param, int num_glyphs, On 2011/02/11 21:27:25, Chris Guillory wrote: > On 2011/02/11 19:01:15, Steve VanDeBogart wrote: > > This can be platform specific. > > Done. nit: I was thinking of putting it in SkFontHost_{FreetType,win}.cpp instead of ifdefing. Will that work? http://codereview.appspot.com/4174041/diff/13001/src/ports/SkFontHost_win.cpp File src/ports/SkFontHost_win.cpp (right): http://codereview.appspot.com/4174041/diff/13001/src/ports/SkFontHost_win.cpp... src/ports/SkFontHost_win.cpp:549: char stem_chars[] = {'i', 'I', '!', '1'}; On 2011/02/11 21:27:25, Chris Guillory wrote: > On 2011/02/11 19:01:15, Steve VanDeBogart wrote: > > Should init info->fStemV to 0 here. Also a bug in the freetype > implementation. > > Done. Changed in freetype implementation also. Mmm, this looks like a bug. if min_width is initialized to 0 and advances are usually > 0, then we'll also leave min_width as 0 (and never set info->fStemV). What I was saying is that if GetCharABCWidth always fails (i.e. because the font doesn't contain any of those characters) then info->fStemV will contain garbage, so we should init it to 0 before starting. http://codereview.appspot.com/4174041/diff/7004/include/core/SkAdvancedTypefa... File include/core/SkAdvancedTypefaceMetrics.h (right): http://codereview.appspot.com/4174041/diff/7004/include/core/SkAdvancedTypefa... include/core/SkAdvancedTypefaceMetrics.h:128: template <typename Data, typename Param> nit: FontHandle instead of Param? http://codereview.appspot.com/4174041/diff/7004/src/core/SkAdvancedTypefaceMe... File src/core/SkAdvancedTypefaceMetrics.cpp (right): http://codereview.appspot.com/4174041/diff/7004/src/core/SkAdvancedTypefaceMe... src/core/SkAdvancedTypefaceMetrics.cpp:22: #include FT_SIZES_H // for SHRT_MIN How about SK_MinS16 from SkTypes.h instead of needing to include all of this? http://codereview.appspot.com/4174041/diff/7004/src/core/SkAdvancedTypefaceMe... src/core/SkAdvancedTypefaceMetrics.cpp:123: typedef SkAdvancedTypefaceMetrics::AdvanceMetric<int16_t> AdvanceMetric; Can you just use SkAdvancedTypefaceMetrics::WidthRange instead of this typedef? http://codereview.appspot.com/4174041/diff/7004/src/ports/SkFontHost_FreeType... File src/ports/SkFontHost_FreeType.cpp (right): http://codereview.appspot.com/4174041/diff/7004/src/ports/SkFontHost_FreeType... src/ports/SkFontHost_FreeType.cpp:338: SkAssertResult(getAdvances(face, gId, 1, FT_LOAD_NO_SCALE, &advance) == 0); Now that we can handle a failure, return false if this fails? http://codereview.appspot.com/4174041/diff/7004/src/ports/SkFontHost_win.cpp File src/ports/SkFontHost_win.cpp (right): http://codereview.appspot.com/4174041/diff/7004/src/ports/SkFontHost_win.cpp#... src/ports/SkFontHost_win.cpp:491: return NULL; Do you need to free HDC or anything else? http://codereview.appspot.com/4174041/diff/7004/src/ports/SkFontHost_win.cpp#... src/ports/SkFontHost_win.cpp:515: info->fType = SkAdvancedTypefaceMetrics::kTrueType_Font; Will the rest of this code be ok for non TT font, or should it be in the if body?
Sign in to reply to this message.
On 2011/02/11 21:54:45, Steve VanDeBogart wrote: > http://codereview.appspot.com/4174041/diff/13001/src/core/SkAdvancedTypefaceM... > File src/core/SkAdvancedTypefaceMetrics.cpp (right): > > http://codereview.appspot.com/4174041/diff/13001/src/core/SkAdvancedTypefaceM... > src/core/SkAdvancedTypefaceMetrics.cpp:115: // int16_t. > On 2011/02/11 21:27:25, Chris Guillory wrote: > > On 2011/02/11 19:01:15, Steve VanDeBogart wrote: > > > We haven't used it yet, but should probably do this for VerticalMetric as > > well. > > You mean move the helpers into this file? > > No, I meant also declare the helpers for vertical advances... i.e. > template void resetRange(SkAdvancedTypefaceMetrics::VerticalAdvanceRane* range, > int startId); > etc. Done. > > http://codereview.appspot.com/4174041/diff/13001/src/core/SkAdvancedTypefaceM... > src/core/SkAdvancedTypefaceMetrics.cpp:117: template AdvanceMetric* > getAdvanceData(void* param, int num_glyphs, > On 2011/02/11 21:27:25, Chris Guillory wrote: > > On 2011/02/11 19:01:15, Steve VanDeBogart wrote: > > > This can be platform specific. > > > > Done. > > nit: I was thinking of putting it in SkFontHost_{FreetType,win}.cpp instead of > ifdefing. Will that work? If I keep the template definitions a cpp file I'd have to make the template instantiations there. I could them it in a header that SkFontHost_{FreeType,win} will both include. SkAdvanceTypefaceMetricsUtils.h? > > http://codereview.appspot.com/4174041/diff/13001/src/ports/SkFontHost_win.cpp > File src/ports/SkFontHost_win.cpp (right): > > http://codereview.appspot.com/4174041/diff/13001/src/ports/SkFontHost_win.cpp... > src/ports/SkFontHost_win.cpp:549: char stem_chars[] = {'i', 'I', '!', '1'}; > On 2011/02/11 21:27:25, Chris Guillory wrote: > > On 2011/02/11 19:01:15, Steve VanDeBogart wrote: > > > Should init info->fStemV to 0 here. Also a bug in the freetype > > implementation. > > > > Done. Changed in freetype implementation also. > > Mmm, this looks like a bug. if min_width is initialized to 0 and advances are > usually > 0, then we'll also leave min_width as 0 (and never set info->fStemV). > > What I was saying is that if GetCharABCWidth always fails (i.e. because the font > doesn't contain any of those characters) then info->fStemV will contain garbage, > so we should init it to 0 before starting. Initially setting info->fStemV to 0 now. http://codereview.appspot.com/4174041/diff/7004/include/core/SkAdvancedTypefa... File include/core/SkAdvancedTypefaceMetrics.h (right): http://codereview.appspot.com/4174041/diff/7004/include/core/SkAdvancedTypefa... include/core/SkAdvancedTypefaceMetrics.h:128: template <typename Data, typename Param> On 2011/02/11 21:54:45, Steve VanDeBogart wrote: > nit: FontHandle instead of Param? Done. http://codereview.appspot.com/4174041/diff/7004/src/core/SkAdvancedTypefaceMe... File src/core/SkAdvancedTypefaceMetrics.cpp (right): http://codereview.appspot.com/4174041/diff/7004/src/core/SkAdvancedTypefaceMe... src/core/SkAdvancedTypefaceMetrics.cpp:22: #include FT_SIZES_H // for SHRT_MIN On 2011/02/11 21:54:45, Steve VanDeBogart wrote: > How about SK_MinS16 from SkTypes.h instead of needing to include all of this? Done. http://codereview.appspot.com/4174041/diff/7004/src/core/SkAdvancedTypefaceMe... src/core/SkAdvancedTypefaceMetrics.cpp:123: typedef SkAdvancedTypefaceMetrics::AdvanceMetric<int16_t> AdvanceMetric; On 2011/02/11 21:54:45, Steve VanDeBogart wrote: > Can you just use SkAdvancedTypefaceMetrics::WidthRange instead of this typedef? Done. http://codereview.appspot.com/4174041/diff/7004/src/ports/SkFontHost_FreeType... File src/ports/SkFontHost_FreeType.cpp (right): http://codereview.appspot.com/4174041/diff/7004/src/ports/SkFontHost_FreeType... src/ports/SkFontHost_FreeType.cpp:338: SkAssertResult(getAdvances(face, gId, 1, FT_LOAD_NO_SCALE, &advance) == 0); On 2011/02/11 21:54:45, Steve VanDeBogart wrote: > Now that we can handle a failure, return false if this fails? Done. http://codereview.appspot.com/4174041/diff/7004/src/ports/SkFontHost_win.cpp File src/ports/SkFontHost_win.cpp (right): http://codereview.appspot.com/4174041/diff/7004/src/ports/SkFontHost_win.cpp#... src/ports/SkFontHost_win.cpp:491: return NULL; On 2011/02/11 21:54:45, Steve VanDeBogart wrote: > Do you need to free HDC or anything else? Yes. Added an Error label. http://codereview.appspot.com/4174041/diff/7004/src/ports/SkFontHost_win.cpp#... src/ports/SkFontHost_win.cpp:515: info->fType = SkAdvancedTypefaceMetrics::kTrueType_Font; On 2011/02/11 21:54:45, Steve VanDeBogart wrote: > Will the rest of this code be ok for non TT font, or should it be in the if > body? The docs say GetOutlineTextMetrics is for TT fonts. This is returning data for the Type1 font though. I'd need to verify correctness though. Docs also say GetCharABCWidths is for TTfonts and to use GetCharWidth for non-TT fonts.
Sign in to reply to this message.
Mike: what do you think of goto's, see below? LGTM with nits. http://codereview.appspot.com/4174041/diff/6003/src/core/SkAdvancedTypefaceMe... File src/core/SkAdvancedTypefaceMetrics.cpp (right): http://codereview.appspot.com/4174041/diff/6003/src/core/SkAdvancedTypefaceMe... src/core/SkAdvancedTypefaceMetrics.cpp:17: #include "SkAdvancedTypefaceMetrics.h" nit: include what you use - #include "SkTypes.h" http://codereview.appspot.com/4174041/diff/6003/src/core/SkAdvancedTypefaceMe... src/core/SkAdvancedTypefaceMetrics.cpp:125: #include FT_FREETYPE_H nit: Ahh, didn't realize that these were FT_Face. They should go at the top where you had them. http://codereview.appspot.com/4174041/diff/6003/src/ports/SkFontHost_win.cpp File src/ports/SkFontHost_win.cpp (right): http://codereview.appspot.com/4174041/diff/6003/src/ports/SkFontHost_win.cpp#... src/ports/SkFontHost_win.cpp:492: goto Error; Hmm, I don't know how Mike feels about goto's. I think this is the right place for them, but skia doesn't currently have any...
Sign in to reply to this message.
http://codereview.appspot.com/4174041/diff/6003/src/core/SkAdvancedTypefaceMe... File src/core/SkAdvancedTypefaceMetrics.cpp (right): http://codereview.appspot.com/4174041/diff/6003/src/core/SkAdvancedTypefaceMe... src/core/SkAdvancedTypefaceMetrics.cpp:17: #include "SkAdvancedTypefaceMetrics.h" On 2011/02/12 00:21:45, Steve VanDeBogart wrote: > nit: include what you use - #include "SkTypes.h" Done. http://codereview.appspot.com/4174041/diff/6003/src/core/SkAdvancedTypefaceMe... src/core/SkAdvancedTypefaceMetrics.cpp:125: #include FT_FREETYPE_H On 2011/02/12 00:21:45, Steve VanDeBogart wrote: > nit: Ahh, didn't realize that these were FT_Face. They should go at the top > where you had them. Done. Just moved the includes. http://codereview.appspot.com/4174041/diff/6003/src/ports/SkFontHost_win.cpp File src/ports/SkFontHost_win.cpp (right): http://codereview.appspot.com/4174041/diff/6003/src/ports/SkFontHost_win.cpp#... src/ports/SkFontHost_win.cpp:492: goto Error; On 2011/02/12 00:21:45, Steve VanDeBogart wrote: > Hmm, I don't know how Mike feels about goto's. I think this is the right place > for them, but skia doesn't currently have any... I see many files with gotos in skia code. One example: SkScalerContext_FreeType::generateMetrics.
Sign in to reply to this message.
Mike: nevermind :-) http://codereview.appspot.com/4174041/diff/6003/src/ports/SkFontHost_win.cpp File src/ports/SkFontHost_win.cpp (right): http://codereview.appspot.com/4174041/diff/6003/src/ports/SkFontHost_win.cpp#... src/ports/SkFontHost_win.cpp:492: goto Error; On 2011/02/12 00:32:42, Chris Guillory wrote: > On 2011/02/12 00:21:45, Steve VanDeBogart wrote: > > Hmm, I don't know how Mike feels about goto's. I think this is the right > place > > for them, but skia doesn't currently have any... > I see many files with gotos in skia code. One example: > SkScalerContext_FreeType::generateMetrics. Oops, my bad. Forgot the -r to grep.
Sign in to reply to this message.
http://codereview.appspot.com/4174041/diff/10003/src/ports/SkFontHost_FreeTyp... File src/ports/SkFontHost_FreeType.cpp (left): http://codereview.appspot.com/4174041/diff/10003/src/ports/SkFontHost_FreeTyp... src/ports/SkFontHost_FreeType.cpp:337: return advance; SkASSERT(data); http://codereview.appspot.com/4174041/diff/10003/src/ports/SkFontHost_FreeTyp... File src/ports/SkFontHost_FreeType.cpp (right): http://codereview.appspot.com/4174041/diff/10003/src/ports/SkFontHost_FreeTyp... src/ports/SkFontHost_FreeType.cpp:338: if (getAdvances(face, gId, 1, FT_LOAD_NO_SCALE, &advance)) need { } around all "if" statements -- but can fix in a subsequent CL http://codereview.appspot.com/4174041/diff/10003/src/ports/SkFontHost_win.cpp File src/ports/SkFontHost_win.cpp (right): http://codereview.appspot.com/4174041/diff/10003/src/ports/SkFontHost_win.cpp... src/ports/SkFontHost_win.cpp:465: static MAT2 mat2 = {SkScalarToFIXED(1), SkScalarToFIXED(0), can mat2 be const? http://codereview.appspot.com/4174041/diff/10003/src/ports/SkFontHost_win.cpp... src/ports/SkFontHost_win.cpp:469: if (GDI_ERROR == GetGlyphOutline(hdc, gId, flags, &gm, 0, NULL, &mat2)) { } http://codereview.appspot.com/4174041/diff/10003/src/ports/SkFontHost_win.cpp... src/ports/SkFontHost_win.cpp:492: !GetTextFace(hdc, LF_FACESIZE, lf.lfFaceName)) {} http://codereview.appspot.com/4174041/diff/10003/src/ports/SkFontHost_win.cpp... src/ports/SkFontHost_win.cpp:497: if (!GetOutlineTextMetrics(hdc, sizeof(otm), &otm)) {} http://codereview.appspot.com/4174041/diff/10003/src/ports/SkFontHost_win.cpp... src/ports/SkFontHost_win.cpp:526: if (!(otm.otmTextMetrics.tmPitchAndFamily & TMPF_FIXED_PITCH)) {}
Sign in to reply to this message.
http://codereview.appspot.com/4174041/diff/10003/src/ports/SkFontHost_FreeTyp... File src/ports/SkFontHost_FreeType.cpp (left): http://codereview.appspot.com/4174041/diff/10003/src/ports/SkFontHost_FreeTyp... src/ports/SkFontHost_FreeType.cpp:337: return advance; On 2011/02/14 12:54:55, reed1 wrote: > SkASSERT(data); Done. http://codereview.appspot.com/4174041/diff/10003/src/ports/SkFontHost_FreeTyp... File src/ports/SkFontHost_FreeType.cpp (right): http://codereview.appspot.com/4174041/diff/10003/src/ports/SkFontHost_FreeTyp... src/ports/SkFontHost_FreeType.cpp:338: if (getAdvances(face, gId, 1, FT_LOAD_NO_SCALE, &advance)) On 2011/02/14 12:54:55, reed1 wrote: > need { } around all "if" statements -- but can fix in a subsequent CL Done. http://codereview.appspot.com/4174041/diff/10003/src/ports/SkFontHost_win.cpp File src/ports/SkFontHost_win.cpp (right): http://codereview.appspot.com/4174041/diff/10003/src/ports/SkFontHost_win.cpp... src/ports/SkFontHost_win.cpp:465: static MAT2 mat2 = {SkScalarToFIXED(1), SkScalarToFIXED(0), On 2011/02/14 12:54:55, reed1 wrote: > can mat2 be const? Yes. http://codereview.appspot.com/4174041/diff/10003/src/ports/SkFontHost_win.cpp... src/ports/SkFontHost_win.cpp:469: if (GDI_ERROR == GetGlyphOutline(hdc, gId, flags, &gm, 0, NULL, &mat2)) On 2011/02/14 12:54:55, reed1 wrote: > { } Done. http://codereview.appspot.com/4174041/diff/10003/src/ports/SkFontHost_win.cpp... src/ports/SkFontHost_win.cpp:492: !GetTextFace(hdc, LF_FACESIZE, lf.lfFaceName)) On 2011/02/14 12:54:55, reed1 wrote: > {} Done. http://codereview.appspot.com/4174041/diff/10003/src/ports/SkFontHost_win.cpp... src/ports/SkFontHost_win.cpp:497: if (!GetOutlineTextMetrics(hdc, sizeof(otm), &otm)) On 2011/02/14 12:54:55, reed1 wrote: > {} Done. http://codereview.appspot.com/4174041/diff/10003/src/ports/SkFontHost_win.cpp... src/ports/SkFontHost_win.cpp:526: if (!(otm.otmTextMetrics.tmPitchAndFamily & TMPF_FIXED_PITCH)) On 2011/02/14 12:54:55, reed1 wrote: > {} Done.
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
