Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7381004:
Add sfntly library for font subsetting used in print preview. (Closed)
|
Created:
9 years, 5 months ago by arthurhsu Modified:
9 years, 5 months ago CC:
chromium-reviews, James Hawkins, csilv, Chris Guillory Visibility:
Public. |
DescriptionAdd sfntly library for font subsetting used in print preview.
BUG=80907
TEST=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=93672
Patch Set 1 #
Total comments: 14
Patch Set 2 : Update per code review #
Total comments: 29
Patch Set 3 : Update per code review #Patch Set 4 : Update per code review #
Total comments: 10
Patch Set 5 : Update per code review #
Total comments: 9
Patch Set 6 : Upload per code review #
Total comments: 8
Patch Set 7 : Update per code review #Patch Set 8 : Update per code review #Patch Set 9 : Update google code URL #Patch Set 10 : Update DEPS so that commit bot can successfully patch #Patch Set 11 : Update sfntly version to the one clang friendly #
Messages
Total messages: 20 (0 generated)
Some comments. http://codereview.chromium.org/7381004/diff/1/DEPS File DEPS (right): http://codereview.chromium.org/7381004/diff/1/DEPS#newcode102 DEPS:102: (Var("googlecode_url") % "sfntly") + "/trunk/cpp/src/sfntly@9", Can you make a variable for the sfntly version like examples below? http://codereview.chromium.org/7381004/diff/1/third_party/sfntly/README.chromium File third_party/sfntly/README.chromium (right): http://codereview.chromium.org/7381004/diff/1/third_party/sfntly/README.chrom... third_party/sfntly/README.chromium:14: README.chromium Can LICENSE be taken upstream? You can add a 'License File:' line to README.chromium with the appropriate path if there's already a LICENSE file there. http://codereview.chromium.org/7381004/diff/1/third_party/sfntly/src/subsette... File third_party/sfntly/src/subsetter/font_subsetter.cc (right): http://codereview.chromium.org/7381004/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/font_subsetter.cc:20: int subsetFont(const char* font_name, nit: document this function. http://codereview.chromium.org/7381004/diff/1/third_party/sfntly/src/subsette... File third_party/sfntly/src/subsetter/font_subsetter.h (right): http://codereview.chromium.org/7381004/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/font_subsetter.h:17: // Font subsetting API nit: Move this down adjacent to function. http://codereview.chromium.org/7381004/diff/1/third_party/sfntly/src/subsette... File third_party/sfntly/src/subsetter/subsetter_impl.cc (right): http://codereview.chromium.org/7381004/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/subsetter_impl.cc:28: #include "subsetter_impl.h" This should be first. See http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Names_and_Orde... http://codereview.chromium.org/7381004/diff/1/third_party/sfntly/src/subsette... File third_party/sfntly/src/subsetter/subsetter_impl.h (right): http://codereview.chromium.org/7381004/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/subsetter_impl.h:40: CALLER_ATTACH Font* subset(const unsigned int* glyph_ids, Can this be forward declared instead of included? Also, What's the memory contract for this return value? Can you write more documentation?
http://codereview.chromium.org/7381004/diff/1/DEPS File DEPS (right): http://codereview.chromium.org/7381004/diff/1/DEPS#newcode102 DEPS:102: (Var("googlecode_url") % "sfntly") + "/trunk/cpp/src/sfntly@9", On 2011/07/14 22:35:51, Sheridan Rawlins wrote: > Can you make a variable for the sfntly version like examples below? Done. http://codereview.chromium.org/7381004/diff/1/third_party/sfntly/README.chromium File third_party/sfntly/README.chromium (right): http://codereview.chromium.org/7381004/diff/1/third_party/sfntly/README.chrom... third_party/sfntly/README.chromium:14: README.chromium On 2011/07/14 22:35:51, Sheridan Rawlins wrote: > Can LICENSE be taken upstream? > > You can add a 'License File:' line to README.chromium with the appropriate path > if there's already a LICENSE file there. The license sits in a different folder upstream and I don't want to copy unnecessary files (e.g. Java code). I've added description about this. http://codereview.chromium.org/7381004/diff/1/third_party/sfntly/src/subsette... File third_party/sfntly/src/subsetter/font_subsetter.cc (right): http://codereview.chromium.org/7381004/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/font_subsetter.cc:20: int subsetFont(const char* font_name, On 2011/07/14 22:35:51, Sheridan Rawlins wrote: > nit: document this function. already documented in .h http://codereview.chromium.org/7381004/diff/1/third_party/sfntly/src/subsette... File third_party/sfntly/src/subsetter/font_subsetter.h (right): http://codereview.chromium.org/7381004/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/font_subsetter.h:17: // Font subsetting API On 2011/07/14 22:35:51, Sheridan Rawlins wrote: > nit: Move this down adjacent to function. Done. http://codereview.chromium.org/7381004/diff/1/third_party/sfntly/src/subsette... File third_party/sfntly/src/subsetter/subsetter_impl.cc (right): http://codereview.chromium.org/7381004/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/subsetter_impl.cc:28: #include "subsetter_impl.h" On 2011/07/14 22:35:51, Sheridan Rawlins wrote: > This should be first. See > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Names_and_Orde... standard order is C library, C++ library, other libraries' .h, your project's .h. In order to avoid compilation problem in VC2008, we are forced to move "sfntly/port/type.h" before C-style ICU headers. http://codereview.chromium.org/7381004/diff/1/third_party/sfntly/src/subsette... File third_party/sfntly/src/subsetter/subsetter_impl.h (right): http://codereview.chromium.org/7381004/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/subsetter_impl.h:40: CALLER_ATTACH Font* subset(const unsigned int* glyph_ids, On 2011/07/14 22:35:51, Sheridan Rawlins wrote: > Can this be forward declared instead of included? > > Also, What's the memory contract for this return value? Can you write more > documentation? It accesses data member and has to be a member function. Added comments regarding memory contracts.
http://codereview.chromium.org/7381004/diff/1/third_party/sfntly/src/subsette... File third_party/sfntly/src/subsetter/subsetter_impl.cc (right): http://codereview.chromium.org/7381004/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/subsetter_impl.cc:28: #include "subsetter_impl.h" Standard order starts with the header for this impl and _then_ the C, C++, etc. The reason is to be a courtesy to others by ensuring that you don't need to include anything else to include your header. You can include sfntly/port/type.h in that header first if necessary. On 2011/07/14 23:52:35, arthurhsu wrote: > On 2011/07/14 22:35:51, Sheridan Rawlins wrote: > > This should be first. See > > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Names_and_Orde... > > standard order is C library, C++ library, other libraries' .h, your project's > .h. > > In order to avoid compilation problem in VC2008, we are forced to move > "sfntly/port/type.h" before C-style ICU headers.
Hi Brett, BenG recommended you for reviewing this CL so I'm sending it your way. Let me know if that's okay for you, thanks.
Hi Brett, Is this review at the front line of your queue? If not we'd like to add other reviewers to share the load. Let me know if it's okay. Thanks, Arthur
http://codereview.chromium.org/7381004/diff/20/third_party/sfntly/src/subsett... File third_party/sfntly/src/subsetter/font_subsetter.h (right): http://codereview.chromium.org/7381004/diff/20/third_party/sfntly/src/subsett... third_party/sfntly/src/subsetter/font_subsetter.h:39: extern int subsetFont(const char* font_name, It's not clear to me what's going on here. This subsetter code isn't part of the sfntly library? Is this coming from some other place? If Chrome is the canonial repo, it doesn't seem like it should be in third_party.
On 2011/07/18 22:57:52, brettw wrote: > http://codereview.chromium.org/7381004/diff/20/third_party/sfntly/src/subsett... > File third_party/sfntly/src/subsetter/font_subsetter.h (right): > > http://codereview.chromium.org/7381004/diff/20/third_party/sfntly/src/subsett... > third_party/sfntly/src/subsetter/font_subsetter.h:39: extern int > subsetFont(const char* font_name, > It's not clear to me what's going on here. This subsetter code isn't part of the > sfntly library? Is this coming from some other place? If Chrome is the canonial > repo, it doesn't seem like it should be in third_party. The subsetting is provided as a sample in sfntly and performs glyph reordering. What we need is a subsetter that's not doing glyph reordering, and also is able to get font from TTC by name. I've talked to Stuart Gill (sfntly's architect) and he does not think it's suitable to put these in sfntly. As a result, we'll need to put it in Chromium. I've also asked around in Chromium-dev regarding where to put these code. The suggestion is to leave them in third_party. http://groups.google.com/a/chromium.org/group/chromium-dev/browse_thread/thre...
http://codereview.chromium.org/7381004/diff/20/third_party/sfntly/src/subsett... File third_party/sfntly/src/subsetter/font_subsetter.cc (right): http://codereview.chromium.org/7381004/diff/20/third_party/sfntly/src/subsett... third_party/sfntly/src/subsetter/font_subsetter.cc:25: unsigned char** output_buffer) { I actually wonder if this function is worth having. It seems like if any of these checks are important, they should be in the impl. Isn't it easy enough for callers to construct the object and call Load and Subset? http://codereview.chromium.org/7381004/diff/20/third_party/sfntly/src/subsett... File third_party/sfntly/src/subsetter/font_subsetter.h (right): http://codereview.chromium.org/7381004/diff/20/third_party/sfntly/src/subsett... third_party/sfntly/src/subsetter/font_subsetter.h:17: #ifndef FONT_SUBSETTER_H_ If this is going to be our code, I'd like to have it use Google style, so full include guards (THIRD_PARTY_SFNTLY_SRC_SUBSETTER_FONT_SUBSETTER_H_) and header include paths (for the .cc files). http://codereview.chromium.org/7381004/diff/20/third_party/sfntly/src/subsett... third_party/sfntly/src/subsetter/font_subsetter.h:39: extern int subsetFont(const char* font_name, Function names should be capital. I'm also wondering if there should be some kind of namespace? This is a pretty generic name. I notice you used sfntly in the implementation, I'd prefer that here. If you want to hide those other functions, you can put it in sfntly::internal http://codereview.chromium.org/7381004/diff/20/third_party/sfntly/src/subsett... third_party/sfntly/src/subsetter/font_subsetter.h:41: const size_t font_size, Generally I don't see people writing const on copied POD input arguments. http://codereview.chromium.org/7381004/diff/20/third_party/sfntly/src/subsett... File third_party/sfntly/src/subsetter/subsetter_impl.cc (right): http://codereview.chromium.org/7381004/diff/20/third_party/sfntly/src/subsett... third_party/sfntly/src/subsetter/subsetter_impl.cc:88: const unsigned char* original_font, Be sure these are aligned. http://codereview.chromium.org/7381004/diff/20/third_party/sfntly/src/subsett... third_party/sfntly/src/subsetter/subsetter_impl.cc:113: FontPtr new_font; Can you avoid the "Ptr" things and just write Font* so it's more clear. I also noted this in the sfntly readability review. Ditto for the other cases below. http://codereview.chromium.org/7381004/diff/20/third_party/sfntly/src/subsett... third_party/sfntly/src/subsetter/subsetter_impl.cc:121: int length = (int)output_stream.size(); C++ casts. Same in a few other places in this file. http://codereview.chromium.org/7381004/diff/20/third_party/sfntly/src/subsett... third_party/sfntly/src/subsetter/subsetter_impl.cc:130: Font* SubsetterImpl::findFont(const char* font_name, FontArray& font_array) { If the FontArray is an input parameter, it should be const, if it's not, it should be a pointer. http://codereview.chromium.org/7381004/diff/20/third_party/sfntly/src/subsett... third_party/sfntly/src/subsetter/subsetter_impl.cc:168: int32_t hash_code = hashCode(name_table->platformId(i), This should only be indented 2 spaces, and put the break inside the {} http://codereview.chromium.org/7381004/diff/20/third_party/sfntly/src/subsett... third_party/sfntly/src/subsetter/subsetter_impl.cc:212: font_builder->newTableBuilder(Tag::glyf))); These are indented 5 spaces. http://codereview.chromium.org/7381004/diff/20/third_party/sfntly/src/subsett... third_party/sfntly/src/subsetter/subsetter_impl.cc:223: glyph_id_set.insert(0); // always include glyph id 0 Be sure all the comments begin with uppercase and end with periods http://codereview.chromium.org/7381004/diff/20/third_party/sfntly/src/subsett... third_party/sfntly/src/subsetter/subsetter_impl.cc:250: GlyphPtr glyph; Ditto with no "Ptr" http://codereview.chromium.org/7381004/diff/20/third_party/sfntly/src/subsett... File third_party/sfntly/src/subsetter/subsetter_impl.h (right): http://codereview.chromium.org/7381004/diff/20/third_party/sfntly/src/subsett... third_party/sfntly/src/subsetter/subsetter_impl.h:30: bool loadFont(const char* font_name, Upper-case function names http://codereview.chromium.org/7381004/diff/20/third_party/sfntly/src/subsett... third_party/sfntly/src/subsetter/subsetter_impl.h:49: } This should have a comment " // namespace sfntly"
http://codereview.chromium.org/7381004/diff/20/third_party/sfntly/src/subsett... File third_party/sfntly/src/subsetter/font_subsetter.cc (right): http://codereview.chromium.org/7381004/diff/20/third_party/sfntly/src/subsett... third_party/sfntly/src/subsetter/font_subsetter.cc:25: unsigned char** output_buffer) { On 2011/07/19 05:09:42, brettw wrote: > I actually wonder if this function is worth having. It seems like if any of > these checks are important, they should be in the impl. Isn't it easy enough for > callers to construct the object and call Load and Subset? The general idea is to shield the caller from inclusion of the whole sfntly tree and its dependency, ICU. Ideally we compile this font subsetting function as a library and caller includes only font_subsetter.h. http://codereview.chromium.org/7381004/diff/20/third_party/sfntly/src/subsett... File third_party/sfntly/src/subsetter/font_subsetter.h (right): http://codereview.chromium.org/7381004/diff/20/third_party/sfntly/src/subsett... third_party/sfntly/src/subsetter/font_subsetter.h:17: #ifndef FONT_SUBSETTER_H_ On 2011/07/19 05:09:42, brettw wrote: > If this is going to be our code, I'd like to have it use Google style, so full > include guards (THIRD_PARTY_SFNTLY_SRC_SUBSETTER_FONT_SUBSETTER_H_) and header > include paths (for the .cc files). Done. http://codereview.chromium.org/7381004/diff/20/third_party/sfntly/src/subsett... third_party/sfntly/src/subsetter/font_subsetter.h:39: extern int subsetFont(const char* font_name, On 2011/07/19 05:09:42, brettw wrote: > Function names should be capital. I'm also wondering if there should be some > kind of namespace? This is a pretty generic name. I notice you used sfntly in > the implementation, I'd prefer that here. If you want to hide those other > functions, you can put it in sfntly::internal Done. Wrapped as a static method in SfntlyWrapper class. http://codereview.chromium.org/7381004/diff/20/third_party/sfntly/src/subsett... third_party/sfntly/src/subsetter/font_subsetter.h:39: extern int subsetFont(const char* font_name, On 2011/07/18 22:57:52, brettw wrote: > It's not clear to me what's going on here. This subsetter code isn't part of the > sfntly library? Is this coming from some other place? If Chrome is the canonial > repo, it doesn't seem like it should be in third_party. sfntly disagreed the subsetter code to be upstream so we need to put them somewhere in Chrome. As mentioned, I've asked in Chromium-dev and the suggestion from there is to stay in third_party. Do you think I should move the subsetter code to Chromium src/sfntly instead? http://codereview.chromium.org/7381004/diff/20/third_party/sfntly/src/subsett... third_party/sfntly/src/subsetter/font_subsetter.h:41: const size_t font_size, On 2011/07/19 05:09:42, brettw wrote: > Generally I don't see people writing const on copied POD input arguments. Done. http://codereview.chromium.org/7381004/diff/20/third_party/sfntly/src/subsett... File third_party/sfntly/src/subsetter/subsetter_impl.cc (right): http://codereview.chromium.org/7381004/diff/20/third_party/sfntly/src/subsett... third_party/sfntly/src/subsetter/subsetter_impl.cc:88: const unsigned char* original_font, On 2011/07/19 05:09:42, brettw wrote: > Be sure these are aligned. Done. http://codereview.chromium.org/7381004/diff/20/third_party/sfntly/src/subsett... third_party/sfntly/src/subsetter/subsetter_impl.cc:113: FontPtr new_font; On 2011/07/19 05:09:42, brettw wrote: > Can you avoid the "Ptr" things and just write Font* so it's more clear. I also > noted this in the sfntly readability review. Ditto for the other cases below. The Ptr is a smart pointer class that acts like CComPtr<> and maintains object ref count automatically. I've added usage comment in the .h file. http://codereview.chromium.org/7381004/diff/20/third_party/sfntly/src/subsett... third_party/sfntly/src/subsetter/subsetter_impl.cc:121: int length = (int)output_stream.size(); On 2011/07/19 05:09:42, brettw wrote: > C++ casts. Same in a few other places in this file. Done. http://codereview.chromium.org/7381004/diff/20/third_party/sfntly/src/subsett... third_party/sfntly/src/subsetter/subsetter_impl.cc:130: Font* SubsetterImpl::findFont(const char* font_name, FontArray& font_array) { On 2011/07/19 05:09:42, brettw wrote: > If the FontArray is an input parameter, it should be const, if it's not, it > should be a pointer. Done. http://codereview.chromium.org/7381004/diff/20/third_party/sfntly/src/subsett... third_party/sfntly/src/subsetter/subsetter_impl.cc:168: int32_t hash_code = hashCode(name_table->platformId(i), On 2011/07/19 05:09:42, brettw wrote: > This should only be indented 2 spaces, and put the break inside the {} Done. http://codereview.chromium.org/7381004/diff/20/third_party/sfntly/src/subsett... third_party/sfntly/src/subsetter/subsetter_impl.cc:212: font_builder->newTableBuilder(Tag::glyf))); On 2011/07/19 05:09:42, brettw wrote: > These are indented 5 spaces. Done. http://codereview.chromium.org/7381004/diff/20/third_party/sfntly/src/subsett... third_party/sfntly/src/subsetter/subsetter_impl.cc:223: glyph_id_set.insert(0); // always include glyph id 0 On 2011/07/19 05:09:42, brettw wrote: > Be sure all the comments begin with uppercase and end with periods Done. http://codereview.chromium.org/7381004/diff/20/third_party/sfntly/src/subsett... File third_party/sfntly/src/subsetter/subsetter_impl.h (right): http://codereview.chromium.org/7381004/diff/20/third_party/sfntly/src/subsett... third_party/sfntly/src/subsetter/subsetter_impl.h:30: bool loadFont(const char* font_name, On 2011/07/19 05:09:42, brettw wrote: > Upper-case function names Done. http://codereview.chromium.org/7381004/diff/20/third_party/sfntly/src/subsett... third_party/sfntly/src/subsetter/subsetter_impl.h:49: } On 2011/07/19 05:09:42, brettw wrote: > This should have a comment " // namespace sfntly" Done.
I didn't really check the logic, which I assume the people working on it can do better than I. LGTM with these style fixes. http://codereview.chromium.org/7381004/diff/17001/third_party/sfntly/src/subs... File third_party/sfntly/src/subsetter/font_subsetter.cc (right): http://codereview.chromium.org/7381004/diff/17001/third_party/sfntly/src/subs... third_party/sfntly/src/subsetter/font_subsetter.cc:17: #include "font_subsetter.h" Please use full include paths. http://codereview.chromium.org/7381004/diff/17001/third_party/sfntly/src/subs... File third_party/sfntly/src/subsetter/font_subsetter.h (right): http://codereview.chromium.org/7381004/diff/17001/third_party/sfntly/src/subs... third_party/sfntly/src/subsetter/font_subsetter.h:19: // Input TTF/TTC/OTF fonts, specify the glyph IDs to subset, and the subset font This comment should go above the static function. http://codereview.chromium.org/7381004/diff/17001/third_party/sfntly/src/subs... third_party/sfntly/src/subsetter/font_subsetter.h:33: // |output_buffer| Generated subset font. Can you mention which method the caller should delete this value with? (delete, delete[], or free). http://codereview.chromium.org/7381004/diff/17001/third_party/sfntly/src/subs... File third_party/sfntly/src/subsetter/subsetter_impl.cc (right): http://codereview.chromium.org/7381004/diff/17001/third_party/sfntly/src/subs... third_party/sfntly/src/subsetter/subsetter_impl.cc:22: #include "subsetter_impl.h" Ditto on the include paths. And this include should be first (above string.h). http://codereview.chromium.org/7381004/diff/17001/third_party/sfntly/src/subs... File third_party/sfntly/src/subsetter/subsetter_impl.h (right): http://codereview.chromium.org/7381004/diff/17001/third_party/sfntly/src/subs... third_party/sfntly/src/subsetter/subsetter_impl.h:20: #include "sfntly/font.h" Please use full include paths.
Sheridan please LGTM if no further comments, thanks. http://codereview.chromium.org/7381004/diff/1/third_party/sfntly/src/subsette... File third_party/sfntly/src/subsetter/subsetter_impl.cc (right): http://codereview.chromium.org/7381004/diff/1/third_party/sfntly/src/subsette... third_party/sfntly/src/subsetter/subsetter_impl.cc:28: #include "subsetter_impl.h" On 2011/07/15 01:19:07, Sheridan Rawlins wrote: > Standard order starts with the header for this impl and _then_ the C, C++, etc. > The reason is to be a courtesy to others by ensuring that you don't need to > include anything else to include your header. You can include > sfntly/port/type.h in that header first if necessary. > > On 2011/07/14 23:52:35, arthurhsu wrote: > > On 2011/07/14 22:35:51, Sheridan Rawlins wrote: > > > This should be first. See > > > > > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Names_and_Orde... > > > > standard order is C library, C++ library, other libraries' .h, your project's > > .h. > > > > In order to avoid compilation problem in VC2008, we are forced to move > > "sfntly/port/type.h" before C-style ICU headers. > Done. http://codereview.chromium.org/7381004/diff/17001/third_party/sfntly/src/subs... File third_party/sfntly/src/subsetter/font_subsetter.cc (right): http://codereview.chromium.org/7381004/diff/17001/third_party/sfntly/src/subs... third_party/sfntly/src/subsetter/font_subsetter.cc:17: #include "font_subsetter.h" On 2011/07/19 21:00:33, brettw wrote: > Please use full include paths. Done. http://codereview.chromium.org/7381004/diff/17001/third_party/sfntly/src/subs... File third_party/sfntly/src/subsetter/font_subsetter.h (right): http://codereview.chromium.org/7381004/diff/17001/third_party/sfntly/src/subs... third_party/sfntly/src/subsetter/font_subsetter.h:19: // Input TTF/TTC/OTF fonts, specify the glyph IDs to subset, and the subset font On 2011/07/19 21:00:33, brettw wrote: > This comment should go above the static function. Done. http://codereview.chromium.org/7381004/diff/17001/third_party/sfntly/src/subs... third_party/sfntly/src/subsetter/font_subsetter.h:33: // |output_buffer| Generated subset font. On 2011/07/19 21:00:33, brettw wrote: > Can you mention which method the caller should delete this value with? (delete, > delete[], or free). Done. http://codereview.chromium.org/7381004/diff/17001/third_party/sfntly/src/subs... File third_party/sfntly/src/subsetter/subsetter_impl.cc (right): http://codereview.chromium.org/7381004/diff/17001/third_party/sfntly/src/subs... third_party/sfntly/src/subsetter/subsetter_impl.cc:22: #include "subsetter_impl.h" On 2011/07/19 21:00:33, brettw wrote: > Ditto on the include paths. And this include should be first (above string.h). Done. http://codereview.chromium.org/7381004/diff/17001/third_party/sfntly/src/subs... File third_party/sfntly/src/subsetter/subsetter_impl.h (right): http://codereview.chromium.org/7381004/diff/17001/third_party/sfntly/src/subs... third_party/sfntly/src/subsetter/subsetter_impl.h:20: #include "sfntly/font.h" On 2011/07/19 21:00:33, brettw wrote: > Please use full include paths. Done.
LGTM with nits. http://codereview.chromium.org/7381004/diff/23001/third_party/sfntly/src/subs... File third_party/sfntly/src/subsetter/font_subsetter.cc (right): http://codereview.chromium.org/7381004/diff/23001/third_party/sfntly/src/subs... third_party/sfntly/src/subsetter/font_subsetter.cc:18: #include "third_party/sfntly/src/subsetter/subsetter_impl.h" nit: after include of this file's matching header. http://codereview.chromium.org/7381004/diff/23001/third_party/sfntly/src/subs... File third_party/sfntly/src/subsetter/subsetter_impl.cc (right): http://codereview.chromium.org/7381004/diff/23001/third_party/sfntly/src/subs... third_party/sfntly/src/subsetter/subsetter_impl.cc:52: break; nit: should have NOTREACHED();
http://codereview.chromium.org/7381004/diff/23001/third_party/sfntly/src/subs... File third_party/sfntly/src/subsetter/font_subsetter.cc (right): http://codereview.chromium.org/7381004/diff/23001/third_party/sfntly/src/subs... third_party/sfntly/src/subsetter/font_subsetter.cc:18: #include "third_party/sfntly/src/subsetter/subsetter_impl.h" On 2011/07/20 01:48:00, Sheridan Rawlins wrote: > nit: after include of this file's matching header. Please include space after the include to make this a separate group. Sorry for the missing words in last comment :)
I'll send this to commit bot if no objections found tomorrow morning. Thanks. http://codereview.chromium.org/7381004/diff/23001/third_party/sfntly/src/subs... File third_party/sfntly/src/subsetter/font_subsetter.cc (right): http://codereview.chromium.org/7381004/diff/23001/third_party/sfntly/src/subs... third_party/sfntly/src/subsetter/font_subsetter.cc:18: #include "third_party/sfntly/src/subsetter/subsetter_impl.h" On 2011/07/20 01:48:00, Sheridan Rawlins wrote: > nit: after include of this file's matching header. Done. http://codereview.chromium.org/7381004/diff/23001/third_party/sfntly/src/subs... File third_party/sfntly/src/subsetter/subsetter_impl.cc (right): http://codereview.chromium.org/7381004/diff/23001/third_party/sfntly/src/subs... third_party/sfntly/src/subsetter/subsetter_impl.cc:52: break; On 2011/07/20 01:48:00, Sheridan Rawlins wrote: > nit: should have NOTREACHED(); Can't do that. The dependency is chrome->skia->sfntly. We shall not let sfntly back link to chrome and create a perfect circle.
http://codereview.chromium.org/7381004/diff/23001/third_party/sfntly/src/subs... File third_party/sfntly/src/subsetter/subsetter_impl.cc (right): http://codereview.chromium.org/7381004/diff/23001/third_party/sfntly/src/subs... third_party/sfntly/src/subsetter/subsetter_impl.cc:52: break; Got it - external code. Then please revert to google styleguide and put assert() on default: case statement if not expected. Thanks. On 2011/07/20 02:07:06, arthurhsu wrote: > On 2011/07/20 01:48:00, Sheridan Rawlins wrote: > > nit: should have NOTREACHED(); > > Can't do that. The dependency is chrome->skia->sfntly. We shall not let sfntly > back link to chrome and create a perfect circle.
http://codereview.chromium.org/7381004/diff/23001/third_party/sfntly/src/subs... File third_party/sfntly/src/subsetter/subsetter_impl.cc (right): http://codereview.chromium.org/7381004/diff/23001/third_party/sfntly/src/subs... third_party/sfntly/src/subsetter/subsetter_impl.cc:52: break; assert(false) that is :) http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Loops_and_Swit... -SCR On 2011/07/20 14:40:53, Sheridan Rawlins wrote: > Got it - external code. Then please revert to google styleguide and put > assert() on default: case statement if not expected. > > Thanks. > > On 2011/07/20 02:07:06, arthurhsu wrote: > > On 2011/07/20 01:48:00, Sheridan Rawlins wrote: > > > nit: should have NOTREACHED(); > > > > Can't do that. The dependency is chrome->skia->sfntly. We shall not let > sfntly > > back link to chrome and create a perfect circle. >
http://codereview.chromium.org/7381004/diff/23001/third_party/sfntly/src/subs... File third_party/sfntly/src/subsetter/subsetter_impl.cc (right): http://codereview.chromium.org/7381004/diff/23001/third_party/sfntly/src/subs... third_party/sfntly/src/subsetter/subsetter_impl.cc:52: break; On 2011/07/20 02:07:06, arthurhsu wrote: > On 2011/07/20 01:48:00, Sheridan Rawlins wrote: > > nit: should have NOTREACHED(); > > Can't do that. The dependency is chrome->skia->sfntly. We shall not let sfntly > back link to chrome and create a perfect circle. This code get compiled into the sfntly library? It could be compiled into it's own library that depends on chrome and sfntly... but it's probably not worth that just to get NOTREACHED. You may want to add a DEPS file to enforce the dependencies. http://codereview.chromium.org/7381004/diff/25002/third_party/sfntly/src/subs... File third_party/sfntly/src/subsetter/font_subsetter.h (right): http://codereview.chromium.org/7381004/diff/25002/third_party/sfntly/src/subs... third_party/sfntly/src/subsetter/font_subsetter.h:31: // If subsetting failes, a negative value is returned. If none of the glyph nit: fails http://codereview.chromium.org/7381004/diff/25002/third_party/sfntly/src/subs... third_party/sfntly/src/subsetter/font_subsetter.h:34: // |font_name| Font name of the font, required for TTC files. Ifi nit: remove "of the font" nit: Ifi => If http://codereview.chromium.org/7381004/diff/25002/third_party/sfntly/src/subs... third_party/sfntly/src/subsetter/font_subsetter.h:43: const unsigned char* original_font, const void* ? http://codereview.chromium.org/7381004/diff/25002/third_party/sfntly/src/subs... third_party/sfntly/src/subsetter/font_subsetter.h:45: const unsigned int* glyph_ids, const uint16_t* ?
http://codereview.chromium.org/7381004/diff/23001/third_party/sfntly/src/subs... File third_party/sfntly/src/subsetter/subsetter_impl.cc (right): http://codereview.chromium.org/7381004/diff/23001/third_party/sfntly/src/subs... third_party/sfntly/src/subsetter/subsetter_impl.cc:52: break; On 2011/07/20 16:29:59, vandebo wrote: > On 2011/07/20 02:07:06, arthurhsu wrote: > > On 2011/07/20 01:48:00, Sheridan Rawlins wrote: > > > nit: should have NOTREACHED(); > > > > Can't do that. The dependency is chrome->skia->sfntly. We shall not let > sfntly > > back link to chrome and create a perfect circle. > > This code get compiled into the sfntly library? It could be compiled into it's > own library that depends on chrome and sfntly... but it's probably not worth > that just to get NOTREACHED. > > You may want to add a DEPS file to enforce the dependencies. Actually, this is a valid case since font carries copyright info also in name field, which we don't care. I've added comments to clarify. http://codereview.chromium.org/7381004/diff/25002/third_party/sfntly/src/subs... File third_party/sfntly/src/subsetter/font_subsetter.h (right): http://codereview.chromium.org/7381004/diff/25002/third_party/sfntly/src/subs... third_party/sfntly/src/subsetter/font_subsetter.h:31: // If subsetting failes, a negative value is returned. If none of the glyph On 2011/07/20 16:29:59, vandebo wrote: > nit: fails Done. http://codereview.chromium.org/7381004/diff/25002/third_party/sfntly/src/subs... third_party/sfntly/src/subsetter/font_subsetter.h:34: // |font_name| Font name of the font, required for TTC files. Ifi On 2011/07/20 16:29:59, vandebo wrote: > nit: remove "of the font" > nit: Ifi => If Done. http://codereview.chromium.org/7381004/diff/25002/third_party/sfntly/src/subs... third_party/sfntly/src/subsetter/font_subsetter.h:43: const unsigned char* original_font, On 2011/07/20 16:29:59, vandebo wrote: > const void* ? C++ standard says unsigned char takes one byte, so it shall be better than void*. http://codereview.chromium.org/7381004/diff/25002/third_party/sfntly/src/subs... third_party/sfntly/src/subsetter/font_subsetter.h:45: const unsigned int* glyph_ids, On 2011/07/20 16:29:59, vandebo wrote: > const uint16_t* ? Caller may not include/have stdint.h. We use a bigger data type and check the range within.
Try job failure for 7381004-32001 (retry) on linux_clang for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...
Change committed as 93672 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
