Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8744002:
Roll sfntly 111 (Closed)
|
Created:
9 years ago by arthurhsu Modified:
9 years ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionRoll sfntly 111
BUG=90898
TEST=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=112852
Patch Set 1 #
Total comments: 91
Patch Set 2 : Update per code review #Patch Set 3 : Update per code review #
Total comments: 10
Patch Set 4 : Update per code review #
Total comments: 15
Patch Set 5 : Update per code review #Patch Set 6 : Rebase to master #Patch Set 7 : Rebase to master #
Messages
Total messages: 12 (0 generated)
This roll provides bitmap font subsetting and fixed sfntly compilation warnings. Nico: please review the gyp file. Steve: please review the code. Thanks.
gyp changes lgtm. Thanks! http://codereview.chromium.org/8744002/diff/1/third_party/sfntly/sfntly.gyp File third_party/sfntly/sfntly.gyp (right): http://codereview.chromium.org/8744002/diff/1/third_party/sfntly/sfntly.gyp#n... third_party/sfntly/sfntly.gyp:8: }, \o/ Awesome! http://codereview.chromium.org/8744002/diff/1/third_party/sfntly/src/subsette... File third_party/sfntly/src/subsetter/subsetter_impl.cc (right): http://codereview.chromium.org/8744002/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/subsetter_impl.cc:42: #endif Are you sure you need this? In chrome, this bit from third_party/icu/icu.gyp should set things up correctly already: 'target_defaults': { 'direct_dependent_settings': { 'defines': [ # Tell ICU to not insert |using namespace icu;| into its headers, # so that chrome's source explicitly has to use |icu::|. 'U_USING_ICU_NAMESPACE=0', ], }, http://codereview.chromium.org/8744002/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/subsetter_impl.cc:151: return (*b).p_; b->p_ instead of (*b).p_?
http://codereview.chromium.org/8744002/diff/1/third_party/sfntly/src/subsette... File third_party/sfntly/src/subsetter/subsetter_impl.cc (right): http://codereview.chromium.org/8744002/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/subsetter_impl.cc:42: #endif It's not done so because upstream sfntly does not have the support (and yes, this code you see here actually has a copy in upstream's test). On 2011/11/30 18:19:05, Nico wrote: > Are you sure you need this? > > In chrome, this bit from third_party/icu/icu.gyp should set things up correctly > already: > > 'target_defaults': { > 'direct_dependent_settings': { > 'defines': [ > # Tell ICU to not insert |using namespace icu;| into its headers, > # so that chrome's source explicitly has to use |icu::|. > 'U_USING_ICU_NAMESPACE=0', > ], > }, http://codereview.chromium.org/8744002/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/subsetter_impl.cc:151: return (*b).p_; On 2011/11/30 18:19:05, Nico wrote: > b->p_ instead of (*b).p_? Done.
http://codereview.chromium.org/8744002/diff/1/third_party/sfntly/src/subsette... File third_party/sfntly/src/subsetter/subsetter_impl.cc (right): http://codereview.chromium.org/8744002/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/subsetter_impl.cc:53: void ConstructName(UChar* name_part, UnicodeString* name, int32_t name_id) { Shouldn't this be in an anonymous namespace? The same goes for all the non-class member functions. http://codereview.chromium.org/8744002/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/subsetter_impl.cc:148: for (FontArray::const_iterator b = font_array.begin(), e = font_array.end(); nit: b -> i, e -> end ? http://codereview.chromium.org/8744002/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/subsetter_impl.cc:227: bool SetupGlyfBuilders(Font::Builder* builder, nit: SetUp http://codereview.chromium.org/8744002/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/subsetter_impl.cc:303: bool ShallSubset(EbdtTable::Builder* ebdt, EblcTable::Builder* eblc, better name? SetUpBitmapBuilder? http://codereview.chromium.org/8744002/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/subsetter_impl.cc:305: EblcTableBuilderPtr eblc_builder = eblc; Do you need a smart pointer here? The caller isn't going to give up its reference in the lifetime of this function. http://codereview.chromium.org/8744002/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/subsetter_impl.cc:317: for (size_t i = 0; i < strikes->size(); i++) { I didn't follow all the details here, but this looks like an n^3 algorithm. Is there any easy way to reduce the complexity? http://codereview.chromium.org/8744002/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/subsetter_impl.cc:359: e = removed_indexes.rend(); nit: e -> end http://codereview.chromium.org/8744002/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/subsetter_impl.cc:365: return false; // All strikes shall be gone. nit: remove comment, redundant http://codereview.chromium.org/8744002/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/subsetter_impl.cc:368: // Remove unused strikes nit: Remove comment http://codereview.chromium.org/8744002/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/subsetter_impl.cc:369: for (IntegerList::reverse_iterator j = removed_strikes.rbegin(), nit: j -> i http://codereview.chromium.org/8744002/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/subsetter_impl.cc:370: e = removed_strikes.rend(); j != e; j++) { nit: e -> end http://codereview.chromium.org/8744002/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/subsetter_impl.cc:410: for (BitmapGlyphInfoMap::const_iterator i = loca.begin(), e = loca.end(); nit: e -> end http://codereview.chromium.org/8744002/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/subsetter_impl.cc:413: if (gid < lower_bound) { Could you search for lower_bound outside the loop and start the loop there? Or does BitmapGlyphInfoMap not let you search? http://codereview.chromium.org/8744002/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/subsetter_impl.cc:436: offset_pairs.push_back( If you pull this to after the loop, I think you can get rid of last_element and upper_bound_reached. http://codereview.chromium.org/8744002/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/subsetter_impl.cc:478: for (BitmapGlyphInfoMap::const_iterator i = loca.begin(), e = loca.end(); Similar comments apply to this loop. http://codereview.chromium.org/8744002/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/subsetter_impl.cc:522: assert(false); // Shall not be here. Remove comment. http://codereview.chromium.org/8744002/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/subsetter_impl.cc:529: EblcTableBuilderPtr eblc_builder = eblc; No need for a smart pointer here - the caller holds a reference. http://codereview.chromium.org/8744002/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/subsetter_impl.cc:532: return; // No valid EBLC. redundant comment http://codereview.chromium.org/8744002/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/subsetter_impl.cc:550: /****************************************************************************** // style comments are more common than /* */ style. The style guide says to use one or the other. http://codereview.chromium.org/8744002/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/subsetter_impl.cc:551: Long background comments This line isn't necessary http://codereview.chromium.org/8744002/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/subsetter_impl.cc:581: font will have sparse bitmap glyphs. As a result, the EBLC table shall be nit: "As a result..." -> "So reconstruct it as a format 4 or 5 table." http://codereview.chromium.org/8744002/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/subsetter_impl.cc:584: bool SetupBitmapBuilders(Font* font, Font::Builder* builder, nit: SetUp http://codereview.chromium.org/8744002/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/subsetter_impl.cc:590: EbdtTablePtr ebdt_table = This may be the ebdt or bdat table, so calling it the ebdt table could be confusion. Is there a generic name you can use for either table? http://codereview.chromium.org/8744002/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/subsetter_impl.cc:639: if (ebdt_table == NULL && eblc_table == NULL) { What happens if we have just one of these two tables? http://codereview.chromium.org/8744002/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/subsetter_impl.cc:720: According to PDF spec (section 9.9), the following tables must present: nit: must be present http://codereview.chromium.org/8744002/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/subsetter_impl.cc:720: According to PDF spec (section 9.9), the following tables must present: 9.9 in which version of the spec? http://codereview.chromium.org/8744002/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/subsetter_impl.cc:722: cmap if font is used as a TTF and not a CIDFont dict nit: TTF -> "simple font" http://codereview.chromium.org/8744002/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/subsetter_impl.cc:737: cmap - could strip out non-needed cmap subtables We always use TTF fonts as CIDFonts and never as simple fonts in Chromium, so do we need the cmap at all? http://codereview.chromium.org/8744002/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/subsetter_impl.cc:741: silf, glat, gloc, feat - shall be okay to strip out nit: shall -> should http://codereview.chromium.org/8744002/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/subsetter_impl.cc:755: hsty - would be surprised if you saw one of these - used on the Newton nit: "if you saw" -> "to see" http://codereview.chromium.org/8744002/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/subsetter_impl.cc:763: // The const is initialized here to workaround VC bug of rendering all Tag::* hmm? http://codereview.chromium.org/8744002/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/subsetter_impl.cc:766: const int32_t VALID_TABLE_TAG[] = { nit: TABLES_IN_SUBSET http://codereview.chromium.org/8744002/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/subsetter_impl.cc:779: GlyphTablePtr glyph_table = You got these in the caller, maybe just pass them down? http://codereview.chromium.org/8744002/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/subsetter_impl.cc:788: int flag = DetectBitmapBuilders(font_); nit flag -> bitmap_table_type http://codereview.chromium.org/8744002/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/subsetter_impl.cc:794: if (use_ebdt || !subset_success) { I don't understand the lack of symmetry between this line and 799? http://codereview.chromium.org/8744002/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/subsetter_impl.cc:806: IntegerSet allowed_tags; It seems like this should be a static lazy instance. http://codereview.chromium.org/8744002/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/subsetter_impl.cc:810: for (IntegerSet::iterator i = remove_tags.begin(), e = remove_tags.end(); Should this use set_difference?
http://codereview.chromium.org/8744002/diff/1/third_party/sfntly/src/subsette... File third_party/sfntly/src/subsetter/subsetter_impl.cc (right): http://codereview.chromium.org/8744002/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/subsetter_impl.cc:53: void ConstructName(UChar* name_part, UnicodeString* name, int32_t name_id) { On 2011/11/30 22:52:58, vandebo wrote: > Shouldn't this be in an anonymous namespace? The same goes for all the > non-class member functions. Upstream has test cases testing the functions so namespace sfntly is used. http://codereview.chromium.org/8744002/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/subsetter_impl.cc:148: for (FontArray::const_iterator b = font_array.begin(), e = font_array.end(); On 2011/11/30 22:52:58, vandebo wrote: > nit: b -> i, e -> end ? e is used throughout sfntly to represent end. b->i done. http://codereview.chromium.org/8744002/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/subsetter_impl.cc:303: bool ShallSubset(EbdtTable::Builder* ebdt, EblcTable::Builder* eblc, On 2011/11/30 22:52:58, vandebo wrote: > better name? SetUpBitmapBuilder? Changed to InitializeBitmapBuilder http://codereview.chromium.org/8744002/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/subsetter_impl.cc:305: EblcTableBuilderPtr eblc_builder = eblc; On 2011/11/30 22:52:58, vandebo wrote: > Do you need a smart pointer here? The caller isn't going to give up its > reference in the lifetime of this function. It's more consistent to always use smart pointers to host objects ported from Java. http://codereview.chromium.org/8744002/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/subsetter_impl.cc:317: for (size_t i = 0; i < strikes->size(); i++) { On 2011/11/30 22:52:58, vandebo wrote: > I didn't follow all the details here, but this looks like an n^3 algorithm. Is > there any easy way to reduce the complexity? It has to be n^3 unfortunately. We need to go over all subtables and extract the glyph ids that we need. Since index subtables do not overlap each other, we are actually ruling out most of them because we have sparse glyph id distributions. http://codereview.chromium.org/8744002/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/subsetter_impl.cc:365: return false; // All strikes shall be gone. On 2011/11/30 22:52:58, vandebo wrote: > nit: remove comment, redundant Done. http://codereview.chromium.org/8744002/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/subsetter_impl.cc:368: // Remove unused strikes On 2011/11/30 22:52:58, vandebo wrote: > nit: Remove comment Done. http://codereview.chromium.org/8744002/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/subsetter_impl.cc:369: for (IntegerList::reverse_iterator j = removed_strikes.rbegin(), On 2011/11/30 22:52:58, vandebo wrote: > nit: j -> i Done. http://codereview.chromium.org/8744002/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/subsetter_impl.cc:413: if (gid < lower_bound) { On 2011/11/30 22:52:58, vandebo wrote: > Could you search for lower_bound outside the loop and start the loop there? Or > does BitmapGlyphInfoMap not let you search? Done. http://codereview.chromium.org/8744002/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/subsetter_impl.cc:436: offset_pairs.push_back( On 2011/11/30 22:52:58, vandebo wrote: > If you pull this to after the loop, I think you can get rid of last_element and > upper_bound_reached. upper_bound_reached is still needed to avoid adding out-of-range glyph id into index table. The rest is done. http://codereview.chromium.org/8744002/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/subsetter_impl.cc:478: for (BitmapGlyphInfoMap::const_iterator i = loca.begin(), e = loca.end(); On 2011/11/30 22:52:58, vandebo wrote: > Similar comments apply to this loop. Done. http://codereview.chromium.org/8744002/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/subsetter_impl.cc:522: assert(false); // Shall not be here. On 2011/11/30 22:52:58, vandebo wrote: > Remove comment. Done. http://codereview.chromium.org/8744002/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/subsetter_impl.cc:532: return; // No valid EBLC. On 2011/11/30 22:52:58, vandebo wrote: > redundant comment Done. http://codereview.chromium.org/8744002/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/subsetter_impl.cc:550: /****************************************************************************** On 2011/11/30 22:52:58, vandebo wrote: > // style comments are more common than /* */ style. The style guide says to use > one or the other. Done. http://codereview.chromium.org/8744002/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/subsetter_impl.cc:551: Long background comments On 2011/11/30 22:52:58, vandebo wrote: > This line isn't necessary Done. http://codereview.chromium.org/8744002/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/subsetter_impl.cc:590: EbdtTablePtr ebdt_table = On 2011/11/30 22:52:58, vandebo wrote: > This may be the ebdt or bdat table, so calling it the ebdt table could be > confusion. Is there a generic name you can use for either table? bdat is actually zoo'ed and most fonts use ebdt therefore ebdt is chosen in Java code. I use the same name as its class type to avoid confusion. http://codereview.chromium.org/8744002/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/subsetter_impl.cc:639: if (ebdt_table == NULL && eblc_table == NULL) { On 2011/11/30 22:52:58, vandebo wrote: > What happens if we have just one of these two tables? It's a rogue font. If the rogue font reaches here, OTS has a bug to fix since it failed the checking. http://codereview.chromium.org/8744002/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/subsetter_impl.cc:720: According to PDF spec (section 9.9), the following tables must present: On 2011/11/30 22:52:58, vandebo wrote: > nit: must be present This comment is done by stuartg and thus I'd rather keep it as-is. http://codereview.chromium.org/8744002/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/subsetter_impl.cc:737: cmap - could strip out non-needed cmap subtables On 2011/11/30 22:52:58, vandebo wrote: > We always use TTF fonts as CIDFonts and never as simple fonts in Chromium, so do > we need the cmap at all? Theoretically, we don't need it until implementing glyph renumbering (if it's going to be done). However, cmap is a core table for TTF font and relatively small. I don't think we want to risk taking it out giving the limited benefit. http://codereview.chromium.org/8744002/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/subsetter_impl.cc:763: // The const is initialized here to workaround VC bug of rendering all Tag::* On 2011/11/30 22:52:58, vandebo wrote: > hmm? If someone attempt to move this const array outside this function, he/she'll be screwed in VC. gcc does not have the problem. http://codereview.chromium.org/8744002/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/subsetter_impl.cc:766: const int32_t VALID_TABLE_TAG[] = { On 2011/11/30 22:52:58, vandebo wrote: > nit: TABLES_IN_SUBSET Done. http://codereview.chromium.org/8744002/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/subsetter_impl.cc:779: GlyphTablePtr glyph_table = On 2011/11/30 22:52:58, vandebo wrote: > You got these in the caller, maybe just pass them down? It's designed for upstream so that other callers are not required to do the casting. http://codereview.chromium.org/8744002/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/subsetter_impl.cc:788: int flag = DetectBitmapBuilders(font_); On 2011/11/30 22:52:58, vandebo wrote: > nit flag -> bitmap_table_type Done. http://codereview.chromium.org/8744002/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/subsetter_impl.cc:794: if (use_ebdt || !subset_success) { On 2011/11/30 22:52:58, vandebo wrote: > I don't understand the lack of symmetry between this line and 799? bdat shall be gone if ebdt exists, or the bdat contains no glyph of our subset ebdt shall be gone if ebdt exists, and the ebdt contains no glyph of our subset http://codereview.chromium.org/8744002/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/subsetter_impl.cc:806: IntegerSet allowed_tags; On 2011/11/30 22:52:58, vandebo wrote: > It seems like this should be a static lazy instance. There will be one more global variable buying little performance. http://codereview.chromium.org/8744002/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/subsetter_impl.cc:810: for (IntegerSet::iterator i = remove_tags.begin(), e = remove_tags.end(); On 2011/11/30 22:52:58, vandebo wrote: > Should this use set_difference? Done.
http://codereview.chromium.org/8744002/diff/1/third_party/sfntly/src/subsette... File third_party/sfntly/src/subsetter/subsetter_impl.cc (right): http://codereview.chromium.org/8744002/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/subsetter_impl.cc:53: void ConstructName(UChar* name_part, UnicodeString* name, int32_t name_id) { On 2011/12/01 00:59:35, arthurhsu wrote: > On 2011/11/30 22:52:58, vandebo wrote: > > Shouldn't this be in an anonymous namespace? The same goes for all the > > non-class member functions. > > Upstream has test cases testing the functions so namespace sfntly is used. Can we put the ones with explicit unit tests into a namespace sftly_unittested (or similar) and move the rest to the anonymous namespace? http://codereview.chromium.org/8744002/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/subsetter_impl.cc:227: bool SetupGlyfBuilders(Font::Builder* builder, On 2011/11/30 22:52:58, vandebo wrote: > nit: SetUp FYI: http://notaverb.com/setup http://codereview.chromium.org/8744002/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/subsetter_impl.cc:305: EblcTableBuilderPtr eblc_builder = eblc; On 2011/12/01 00:59:35, arthurhsu wrote: > On 2011/11/30 22:52:58, vandebo wrote: > > Do you need a smart pointer here? The caller isn't going to give up its > > reference in the lifetime of this function. > > It's more consistent to always use smart pointers to host objects ported from > Java. But it's not free. Every time you use a smart pointer, it requires that the ref count of the object be updated. That usually requires an atomic instruction, which can be quiet expensive on some platforms. In this case (and several other in this file), we are sure that the reference count of an object won't reach zero while in the body of a function because the caller has a reference. In those cases, I'd prefer we not take another reference. http://codereview.chromium.org/8744002/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/subsetter_impl.cc:436: offset_pairs.push_back( On 2011/12/01 00:59:35, arthurhsu wrote: > On 2011/11/30 22:52:58, vandebo wrote: > > If you pull this to after the loop, I think you can get rid of last_element > and > > upper_bound_reached. > > upper_bound_reached is still needed to avoid adding out-of-range glyph id into > index table. The rest is done. Not the way I read the code, do I misunderstand? http://codereview.chromium.org/8744002/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/subsetter_impl.cc:581: font will have sparse bitmap glyphs. As a result, the EBLC table shall be On 2011/11/30 22:52:58, vandebo wrote: > nit: "As a result..." -> "So reconstruct it as a format 4 or 5 table." Missed http://codereview.chromium.org/8744002/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/subsetter_impl.cc:639: if (ebdt_table == NULL && eblc_table == NULL) { On 2011/12/01 00:59:35, arthurhsu wrote: > On 2011/11/30 22:52:58, vandebo wrote: > > What happens if we have just one of these two tables? > > It's a rogue font. If the rogue font reaches here, OTS has a bug to fix since > it failed the checking. I think an easier and more fault tolerant structure would then be: if (ebdt_table != NULL && eblc_table != NULL) return kEBDTFound; ... if (bdat_table != NULL && bloc_table != NULL) return kEBDTFound; return kNotFound; http://codereview.chromium.org/8744002/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/subsetter_impl.cc:720: According to PDF spec (section 9.9), the following tables must present: On 2011/12/01 00:59:35, arthurhsu wrote: > On 2011/11/30 22:52:58, vandebo wrote: > > nit: must be present > > This comment is done by stuartg and thus I'd rather keep it as-is. My comments only request clarification or fix grammar issues, so please address them. http://codereview.chromium.org/8744002/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/subsetter_impl.cc:763: // The const is initialized here to workaround VC bug of rendering all Tag::* On 2011/12/01 00:59:35, arthurhsu wrote: > On 2011/11/30 22:52:58, vandebo wrote: > > hmm? > > If someone attempt to move this const array outside this function, he/she'll be > screwed in VC. gcc does not have the problem. See if you can get stuart to accept a preprocessor macro definition of GenerateTag http://codereview.chromium.org/8744002/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/subsetter_impl.cc:779: GlyphTablePtr glyph_table = On 2011/12/01 00:59:35, arthurhsu wrote: > On 2011/11/30 22:52:58, vandebo wrote: > > You got these in the caller, maybe just pass them down? > > It's designed for upstream so that other callers are not required to do the > casting. I didn't mean just the cast, I meant the entire table lookup. As I understand it, the only upstream callers are tests - they can do the extra work instead of looking up the tables twice? http://codereview.chromium.org/8744002/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/subsetter_impl.cc:806: IntegerSet allowed_tags; On 2011/12/01 00:59:35, arthurhsu wrote: > On 2011/11/30 22:52:58, vandebo wrote: > > It seems like this should be a static lazy instance. > > There will be one more global variable buying little performance. What's wrong with a class static? It seems wasteful to recompute it each time. http://codereview.chromium.org/8744002/diff/10001/third_party/sfntly/src/subs... File third_party/sfntly/src/subsetter/subsetter_impl.cc (right): http://codereview.chromium.org/8744002/diff/10001/third_party/sfntly/src/subs... third_party/sfntly/src/subsetter/subsetter_impl.cc:414: if (!lower_bound_reached) { You can pull this above the loop. http://codereview.chromium.org/8744002/diff/10001/third_party/sfntly/src/subs... third_party/sfntly/src/subsetter/subsetter_impl.cc:422: upper_bound_reached = true; Change this to a break and remove the next conditional. http://codereview.chromium.org/8744002/diff/10001/third_party/sfntly/src/subs... third_party/sfntly/src/subsetter/subsetter_impl.cc:472: if (!lower_bound_reached) { Same here. http://codereview.chromium.org/8744002/diff/10001/third_party/sfntly/src/subs... third_party/sfntly/src/subsetter/subsetter_impl.cc:481: upper_bound_reached = true; And here. http://codereview.chromium.org/8744002/diff/10001/third_party/sfntly/src/subs... third_party/sfntly/src/subsetter/subsetter_impl.cc:773: SetupBitmapBuilders(font_, font_builder, glyph_ids, use_ebdt); It seems like combining SetupBitmapBuilders and DetectBitmapBuilders would simplfy things. SetupBitmapBuilders could return enum BitmapDetection and just try bdat if ebdt isn't there at the start of SetupBitmapBuilders
http://codereview.chromium.org/8744002/diff/1/third_party/sfntly/src/subsette... File third_party/sfntly/src/subsetter/subsetter_impl.cc (right): http://codereview.chromium.org/8744002/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/subsetter_impl.cc:53: void ConstructName(UChar* name_part, UnicodeString* name, int32_t name_id) { On 2011/12/01 21:50:28, vandebo wrote: > On 2011/12/01 00:59:35, arthurhsu wrote: > > On 2011/11/30 22:52:58, vandebo wrote: > > > Shouldn't this be in an anonymous namespace? The same goes for all the > > > non-class member functions. > > > > Upstream has test cases testing the functions so namespace sfntly is used. > > Can we put the ones with explicit unit tests into a namespace sftly_unittested > (or similar) and move the rest to the anonymous namespace? Moved those are not used for now. namespace hierarchy change will need stuartg/bstell to okay it. http://codereview.chromium.org/8744002/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/subsetter_impl.cc:305: EblcTableBuilderPtr eblc_builder = eblc; On 2011/12/01 21:50:28, vandebo wrote: > On 2011/12/01 00:59:35, arthurhsu wrote: > > On 2011/11/30 22:52:58, vandebo wrote: > > > Do you need a smart pointer here? The caller isn't going to give up its > > > reference in the lifetime of this function. > > > > It's more consistent to always use smart pointers to host objects ported from > > Java. > > But it's not free. Every time you use a smart pointer, it requires that the ref > count of the object be updated. That usually requires an atomic instruction, > which can be quiet expensive on some platforms. In this case (and several other > in this file), we are sure that the reference count of an object won't reach > zero while in the body of a function because the caller has a reference. In > those cases, I'd prefer we not take another reference. Done. http://codereview.chromium.org/8744002/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/subsetter_impl.cc:436: offset_pairs.push_back( On 2011/12/01 21:50:28, vandebo wrote: > On 2011/12/01 00:59:35, arthurhsu wrote: > > On 2011/11/30 22:52:58, vandebo wrote: > > > If you pull this to after the loop, I think you can get rid of last_element > > and > > > upper_bound_reached. > > > > upper_bound_reached is still needed to avoid adding out-of-range glyph id into > > index table. The rest is done. > > Not the way I read the code, do I misunderstand? |gid| can't be greater than |upper_bound|. Using std::lower_bound(upper_bound) here will still return a |gid| greater or equal to the |upper_bound|. http://codereview.chromium.org/8744002/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/subsetter_impl.cc:529: EblcTableBuilderPtr eblc_builder = eblc; On 2011/11/30 22:52:58, vandebo wrote: > No need for a smart pointer here - the caller holds a reference. Done. http://codereview.chromium.org/8744002/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/subsetter_impl.cc:581: font will have sparse bitmap glyphs. As a result, the EBLC table shall be On 2011/12/01 21:50:28, vandebo wrote: > On 2011/11/30 22:52:58, vandebo wrote: > > nit: "As a result..." -> "So reconstruct it as a format 4 or 5 table." > > Missed Done. http://codereview.chromium.org/8744002/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/subsetter_impl.cc:639: if (ebdt_table == NULL && eblc_table == NULL) { On 2011/12/01 21:50:28, vandebo wrote: > On 2011/12/01 00:59:35, arthurhsu wrote: > > On 2011/11/30 22:52:58, vandebo wrote: > > > What happens if we have just one of these two tables? > > > > It's a rogue font. If the rogue font reaches here, OTS has a bug to fix since > > it failed the checking. > > I think an easier and more fault tolerant structure would then be: > > if (ebdt_table != NULL && eblc_table != NULL) > return kEBDTFound; > ... > if (bdat_table != NULL && bloc_table != NULL) > return kEBDTFound; > return kNotFound; Done. http://codereview.chromium.org/8744002/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/subsetter_impl.cc:720: According to PDF spec (section 9.9), the following tables must present: On 2011/12/01 21:50:28, vandebo wrote: > On 2011/12/01 00:59:35, arthurhsu wrote: > > On 2011/11/30 22:52:58, vandebo wrote: > > > nit: must be present > > > > This comment is done by stuartg and thus I'd rather keep it as-is. > > My comments only request clarification or fix grammar issues, so please address > them. Done. http://codereview.chromium.org/8744002/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/subsetter_impl.cc:722: cmap if font is used as a TTF and not a CIDFont dict On 2011/11/30 22:52:58, vandebo wrote: > nit: TTF -> "simple font" Done. http://codereview.chromium.org/8744002/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/subsetter_impl.cc:741: silf, glat, gloc, feat - shall be okay to strip out On 2011/11/30 22:52:58, vandebo wrote: > nit: shall -> should Done. http://codereview.chromium.org/8744002/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/subsetter_impl.cc:755: hsty - would be surprised if you saw one of these - used on the Newton On 2011/11/30 22:52:58, vandebo wrote: > nit: "if you saw" -> "to see" Done. http://codereview.chromium.org/8744002/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/subsetter_impl.cc:763: // The const is initialized here to workaround VC bug of rendering all Tag::* On 2011/12/01 21:50:28, vandebo wrote: > On 2011/12/01 00:59:35, arthurhsu wrote: > > On 2011/11/30 22:52:58, vandebo wrote: > > > hmm? > > > > If someone attempt to move this const array outside this function, he/she'll > be > > screwed in VC. gcc does not have the problem. > > See if you can get stuart to accept a preprocessor macro definition of > GenerateTag That will be an upstream change and won't be in this roll. http://codereview.chromium.org/8744002/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/subsetter_impl.cc:779: GlyphTablePtr glyph_table = On 2011/12/01 21:50:28, vandebo wrote: > On 2011/12/01 00:59:35, arthurhsu wrote: > > On 2011/11/30 22:52:58, vandebo wrote: > > > You got these in the caller, maybe just pass them down? > > > > It's designed for upstream so that other callers are not required to do the > > casting. > > I didn't mean just the cast, I meant the entire table lookup. > > As I understand it, the only upstream callers are tests - they can do the extra > work instead of looking up the tables twice? Done. http://codereview.chromium.org/8744002/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/subsetter_impl.cc:806: IntegerSet allowed_tags; On 2011/12/01 21:50:28, vandebo wrote: > On 2011/12/01 00:59:35, arthurhsu wrote: > > On 2011/11/30 22:52:58, vandebo wrote: > > > It seems like this should be a static lazy instance. > > > > There will be one more global variable buying little performance. > > What's wrong with a class static? It seems wasteful to recompute it each time. The set is static initialized than it will stay for the process's lifetime. Since most people don't print, I'd opt to use more CPU's during printing than use more memory for the process' lifetime. http://codereview.chromium.org/8744002/diff/10001/third_party/sfntly/src/subs... File third_party/sfntly/src/subsetter/subsetter_impl.cc (right): http://codereview.chromium.org/8744002/diff/10001/third_party/sfntly/src/subs... third_party/sfntly/src/subsetter/subsetter_impl.cc:414: if (!lower_bound_reached) { On 2011/12/01 21:50:28, vandebo wrote: > You can pull this above the loop. Done. http://codereview.chromium.org/8744002/diff/10001/third_party/sfntly/src/subs... third_party/sfntly/src/subsetter/subsetter_impl.cc:422: upper_bound_reached = true; On 2011/12/01 21:50:28, vandebo wrote: > Change this to a break and remove the next conditional. Done. http://codereview.chromium.org/8744002/diff/10001/third_party/sfntly/src/subs... third_party/sfntly/src/subsetter/subsetter_impl.cc:472: if (!lower_bound_reached) { On 2011/12/01 21:50:28, vandebo wrote: > Same here. Done. http://codereview.chromium.org/8744002/diff/10001/third_party/sfntly/src/subs... third_party/sfntly/src/subsetter/subsetter_impl.cc:481: upper_bound_reached = true; On 2011/12/01 21:50:28, vandebo wrote: > And here. Done. http://codereview.chromium.org/8744002/diff/10001/third_party/sfntly/src/subs... third_party/sfntly/src/subsetter/subsetter_impl.cc:773: SetupBitmapBuilders(font_, font_builder, glyph_ids, use_ebdt); On 2011/12/01 21:50:28, vandebo wrote: > It seems like combining SetupBitmapBuilders and DetectBitmapBuilders would > simplfy things. SetupBitmapBuilders could return enum BitmapDetection and just > try bdat if ebdt isn't there at the start of SetupBitmapBuilders Done.
LGTM with comments. http://codereview.chromium.org/8744002/diff/15001/third_party/sfntly/src/subs... File third_party/sfntly/src/subsetter/subsetter_impl.cc (right): http://codereview.chromium.org/8744002/diff/15001/third_party/sfntly/src/subs... third_party/sfntly/src/subsetter/subsetter_impl.cc:157: bool ResolveCompositeGlyphs(GlyphTable* glyf, glyf -> glyph http://codereview.chromium.org/8744002/diff/15001/third_party/sfntly/src/subs... third_party/sfntly/src/subsetter/subsetter_impl.cc:168: GlyphTablePtr glyph_table = glyf; Don't need smart pointers here either. http://codereview.chromium.org/8744002/diff/15001/third_party/sfntly/src/subs... third_party/sfntly/src/subsetter/subsetter_impl.cc:225: bool SetupGlyfBuilders(Font::Builder* builder, Glyf -> Glyph http://codereview.chromium.org/8744002/diff/15001/third_party/sfntly/src/subs... third_party/sfntly/src/subsetter/subsetter_impl.cc:226: GlyphTable* glyf, glyf -> glyph http://codereview.chromium.org/8744002/diff/15001/third_party/sfntly/src/subs... third_party/sfntly/src/subsetter/subsetter_impl.cc:234: GlyphTablePtr glyph_table = glyf; Don't need smart pointers here. http://codereview.chromium.org/8744002/diff/15001/third_party/sfntly/src/subs... third_party/sfntly/src/subsetter/subsetter_impl.cc:237: FontBuilderPtr font_builder = builder; Or here. http://codereview.chromium.org/8744002/diff/15001/third_party/sfntly/src/subs... third_party/sfntly/src/subsetter/subsetter_impl.cc:569: if (!use_ebdt) { Maybe add "BuilderToRemove ret;" and return that instead of calculating which value to return in each of your error cases. Your choice. http://codereview.chromium.org/8744002/diff/15001/third_party/sfntly/src/subs... third_party/sfntly/src/subsetter/subsetter_impl.cc:584: FontBuilderPtr font_builder = builder; Don't need smart pointer here. http://codereview.chromium.org/8744002/diff/15001/third_party/sfntly/src/subs... third_party/sfntly/src/subsetter/subsetter_impl.cc:596: if (!InitializeBitmapBuilder(ebdt_table_builder, eblc_table_builder, nit: no space http://codereview.chromium.org/8744002/diff/15001/third_party/sfntly/src/subs... third_party/sfntly/src/subsetter/subsetter_impl.cc:722: Font* SubsetterImpl::Subset(const IntegerSet& glyph_ids, GlyphTable* glyf, glyf -> glyph
http://codereview.chromium.org/8744002/diff/15001/third_party/sfntly/src/subs... File third_party/sfntly/src/subsetter/subsetter_impl.cc (right): http://codereview.chromium.org/8744002/diff/15001/third_party/sfntly/src/subs... third_party/sfntly/src/subsetter/subsetter_impl.cc:157: bool ResolveCompositeGlyphs(GlyphTable* glyf, On 2011/12/02 02:21:20, vandebo wrote: > glyf -> glyph Glyph table is named "glyf" in TTF spec, therefore it's usually named glyf in order to distinguish with individual glyph objects in the code. http://codereview.chromium.org/8744002/diff/15001/third_party/sfntly/src/subs... third_party/sfntly/src/subsetter/subsetter_impl.cc:168: GlyphTablePtr glyph_table = glyf; On 2011/12/02 02:21:20, vandebo wrote: > Don't need smart pointers here either. Done. http://codereview.chromium.org/8744002/diff/15001/third_party/sfntly/src/subs... third_party/sfntly/src/subsetter/subsetter_impl.cc:234: GlyphTablePtr glyph_table = glyf; On 2011/12/02 02:21:20, vandebo wrote: > Don't need smart pointers here. Done. http://codereview.chromium.org/8744002/diff/15001/third_party/sfntly/src/subs... third_party/sfntly/src/subsetter/subsetter_impl.cc:237: FontBuilderPtr font_builder = builder; On 2011/12/02 02:21:20, vandebo wrote: > Or here. Done. http://codereview.chromium.org/8744002/diff/15001/third_party/sfntly/src/subs... third_party/sfntly/src/subsetter/subsetter_impl.cc:584: FontBuilderPtr font_builder = builder; On 2011/12/02 02:21:20, vandebo wrote: > Don't need smart pointer here. Done.
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/arthurhsu@chromium.org/8744002/21001
Change committed as 112852 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
