Issue 4428082: Make text searchable in PDF (Closed)
|
Created:
13 years, 7 months ago by arthurhsu Modified:
13 years, 7 months ago CC:
skia-review_googlegroups.com Base URL:
https://skia.googlecode.com/svn/trunk Visibility:
Public. |
DescriptionMake text searchable in PDF
Chromium bug 81308
BUG=
TEST=
Patch Set 1 #
Total comments: 32
Patch Set 2 : Upload per code review #
Total comments: 56
Patch Set 3 : Upload per code review #Patch Set 4 : Update per code review #
Total comments: 24
Patch Set 5 : Update per code review #Patch Set 6 : Add Linux port #
Total comments: 15
Patch Set 7 : Update per code review #
Total comments: 2
Patch Set 8 : Update per code review #
Total comments: 22
Patch Set 9 : Update per code review #
Total comments: 32
Patch Set 10 : Update per code review #Patch Set 11 : Upload per code review #
MessagesTotal messages: 24
http://codereview.appspot.com/4428082/diff/1/include/core/SkAdvancedTypefaceM... File include/core/SkAdvancedTypefaceMetrics.h (right): http://codereview.appspot.com/4428082/diff/1/include/core/SkAdvancedTypefaceM... include/core/SkAdvancedTypefaceMetrics.h:117: // The mappings from glyph to Unicode. Default is NULL. UTF16? What about conjugate pairs? Would it be easier to do UTF32? What's "Default is NULL." mean? We probably should add an entry in PerGlyphInfo to control if this field is populated or not. http://codereview.appspot.com/4428082/diff/1/src/pdf/SkPDFFont.cpp File src/pdf/SkPDFFont.cpp (right): http://codereview.appspot.com/4428082/diff/1/src/pdf/SkPDFFont.cpp#newcode320 src/pdf/SkPDFFont.cpp:320: void generateHeader(SkDynamicMemoryWStream* cmap) { Skia style convention (recently codified), says this (and others) should be a static function with the name generate_header. Though how about append_cmap_header ? http://codereview.appspot.com/4428082/diff/1/src/pdf/SkPDFFont.cpp#newcode331 src/pdf/SkPDFFont.cpp:331: const char* kSysInfo = Will this work with Type 3 and Type 1 fonts as well, or only for Type 0 fonts? We'd like to support all of them, so if not, another argument to this function may be required. http://codereview.appspot.com/4428082/diff/1/src/pdf/SkPDFFont.cpp#newcode343 src/pdf/SkPDFFont.cpp:343: // PDF processor the valid range. It does not matter whether complete nit: "the PDF processor", "whether a complete" http://codereview.appspot.com/4428082/diff/1/src/pdf/SkPDFFont.cpp#newcode354 src/pdf/SkPDFFont.cpp:354: void generateBFCharTable(SkDynamicMemoryWStream* cmap, uint16_t* glyph_id, append_cmap_bfchar_table? http://codereview.appspot.com/4428082/diff/1/src/pdf/SkPDFFont.cpp#newcode358 src/pdf/SkPDFFont.cpp:358: sprintf(line_buffer, "%d beginbfchar\n", dimension); We probably don't need a buffer here, see SkWStream::writeHexAsText, writeText, writeDecAsText, etc. http://codereview.appspot.com/4428082/diff/1/src/pdf/SkPDFFont.cpp#newcode369 src/pdf/SkPDFFont.cpp:369: void generateFooter(SkDynamicMemoryWStream* cmap) { append_cmap_footer ? http://codereview.appspot.com/4428082/diff/1/src/pdf/SkPDFFont.cpp#newcode385 src/pdf/SkPDFFont.cpp:385: uint16_t glyph_id[100] = {0}; Where's 100 come from? Does the spec recommend multiple beginbfchar chunks of a specific/reasonable size? If so, we should so something like: static const int kBFCharTableSize = 100; and explain the value chosen. http://codereview.appspot.com/4428082/diff/1/src/pdf/SkPDFFont.cpp#newcode417 src/pdf/SkPDFFont.cpp:417: } // namespace Skia code style doesn't use anonymous namespaces. You don't have to fix the existing violations, but we shouldn't add more. http://codereview.appspot.com/4428082/diff/1/src/pdf/SkPDFFont.cpp#newcode613 src/pdf/SkPDFFont.cpp:613: SkDynamicMemoryWStream cmap; Can we hide this extra stuff in generateToUnicode? i.e. void addToUnicode(SkAdvancedTypefaceMetrics* fontInfo) ? http://codereview.appspot.com/4428082/diff/1/src/ports/SkFontHost_win.cpp File src/ports/SkFontHost_win.cpp (right): http://codereview.appspot.com/4428082/diff/1/src/ports/SkFontHost_win.cpp#new... src/ports/SkFontHost_win.cpp:1: /* Windows is first priority, but we also want to update the Linux/freetype file (the other place that GetAdvancedTypefaceMetrics is implemented). http://codereview.appspot.com/4428082/diff/1/src/ports/SkFontHost_win.cpp#new... src/ports/SkFontHost_win.cpp:663: // Construct Glyph to Unicode table. Is the } misplaced? We'd like to do this for all fonts, not just non-TTF fonts. http://codereview.appspot.com/4428082/diff/1/src/ports/SkFontHost_win.cpp#new... src/ports/SkFontHost_win.cpp:663: // Construct Glyph to Unicode table. As mentioned earlier, we probably should key this off a new value in perGlyphInfo. http://codereview.appspot.com/4428082/diff/1/src/ports/SkFontHost_win.cpp#new... src/ports/SkFontHost_win.cpp:674: if (GetFontUnicodeRanges(hdc, glyph_set) == buffer_size) { Does this handle unicode points outside the BMP or unicode points that are drawn with multiple glyphs? It's ok to punt on either of these, but we should note it.
Sign in to reply to this message.
http://codereview.appspot.com/4428082/diff/1/include/core/SkAdvancedTypefaceM... File include/core/SkAdvancedTypefaceMetrics.h (right): http://codereview.appspot.com/4428082/diff/1/include/core/SkAdvancedTypefaceM... include/core/SkAdvancedTypefaceMetrics.h:117: // The mappings from glyph to Unicode. Default is NULL. On 2011/05/03 17:49:08, Steve VanDeBogart wrote: > UTF16? What about conjugate pairs? Would it be easier to do UTF32? What's > "Default is NULL." mean? We probably should add an entry in PerGlyphInfo to > control if this field is populated or not. TTF CMap only supports UTF16. HTML also discourages usage of UTF32 (http://dev.w3.org/html5/spec/semantics.html#charset). Comments are updated. http://codereview.appspot.com/4428082/diff/1/src/pdf/SkPDFFont.cpp File src/pdf/SkPDFFont.cpp (right): http://codereview.appspot.com/4428082/diff/1/src/pdf/SkPDFFont.cpp#newcode320 src/pdf/SkPDFFont.cpp:320: void generateHeader(SkDynamicMemoryWStream* cmap) { On 2011/05/03 17:49:08, Steve VanDeBogart wrote: > Skia style convention (recently codified), says this (and others) should be a > static function with the name generate_header. Though how about > append_cmap_header ? Done. http://codereview.appspot.com/4428082/diff/1/src/pdf/SkPDFFont.cpp#newcode331 src/pdf/SkPDFFont.cpp:331: const char* kSysInfo = On 2011/05/03 17:49:08, Steve VanDeBogart wrote: > Will this work with Type 3 and Type 1 fonts as well, or only for Type 0 fonts? > We'd like to support all of them, so if not, another argument to this function > may be required. Currently it works with TTF only because TTF carries CMap. For Type1 and Type3 fonts, extra code needs to be done to construct CMap table. Typically Type1 and Type3 already defined its /FirstChar /LastChar and /Encoding and readers can use that to translate glyph to text without ToUnicode table. http://codereview.appspot.com/4428082/diff/1/src/pdf/SkPDFFont.cpp#newcode343 src/pdf/SkPDFFont.cpp:343: // PDF processor the valid range. It does not matter whether complete On 2011/05/03 17:49:08, Steve VanDeBogart wrote: > nit: "the PDF processor", "whether a complete" Done. http://codereview.appspot.com/4428082/diff/1/src/pdf/SkPDFFont.cpp#newcode354 src/pdf/SkPDFFont.cpp:354: void generateBFCharTable(SkDynamicMemoryWStream* cmap, uint16_t* glyph_id, On 2011/05/03 17:49:08, Steve VanDeBogart wrote: > append_cmap_bfchar_table? Done. http://codereview.appspot.com/4428082/diff/1/src/pdf/SkPDFFont.cpp#newcode358 src/pdf/SkPDFFont.cpp:358: sprintf(line_buffer, "%d beginbfchar\n", dimension); On 2011/05/03 17:49:08, Steve VanDeBogart wrote: > We probably don't need a buffer here, see SkWStream::writeHexAsText, writeText, > writeDecAsText, etc. Done. http://codereview.appspot.com/4428082/diff/1/src/pdf/SkPDFFont.cpp#newcode369 src/pdf/SkPDFFont.cpp:369: void generateFooter(SkDynamicMemoryWStream* cmap) { On 2011/05/03 17:49:08, Steve VanDeBogart wrote: > append_cmap_footer ? Done. http://codereview.appspot.com/4428082/diff/1/src/pdf/SkPDFFont.cpp#newcode385 src/pdf/SkPDFFont.cpp:385: uint16_t glyph_id[100] = {0}; On 2011/05/03 17:49:08, Steve VanDeBogart wrote: > Where's 100 come from? Does the spec recommend multiple beginbfchar chunks of a > specific/reasonable size? > > If so, we should so something like: > static const int kBFCharTableSize = 100; > and explain the value chosen. Done. http://codereview.appspot.com/4428082/diff/1/src/pdf/SkPDFFont.cpp#newcode417 src/pdf/SkPDFFont.cpp:417: } // namespace On 2011/05/03 17:49:08, Steve VanDeBogart wrote: > Skia code style doesn't use anonymous namespaces. You don't have to fix the > existing violations, but we shouldn't add more. Done. http://codereview.appspot.com/4428082/diff/1/src/pdf/SkPDFFont.cpp#newcode613 src/pdf/SkPDFFont.cpp:613: SkDynamicMemoryWStream cmap; On 2011/05/03 17:49:08, Steve VanDeBogart wrote: > Can we hide this extra stuff in generateToUnicode? i.e. void > addToUnicode(SkAdvancedTypefaceMetrics* fontInfo) ? Done. http://codereview.appspot.com/4428082/diff/1/src/ports/SkFontHost_win.cpp File src/ports/SkFontHost_win.cpp (right): http://codereview.appspot.com/4428082/diff/1/src/ports/SkFontHost_win.cpp#new... src/ports/SkFontHost_win.cpp:663: // Construct Glyph to Unicode table. On 2011/05/03 17:49:08, Steve VanDeBogart wrote: > As mentioned earlier, we probably should key this off a new value in > perGlyphInfo. Added check if advanced bit is set. Publish ToUnicode only when that's the case. http://codereview.appspot.com/4428082/diff/1/src/ports/SkFontHost_win.cpp#new... src/ports/SkFontHost_win.cpp:663: // Construct Glyph to Unicode table. On 2011/05/03 17:49:08, Steve VanDeBogart wrote: > Is the } misplaced? We'd like to do this for all fonts, not just non-TTF fonts. Done. http://codereview.appspot.com/4428082/diff/1/src/ports/SkFontHost_win.cpp#new... src/ports/SkFontHost_win.cpp:674: if (GetFontUnicodeRanges(hdc, glyph_set) == buffer_size) { On 2011/05/03 17:49:08, Steve VanDeBogart wrote: > Does this handle unicode points outside the BMP or unicode points that are drawn > with multiple glyphs? It's ok to punt on either of these, but we should note > it. This call gets all possible Unicode ranges supported by the font by querying the forward map keys. I've added more comments below.
Sign in to reply to this message.
http://codereview.appspot.com/4428082/diff/1/include/core/SkAdvancedTypefaceM... File include/core/SkAdvancedTypefaceMetrics.h (right): http://codereview.appspot.com/4428082/diff/1/include/core/SkAdvancedTypefaceM... include/core/SkAdvancedTypefaceMetrics.h:117: // The mappings from glyph to Unicode. Default is NULL. On 2011/05/03 20:09:02, arthurhsu wrote: > On 2011/05/03 17:49:08, Steve VanDeBogart wrote: > > UTF16? What about conjugate pairs? Would it be easier to do UTF32? What's > > "Default is NULL." mean? We probably should add an entry in PerGlyphInfo to > > control if this field is populated or not. > > TTF CMap only supports UTF16. HTML also discourages usage of UTF32 > (http://dev.w3.org/html5/spec/semantics.html#charset). > > Comments are updated. The question was only about the transit format. In order to encode code points outside the BMP, UTF16 uses conjugate pairs, i.e. 32 bits of storage. The default comment is still not clear to me. Do you mean that the array is empty by default? If so, a more specific comment would be usefull, i.e. "Only populated if kToUnicodeInfo is passed to GetAdvancedTypefaceMetrics." http://codereview.appspot.com/4428082/diff/1/src/pdf/SkPDFFont.cpp File src/pdf/SkPDFFont.cpp (right): http://codereview.appspot.com/4428082/diff/1/src/pdf/SkPDFFont.cpp#newcode331 src/pdf/SkPDFFont.cpp:331: const char* kSysInfo = Thinking about it a bit further (and testing on Linux), shows that Type 1 doesn't need anything extra (the way glyph encoding is done on Type 1 fonts obviates the need to do anything extra). But Type 3 fonts need a ToUnicode mapping. On windows, we encode Type 1 fonts as Type 3 fonts because we aren't able to extract the glyph names (without parsing the the Type 1 file). http://codereview.appspot.com/4428082/diff/1/src/ports/SkFontHost_win.cpp File src/ports/SkFontHost_win.cpp (right): http://codereview.appspot.com/4428082/diff/1/src/ports/SkFontHost_win.cpp#new... src/ports/SkFontHost_win.cpp:674: if (GetFontUnicodeRanges(hdc, glyph_set) == buffer_size) { On 2011/05/03 20:09:02, arthurhsu wrote: > On 2011/05/03 17:49:08, Steve VanDeBogart wrote: > > Does this handle unicode points outside the BMP or unicode points that are > drawn > > with multiple glyphs? It's ok to punt on either of these, but we should note > > it. > > This call gets all possible Unicode ranges supported by the font by querying the > forward map keys. I've added more comments below. Chris found that some of the GDI commands don't support conjugate pair UTF16 and you have to use the uniscribe API to get at them... http://codereview.appspot.com/4428082/diff/4001/include/core/SkAdvancedTypefa... File include/core/SkAdvancedTypefaceMetrics.h (right): http://codereview.appspot.com/4428082/diff/4001/include/core/SkAdvancedTypefa... include/core/SkAdvancedTypefaceMetrics.h:85: kToUnicodeInfo = 0x8, // Populate ToUnicode table. +"(except for Type 1)." Skia naming conversion has enum type at end. i.e. kToUnicodeInfo_PerGlyphInfo http://codereview.appspot.com/4428082/diff/4001/include/pdf/SkPDFFont.h File include/pdf/SkPDFFont.h (right): http://codereview.appspot.com/4428082/diff/4001/include/pdf/SkPDFFont.h#newco... include/pdf/SkPDFFont.h:123: void populateToUnicodeTable(); Move this down with the other support functions, like addFontDescriptor. http://codereview.appspot.com/4428082/diff/4001/src/pdf/SkPDFFont.cpp File src/pdf/SkPDFFont.cpp (right): http://codereview.appspot.com/4428082/diff/4001/src/pdf/SkPDFFont.cpp#newcode322 src/pdf/SkPDFFont.cpp:322: void generate_header(SkDynamicMemoryWStream* cmap) { static void generate_cmap_header, Skia style requires static, generate_header is too generic: what kind of header? http://codereview.appspot.com/4428082/diff/4001/src/pdf/SkPDFFont.cpp#newcode356 src/pdf/SkPDFFont.cpp:356: void append_cmap_bfchar_table(SkDynamicMemoryWStream* cmap, uint16_t* glyph_id, We generally prefer input arguments first, and output arguments last, so lets move cmap to the last argument. http://codereview.appspot.com/4428082/diff/4001/src/pdf/SkPDFFont.cpp#newcode357 src/pdf/SkPDFFont.cpp:357: uint16_t* unicode, size_t dimension) { dimension is unclear, how about count? http://codereview.appspot.com/4428082/diff/4001/src/pdf/SkPDFFont.cpp#newcode358 src/pdf/SkPDFFont.cpp:358: const char* kBeginBFChar = " beginbfchar\n"; There's not really a need for constants here, just use SkWStream::writeText http://codereview.appspot.com/4428082/diff/4001/src/pdf/SkPDFFont.cpp#newcode362 src/pdf/SkPDFFont.cpp:362: for (size_t i=0; i<dimension; i++) { i < dimension http://codereview.appspot.com/4428082/diff/4001/src/pdf/SkPDFFont.cpp#newcode363 src/pdf/SkPDFFont.cpp:363: cmap->write("<", 1); use writeText instead so there's no need to keep the string constant and lengths in sync. I'm pretty sure the compiler does constant propagation for the length since the writeText argument is const char text[]. You can change most places where you've used write(constant, strlen(constant)) http://codereview.appspot.com/4428082/diff/4001/src/pdf/SkPDFFont.cpp#newcode382 src/pdf/SkPDFFont.cpp:382: // Although spec said one can use <bfrange> tag to reduce the size of mapping nit: said->says http://codereview.appspot.com/4428082/diff/4001/src/pdf/SkPDFFont.cpp#newcode386 src/pdf/SkPDFFont.cpp:386: void append_cmap_bfchar_sections(SkDynamicMemoryWStream* cmap, cmap to end http://codereview.appspot.com/4428082/diff/4001/src/pdf/SkPDFFont.cpp#newcode388 src/pdf/SkPDFFont.cpp:388: // PDF spec defined that every bf* list can have at most 100 entries. nit: defined -> defines http://codereview.appspot.com/4428082/diff/4001/src/pdf/SkPDFFont.cpp#newcode390 src/pdf/SkPDFFont.cpp:390: uint16_t glyph_id[kMaxEntries] = {0}; Initialization isn't needed. http://codereview.appspot.com/4428082/diff/4001/src/pdf/SkPDFFont.cpp#newcode392 src/pdf/SkPDFFont.cpp:392: size_t j=0; i/j are ok for loop indices, but otherwise we want real variable names. currentGlyphCount ? http://codereview.appspot.com/4428082/diff/4001/src/pdf/SkPDFFont.cpp#newcode393 src/pdf/SkPDFFont.cpp:393: for (int i=0; i<glyph_unicode.count(); i++) { i = 0; i < glyph... http://codereview.appspot.com/4428082/diff/4001/src/pdf/SkPDFFont.cpp#newcode401 src/pdf/SkPDFFont.cpp:401: memset(glyph_id, 0, sizeof(uint16_t)*100); The memset's aren't needed. http://codereview.appspot.com/4428082/diff/4001/src/pdf/SkPDFFont.cpp#newcode497 src/pdf/SkPDFFont.cpp:497: SkAdvancedTypefaceMetrics::kGlyphNames_PerGlyphInfo)); This should also include kToUnicodeInfo_PerGlyphInfo. http://codereview.appspot.com/4428082/diff/4001/src/pdf/SkPDFFont.cpp#newcode615 src/pdf/SkPDFFont.cpp:615: SkRefPtr<SkMemoryStream> map_obj = new SkMemoryStream(); Skia style convention for variables is mapObj. http://code.google.com/p/skia/wiki/CodingStyleGuidelines http://codereview.appspot.com/4428082/diff/4001/src/ports/SkFontHost_win.cpp File src/ports/SkFontHost_win.cpp (right): http://codereview.appspot.com/4428082/diff/4001/src/ports/SkFontHost_win.cpp#... src/ports/SkFontHost_win.cpp:653: if ((perGlyphInfo & SkAdvancedTypefaceMetrics::kHAdvance_PerGlyphInfo) || This should check kToUnicodeInfo_PerGlyphInfo. http://codereview.appspot.com/4428082/diff/4001/src/ports/SkFontHost_win.cpp#... src/ports/SkFontHost_win.cpp:657: memset(info->fGlyphToUnicode.begin(), 0, glyphCount*sizeof(uint16_t)); glyhCount * sizeof(... http://codereview.appspot.com/4428082/diff/4001/src/ports/SkFontHost_win.cpp#... src/ports/SkFontHost_win.cpp:660: BYTE* buffer; If you can think of a more descriptive name than buffer, than might good. http://codereview.appspot.com/4428082/diff/4001/src/ports/SkFontHost_win.cpp#... src/ports/SkFontHost_win.cpp:661: DWORD buffer_size = GetFontUnicodeRanges(hdc, NULL); bufferSize http://codereview.appspot.com/4428082/diff/4001/src/ports/SkFontHost_win.cpp#... src/ports/SkFontHost_win.cpp:662: bool reset_table = true; foundUnicodeMap = false ? http://codereview.appspot.com/4428082/diff/4001/src/ports/SkFontHost_win.cpp#... src/ports/SkFontHost_win.cpp:664: buffer = new BYTE[buffer_size]; You might want to use SkAutoTDeleteArray here (in include/core/SkTemplates.h) http://codereview.appspot.com/4428082/diff/4001/src/ports/SkFontHost_win.cpp#... src/ports/SkFontHost_win.cpp:668: for (DWORD i=0; i<glyph_set->cRanges; ++i) { more spaces here and the next line. http://codereview.appspot.com/4428082/diff/4001/src/ports/SkFontHost_win.cpp#... src/ports/SkFontHost_win.cpp:683: // 1. Just carry over the text into PDF. As far as I know, there's no way to do this. http://codereview.appspot.com/4428082/diff/4001/src/ports/SkFontHost_win.cpp#... src/ports/SkFontHost_win.cpp:698: info->fGlyphNames.reset(); fGlyphToUnicode http://codereview.appspot.com/4428082/diff/4001/src/ports/SkFontHost_win.cpp#... src/ports/SkFontHost_win.cpp:700: } // if perGlyphInfo contains advanced bit Update or remove this comment as well.
Sign in to reply to this message.
http://codereview.appspot.com/4428082/diff/1/include/core/SkAdvancedTypefaceM... File include/core/SkAdvancedTypefaceMetrics.h (right): http://codereview.appspot.com/4428082/diff/1/include/core/SkAdvancedTypefaceM... include/core/SkAdvancedTypefaceMetrics.h:117: // The mappings from glyph to Unicode. Default is NULL. On 2011/05/04 20:31:53, Steve VanDeBogart wrote: > On 2011/05/03 20:09:02, arthurhsu wrote: > > On 2011/05/03 17:49:08, Steve VanDeBogart wrote: > > > UTF16? What about conjugate pairs? Would it be easier to do UTF32? What's > > > "Default is NULL." mean? We probably should add an entry in PerGlyphInfo to > > > control if this field is populated or not. > > > > TTF CMap only supports UTF16. HTML also discourages usage of UTF32 > > (http://dev.w3.org/html5/spec/semantics.html#charset). > > > > Comments are updated. > > The question was only about the transit format. In order to encode code points > outside the BMP, UTF16 uses conjugate pairs, i.e. 32 bits of storage. > > The default comment is still not clear to me. Do you mean that the array is > empty by default? If so, a more specific comment would be usefull, i.e. "Only > populated if kToUnicodeInfo is passed to GetAdvancedTypefaceMetrics." Done. http://codereview.appspot.com/4428082/diff/1/src/pdf/SkPDFFont.cpp File src/pdf/SkPDFFont.cpp (right): http://codereview.appspot.com/4428082/diff/1/src/pdf/SkPDFFont.cpp#newcode331 src/pdf/SkPDFFont.cpp:331: const char* kSysInfo = On 2011/05/04 20:31:53, Steve VanDeBogart wrote: > Thinking about it a bit further (and testing on Linux), shows that Type 1 > doesn't need anything extra (the way glyph encoding is done on Type 1 fonts > obviates the need to do anything extra). But Type 3 fonts need a ToUnicode > mapping. On windows, we encode Type 1 fonts as Type 3 fonts because we aren't > able to extract the glyph names (without parsing the the Type 1 file). Done. Right now Type 3 will also have ToUnicode published. http://codereview.appspot.com/4428082/diff/4001/include/core/SkAdvancedTypefa... File include/core/SkAdvancedTypefaceMetrics.h (right): http://codereview.appspot.com/4428082/diff/4001/include/core/SkAdvancedTypefa... include/core/SkAdvancedTypefaceMetrics.h:85: kToUnicodeInfo = 0x8, // Populate ToUnicode table. On 2011/05/04 20:31:53, Steve VanDeBogart wrote: > +"(except for Type 1)." > > Skia naming conversion has enum type at end. i.e. kToUnicodeInfo_PerGlyphInfo Done. http://codereview.appspot.com/4428082/diff/4001/include/pdf/SkPDFFont.h File include/pdf/SkPDFFont.h (right): http://codereview.appspot.com/4428082/diff/4001/include/pdf/SkPDFFont.h#newco... include/pdf/SkPDFFont.h:123: void populateToUnicodeTable(); On 2011/05/04 20:31:53, Steve VanDeBogart wrote: > Move this down with the other support functions, like addFontDescriptor. Done. http://codereview.appspot.com/4428082/diff/4001/src/pdf/SkPDFFont.cpp File src/pdf/SkPDFFont.cpp (right): http://codereview.appspot.com/4428082/diff/4001/src/pdf/SkPDFFont.cpp#newcode322 src/pdf/SkPDFFont.cpp:322: void generate_header(SkDynamicMemoryWStream* cmap) { On 2011/05/04 20:31:53, Steve VanDeBogart wrote: > static void generate_cmap_header, > Skia style requires static, generate_header is too generic: what kind of header? Done. http://codereview.appspot.com/4428082/diff/4001/src/pdf/SkPDFFont.cpp#newcode356 src/pdf/SkPDFFont.cpp:356: void append_cmap_bfchar_table(SkDynamicMemoryWStream* cmap, uint16_t* glyph_id, On 2011/05/04 20:31:53, Steve VanDeBogart wrote: > We generally prefer input arguments first, and output arguments last, so lets > move cmap to the last argument. Done. http://codereview.appspot.com/4428082/diff/4001/src/pdf/SkPDFFont.cpp#newcode357 src/pdf/SkPDFFont.cpp:357: uint16_t* unicode, size_t dimension) { On 2011/05/04 20:31:53, Steve VanDeBogart wrote: > dimension is unclear, how about count? Done. http://codereview.appspot.com/4428082/diff/4001/src/pdf/SkPDFFont.cpp#newcode358 src/pdf/SkPDFFont.cpp:358: const char* kBeginBFChar = " beginbfchar\n"; On 2011/05/04 20:31:53, Steve VanDeBogart wrote: > There's not really a need for constants here, just use SkWStream::writeText Done. http://codereview.appspot.com/4428082/diff/4001/src/pdf/SkPDFFont.cpp#newcode362 src/pdf/SkPDFFont.cpp:362: for (size_t i=0; i<dimension; i++) { On 2011/05/04 20:31:53, Steve VanDeBogart wrote: > i < dimension Done. http://codereview.appspot.com/4428082/diff/4001/src/pdf/SkPDFFont.cpp#newcode363 src/pdf/SkPDFFont.cpp:363: cmap->write("<", 1); On 2011/05/04 20:31:53, Steve VanDeBogart wrote: > use writeText instead so there's no need to keep the string constant and lengths > in sync. I'm pretty sure the compiler does constant propagation for the length > since the writeText argument is const char text[]. > > You can change most places where you've used write(constant, strlen(constant)) Done. http://codereview.appspot.com/4428082/diff/4001/src/pdf/SkPDFFont.cpp#newcode382 src/pdf/SkPDFFont.cpp:382: // Although spec said one can use <bfrange> tag to reduce the size of mapping On 2011/05/04 20:31:53, Steve VanDeBogart wrote: > nit: said->says Done. http://codereview.appspot.com/4428082/diff/4001/src/pdf/SkPDFFont.cpp#newcode386 src/pdf/SkPDFFont.cpp:386: void append_cmap_bfchar_sections(SkDynamicMemoryWStream* cmap, On 2011/05/04 20:31:53, Steve VanDeBogart wrote: > cmap to end Done. http://codereview.appspot.com/4428082/diff/4001/src/pdf/SkPDFFont.cpp#newcode388 src/pdf/SkPDFFont.cpp:388: // PDF spec defined that every bf* list can have at most 100 entries. On 2011/05/04 20:31:53, Steve VanDeBogart wrote: > nit: defined -> defines Done. http://codereview.appspot.com/4428082/diff/4001/src/pdf/SkPDFFont.cpp#newcode390 src/pdf/SkPDFFont.cpp:390: uint16_t glyph_id[kMaxEntries] = {0}; On 2011/05/04 20:31:53, Steve VanDeBogart wrote: > Initialization isn't needed. Done. http://codereview.appspot.com/4428082/diff/4001/src/pdf/SkPDFFont.cpp#newcode392 src/pdf/SkPDFFont.cpp:392: size_t j=0; On 2011/05/04 20:31:53, Steve VanDeBogart wrote: > i/j are ok for loop indices, but otherwise we want real variable names. > currentGlyphCount ? Done. http://codereview.appspot.com/4428082/diff/4001/src/pdf/SkPDFFont.cpp#newcode393 src/pdf/SkPDFFont.cpp:393: for (int i=0; i<glyph_unicode.count(); i++) { On 2011/05/04 20:31:53, Steve VanDeBogart wrote: > i = 0; i < glyph... Done. http://codereview.appspot.com/4428082/diff/4001/src/pdf/SkPDFFont.cpp#newcode401 src/pdf/SkPDFFont.cpp:401: memset(glyph_id, 0, sizeof(uint16_t)*100); On 2011/05/04 20:31:53, Steve VanDeBogart wrote: > The memset's aren't needed. Done. http://codereview.appspot.com/4428082/diff/4001/src/pdf/SkPDFFont.cpp#newcode497 src/pdf/SkPDFFont.cpp:497: SkAdvancedTypefaceMetrics::kGlyphNames_PerGlyphInfo)); On 2011/05/04 20:31:53, Steve VanDeBogart wrote: > This should also include kToUnicodeInfo_PerGlyphInfo. Done. http://codereview.appspot.com/4428082/diff/4001/src/pdf/SkPDFFont.cpp#newcode615 src/pdf/SkPDFFont.cpp:615: SkRefPtr<SkMemoryStream> map_obj = new SkMemoryStream(); On 2011/05/04 20:31:53, Steve VanDeBogart wrote: > Skia style convention for variables is mapObj. > http://code.google.com/p/skia/wiki/CodingStyleGuidelines Done. http://codereview.appspot.com/4428082/diff/4001/src/ports/SkFontHost_win.cpp File src/ports/SkFontHost_win.cpp (right): http://codereview.appspot.com/4428082/diff/4001/src/ports/SkFontHost_win.cpp#... src/ports/SkFontHost_win.cpp:653: if ((perGlyphInfo & SkAdvancedTypefaceMetrics::kHAdvance_PerGlyphInfo) || On 2011/05/04 20:31:53, Steve VanDeBogart wrote: > This should check kToUnicodeInfo_PerGlyphInfo. Done. http://codereview.appspot.com/4428082/diff/4001/src/ports/SkFontHost_win.cpp#... src/ports/SkFontHost_win.cpp:657: memset(info->fGlyphToUnicode.begin(), 0, glyphCount*sizeof(uint16_t)); On 2011/05/04 20:31:53, Steve VanDeBogart wrote: > glyhCount * sizeof(... Done. http://codereview.appspot.com/4428082/diff/4001/src/ports/SkFontHost_win.cpp#... src/ports/SkFontHost_win.cpp:660: BYTE* buffer; On 2011/05/04 20:31:53, Steve VanDeBogart wrote: > If you can think of a more descriptive name than buffer, than might good. Done. http://codereview.appspot.com/4428082/diff/4001/src/ports/SkFontHost_win.cpp#... src/ports/SkFontHost_win.cpp:661: DWORD buffer_size = GetFontUnicodeRanges(hdc, NULL); On 2011/05/04 20:31:53, Steve VanDeBogart wrote: > bufferSize Done. http://codereview.appspot.com/4428082/diff/4001/src/ports/SkFontHost_win.cpp#... src/ports/SkFontHost_win.cpp:662: bool reset_table = true; On 2011/05/04 20:31:53, Steve VanDeBogart wrote: > foundUnicodeMap = false ? Done. http://codereview.appspot.com/4428082/diff/4001/src/ports/SkFontHost_win.cpp#... src/ports/SkFontHost_win.cpp:664: buffer = new BYTE[buffer_size]; On 2011/05/04 20:31:53, Steve VanDeBogart wrote: > You might want to use SkAutoTDeleteArray here (in include/core/SkTemplates.h) Done. http://codereview.appspot.com/4428082/diff/4001/src/ports/SkFontHost_win.cpp#... src/ports/SkFontHost_win.cpp:668: for (DWORD i=0; i<glyph_set->cRanges; ++i) { On 2011/05/04 20:31:53, Steve VanDeBogart wrote: > more spaces here and the next line. Done. http://codereview.appspot.com/4428082/diff/4001/src/ports/SkFontHost_win.cpp#... src/ports/SkFontHost_win.cpp:683: // 1. Just carry over the text into PDF. On 2011/05/04 20:31:53, Steve VanDeBogart wrote: > As far as I know, there's no way to do this. Done. http://codereview.appspot.com/4428082/diff/4001/src/ports/SkFontHost_win.cpp#... src/ports/SkFontHost_win.cpp:698: info->fGlyphNames.reset(); On 2011/05/04 20:31:53, Steve VanDeBogart wrote: > fGlyphToUnicode Done. http://codereview.appspot.com/4428082/diff/4001/src/ports/SkFontHost_win.cpp#... src/ports/SkFontHost_win.cpp:700: } // if perGlyphInfo contains advanced bit On 2011/05/04 20:31:53, Steve VanDeBogart wrote: > Update or remove this comment as well. Done.
Sign in to reply to this message.
Updated wrong coding style usage
Sign in to reply to this message.
Most of the comments are just style nits. The questions that I still have about this CL: 1) Have you tested ToUnicode for Type 3 fonts? for Conjugate pairs? 2) We should document any constraints i.e. it doesn't support unicode code points that require conjugate pairs in utf16. 3) We want Linux support as well. If you want to do that in a follow up CL, say so. http://codereview.appspot.com/4428082/diff/16001/include/core/SkAdvancedTypef... File include/core/SkAdvancedTypefaceMetrics.h (right): http://codereview.appspot.com/4428082/diff/16001/include/core/SkAdvancedTypef... include/core/SkAdvancedTypefaceMetrics.h:85: kToUnicode_PerGlyphInfo = 0x8, // Populate ToUnicode table. nit: Note that this flag is ignored for Type 1 fonts. http://codereview.appspot.com/4428082/diff/16001/include/core/SkAdvancedTypef... include/core/SkAdvancedTypefaceMetrics.h:118: // The mappings from glyph to Unicode, only populated if kToUnicodeInfo is mappings -> mapping kToUnicodeInfo -> kToUnicode_PerGlyphInfo. http://codereview.appspot.com/4428082/diff/16001/src/pdf/SkPDFFont.cpp File src/pdf/SkPDFFont.cpp (right): http://codereview.appspot.com/4428082/diff/16001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:322: static void generate_tounicode_header(SkDynamicMemoryWStream* cmap) { append_tounicode_header, to be consistent http://codereview.appspot.com/4428082/diff/16001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:325: "12 dict begin\n" \ What's this 12 mean? http://codereview.appspot.com/4428082/diff/16001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:388: size_t currentGlyphIndex = 0; nit: maybe index would be a better name? http://codereview.appspot.com/4428082/diff/16001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:401: if (currentGlyphIndex) Skia style is to always use {}'s http://codereview.appspot.com/4428082/diff/16001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:492: info = SkTBitOr<SkAdvancedTypefaceMetrics::PerGlyphInfo>(info, info should drop to the next line. (The first argument on any line should line up with the arguments of the previous line). http://codereview.appspot.com/4428082/diff/16001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:607: fFontInfo->fGlyphToUnicode.begin() == NULL) {}'s http://codereview.appspot.com/4428082/diff/16001/src/ports/SkFontHost_win.cpp File src/ports/SkFontHost_win.cpp (right): http://codereview.appspot.com/4428082/diff/16001/src/ports/SkFontHost_win.cpp... src/ports/SkFontHost_win.cpp:659: DWORD bufferSize = GetFontUnicodeRanges(hdc, NULL); glyphSetBufferSize ? http://codereview.appspot.com/4428082/diff/16001/src/ports/SkFontHost_win.cpp... src/ports/SkFontHost_win.cpp:666: foundUnicodeMap = true; Can you not just resize and zero fGlyphToUnicode here? http://codereview.appspot.com/4428082/diff/16001/src/ports/SkFontHost_win.cpp... src/ports/SkFontHost_win.cpp:681: // TODO: better improve this. e.g. Get all used char TODO's always need an owner... TODO(arthurhsu) http://codereview.appspot.com/4428082/diff/16001/src/ports/SkFontHost_win.cpp... src/ports/SkFontHost_win.cpp:688: info->fGlyphToUnicode[glyph] = s[0]; skia indent is 4
Sign in to reply to this message.
The code is tested against both TTF and Type 3 (converted Type 1) on Windows. Linux implementation will be in a separate CL. http://codereview.appspot.com/4428082/diff/16001/include/core/SkAdvancedTypef... File include/core/SkAdvancedTypefaceMetrics.h (right): http://codereview.appspot.com/4428082/diff/16001/include/core/SkAdvancedTypef... include/core/SkAdvancedTypefaceMetrics.h:85: kToUnicode_PerGlyphInfo = 0x8, // Populate ToUnicode table. On 2011/05/05 21:37:32, Steve VanDeBogart wrote: > nit: Note that this flag is ignored for Type 1 fonts. Done. http://codereview.appspot.com/4428082/diff/16001/include/core/SkAdvancedTypef... include/core/SkAdvancedTypefaceMetrics.h:118: // The mappings from glyph to Unicode, only populated if kToUnicodeInfo is On 2011/05/05 21:37:32, Steve VanDeBogart wrote: > mappings -> mapping > kToUnicodeInfo -> kToUnicode_PerGlyphInfo. Done. http://codereview.appspot.com/4428082/diff/16001/src/pdf/SkPDFFont.cpp File src/pdf/SkPDFFont.cpp (right): http://codereview.appspot.com/4428082/diff/16001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:322: static void generate_tounicode_header(SkDynamicMemoryWStream* cmap) { On 2011/05/05 21:37:32, Steve VanDeBogart wrote: > append_tounicode_header, to be consistent Done. http://codereview.appspot.com/4428082/diff/16001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:325: "12 dict begin\n" \ On 2011/05/05 21:37:32, Steve VanDeBogart wrote: > What's this 12 mean? Adobe suggested value. For this "dict" (actually it's not), there are 8 entries, and Adobe suggested putting 3 to 4 more than 8 to prevent old version of Adobe Reader from malfuntion. So 12 it is. http://codereview.appspot.com/4428082/diff/16001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:388: size_t currentGlyphIndex = 0; On 2011/05/05 21:37:32, Steve VanDeBogart wrote: > nit: maybe index would be a better name? Done. http://codereview.appspot.com/4428082/diff/16001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:401: if (currentGlyphIndex) On 2011/05/05 21:37:32, Steve VanDeBogart wrote: > Skia style is to always use {}'s Done. http://codereview.appspot.com/4428082/diff/16001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:492: info = SkTBitOr<SkAdvancedTypefaceMetrics::PerGlyphInfo>(info, On 2011/05/05 21:37:32, Steve VanDeBogart wrote: > info should drop to the next line. (The first argument on any line should line > up with the arguments of the previous line). Done. http://codereview.appspot.com/4428082/diff/16001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:607: fFontInfo->fGlyphToUnicode.begin() == NULL) On 2011/05/05 21:37:32, Steve VanDeBogart wrote: > {}'s Done. http://codereview.appspot.com/4428082/diff/16001/src/ports/SkFontHost_win.cpp File src/ports/SkFontHost_win.cpp (right): http://codereview.appspot.com/4428082/diff/16001/src/ports/SkFontHost_win.cpp... src/ports/SkFontHost_win.cpp:659: DWORD bufferSize = GetFontUnicodeRanges(hdc, NULL); On 2011/05/05 21:37:32, Steve VanDeBogart wrote: > glyphSetBufferSize ? Done. http://codereview.appspot.com/4428082/diff/16001/src/ports/SkFontHost_win.cpp... src/ports/SkFontHost_win.cpp:666: foundUnicodeMap = true; On 2011/05/05 21:37:32, Steve VanDeBogart wrote: > Can you not just resize and zero fGlyphToUnicode here? Done. http://codereview.appspot.com/4428082/diff/16001/src/ports/SkFontHost_win.cpp... src/ports/SkFontHost_win.cpp:681: // TODO: better improve this. e.g. Get all used char On 2011/05/05 21:37:32, Steve VanDeBogart wrote: > TODO's always need an owner... TODO(arthurhsu) Done. http://codereview.appspot.com/4428082/diff/16001/src/ports/SkFontHost_win.cpp... src/ports/SkFontHost_win.cpp:688: info->fGlyphToUnicode[glyph] = s[0]; On 2011/05/05 21:37:32, Steve VanDeBogart wrote: > skia indent is 4 Done.
Sign in to reply to this message.
Chris, can you give this a once over when you get a chance?
Sign in to reply to this message.
On 2011/05/05 22:30:44, Steve VanDeBogart wrote: > Chris, can you give this a once over when you get a chance? Yes. I want to look this over also.
Sign in to reply to this message.
Sorry, Linux port is much easier than I originally expected, therefore I add it to the same CL. The major difference is that Linux port supports UCS-32 thanks to FreeType's built-in support.
Sign in to reply to this message.
Comments on Linux implementation. http://codereview.appspot.com/4428082/diff/23001/src/ports/SkFontHost_FreeTyp... File src/ports/SkFontHost_FreeType.cpp (right): http://codereview.appspot.com/4428082/diff/23001/src/ports/SkFontHost_FreeTyp... src/ports/SkFontHost_FreeType.cpp:513: info->fType == SkAdvancedTypefaceMetrics::kTrueType_Font && Should this be != Type1 instead of == TrueType ? http://codereview.appspot.com/4428082/diff/23001/src/ports/SkFontHost_FreeTyp... src/ports/SkFontHost_FreeType.cpp:546: // Keep walking if the current charmap is not Unicode. This entire logic can be made much simpler: if (face->charmaps[i]->platform_id != 3 || (face->charmaps[i]->encoding_id != 1 && face->charmaps[i]->encoding_id != 10)) { continue; } http://codereview.appspot.com/4428082/diff/23001/src/ports/SkFontHost_FreeTyp... src/ports/SkFontHost_FreeType.cpp:553: face->charmaps[i]->platform_id == 2)); platform_id == 2 is trivially false. http://codereview.appspot.com/4428082/diff/23001/src/ports/SkFontHost_FreeTyp... src/ports/SkFontHost_FreeType.cpp:564: SkUnichar charCode = FT_Get_First_Char(face, &glyphIndex); Would it be easy to combine this if and while? Maybe with a do { } while, or with a break? http://codereview.appspot.com/4428082/diff/23001/src/ports/SkFontHost_FreeTyp... src/ports/SkFontHost_FreeType.cpp:832: SkUnichar SkScalerContext_FreeType::generateGlyphToChar(uint16_t glyph) { extra whitespace
Sign in to reply to this message.
global comment (not meant to halt this CL) getAdvancedType...() is probably large and complex enough that it could be in its own file. http://codereview.appspot.com/4428082/diff/23001/src/ports/SkFontHost_FreeTyp... File src/ports/SkFontHost_FreeType.cpp (left): http://codereview.appspot.com/4428082/diff/23001/src/ports/SkFontHost_FreeTyp... src/ports/SkFontHost_FreeType.cpp:511: This large block might justify a local helper function... loadGlyphToUnichar(array, face, ...) Just to encapsulate the functionality, and make the outer function easy to see at a glance.
Sign in to reply to this message.
http://codereview.appspot.com/4428082/diff/23001/src/ports/SkFontHost_FreeTyp... File src/ports/SkFontHost_FreeType.cpp (left): http://codereview.appspot.com/4428082/diff/23001/src/ports/SkFontHost_FreeTyp... src/ports/SkFontHost_FreeType.cpp:511: On 2011/05/06 17:25:14, reed1 wrote: > This large block might justify a local helper function... > > loadGlyphToUnichar(array, face, ...) > > Just to encapsulate the functionality, and make the outer function easy to see > at a glance. Done. http://codereview.appspot.com/4428082/diff/23001/src/ports/SkFontHost_FreeTyp... File src/ports/SkFontHost_FreeType.cpp (right): http://codereview.appspot.com/4428082/diff/23001/src/ports/SkFontHost_FreeTyp... src/ports/SkFontHost_FreeType.cpp:513: info->fType == SkAdvancedTypefaceMetrics::kTrueType_Font && On 2011/05/06 16:52:02, Steve VanDeBogart wrote: > Should this be != Type1 instead of == TrueType ? Done. http://codereview.appspot.com/4428082/diff/23001/src/ports/SkFontHost_FreeTyp... src/ports/SkFontHost_FreeType.cpp:546: // Keep walking if the current charmap is not Unicode. On 2011/05/06 16:52:02, Steve VanDeBogart wrote: > This entire logic can be made much simpler: > > if (face->charmaps[i]->platform_id != 3 || (face->charmaps[i]->encoding_id != 1 > && face->charmaps[i]->encoding_id != 10)) { continue; } When platform_id == 0 we need to let it continue (Apple Unicode). Suggested new logic will block Apple fonts. I've updated the logic to be simpler. http://codereview.appspot.com/4428082/diff/23001/src/ports/SkFontHost_FreeTyp... src/ports/SkFontHost_FreeType.cpp:553: face->charmaps[i]->platform_id == 2)); On 2011/05/06 16:52:02, Steve VanDeBogart wrote: > platform_id == 2 is trivially false. Fixed, should be Apple's, not platform_id == 2. Thanks for the catch. http://codereview.appspot.com/4428082/diff/23001/src/ports/SkFontHost_FreeTyp... src/ports/SkFontHost_FreeType.cpp:564: SkUnichar charCode = FT_Get_First_Char(face, &glyphIndex); On 2011/05/06 16:52:02, Steve VanDeBogart wrote: > Would it be easy to combine this if and while? Maybe with a do { } while, or > with a break? This is a get-first/get-next pattern. I'd like to know if there's a simpler way, too. http://codereview.appspot.com/4428082/diff/23001/src/ports/SkFontHost_FreeTyp... src/ports/SkFontHost_FreeType.cpp:832: SkUnichar SkScalerContext_FreeType::generateGlyphToChar(uint16_t glyph) { On 2011/05/06 16:52:02, Steve VanDeBogart wrote: > extra whitespace Done.
Sign in to reply to this message.
http://codereview.appspot.com/4428082/diff/23001/src/ports/SkFontHost_FreeTyp... File src/ports/SkFontHost_FreeType.cpp (right): http://codereview.appspot.com/4428082/diff/23001/src/ports/SkFontHost_FreeTyp... src/ports/SkFontHost_FreeType.cpp:546: // Keep walking if the current charmap is not Unicode. On 2011/05/06 17:45:31, arthurhsu wrote: > On 2011/05/06 16:52:02, Steve VanDeBogart wrote: > > This entire logic can be made much simpler: > > > > if (face->charmaps[i]->platform_id != 3 || (face->charmaps[i]->encoding_id != > 1 > > && face->charmaps[i]->encoding_id != 10)) { continue; } > > When platform_id == 0 we need to let it continue (Apple Unicode). Suggested new > logic will block Apple fonts. I've updated the logic to be simpler. Oh, I missed that fall through - they can be tricky :-) http://codereview.appspot.com/4428082/diff/23001/src/ports/SkFontHost_FreeTyp... src/ports/SkFontHost_FreeType.cpp:564: SkUnichar charCode = FT_Get_First_Char(face, &glyphIndex); Won't this work? FT_UInt glyphIndex; for(SkUnichar charCode = FT_Get_First_Char(face, &glyphIndex), glyphIndex != 0, charCode = FT_Get_Next_Char(face, charCode, &glyphIndex)) { if (charCode && (info->fGlyphToUnicode[glyphIndex] == 0 || preferredMap)) { info->fGlyphToUnicode[glyphIndex] = charCode; } } http://codereview.appspot.com/4428082/diff/26001/src/ports/SkFontHost_FreeTyp... src/ports/SkFontHost_FreeType.cpp:136: uint32_t fLoadGlyphFlags; Let's swap some lines so we test platform then encoding (easier to read). i.e. (p=3 && e=10) || (p=0 && e=3)
Sign in to reply to this message.
http://codereview.appspot.com/4428082/diff/23001/src/ports/SkFontHost_FreeTyp... File src/ports/SkFontHost_FreeType.cpp (right): http://codereview.appspot.com/4428082/diff/23001/src/ports/SkFontHost_FreeTyp... src/ports/SkFontHost_FreeType.cpp:564: SkUnichar charCode = FT_Get_First_Char(face, &glyphIndex); On 2011/05/06 18:25:37, Steve VanDeBogart wrote: > Won't this work? > > > FT_UInt glyphIndex; > for(SkUnichar charCode = FT_Get_First_Char(face, &glyphIndex), > glyphIndex != 0, > charCode = FT_Get_Next_Char(face, charCode, &glyphIndex)) { > if (charCode && (info->fGlyphToUnicode[glyphIndex] == 0 || preferredMap)) { > info->fGlyphToUnicode[glyphIndex] = charCode; > } > } Done. http://codereview.appspot.com/4428082/diff/26001/src/ports/SkFontHost_FreeTyp... src/ports/SkFontHost_FreeType.cpp:136: uint32_t fLoadGlyphFlags; On 2011/05/06 18:25:37, Steve VanDeBogart wrote: > Let's swap some lines so we test platform then encoding (easier to read). i.e. > (p=3 && e=10) || (p=0 && e=3) Done.
Sign in to reply to this message.
Looking good. http://codereview.appspot.com/4428082/diff/4001/include/core/SkAdvancedTypefa... File include/core/SkAdvancedTypefaceMetrics.h (right): http://codereview.appspot.com/4428082/diff/4001/include/core/SkAdvancedTypefa... include/core/SkAdvancedTypefaceMetrics.h:118: // The mappings from glyph to Unicode. Default is empty. Nit: Usually we have one space between sentences. http://codereview.appspot.com/4428082/diff/31001/include/core/SkAdvancedTypef... include/core/SkAdvancedTypefaceMetrics.h:81: kNo_PerGlyphInfo = 0x0, // Don't populate any per glyph info. Nit: Nit: No need to add extra spaces on these lines. http://codereview.appspot.com/4428082/diff/31001/src/pdf/SkPDFFont.cpp File src/pdf/SkPDFFont.cpp (right): http://codereview.appspot.com/4428082/diff/31001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:326: "/CIDInit /ProcSet findresource begin\n" \ Nit: Seams cleaner to remove the trailing '\' here and elsewhere in favor of string literal concatenation. http://codereview.appspot.com/4428082/diff/31001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:334: // different. This is not a reference object. Nit: One space between sentences. http://codereview.appspot.com/4428082/diff/31001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:346: // code table from unsigned short (16-bits). Codespace range just tells Nit: One space between sentences. http://codereview.appspot.com/4428082/diff/31001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:619: mapObj->unref(); Comment: // SkRefPtr and new took a reference. http://codereview.appspot.com/4428082/diff/31001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:622: pdfMap->unref(); Comment: // SkRefPtr and new took a reference. http://codereview.appspot.com/4428082/diff/31001/src/ports/SkFontHost_win.cpp File src/ports/SkFontHost_win.cpp (right): http://codereview.appspot.com/4428082/diff/31001/src/ports/SkFontHost_win.cpp... src/ports/SkFontHost_win.cpp:656: // GetFontUnicodeRanges(). TTF cmap table format 4/12 handles conjugate Nit: One space between sentences. http://codereview.appspot.com/4428082/diff/31001/src/ports/SkFontHost_win.cpp... src/ports/SkFontHost_win.cpp:669: glyphCount * sizeof(uint16_t)); sizeof(uint16_t) -> sizeof(SkUnichar) http://codereview.appspot.com/4428082/diff/31001/src/ports/SkFontHost_win.cpp... src/ports/SkFontHost_win.cpp:672: // There is no guarantee that within a Unicode range, Not following, why can we not make one call to GetGlyphIndices for the entire WCRANGE here? We have GGI_MARK_NONEXISTING_GLYPHS. http://codereview.appspot.com/4428082/diff/31001/src/ports/SkFontHost_win.cpp... src/ports/SkFontHost_win.cpp:676: TCHAR s[2] = {0}; Nit: Why not TCHAR -> WCHAR. http://codereview.appspot.com/4428082/diff/31001/src/ports/SkFontHost_win.cpp... src/ports/SkFontHost_win.cpp:697: } // if perGlyphInfo contains kToUnicode_PerGlyphInfo bit Nit: If we need this comment then there should probably be a different function to populate info->fGlyphToUnicode.
Sign in to reply to this message.
http://codereview.appspot.com/4428082/diff/4001/include/core/SkAdvancedTypefa... File include/core/SkAdvancedTypefaceMetrics.h (right): http://codereview.appspot.com/4428082/diff/4001/include/core/SkAdvancedTypefa... include/core/SkAdvancedTypefaceMetrics.h:118: // The mappings from glyph to Unicode. Default is empty. On 2011/05/06 22:21:45, Chris Guillory wrote: > Nit: Usually we have one space between sentences. Done in newer check-ins. http://codereview.appspot.com/4428082/diff/31001/include/core/SkAdvancedTypef... include/core/SkAdvancedTypefaceMetrics.h:81: kNo_PerGlyphInfo = 0x0, // Don't populate any per glyph info. On 2011/05/06 22:21:45, Chris Guillory wrote: > Nit: Nit: No need to add extra spaces on these lines. Done. http://codereview.appspot.com/4428082/diff/31001/src/pdf/SkPDFFont.cpp File src/pdf/SkPDFFont.cpp (right): http://codereview.appspot.com/4428082/diff/31001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:326: "/CIDInit /ProcSet findresource begin\n" \ On 2011/05/06 22:21:45, Chris Guillory wrote: > Nit: Seams cleaner to remove the trailing '\' here and elsewhere in favor of > string literal concatenation. Done. http://codereview.appspot.com/4428082/diff/31001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:334: // different. This is not a reference object. On 2011/05/06 22:21:45, Chris Guillory wrote: > Nit: One space between sentences. Done. http://codereview.appspot.com/4428082/diff/31001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:346: // code table from unsigned short (16-bits). Codespace range just tells On 2011/05/06 22:21:45, Chris Guillory wrote: > Nit: One space between sentences. Done. http://codereview.appspot.com/4428082/diff/31001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:619: mapObj->unref(); On 2011/05/06 22:21:45, Chris Guillory wrote: > Comment: // SkRefPtr and new took a reference. Done. http://codereview.appspot.com/4428082/diff/31001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:622: pdfMap->unref(); On 2011/05/06 22:21:45, Chris Guillory wrote: > Comment: // SkRefPtr and new took a reference. Done. http://codereview.appspot.com/4428082/diff/31001/src/ports/SkFontHost_win.cpp File src/ports/SkFontHost_win.cpp (right): http://codereview.appspot.com/4428082/diff/31001/src/ports/SkFontHost_win.cpp... src/ports/SkFontHost_win.cpp:656: // GetFontUnicodeRanges(). TTF cmap table format 4/12 handles conjugate On 2011/05/06 22:21:45, Chris Guillory wrote: > Nit: One space between sentences. Done. http://codereview.appspot.com/4428082/diff/31001/src/ports/SkFontHost_win.cpp... src/ports/SkFontHost_win.cpp:669: glyphCount * sizeof(uint16_t)); On 2011/05/06 22:21:45, Chris Guillory wrote: > sizeof(uint16_t) -> sizeof(SkUnichar) Done. http://codereview.appspot.com/4428082/diff/31001/src/ports/SkFontHost_win.cpp... src/ports/SkFontHost_win.cpp:672: // There is no guarantee that within a Unicode range, On 2011/05/06 22:21:45, Chris Guillory wrote: > Not following, why can we not make one call to GetGlyphIndices for the entire > WCRANGE here? We have GGI_MARK_NONEXISTING_GLYPHS. Done. http://codereview.appspot.com/4428082/diff/31001/src/ports/SkFontHost_win.cpp... src/ports/SkFontHost_win.cpp:676: TCHAR s[2] = {0}; On 2011/05/06 22:21:45, Chris Guillory wrote: > Nit: Why not TCHAR -> WCHAR. Done. http://codereview.appspot.com/4428082/diff/31001/src/ports/SkFontHost_win.cpp... src/ports/SkFontHost_win.cpp:697: } // if perGlyphInfo contains kToUnicode_PerGlyphInfo bit On 2011/05/06 22:21:45, Chris Guillory wrote: > Nit: If we need this comment then there should probably be a different function > to populate info->fGlyphToUnicode. Done.
Sign in to reply to this message.
LGTM with Nits. http://codereview.appspot.com/4428082/diff/26002/src/ports/SkFontHost_win.cpp File src/ports/SkFontHost_win.cpp (right): http://codereview.appspot.com/4428082/diff/26002/src/ports/SkFontHost_win.cpp... src/ports/SkFontHost_win.cpp:207: if (glyphSetBufferSize) { Nit: Can change to: if (!glyphSetBufferSize) return; http://codereview.appspot.com/4428082/diff/26002/src/ports/SkFontHost_win.cpp... src/ports/SkFontHost_win.cpp:212: if (GetFontUnicodeRanges(fontHdc, glyphSet) == glyphSetBufferSize) { Nit: Can change if to do early return. http://codereview.appspot.com/4428082/diff/26002/src/ports/SkFontHost_win.cpp... src/ports/SkFontHost_win.cpp:219: // Therefore, we need to enumerate them one by one. This comment seems out of place. Is it referring to the last for loop? http://codereview.appspot.com/4428082/diff/26002/src/ports/SkFontHost_win.cpp... src/ports/SkFontHost_win.cpp:221: SkAutoTArray<WCHAR> s(count + 1); Nit: Optional: s can be more descriptive. s -> chars? s -> string?
Sign in to reply to this message.
In Skia convention you can remove the Test= and Bug= in the description entirely when not needed.
Sign in to reply to this message.
http://codereview.appspot.com/4428082/diff/26002/src/ports/SkFontHost_win.cpp File src/ports/SkFontHost_win.cpp (right): http://codereview.appspot.com/4428082/diff/26002/src/ports/SkFontHost_win.cpp... src/ports/SkFontHost_win.cpp:207: if (glyphSetBufferSize) { On 2011/05/07 00:41:50, Chris Guillory wrote: > Nit: Can change to: > if (!glyphSetBufferSize) > return; Done. http://codereview.appspot.com/4428082/diff/26002/src/ports/SkFontHost_win.cpp... src/ports/SkFontHost_win.cpp:212: if (GetFontUnicodeRanges(fontHdc, glyphSet) == glyphSetBufferSize) { On 2011/05/07 00:41:50, Chris Guillory wrote: > Nit: Can change if to do early return. Done. http://codereview.appspot.com/4428082/diff/26002/src/ports/SkFontHost_win.cpp... src/ports/SkFontHost_win.cpp:219: // Therefore, we need to enumerate them one by one. On 2011/05/07 00:41:50, Chris Guillory wrote: > This comment seems out of place. Is it referring to the last for loop? Updated. I mean that one can't just use the first and last entry of range to compute result. http://codereview.appspot.com/4428082/diff/26002/src/ports/SkFontHost_win.cpp... src/ports/SkFontHost_win.cpp:221: SkAutoTArray<WCHAR> s(count + 1); On 2011/05/07 00:41:50, Chris Guillory wrote: > Nit: Optional: s can be more descriptive. s -> chars? s -> string? Done.
Sign in to reply to this message.
http://codereview.appspot.com/4428082/diff/26002/src/pdf/SkPDFFont.cpp File src/pdf/SkPDFFont.cpp (right): http://codereview.appspot.com/4428082/diff/26002/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:618: SkRefPtr<SkMemoryStream> mapObj = new SkMemoryStream(); nit: mapObj -> cmapStream http://codereview.appspot.com/4428082/diff/26002/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:621: SkRefPtr<SkPDFStream> pdfMap = new SkPDFStream(mapObj.get()); nit: pdfMap -> pdfCmap or cmapObj http://codereview.appspot.com/4428082/diff/26002/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:623: fResources.push(pdfMap.get()); The ref count is wrong on pdfMap. Inserting into fResources doesn't trigger a ref() op, so you need to pdfMap->ref. However, there's a pdfMap->unref() just above, so I would do: SkRefPtr<SkPDFStream> pdfMap = new SkPDFStream(mapObj.get()); fResources.push(pdfMap.get()); // Pass reference from new. http://codereview.appspot.com/4428082/diff/26002/src/ports/SkFontHost_FreeTyp... File src/ports/SkFontHost_FreeType.cpp (right): http://codereview.appspot.com/4428082/diff/26002/src/ports/SkFontHost_FreeTyp... src/ports/SkFontHost_FreeType.cpp:108: static void loadGlyphToUnicode(FT_Face& face, static void populate_glyph_to_unicode http://codereview.appspot.com/4428082/diff/26002/src/ports/SkFontHost_FreeTyp... src/ports/SkFontHost_FreeType.cpp:108: static void loadGlyphToUnicode(FT_Face& face, Up to you, but this could live just above GetAdvacedTypefaceMetrics. http://codereview.appspot.com/4428082/diff/26002/src/ports/SkFontHost_FreeTyp... src/ports/SkFontHost_FreeType.cpp:109: SkTDArray<SkUnichar>& glyphToUnicode) { I don't think there's explicit policy in Skia, but Chrome policy (which is what I'll ask you to follow in this case) is to only use const references, so use a pointer in this case. http://codereview.appspot.com/4428082/diff/26002/src/ports/SkFontHost_FreeTyp... src/ports/SkFontHost_FreeType.cpp:124: // http://www.microsoft.com/typography/otspec/cmap.htm nit: It might make the rest of the code more readable to introduce a couple local variables, int platform, encoding http://codereview.appspot.com/4428082/diff/26002/src/ports/SkFontHost_FreeTyp... src/ports/SkFontHost_FreeType.cpp:153: if (charCode && glyphIndex && it's not possible for glyphIndex to be zero inside the loop => no reason to check it. http://codereview.appspot.com/4428082/diff/26002/src/ports/SkFontHost_win.cpp File src/ports/SkFontHost_win.cpp (right): http://codereview.appspot.com/4428082/diff/26002/src/ports/SkFontHost_win.cpp... src/ports/SkFontHost_win.cpp:200: // TODO(arthurhsu): directly parse TTF cmap table instead of calling For a todo, it's better to state the objective, and then note how you might accomplish it. i.e. TODO(arthurhsu): Add support for conjugate pairs. It looks like that may require parsing the TTF cmap table (platform 4, encoding 12) directly instead of calling GetFontUnicodeRange(). http://codereview.appspot.com/4428082/diff/26002/src/ports/SkFontHost_win.cpp... src/ports/SkFontHost_win.cpp:203: static void generateGlyphToUnicode(HDC fontHdc, const unsigned glyphCount, static void populate_glyph_to_unicode http://codereview.appspot.com/4428082/diff/26002/src/ports/SkFontHost_win.cpp... src/ports/SkFontHost_win.cpp:204: SkTDArray<SkUnichar>& glyphToUnicode) { SkTDArray<SkUniChar>* http://codereview.appspot.com/4428082/diff/26002/src/ports/SkFontHost_win.cpp... src/ports/SkFontHost_win.cpp:205: // Get font Unicode ranges. This comment is highly redundant with the code.
Sign in to reply to this message.
http://codereview.appspot.com/4428082/diff/26002/src/pdf/SkPDFFont.cpp File src/pdf/SkPDFFont.cpp (right): http://codereview.appspot.com/4428082/diff/26002/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:618: SkRefPtr<SkMemoryStream> mapObj = new SkMemoryStream(); On 2011/05/07 01:04:40, Steve VanDeBogart wrote: > nit: mapObj -> cmapStream Done. http://codereview.appspot.com/4428082/diff/26002/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:621: SkRefPtr<SkPDFStream> pdfMap = new SkPDFStream(mapObj.get()); On 2011/05/07 01:04:40, Steve VanDeBogart wrote: > nit: pdfMap -> pdfCmap or cmapObj Done. http://codereview.appspot.com/4428082/diff/26002/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:623: fResources.push(pdfMap.get()); On 2011/05/07 01:04:40, Steve VanDeBogart wrote: > The ref count is wrong on pdfMap. Inserting into fResources doesn't trigger a > ref() op, so you need to pdfMap->ref. However, there's a pdfMap->unref() just > above, so I would do: > SkRefPtr<SkPDFStream> pdfMap = new SkPDFStream(mapObj.get()); > fResources.push(pdfMap.get()); // Pass reference from new. Done. http://codereview.appspot.com/4428082/diff/26002/src/ports/SkFontHost_FreeTyp... File src/ports/SkFontHost_FreeType.cpp (right): http://codereview.appspot.com/4428082/diff/26002/src/ports/SkFontHost_FreeTyp... src/ports/SkFontHost_FreeType.cpp:108: static void loadGlyphToUnicode(FT_Face& face, On 2011/05/07 01:04:40, Steve VanDeBogart wrote: > static void populate_glyph_to_unicode Done. http://codereview.appspot.com/4428082/diff/26002/src/ports/SkFontHost_FreeTyp... src/ports/SkFontHost_FreeType.cpp:109: SkTDArray<SkUnichar>& glyphToUnicode) { On 2011/05/07 01:04:40, Steve VanDeBogart wrote: > I don't think there's explicit policy in Skia, but Chrome policy (which is what > I'll ask you to follow in this case) is to only use const references, so use a > pointer in this case. Done. http://codereview.appspot.com/4428082/diff/26002/src/ports/SkFontHost_FreeTyp... src/ports/SkFontHost_FreeType.cpp:124: // http://www.microsoft.com/typography/otspec/cmap.htm On 2011/05/07 01:04:40, Steve VanDeBogart wrote: > nit: It might make the rest of the code more readable to introduce a couple > local variables, int platform, encoding Done. http://codereview.appspot.com/4428082/diff/26002/src/ports/SkFontHost_FreeTyp... src/ports/SkFontHost_FreeType.cpp:153: if (charCode && glyphIndex && On 2011/05/07 01:04:40, Steve VanDeBogart wrote: > it's not possible for glyphIndex to be zero inside the loop => no reason to > check it. When FT_Get_Next_Char() is not able to get any, it will set glyphIndex to zero, and that's the loop's stop condition. http://codereview.appspot.com/4428082/diff/26002/src/ports/SkFontHost_win.cpp File src/ports/SkFontHost_win.cpp (right): http://codereview.appspot.com/4428082/diff/26002/src/ports/SkFontHost_win.cpp... src/ports/SkFontHost_win.cpp:200: // TODO(arthurhsu): directly parse TTF cmap table instead of calling On 2011/05/07 01:04:40, Steve VanDeBogart wrote: > For a todo, it's better to state the objective, and then note how you might > accomplish it. i.e. > TODO(arthurhsu): Add support for conjugate pairs. It looks like that may > require parsing the TTF cmap table (platform 4, encoding 12) directly instead of > calling GetFontUnicodeRange(). Done. http://codereview.appspot.com/4428082/diff/26002/src/ports/SkFontHost_win.cpp... src/ports/SkFontHost_win.cpp:203: static void generateGlyphToUnicode(HDC fontHdc, const unsigned glyphCount, On 2011/05/07 01:04:40, Steve VanDeBogart wrote: > static void populate_glyph_to_unicode Done. http://codereview.appspot.com/4428082/diff/26002/src/ports/SkFontHost_win.cpp... src/ports/SkFontHost_win.cpp:204: SkTDArray<SkUnichar>& glyphToUnicode) { On 2011/05/07 01:04:40, Steve VanDeBogart wrote: > SkTDArray<SkUniChar>* Done. http://codereview.appspot.com/4428082/diff/26002/src/ports/SkFontHost_win.cpp... src/ports/SkFontHost_win.cpp:205: // Get font Unicode ranges. On 2011/05/07 01:04:40, Steve VanDeBogart wrote: > This comment is highly redundant with the code. Done.
Sign in to reply to this message.
This looks good. I'll pull down the patch and push it into the tree on your behalf. http://codereview.appspot.com/4428082/diff/26002/src/ports/SkFontHost_FreeTyp... File src/ports/SkFontHost_FreeType.cpp (right): http://codereview.appspot.com/4428082/diff/26002/src/ports/SkFontHost_FreeTyp... src/ports/SkFontHost_FreeType.cpp:153: if (charCode && glyphIndex && On 2011/05/09 17:45:56, arthurhsu wrote: > On 2011/05/07 01:04:40, Steve VanDeBogart wrote: > > it's not possible for glyphIndex to be zero inside the loop => no reason to > > check it. > > When FT_Get_Next_Char() is not able to get any, it will set glyphIndex to zero, > and that's the loop's stop condition. Right, so inside the loop, it is trivially true.
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
