Issue 4633050: Refactor SkPDFFont to enable font/cmap subsetting (Closed)
|
Created:
12 years, 10 months ago by arthurhsu Modified:
12 years, 9 months ago CC:
skia-review_googlegroups.com Base URL:
http://skia.googlecode.com/svn/trunk Visibility:
Public. |
DescriptionRefactor SkPDFFont to enable font/cmap subsetting
BUG=none
TEST=none
Rolled out r1956
Patch Set 1 #Patch Set 2 : Update per Mike Reed's code review #
Total comments: 40
Patch Set 3 : Update per code review #Patch Set 4 : Update per code review #
Total comments: 68
Patch Set 5 : Update per feedback #Patch Set 6 : Update per code review #Patch Set 7 : Update per code review, fix patching error #
Total comments: 34
Patch Set 8 : Update after branching bitset and catalog #Patch Set 9 : Update misplaced file #
Total comments: 42
Patch Set 10 : Update per code review #
Total comments: 36
Patch Set 11 : Update per code review #
Total comments: 6
Patch Set 12 : Update per code review #
Total comments: 35
Patch Set 13 : Update per code review #
Total comments: 12
Patch Set 14 : Update per code review #
Total comments: 89
Patch Set 15 : Update per code review #Patch Set 16 : Update iterator #
Total comments: 30
Patch Set 17 : Update per code review #
Total comments: 26
Patch Set 18 : Update per code review #
Total comments: 3
Patch Set 19 : Fix corrupted xref table #
MessagesTotal messages: 41
Lets try to minimize what we make public. Can some of these either helper classes or subclasses be moved into a .h in src/pdf/? In general lets only put definitions necessary for other public clients in the include/pdf directory.
Sign in to reply to this message.
On 2011/06/20 12:39:19, reed1 wrote: > Lets try to minimize what we make public. Can some of these either helper > classes or subclasses be moved into a .h in src/pdf/? In general lets only put > definitions necessary for other public clients in the include/pdf directory. Updated, implementation details moved to SkPDFFontImpl.h.
Sign in to reply to this message.
Initial comments. http://codereview.appspot.com/4633050/diff/3001/include/pdf/SkPDFDevice.h File include/pdf/SkPDFDevice.h (right): http://codereview.appspot.com/4633050/diff/3001/include/pdf/SkPDFDevice.h#new... include/pdf/SkPDFDevice.h:118: virtual void drawDevice(const SkDraw&, SkDevice*, int x, int y, drawDevice needs to merge glyph sets from the drawn device. http://codereview.appspot.com/4633050/diff/3001/include/pdf/SkPDFDevice.h#new... include/pdf/SkPDFDevice.h:175: SkTScopedPtr<SkPDFFontGlyphUsageSet> fFontGlyphUsageSet; We can probably make this a direct member - it will just require a reset method on SkPDFFontGlyphUsageSet http://codereview.appspot.com/4633050/diff/3001/include/pdf/SkPDFFont.h File include/pdf/SkPDFFont.h (right): http://codereview.appspot.com/4633050/diff/3001/include/pdf/SkPDFFont.h#newco... include/pdf/SkPDFFont.h:28: class SkPDFGlyphUsage : public SkNoncopyable { Lets be consistent between SkPDFFontGlyph.... and SkPDFGlyph... GlyphUsage is pretty generic - this doesn't have any functionality beyond a set, so lets just call it that... SkPDFGlyphSet? http://codereview.appspot.com/4633050/diff/3001/include/pdf/SkPDFFont.h#newco... include/pdf/SkPDFFont.h:94: SK_API virtual bool supportSubset() = 0; We probably don't need supportSubset, instead just make subset return a bool? http://codereview.appspot.com/4633050/diff/3001/include/pdf/SkPDFFont.h#newco... include/pdf/SkPDFFont.h:95: SK_API virtual void subset(SkPDFGlyphUsage* usage) = 0; In general, please try to use the doxygen comment style to comments public methods. http://codereview.appspot.com/4633050/diff/3001/include/pdf/SkPDFFont.h#newco... include/pdf/SkPDFFont.h:130: static SkPDFFont* create(SkAdvancedTypefaceMetrics* fontInfo, Static methods start with a capital. http://codereview.appspot.com/4633050/diff/3001/src/pdf/SkPDFDevice.cpp File src/pdf/SkPDFDevice.cpp (right): http://codereview.appspot.com/4633050/diff/3001/src/pdf/SkPDFDevice.cpp#newco... src/pdf/SkPDFDevice.cpp:1514: void SkPDFDevice::reportFontGlyphUsage(SkPDFFontGlyphUsageSet* usage) { Report could be confused for note/set... maybe getGlyphUsage? http://codereview.appspot.com/4633050/diff/3001/src/pdf/SkPDFDocument.cpp File src/pdf/SkPDFDocument.cpp (right): http://codereview.appspot.com/4633050/diff/3001/src/pdf/SkPDFDocument.cpp#new... src/pdf/SkPDFDocument.cpp:81: // Build font subsetting info before proceeding. Let's put this in a conditional on an SkPDFDocument class field. i.e. so we can create documents with subset fonts and those without. http://codereview.appspot.com/4633050/diff/3001/src/pdf/SkPDFFont.cpp File src/pdf/SkPDFFont.cpp (right): http://codereview.appspot.com/4633050/diff/3001/src/pdf/SkPDFFont.cpp#newcode393 src/pdf/SkPDFFont.cpp:393: if (glyphUnicode[i]) { Combine these two if's http://codereview.appspot.com/4633050/diff/3001/src/pdf/SkPDFFont.cpp#newcode411 src/pdf/SkPDFFont.cpp:411: static SkPDFStream* generate_cmap(const SkTDArray<SkUnichar>& glyphUnicode, this is a tounicode cmap, not a regular cmap: generate_to_unicode_cmap ? http://codereview.appspot.com/4633050/diff/3001/src/pdf/SkPDFFont.cpp#newcode426 src/pdf/SkPDFFont.cpp:426: SkPDFGlyphUsage::SkPDFGlyphUsage() { We should probably abstract this into two - implement a SkBitSet class and then use it in SkPDFGlyphUsage http://codereview.appspot.com/4633050/diff/3001/src/pdf/SkPDFFont.cpp#newcode427 src/pdf/SkPDFFont.cpp:427: fBitmap = new uint8_t[DIMENSION]; For alignment reasons, the class size will probably get rounded up to a multiple of 32bits, so the SkBitSet might as well use uint32_t's instead of uint8_t's. http://codereview.appspot.com/4633050/diff/3001/src/pdf/SkPDFFont.cpp#newcode482 src/pdf/SkPDFFont.cpp:482: fSet[index].fSubset = new SkPDFGlyphUsage(); These will leak - the class destructor probably needs something. http://codereview.appspot.com/4633050/diff/3001/src/pdf/SkPDFFont.cpp#newcode532 src/pdf/SkPDFFont.cpp:532: SkAdvancedTypefaceMetrics::FontType SkPDFFont::getType() { Should this just be a virtual and push special cases into the subclass implementing it? http://codereview.appspot.com/4633050/diff/3001/src/pdf/SkPDFFont.cpp#newcode674 src/pdf/SkPDFFont.cpp:674: if (newFont->populate(glyphID)) { Should this just go in the Type1 constructor? http://codereview.appspot.com/4633050/diff/3001/src/pdf/SkPDFFontImpl.h File src/pdf/SkPDFFontImpl.h (right): http://codereview.appspot.com/4633050/diff/3001/src/pdf/SkPDFFontImpl.h#newco... src/pdf/SkPDFFontImpl.h:57: SkMutex fEmitting; I think we can make this work without a lock. http://codereview.appspot.com/4633050/diff/3001/src/pdf/SkPDFFontImpl.h#newco... src/pdf/SkPDFFontImpl.h:109: class SkPDFFontGlyphUsageSet : public SkNoncopyable { This should probably go in SkPDFFont http://codereview.appspot.com/4633050/diff/3001/src/pdf/SkPDFFontImpl.h#newco... src/pdf/SkPDFFontImpl.h:109: class SkPDFFontGlyphUsageSet : public SkNoncopyable { SkPDFGlyphSetMap/SkPDFFontGlyphSetMap ? http://codereview.appspot.com/4633050/diff/3001/src/pdf/SkPDFFontImpl.h#newco... src/pdf/SkPDFFontImpl.h:118: class FontGlyphPair { FontGlyphSetPair http://codereview.appspot.com/4633050/diff/3001/src/pdf/SkPDFFontImpl.h#newco... src/pdf/SkPDFFontImpl.h:124: SkPDFGlyphUsage* fSubset; SkTScopedPtr<SkPDFGlyphUsage> fSubset
Sign in to reply to this message.
http://codereview.appspot.com/4633050/diff/3001/include/pdf/SkPDFDevice.h File include/pdf/SkPDFDevice.h (right): http://codereview.appspot.com/4633050/diff/3001/include/pdf/SkPDFDevice.h#new... include/pdf/SkPDFDevice.h:118: virtual void drawDevice(const SkDraw&, SkDevice*, int x, int y, On 2011/06/23 00:18:27, Steve VanDeBogart wrote: > drawDevice needs to merge glyph sets from the drawn device. Done. http://codereview.appspot.com/4633050/diff/3001/include/pdf/SkPDFDevice.h#new... include/pdf/SkPDFDevice.h:175: SkTScopedPtr<SkPDFFontGlyphUsageSet> fFontGlyphUsageSet; On 2011/06/23 00:18:27, Steve VanDeBogart wrote: > We can probably make this a direct member - it will just require a reset method > on SkPDFFontGlyphUsageSet A reset method is added, however, I'd like to keep it in a scoped pointer to avoid cross-reference of headers. http://codereview.appspot.com/4633050/diff/3001/include/pdf/SkPDFFont.h File include/pdf/SkPDFFont.h (right): http://codereview.appspot.com/4633050/diff/3001/include/pdf/SkPDFFont.h#newco... include/pdf/SkPDFFont.h:28: class SkPDFGlyphUsage : public SkNoncopyable { On 2011/06/23 00:18:27, Steve VanDeBogart wrote: > Lets be consistent between SkPDFFontGlyph.... and SkPDFGlyph... GlyphUsage is > pretty generic - this doesn't have any functionality beyond a set, so lets just > call it that... SkPDFGlyphSet? Done. http://codereview.appspot.com/4633050/diff/3001/include/pdf/SkPDFFont.h#newco... include/pdf/SkPDFFont.h:94: SK_API virtual bool supportSubset() = 0; On 2011/06/23 00:18:27, Steve VanDeBogart wrote: > We probably don't need supportSubset, instead just make subset return a bool? Done. http://codereview.appspot.com/4633050/diff/3001/include/pdf/SkPDFFont.h#newco... include/pdf/SkPDFFont.h:95: SK_API virtual void subset(SkPDFGlyphUsage* usage) = 0; On 2011/06/23 00:18:27, Steve VanDeBogart wrote: > In general, please try to use the doxygen comment style to comments public > methods. Done. http://codereview.appspot.com/4633050/diff/3001/include/pdf/SkPDFFont.h#newco... include/pdf/SkPDFFont.h:130: static SkPDFFont* create(SkAdvancedTypefaceMetrics* fontInfo, On 2011/06/23 00:18:27, Steve VanDeBogart wrote: > Static methods start with a capital. Done. http://codereview.appspot.com/4633050/diff/3001/src/pdf/SkPDFDevice.cpp File src/pdf/SkPDFDevice.cpp (right): http://codereview.appspot.com/4633050/diff/3001/src/pdf/SkPDFDevice.cpp#newco... src/pdf/SkPDFDevice.cpp:1514: void SkPDFDevice::reportFontGlyphUsage(SkPDFFontGlyphUsageSet* usage) { On 2011/06/23 00:18:27, Steve VanDeBogart wrote: > Report could be confused for note/set... maybe getGlyphUsage? Done. http://codereview.appspot.com/4633050/diff/3001/src/pdf/SkPDFDocument.cpp File src/pdf/SkPDFDocument.cpp (right): http://codereview.appspot.com/4633050/diff/3001/src/pdf/SkPDFDocument.cpp#new... src/pdf/SkPDFDocument.cpp:81: // Build font subsetting info before proceeding. On 2011/06/23 00:18:27, Steve VanDeBogart wrote: > Let's put this in a conditional on an SkPDFDocument class field. i.e. so we can > create documents with subset fonts and those without. Done. http://codereview.appspot.com/4633050/diff/3001/src/pdf/SkPDFFont.cpp File src/pdf/SkPDFFont.cpp (right): http://codereview.appspot.com/4633050/diff/3001/src/pdf/SkPDFFont.cpp#newcode393 src/pdf/SkPDFFont.cpp:393: if (glyphUnicode[i]) { On 2011/06/23 00:18:27, Steve VanDeBogart wrote: > Combine these two if's Done. http://codereview.appspot.com/4633050/diff/3001/src/pdf/SkPDFFont.cpp#newcode411 src/pdf/SkPDFFont.cpp:411: static SkPDFStream* generate_cmap(const SkTDArray<SkUnichar>& glyphUnicode, On 2011/06/23 00:18:27, Steve VanDeBogart wrote: > this is a tounicode cmap, not a regular cmap: generate_to_unicode_cmap ? Done. http://codereview.appspot.com/4633050/diff/3001/src/pdf/SkPDFFont.cpp#newcode426 src/pdf/SkPDFFont.cpp:426: SkPDFGlyphUsage::SkPDFGlyphUsage() { On 2011/06/23 00:18:27, Steve VanDeBogart wrote: > We should probably abstract this into two - implement a SkBitSet class and then > use it in SkPDFGlyphUsage Done. http://codereview.appspot.com/4633050/diff/3001/src/pdf/SkPDFFont.cpp#newcode427 src/pdf/SkPDFFont.cpp:427: fBitmap = new uint8_t[DIMENSION]; On 2011/06/23 00:18:27, Steve VanDeBogart wrote: > For alignment reasons, the class size will probably get rounded up to a multiple > of 32bits, so the SkBitSet might as well use uint32_t's instead of uint8_t's. Alignment should be non-issue if dimension of fBitmap is rounded to 32-bits boundary. http://codereview.appspot.com/4633050/diff/3001/src/pdf/SkPDFFont.cpp#newcode482 src/pdf/SkPDFFont.cpp:482: fSet[index].fSubset = new SkPDFGlyphUsage(); On 2011/06/23 00:18:27, Steve VanDeBogart wrote: > These will leak - the class destructor probably needs something. Done. http://codereview.appspot.com/4633050/diff/3001/src/pdf/SkPDFFont.cpp#newcode532 src/pdf/SkPDFFont.cpp:532: SkAdvancedTypefaceMetrics::FontType SkPDFFont::getType() { On 2011/06/23 00:18:27, Steve VanDeBogart wrote: > Should this just be a virtual and push special cases into the subclass > implementing it? Done. http://codereview.appspot.com/4633050/diff/3001/src/pdf/SkPDFFont.cpp#newcode674 src/pdf/SkPDFFont.cpp:674: if (newFont->populate(glyphID)) { On 2011/06/23 00:18:27, Steve VanDeBogart wrote: > Should this just go in the Type1 constructor? Done. http://codereview.appspot.com/4633050/diff/3001/src/pdf/SkPDFFontImpl.h File src/pdf/SkPDFFontImpl.h (right): http://codereview.appspot.com/4633050/diff/3001/src/pdf/SkPDFFontImpl.h#newco... src/pdf/SkPDFFontImpl.h:57: SkMutex fEmitting; On 2011/06/23 00:18:27, Steve VanDeBogart wrote: > I think we can make this work without a lock. Done. http://codereview.appspot.com/4633050/diff/3001/src/pdf/SkPDFFontImpl.h#newco... src/pdf/SkPDFFontImpl.h:109: class SkPDFFontGlyphUsageSet : public SkNoncopyable { On 2011/06/23 00:18:27, Steve VanDeBogart wrote: > This should probably go in SkPDFFont Done. http://codereview.appspot.com/4633050/diff/3001/src/pdf/SkPDFFontImpl.h#newco... src/pdf/SkPDFFontImpl.h:109: class SkPDFFontGlyphUsageSet : public SkNoncopyable { On 2011/06/23 00:18:27, Steve VanDeBogart wrote: > SkPDFGlyphSetMap/SkPDFFontGlyphSetMap ? Done. http://codereview.appspot.com/4633050/diff/3001/src/pdf/SkPDFFontImpl.h#newco... src/pdf/SkPDFFontImpl.h:118: class FontGlyphPair { On 2011/06/23 00:18:27, Steve VanDeBogart wrote: > FontGlyphSetPair Done. http://codereview.appspot.com/4633050/diff/3001/src/pdf/SkPDFFontImpl.h#newco... src/pdf/SkPDFFontImpl.h:124: SkPDFGlyphUsage* fSubset; On 2011/06/23 00:18:27, Steve VanDeBogart wrote: > SkTScopedPtr<SkPDFGlyphUsage> fSubset Done.
Sign in to reply to this message.
Some more comments. http://codereview.appspot.com/4633050/diff/10001/gyp/core.gyp File gyp/core.gyp (right): http://codereview.appspot.com/4633050/diff/10001/gyp/core.gyp#newcode31 gyp/core.gyp:31: '../src/core/SkBitSet.cpp', Should add the header file as well. http://codereview.appspot.com/4633050/diff/10001/include/core/SkBitSet.h File include/core/SkBitSet.h (right): http://codereview.appspot.com/4633050/diff/10001/include/core/SkBitSet.h#newc... include/core/SkBitSet.h:25: explicit SkBitSet(int32_t numberOfBits); Why not just document that this will be rounded up to a whole number of bytes, then you can get rid of fDimension? http://codereview.appspot.com/4633050/diff/10001/include/core/SkBitSet.h#newc... include/core/SkBitSet.h:34: // Resize the bitset to number of bits, original data is kept if and only if Use the Doxygen comment format. http://codereview.appspot.com/4633050/diff/10001/include/core/SkBitSet.h#newc... include/core/SkBitSet.h:42: void reset(); private ? http://codereview.appspot.com/4633050/diff/10001/include/core/SkBitSet.h#newc... include/core/SkBitSet.h:48: void set(int32_t index); setBit ? http://codereview.appspot.com/4633050/diff/10001/include/core/SkBitSet.h#newc... include/core/SkBitSet.h:51: void clear(int32_t index); clearBit? Removes false polymorphism between clear() and clear(int) http://codereview.appspot.com/4633050/diff/10001/include/core/SkBitSet.h#newc... include/core/SkBitSet.h:54: void toggle(int32_t index); Lets start with only the methods that we need - it's easy enough to add more if we need them. http://codereview.appspot.com/4633050/diff/10001/include/core/SkBitSet.h#newc... include/core/SkBitSet.h:63: uint8_t* fBitmap; SkAutoFree fBitmap ? http://codereview.appspot.com/4633050/diff/10001/include/pdf/SkPDFFont.h File include/pdf/SkPDFFont.h (right): http://codereview.appspot.com/4633050/diff/10001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:53: class FontGlyphSetPair { struct http://codereview.appspot.com/4633050/diff/10001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:87: SK_API virtual SkAdvancedTypefaceMetrics::FontType getType() = 0; Looks like all the implementations of this do the same thing - doesn't need to be virtual. http://codereview.appspot.com/4633050/diff/10001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:122: * instance of SkPDFFont otherwise. Note that the caller is responsible for unrefing the result. http://codereview.appspot.com/4633050/diff/10001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:127: SkRefPtr<SkTypeface> fTypeface; I think this can be private. http://codereview.appspot.com/4633050/diff/10001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:131: bool fMultiByteGlyphs; protected data is nasty. Maybe just change the accessor to return the right thing based on the font type? For the rest of the protected data, we should either make it private and add a protected accessor, or move it to the subclass. http://codereview.appspot.com/4633050/diff/10001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:166: static bool find(uint32_t fontID, uint16_t glyphID, int* index); static methods should be capitalized. http://codereview.appspot.com/4633050/diff/10001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:168: // Populate font into dict. The glyphID may not be honored due to the Seems like glyphID is still honored for unsupported font, it just doesn't mean anything. http://codereview.appspot.com/4633050/diff/10001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:173: virtual void adjustGlyphRangeForSingleByteEncoding(int16_t glyphID); This probably doesn't need to be virtual - it can consult fMultiByteGlyphs http://codereview.appspot.com/4633050/diff/10001/include/pdf/SkPDFPage.h File include/pdf/SkPDFPage.h (right): http://codereview.appspot.com/4633050/diff/10001/include/pdf/SkPDFPage.h#newc... include/pdf/SkPDFPage.h:93: void getGlyphUsage(SkPDFGlyphSetMap* usage); Document method. http://codereview.appspot.com/4633050/diff/10001/src/pdf/SkPDFDevice.cpp File src/pdf/SkPDFDevice.cpp (right): http://codereview.appspot.com/4633050/diff/10001/src/pdf/SkPDFDevice.cpp#newc... src/pdf/SkPDFDevice.cpp:843: numGlyphs - consumedGlyphCount); second argument should be availableGlyphs http://codereview.appspot.com/4633050/diff/10001/src/pdf/SkPDFDevice.cpp#newc... src/pdf/SkPDFDevice.cpp:1126: *xobject = new SkPDFFormXObject(this); We need to preserve the glyph sets across this call. http://codereview.appspot.com/4633050/diff/10001/src/pdf/SkPDFDocument.cpp File src/pdf/SkPDFDocument.cpp (right): http://codereview.appspot.com/4633050/diff/10001/src/pdf/SkPDFDocument.cpp#ne... src/pdf/SkPDFDocument.cpp:90: // Font swapping: for a given TTF/OTF, the font object is just a As discusses, this won't work if we use a page on multiple devices. http://codereview.appspot.com/4633050/diff/10001/src/pdf/SkPDFFont.cpp File src/pdf/SkPDFFont.cpp (right): http://codereview.appspot.com/4633050/diff/10001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:426: fBitSet.resize(DIMENSION); Why not use the initialization list for this? http://codereview.appspot.com/4633050/diff/10001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:472: FontGlyphSetPair* item = new FontGlyphSetPair(); Why not make the constructor take both the font and SkPDFGlyphSet or constructor a new set? http://codereview.appspot.com/4633050/diff/10001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:646: return new SkPDFUnsupportedFont(fontInfo, typeface, glyphID, We'd like to make a Type3 font in this case (that's what the old code does). We probably don't need SkPDFUnsupportedFont at all. http://codereview.appspot.com/4633050/diff/10001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:735: Add a divider for each implementation: //////////////... http://codereview.appspot.com/4633050/diff/10001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:887: bool SkPDFType0Font::delayedPopulate(SkPDFGlyphSet* subset) { Lets always do a delayed populate (and call it populate), even if the font type doesn't require it - so that everything works the same way (as much as possible).
Sign in to reply to this message.
For now, lets move the bitset helper class into /pdf/
Sign in to reply to this message.
http://codereview.appspot.com/4633050/diff/10001/include/core/SkBitSet.h File include/core/SkBitSet.h (right): http://codereview.appspot.com/4633050/diff/10001/include/core/SkBitSet.h#newc... include/core/SkBitSet.h:38: // Clear all data. clearAll() ? http://codereview.appspot.com/4633050/diff/10001/include/core/SkBitSet.h#newc... include/core/SkBitSet.h:44: // Drop existing set and clone source instead. is this the same as operator= ? http://codereview.appspot.com/4633050/diff/10001/include/core/SkBitSet.h#newc... include/core/SkBitSet.h:53: // Toggle the value of the index-th bit. xor() ? http://codereview.appspot.com/4633050/diff/10001/include/core/SkBitSet.h#newc... include/core/SkBitSet.h:56: // Test if bit index is set. isSet() ? (maybe has is better) http://codereview.appspot.com/4633050/diff/10001/include/core/SkBitSet.h#newc... include/core/SkBitSet.h:59: // OR bits from source. bitOr() or or() -- merge is wacky, and will *always* need to be commented. Document what happens if this and source have different bit-counts. http://codereview.appspot.com/4633050/diff/10001/include/core/SkBitSet.h#newc... include/core/SkBitSet.h:62: private: I'm ok with the raw pointer. For performance when acting on all of the bits, you'll definitely want to have allocated chunks in int or long sizes, rather than bytes. http://codereview.appspot.com/4633050/diff/10001/include/core/SkBitSet.h#newc... include/core/SkBitSet.h:64: size_t fSize; Are these allocation sizes, bit counts? Lets give them very clear names. Do we really need to values? Isn't fBitCount sufficient?
Sign in to reply to this message.
http://codereview.appspot.com/4633050/diff/10001/gyp/core.gyp File gyp/core.gyp (right): http://codereview.appspot.com/4633050/diff/10001/gyp/core.gyp#newcode31 gyp/core.gyp:31: '../src/core/SkBitSet.cpp', On 2011/06/27 22:14:27, Steve VanDeBogart wrote: > Should add the header file as well. Done. Per reed's request, it will be in pdf.gyp instead. http://codereview.appspot.com/4633050/diff/10001/include/core/SkBitSet.h File include/core/SkBitSet.h (right): http://codereview.appspot.com/4633050/diff/10001/include/core/SkBitSet.h#newc... include/core/SkBitSet.h:25: explicit SkBitSet(int32_t numberOfBits); On 2011/06/27 22:14:27, Steve VanDeBogart wrote: > Why not just document that this will be rounded up to a whole number of bytes, > then you can get rid of fDimension? Done. http://codereview.appspot.com/4633050/diff/10001/include/core/SkBitSet.h#newc... include/core/SkBitSet.h:34: // Resize the bitset to number of bits, original data is kept if and only if On 2011/06/27 22:14:27, Steve VanDeBogart wrote: > Use the Doxygen comment format. Done. http://codereview.appspot.com/4633050/diff/10001/include/core/SkBitSet.h#newc... include/core/SkBitSet.h:38: // Clear all data. On 2011/06/27 22:27:13, reed1 wrote: > clearAll() ? Done. http://codereview.appspot.com/4633050/diff/10001/include/core/SkBitSet.h#newc... include/core/SkBitSet.h:42: void reset(); On 2011/06/27 22:14:27, Steve VanDeBogart wrote: > private ? Done. http://codereview.appspot.com/4633050/diff/10001/include/core/SkBitSet.h#newc... include/core/SkBitSet.h:44: // Drop existing set and clone source instead. On 2011/06/27 22:27:13, reed1 wrote: > is this the same as operator= ? That's correct. Both semantics are provided. http://codereview.appspot.com/4633050/diff/10001/include/core/SkBitSet.h#newc... include/core/SkBitSet.h:48: void set(int32_t index); On 2011/06/27 22:14:27, Steve VanDeBogart wrote: > setBit ? Done. http://codereview.appspot.com/4633050/diff/10001/include/core/SkBitSet.h#newc... include/core/SkBitSet.h:51: void clear(int32_t index); On 2011/06/27 22:14:27, Steve VanDeBogart wrote: > clearBit? Removes false polymorphism between clear() and clear(int) Done. http://codereview.appspot.com/4633050/diff/10001/include/core/SkBitSet.h#newc... include/core/SkBitSet.h:53: // Toggle the value of the index-th bit. On 2011/06/27 22:27:13, reed1 wrote: > xor() ? Effectively xor. For consistence of naming, toggleBit is used. http://codereview.appspot.com/4633050/diff/10001/include/core/SkBitSet.h#newc... include/core/SkBitSet.h:54: void toggle(int32_t index); On 2011/06/27 22:14:27, Steve VanDeBogart wrote: > Lets start with only the methods that we need - it's easy enough to add more if > we need them. Done. http://codereview.appspot.com/4633050/diff/10001/include/core/SkBitSet.h#newc... include/core/SkBitSet.h:56: // Test if bit index is set. On 2011/06/27 22:27:13, reed1 wrote: > isSet() ? (maybe has is better) changed to isBitSet http://codereview.appspot.com/4633050/diff/10001/include/core/SkBitSet.h#newc... include/core/SkBitSet.h:59: // OR bits from source. On 2011/06/27 22:27:13, reed1 wrote: > bitOr() or or() -- merge is wacky, and will *always* need to be commented. > > Document what happens if this and source have different bit-counts. Done. http://codereview.appspot.com/4633050/diff/10001/include/core/SkBitSet.h#newc... include/core/SkBitSet.h:62: private: On 2011/06/27 22:27:13, reed1 wrote: > I'm ok with the raw pointer. > > For performance when acting on all of the bits, you'll definitely want to have > allocated chunks in int or long sizes, rather than bytes. This should be non-issue since we already rounded our buffer to 32-bit boundary. http://codereview.appspot.com/4633050/diff/10001/include/core/SkBitSet.h#newc... include/core/SkBitSet.h:63: uint8_t* fBitmap; On 2011/06/27 22:14:27, Steve VanDeBogart wrote: > SkAutoFree fBitmap ? Didn't use this in the first place because it only saves me one line in dtor, but force me to do wacky syntax like fBitmap.get()[index]. http://codereview.appspot.com/4633050/diff/10001/include/core/SkBitSet.h#newc... include/core/SkBitSet.h:64: size_t fSize; On 2011/06/27 22:27:13, reed1 wrote: > Are these allocation sizes, bit counts? Lets give them very clear names. > Do we really need to values? Isn't fBitCount sufficient? Renamed to fByteCount. We need this to keep the rounded number and perform correct assertion/memcpy. http://codereview.appspot.com/4633050/diff/10001/include/pdf/SkPDFFont.h File include/pdf/SkPDFFont.h (right): http://codereview.appspot.com/4633050/diff/10001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:53: class FontGlyphSetPair { On 2011/06/27 22:14:27, Steve VanDeBogart wrote: > struct Does not use struct because it has member functions. Coding guideline said structs are for aggregation of simple-typed data members. http://codereview.appspot.com/4633050/diff/10001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:87: SK_API virtual SkAdvancedTypefaceMetrics::FontType getType() = 0; On 2011/06/27 22:14:27, Steve VanDeBogart wrote: > Looks like all the implementations of this do the same thing - doesn't need to > be virtual. Not really. Unsupported font is doing things differently. It was pushed down to derived class based on last review comments. http://codereview.appspot.com/4633050/diff/10001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:122: * instance of SkPDFFont otherwise. On 2011/06/27 22:14:27, Steve VanDeBogart wrote: > Note that the caller is responsible for unrefing the result. Done. http://codereview.appspot.com/4633050/diff/10001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:127: SkRefPtr<SkTypeface> fTypeface; On 2011/06/27 22:14:27, Steve VanDeBogart wrote: > I think this can be private. Done. http://codereview.appspot.com/4633050/diff/10001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:131: bool fMultiByteGlyphs; On 2011/06/27 22:14:27, Steve VanDeBogart wrote: > protected data is nasty. Maybe just change the accessor to return the right > thing based on the font type? > > For the rest of the protected data, we should either make it private and add a > protected accessor, or move it to the subclass. Done. http://codereview.appspot.com/4633050/diff/10001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:166: static bool find(uint32_t fontID, uint16_t glyphID, int* index); On 2011/06/27 22:14:27, Steve VanDeBogart wrote: > static methods should be capitalized. Done. http://codereview.appspot.com/4633050/diff/10001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:168: // Populate font into dict. The glyphID may not be honored due to the On 2011/06/27 22:14:27, Steve VanDeBogart wrote: > Seems like glyphID is still honored for unsupported font, it just doesn't mean > anything. Done. http://codereview.appspot.com/4633050/diff/10001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:173: virtual void adjustGlyphRangeForSingleByteEncoding(int16_t glyphID); On 2011/06/27 22:14:27, Steve VanDeBogart wrote: > This probably doesn't need to be virtual - it can consult fMultiByteGlyphs After refactoring, it's better to have this virtual. http://codereview.appspot.com/4633050/diff/10001/include/pdf/SkPDFPage.h File include/pdf/SkPDFPage.h (right): http://codereview.appspot.com/4633050/diff/10001/include/pdf/SkPDFPage.h#newc... include/pdf/SkPDFPage.h:93: void getGlyphUsage(SkPDFGlyphSetMap* usage); On 2011/06/27 22:14:27, Steve VanDeBogart wrote: > Document method. Done. http://codereview.appspot.com/4633050/diff/10001/src/pdf/SkPDFDevice.cpp File src/pdf/SkPDFDevice.cpp (right): http://codereview.appspot.com/4633050/diff/10001/src/pdf/SkPDFDevice.cpp#newc... src/pdf/SkPDFDevice.cpp:843: numGlyphs - consumedGlyphCount); On 2011/06/27 22:14:27, Steve VanDeBogart wrote: > second argument should be availableGlyphs Done. http://codereview.appspot.com/4633050/diff/10001/src/pdf/SkPDFDevice.cpp#newc... src/pdf/SkPDFDevice.cpp:1126: *xobject = new SkPDFFormXObject(this); On 2011/06/27 22:14:27, Steve VanDeBogart wrote: > We need to preserve the glyph sets across this call. Recorded glyph sets are not affected by this call. http://codereview.appspot.com/4633050/diff/10001/src/pdf/SkPDFDocument.cpp File src/pdf/SkPDFDocument.cpp (right): http://codereview.appspot.com/4633050/diff/10001/src/pdf/SkPDFDocument.cpp#ne... src/pdf/SkPDFDocument.cpp:90: // Font swapping: for a given TTF/OTF, the font object is just a On 2011/06/27 22:14:27, Steve VanDeBogart wrote: > As discusses, this won't work if we use a page on multiple devices. Done. http://codereview.appspot.com/4633050/diff/10001/src/pdf/SkPDFFont.cpp File src/pdf/SkPDFFont.cpp (right): http://codereview.appspot.com/4633050/diff/10001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:426: fBitSet.resize(DIMENSION); On 2011/06/27 22:14:27, Steve VanDeBogart wrote: > Why not use the initialization list for this? Done. http://codereview.appspot.com/4633050/diff/10001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:472: FontGlyphSetPair* item = new FontGlyphSetPair(); On 2011/06/27 22:14:27, Steve VanDeBogart wrote: > Why not make the constructor take both the font and SkPDFGlyphSet or constructor > a new set? Done. http://codereview.appspot.com/4633050/diff/10001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:646: return new SkPDFUnsupportedFont(fontInfo, typeface, glyphID, On 2011/06/27 22:14:27, Steve VanDeBogart wrote: > We'd like to make a Type3 font in this case (that's what the old code does). We > probably don't need SkPDFUnsupportedFont at all. Done. http://codereview.appspot.com/4633050/diff/10001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:735: On 2011/06/27 22:14:27, Steve VanDeBogart wrote: > Add a divider for each implementation: > //////////////... Done. http://codereview.appspot.com/4633050/diff/10001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:887: bool SkPDFType0Font::delayedPopulate(SkPDFGlyphSet* subset) { On 2011/06/27 22:14:27, Steve VanDeBogart wrote: > Lets always do a delayed populate (and call it populate), even if the font type > doesn't require it - so that everything works the same way (as much as > possible). I've done some refactoring about this confusion. Basically populate() shall not be a method in SkPDFFont. Subclass shall determine the best timing to generate its own contents.
Sign in to reply to this message.
I don't understand your comment about 8bit -vs- 32bit performance. Your loop is still per-byte, instead of per-word if you allocated 32bits at a time.
Sign in to reply to this message.
On 2011/06/28 21:15:52, reed1 wrote: > I don't understand your comment about 8bit -vs- 32bit performance. Your loop is > still per-byte, instead of per-word if you allocated 32bits at a time. My bad, I kept thinking that you are talking about memory alignment and the speed of memcpy, but you're actually talking about reduce loop count in orBits(). It's updated in the lastest upload.
Sign in to reply to this message.
Some comments. I will give more later (interview). http://codereview.appspot.com/4633050/diff/10001/include/core/SkBitSet.h File include/core/SkBitSet.h (right): http://codereview.appspot.com/4633050/diff/10001/include/core/SkBitSet.h#newc... include/core/SkBitSet.h:2: * Copyright (C) 2011 Google Inc. Pull this change into a separate CL that can go in first. http://codereview.appspot.com/4633050/diff/10001/include/core/SkBitSet.h#newc... include/core/SkBitSet.h:63: uint8_t* fBitmap; On 2011/06/28 21:05:57, arthurhsu wrote: > On 2011/06/27 22:14:27, Steve VanDeBogart wrote: > > SkAutoFree fBitmap ? > > Didn't use this in the first place because it only saves me one line in dtor, > but force me to do wacky syntax like fBitmap.get()[index]. But it is much more fragile. How about adding a private accessor method int32_t* getByte(int index) { return fBitmap.get() + index/32; } or something similar? http://codereview.appspot.com/4633050/diff/10001/include/pdf/SkPDFFont.h File include/pdf/SkPDFFont.h (right): http://codereview.appspot.com/4633050/diff/10001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:53: class FontGlyphSetPair { On 2011/06/28 21:05:57, arthurhsu wrote: > On 2011/06/27 22:14:27, Steve VanDeBogart wrote: > > struct > > Does not use struct because it has member functions. Coding guideline said > structs are for aggregation of simple-typed data members. The member functions simply initialize the members, which is permitted by the style guide. http://codereview.appspot.com/4633050/diff/19001/include/pdf/SkBitSet.h File include/pdf/SkBitSet.h (right): http://codereview.appspot.com/4633050/diff/19001/include/pdf/SkBitSet.h#newco... include/pdf/SkBitSet.h:28: explicit SkBitSet(int32_t numberOfBits); int arguments can be of native size, no reason to explicitly specify int32_t http://codereview.appspot.com/4633050/diff/19001/include/pdf/SkBitSet.h#newco... include/pdf/SkBitSet.h:36: const SkBitSet& operator|=(const SkBitSet& rhs); Lets drop this in favor of orBits. http://codereview.appspot.com/4633050/diff/19001/include/pdf/SkBitSet.h#newco... include/pdf/SkBitSet.h:41: void resize(int32_t numberOfBits); I can't think of a reasonable instance where a consumer would want to change the size of a bitset. If you need this for internal purposes, lets make it private, otherwise, remove it. http://codereview.appspot.com/4633050/diff/19001/include/pdf/SkBitSet.h#newco... include/pdf/SkBitSet.h:49: void clone(const SkBitSet& source); We only need one way to do things. Drop this in favor of operator= http://codereview.appspot.com/4633050/diff/19001/include/pdf/SkBitSet.h#newco... include/pdf/SkBitSet.h:53: void setBit(int32_t index); Insted of setBit and clearBit, why not setBit(int index, bool value) ? http://codereview.appspot.com/4633050/diff/19001/include/pdf/SkPDFCatalog.h File include/pdf/SkPDFCatalog.h (right): http://codereview.appspot.com/4633050/diff/19001/include/pdf/SkPDFCatalog.h#n... include/pdf/SkPDFCatalog.h:73: /** Give the substitute object (i.e. subsetted font object) if found. Lets also pull this change into a separate CL so it can go in sooner. I have another use in mind for optional stream compression. http://codereview.appspot.com/4633050/diff/19001/include/pdf/SkPDFCatalog.h#n... include/pdf/SkPDFCatalog.h:110: SkTDArray<SkPDFObject*> fOriginalFonts; This isn't font specific. http://codereview.appspot.com/4633050/diff/19001/include/pdf/SkPDFCatalog.h#n... include/pdf/SkPDFCatalog.h:111: SkTDArray< SkRefPtr<SkPDFObject> > fSubstituteFonts; SkTDArray only works correctly for POD, don't pass it an object. http://codereview.appspot.com/4633050/diff/19001/include/pdf/SkPDFCatalog.h#n... include/pdf/SkPDFCatalog.h:111: SkTDArray< SkRefPtr<SkPDFObject> > fSubstituteFonts; Make a struct for the data, so that we don't have multiple arrays. http://codereview.appspot.com/4633050/diff/19001/src/pdf/SkBitSet.cpp File src/pdf/SkBitSet.cpp (right): http://codereview.appspot.com/4633050/diff/19001/src/pdf/SkBitSet.cpp#newcode44 src/pdf/SkBitSet.cpp:44: } This will return false if both bit sets have a fDwordCount of 0. http://codereview.appspot.com/4633050/diff/19001/src/pdf/SkBitSet.cpp#newcode50 src/pdf/SkBitSet.cpp:50: return !(operator==(rhs)); is !(this == rhs) easier to understand? http://codereview.appspot.com/4633050/diff/19001/src/pdf/SkBitSet.cpp#newcode62 src/pdf/SkBitSet.cpp:62: if (numberOfBits & 0x1f) { This whole if block can be just newSize = (numberOfBits + 31) / 32; http://codereview.appspot.com/4633050/diff/19001/src/pdf/SkBitSet.cpp#newcode87 src/pdf/SkBitSet.cpp:87: delete[] fBitmap; also fBitmap = NULL; http://codereview.appspot.com/4633050/diff/19001/src/pdf/SkPDFCatalog.cpp File src/pdf/SkPDFCatalog.cpp (right): http://codereview.appspot.com/4633050/diff/19001/src/pdf/SkPDFCatalog.cpp#new... src/pdf/SkPDFCatalog.cpp:66: // Let's check one more time if this object is a substitute // If it's not in the many array, check if it's a substitute object. http://codereview.appspot.com/4633050/diff/19001/src/pdf/SkPDFCatalog.cpp#new... src/pdf/SkPDFCatalog.cpp:69: return findObjectIndex(fOriginalFonts[i]); If this is the only direction we do lookups in, then maybe we should store the sub obj and the orig obj index? http://codereview.appspot.com/4633050/diff/19001/src/pdf/SkPDFCatalog.cpp#new... src/pdf/SkPDFCatalog.cpp:78: // Check if this is a substitute object, if so, use its original object. Just let findObjectIndex handle this.
Sign in to reply to this message.
+1 for removing apis we don't need yet, and for having only one way to do something (e.g. assignment and clone) http://codereview.appspot.com/4633050/diff/19001/src/pdf/SkBitSet.cpp File src/pdf/SkBitSet.cpp (right): http://codereview.appspot.com/4633050/diff/19001/src/pdf/SkBitSet.cpp#newcode50 src/pdf/SkBitSet.cpp:50: return !(operator==(rhs)); On 2011/06/29 16:58:06, Steve VanDeBogart wrote: > is !(this == rhs) easier to understand? !(*this == rhs)
Sign in to reply to this message.
Fixed and moved to a separate CR 4635080 Add SkBitSet class for font subsetting. http://codereview.appspot.com/4633050/diff/10001/include/core/SkBitSet.h File include/core/SkBitSet.h (right): http://codereview.appspot.com/4633050/diff/10001/include/core/SkBitSet.h#newc... include/core/SkBitSet.h:2: * Copyright (C) 2011 Google Inc. On 2011/06/29 16:58:06, Steve VanDeBogart wrote: > Pull this change into a separate CL that can go in first. Done. http://codereview.appspot.com/4633050/diff/19001/include/pdf/SkBitSet.h File include/pdf/SkBitSet.h (right): http://codereview.appspot.com/4633050/diff/19001/include/pdf/SkBitSet.h#newco... include/pdf/SkBitSet.h:28: explicit SkBitSet(int32_t numberOfBits); On 2011/06/29 16:58:06, Steve VanDeBogart wrote: > int arguments can be of native size, no reason to explicitly specify int32_t Done. http://codereview.appspot.com/4633050/diff/19001/include/pdf/SkBitSet.h#newco... include/pdf/SkBitSet.h:36: const SkBitSet& operator|=(const SkBitSet& rhs); On 2011/06/29 16:58:06, Steve VanDeBogart wrote: > Lets drop this in favor of orBits. Done. http://codereview.appspot.com/4633050/diff/19001/include/pdf/SkBitSet.h#newco... include/pdf/SkBitSet.h:41: void resize(int32_t numberOfBits); On 2011/06/29 16:58:06, Steve VanDeBogart wrote: > I can't think of a reasonable instance where a consumer would want to change the > size of a bitset. If you need this for internal purposes, lets make it private, > otherwise, remove it. Done. http://codereview.appspot.com/4633050/diff/19001/include/pdf/SkBitSet.h#newco... include/pdf/SkBitSet.h:49: void clone(const SkBitSet& source); On 2011/06/29 16:58:06, Steve VanDeBogart wrote: > We only need one way to do things. Drop this in favor of operator= Done. http://codereview.appspot.com/4633050/diff/19001/include/pdf/SkBitSet.h#newco... include/pdf/SkBitSet.h:53: void setBit(int32_t index); On 2011/06/29 16:58:06, Steve VanDeBogart wrote: > Insted of setBit and clearBit, why not setBit(int index, bool value) ? Done. http://codereview.appspot.com/4633050/diff/19001/src/pdf/SkBitSet.cpp File src/pdf/SkBitSet.cpp (right): http://codereview.appspot.com/4633050/diff/19001/src/pdf/SkBitSet.cpp#newcode44 src/pdf/SkBitSet.cpp:44: } On 2011/06/29 16:58:06, Steve VanDeBogart wrote: > This will return false if both bit sets have a fDwordCount of 0. Done. http://codereview.appspot.com/4633050/diff/19001/src/pdf/SkBitSet.cpp#newcode50 src/pdf/SkBitSet.cpp:50: return !(operator==(rhs)); On 2011/06/29 17:18:20, reed1 wrote: > On 2011/06/29 16:58:06, Steve VanDeBogart wrote: > > is !(this == rhs) easier to understand? > > !(*this == rhs) Done. http://codereview.appspot.com/4633050/diff/19001/src/pdf/SkBitSet.cpp#newcode50 src/pdf/SkBitSet.cpp:50: return !(operator==(rhs)); On 2011/06/29 16:58:06, Steve VanDeBogart wrote: > is !(this == rhs) easier to understand? Done, using Mike's version. http://codereview.appspot.com/4633050/diff/19001/src/pdf/SkBitSet.cpp#newcode62 src/pdf/SkBitSet.cpp:62: if (numberOfBits & 0x1f) { On 2011/06/29 16:58:06, Steve VanDeBogart wrote: > This whole if block can be just > newSize = (numberOfBits + 31) / 32; Done. http://codereview.appspot.com/4633050/diff/19001/src/pdf/SkBitSet.cpp#newcode87 src/pdf/SkBitSet.cpp:87: delete[] fBitmap; On 2011/06/29 16:58:06, Steve VanDeBogart wrote: > also fBitmap = NULL; Done.
Sign in to reply to this message.
Separated to http://codereview.appspot.com/4650060 per Steve's request. http://codereview.appspot.com/4633050/diff/19001/include/pdf/SkPDFCatalog.h File include/pdf/SkPDFCatalog.h (right): http://codereview.appspot.com/4633050/diff/19001/include/pdf/SkPDFCatalog.h#n... include/pdf/SkPDFCatalog.h:73: /** Give the substitute object (i.e. subsetted font object) if found. On 2011/06/29 16:58:06, Steve VanDeBogart wrote: > Lets also pull this change into a separate CL so it can go in sooner. I have > another use in mind for optional stream compression. Done. http://codereview.appspot.com/4633050/diff/19001/include/pdf/SkPDFCatalog.h#n... include/pdf/SkPDFCatalog.h:110: SkTDArray<SkPDFObject*> fOriginalFonts; On 2011/06/29 16:58:06, Steve VanDeBogart wrote: > This isn't font specific. Done. http://codereview.appspot.com/4633050/diff/19001/include/pdf/SkPDFCatalog.h#n... include/pdf/SkPDFCatalog.h:111: SkTDArray< SkRefPtr<SkPDFObject> > fSubstituteFonts; On 2011/06/29 16:58:06, Steve VanDeBogart wrote: > SkTDArray only works correctly for POD, don't pass it an object. Done. http://codereview.appspot.com/4633050/diff/19001/include/pdf/SkPDFCatalog.h#n... include/pdf/SkPDFCatalog.h:111: SkTDArray< SkRefPtr<SkPDFObject> > fSubstituteFonts; On 2011/06/29 16:58:06, Steve VanDeBogart wrote: > Make a struct for the data, so that we don't have multiple arrays. Done. http://codereview.appspot.com/4633050/diff/19001/src/pdf/SkPDFCatalog.cpp File src/pdf/SkPDFCatalog.cpp (right): http://codereview.appspot.com/4633050/diff/19001/src/pdf/SkPDFCatalog.cpp#new... src/pdf/SkPDFCatalog.cpp:66: // Let's check one more time if this object is a substitute On 2011/06/29 16:58:06, Steve VanDeBogart wrote: > // If it's not in the many array, check if it's a substitute object. Done. http://codereview.appspot.com/4633050/diff/19001/src/pdf/SkPDFCatalog.cpp#new... src/pdf/SkPDFCatalog.cpp:69: return findObjectIndex(fOriginalFonts[i]); On 2011/06/29 16:58:06, Steve VanDeBogart wrote: > If this is the only direction we do lookups in, then maybe we should store the > sub obj and the orig obj index? It's the only direction for now, but I'm not that sure it won't change. http://codereview.appspot.com/4633050/diff/19001/src/pdf/SkPDFCatalog.cpp#new... src/pdf/SkPDFCatalog.cpp:78: // Check if this is a substitute object, if so, use its original object. On 2011/06/29 16:58:06, Steve VanDeBogart wrote: > Just let findObjectIndex handle this. Done.
Sign in to reply to this message.
Updated the CL to match latest version of Skia, also reflect changes after submission of CL 4650060 and 4627077
Sign in to reply to this message.
Chris can you help review this change while Steve is out? Thanks.
Sign in to reply to this message.
Sending out these initial comments that are mostly in the headers. In general I think we should rename the variables named 'usage' and 'subset' to be named to match their type unless a different name provides more value. http://codereview.appspot.com/4633050/diff/24001/include/pdf/SkPDFDevice.h File include/pdf/SkPDFDevice.h (right): http://codereview.appspot.com/4633050/diff/24001/include/pdf/SkPDFDevice.h#ne... include/pdf/SkPDFDevice.h:37: class SkPDFGlyphSet; Please put these in sort order. http://codereview.appspot.com/4633050/diff/24001/include/pdf/SkPDFDevice.h#ne... include/pdf/SkPDFDevice.h:149: void getGlyphUsage(SkPDFGlyphSetMap* usage); Nit: Can we call this function getGlyphSetMap? Perhaps we can also add a comment describing what this function returns. Looks like this function was named with 'usage' in the name because the type of the argument used to have 'usage' in it's name but it doesn't anymore. http://codereview.appspot.com/4633050/diff/24001/include/pdf/SkPDFDevice.h#ne... include/pdf/SkPDFDevice.h:149: void getGlyphUsage(SkPDFGlyphSetMap* usage); Can we have this function return a const reference to the SkPDFGlyphSetMap object. Callers can use the merge function on the returned reference. http://codereview.appspot.com/4633050/diff/24001/include/pdf/SkPDFDevice.h#ne... include/pdf/SkPDFDevice.h:170: SkTScopedPtr<SkPDFGlyphSetMap> fFontGlyphUsageSet; Nit: Can we call this member fFontGlyphSetMap? http://codereview.appspot.com/4633050/diff/24001/include/pdf/SkPDFDevice.h#ne... include/pdf/SkPDFDevice.h:227: void noteGlyphUsage(SkPDFFont* font, uint16_t* glyphIDs, size_t numGlyphs); Can we move this function to the SkPDFGlyphSetMap class? http://codereview.appspot.com/4633050/diff/24001/include/pdf/SkPDFDocument.h File include/pdf/SkPDFDocument.h (right): http://codereview.appspot.com/4633050/diff/24001/include/pdf/SkPDFDocument.h#... include/pdf/SkPDFDocument.h:65: bool fForPreview; I don't see any code that sets this member (outside of the constructor). Do we want to omit it until such code exists? http://codereview.appspot.com/4633050/diff/24001/include/pdf/SkPDFFont.h File include/pdf/SkPDFFont.h (right): http://codereview.appspot.com/4633050/diff/24001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:25: #include "SkBitSet.h" Headers aren't in sort order. http://codereview.appspot.com/4633050/diff/24001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:34: void set(uint16_t glyphID); Nit: Newline here would make this look cleaner. http://codereview.appspot.com/4633050/diff/24001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:35: void set(uint16_t* glyphIDs, size_t numGlyphs); Can you make the glyphIDs argument const. http://codereview.appspot.com/4633050/diff/24001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:37: void merge(SkPDFGlyphSet* usage); Can you make the usage argument const. http://codereview.appspot.com/4633050/diff/24001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:49: SkPDFGlyphSet* get(SkPDFFont* font); Is it possible to make this function return a reference? Also can you make the font argument const. http://codereview.appspot.com/4633050/diff/24001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:57: void subset(SkPDFCatalog* catalog); This function modifies SkPDFCatalog? Seems like it should be added as a function on SkPDFCatalog. Perhaps SkPDFCatalog::performFontSubsetting. http://codereview.appspot.com/4633050/diff/24001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:67: SkTScopedPtr<SkPDFGlyphSet> fSubset; Optional: fSubset -> fGylphSet http://codereview.appspot.com/4633050/diff/24001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:99: SK_API virtual bool multiByteGlyphs() = 0; This can be a const function. http://codereview.appspot.com/4633050/diff/24001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:132: SK_API virtual SkPDFFont* subset(SkPDFGlyphSet* usage) = 0; Can we name this function getFontSubset. http://codereview.appspot.com/4633050/diff/24001/include/pdf/SkPDFPage.h File include/pdf/SkPDFPage.h (right): http://codereview.appspot.com/4633050/diff/24001/include/pdf/SkPDFPage.h#newc... include/pdf/SkPDFPage.h:95: void getGlyphUsage(SkPDFGlyphSetMap* usage); getGlyphSetMap http://codereview.appspot.com/4633050/diff/24001/src/pdf/SkPDFCatalog.cpp File src/pdf/SkPDFCatalog.cpp (left): http://codereview.appspot.com/4633050/diff/24001/src/pdf/SkPDFCatalog.cpp#old... src/pdf/SkPDFCatalog.cpp:209: } Is there a change here? If not can we remove this file from the change. http://codereview.appspot.com/4633050/diff/24001/src/pdf/SkPDFFont.cpp File src/pdf/SkPDFFont.cpp (right): http://codereview.appspot.com/4633050/diff/24001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:441: SkPDFGlyphSet::SkPDFGlyphSet() : fBitSet(DIMENSION) { Can we just use SK_MaxU16 + 1 or 0x10000 as the argument here instead of creating the static variable DIMENSION? http://codereview.appspot.com/4633050/diff/24001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:502: for (int i = 0; i < usage->fSet.count(); ++i) { The SkPDFGlyphSet::merge function modifies the current instance while this function modifies the argument. We should make this consistent. I think we should modify the current instance and make the usage argument const.
Sign in to reply to this message.
Please add dox/comments in headers for all new methods http://codereview.appspot.com/4633050/diff/24001/include/pdf/SkPDFDevice.h File include/pdf/SkPDFDevice.h (right): http://codereview.appspot.com/4633050/diff/24001/include/pdf/SkPDFDevice.h#ne... include/pdf/SkPDFDevice.h:227: void noteGlyphUsage(SkPDFFont* font, uint16_t* glyphIDs, size_t numGlyphs); Can glyphsIDs be const? Skia uses int for array indices and counts, and only size_t for byteSizes (rare) http://codereview.appspot.com/4633050/diff/24001/include/pdf/SkPDFFont.h File include/pdf/SkPDFFont.h (right): http://codereview.appspot.com/4633050/diff/24001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:35: void set(uint16_t* glyphIDs, size_t numGlyphs); can glyphIDs be const? Use int for array indices and counts, not size_t
Sign in to reply to this message.
http://codereview.appspot.com/4633050/diff/24001/include/pdf/SkPDFDevice.h File include/pdf/SkPDFDevice.h (right): http://codereview.appspot.com/4633050/diff/24001/include/pdf/SkPDFDevice.h#ne... include/pdf/SkPDFDevice.h:37: class SkPDFGlyphSet; On 2011/07/12 21:30:29, Chris Guillory wrote: > Please put these in sort order. Done. http://codereview.appspot.com/4633050/diff/24001/include/pdf/SkPDFDevice.h#ne... include/pdf/SkPDFDevice.h:149: void getGlyphUsage(SkPDFGlyphSetMap* usage); On 2011/07/12 21:30:29, Chris Guillory wrote: > Nit: Can we call this function getGlyphSetMap? Perhaps we can also add a comment > describing what this function returns. > > Looks like this function was named with 'usage' in the name because the type of > the argument used to have 'usage' in it's name but it doesn't anymore. Renamed to getFontGlyphUsage and add comments about what's it used for. http://codereview.appspot.com/4633050/diff/24001/include/pdf/SkPDFDevice.h#ne... include/pdf/SkPDFDevice.h:149: void getGlyphUsage(SkPDFGlyphSetMap* usage); On 2011/07/12 21:30:29, Chris Guillory wrote: > Can we have this function return a const reference to the SkPDFGlyphSetMap > object. Callers can use the merge function on the returned reference. Done. http://codereview.appspot.com/4633050/diff/24001/include/pdf/SkPDFDevice.h#ne... include/pdf/SkPDFDevice.h:170: SkTScopedPtr<SkPDFGlyphSetMap> fFontGlyphUsageSet; On 2011/07/12 21:30:29, Chris Guillory wrote: > Nit: Can we call this member fFontGlyphSetMap? Renamed to fFontGlyphUsage to be consistent with the getFontGlyphUsage() function. http://codereview.appspot.com/4633050/diff/24001/include/pdf/SkPDFDevice.h#ne... include/pdf/SkPDFDevice.h:227: void noteGlyphUsage(SkPDFFont* font, uint16_t* glyphIDs, size_t numGlyphs); On 2011/07/13 13:22:31, reed1 wrote: > Can glyphsIDs be const? > Skia uses int for array indices and counts, and only size_t for byteSizes (rare) Done. http://codereview.appspot.com/4633050/diff/24001/include/pdf/SkPDFDevice.h#ne... include/pdf/SkPDFDevice.h:227: void noteGlyphUsage(SkPDFFont* font, uint16_t* glyphIDs, size_t numGlyphs); On 2011/07/12 21:30:29, Chris Guillory wrote: > Can we move this function to the SkPDFGlyphSetMap class? Done. http://codereview.appspot.com/4633050/diff/24001/include/pdf/SkPDFDocument.h File include/pdf/SkPDFDocument.h (right): http://codereview.appspot.com/4633050/diff/24001/include/pdf/SkPDFDocument.h#... include/pdf/SkPDFDocument.h:65: bool fForPreview; On 2011/07/12 21:30:29, Chris Guillory wrote: > I don't see any code that sets this member (outside of the constructor). Do we > want to omit it until such code exists? Done. http://codereview.appspot.com/4633050/diff/24001/include/pdf/SkPDFFont.h File include/pdf/SkPDFFont.h (right): http://codereview.appspot.com/4633050/diff/24001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:25: #include "SkBitSet.h" On 2011/07/12 21:30:29, Chris Guillory wrote: > Headers aren't in sort order. Done. http://codereview.appspot.com/4633050/diff/24001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:34: void set(uint16_t glyphID); On 2011/07/12 21:30:29, Chris Guillory wrote: > Nit: Newline here would make this look cleaner. Done. http://codereview.appspot.com/4633050/diff/24001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:35: void set(uint16_t* glyphIDs, size_t numGlyphs); On 2011/07/12 21:30:29, Chris Guillory wrote: > Can you make the glyphIDs argument const. Done. http://codereview.appspot.com/4633050/diff/24001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:35: void set(uint16_t* glyphIDs, size_t numGlyphs); On 2011/07/13 13:22:31, reed1 wrote: > can glyphIDs be const? > Use int for array indices and counts, not size_t Done. http://codereview.appspot.com/4633050/diff/24001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:37: void merge(SkPDFGlyphSet* usage); On 2011/07/12 21:30:29, Chris Guillory wrote: > Can you make the usage argument const. Done. http://codereview.appspot.com/4633050/diff/24001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:49: SkPDFGlyphSet* get(SkPDFFont* font); On 2011/07/12 21:30:29, Chris Guillory wrote: > Is it possible to make this function return a reference? Also can you make the > font argument const. The return value can be NULL so returning a pointer. The argument unfortunately can't be const. It is used in the FontGlyphSetPair, which in turn will be used to call the subset() method of SkPDFFont. Deco it with const will start compiler complaints all over the place which really does not justify the benefit. http://codereview.appspot.com/4633050/diff/24001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:57: void subset(SkPDFCatalog* catalog); On 2011/07/12 21:30:29, Chris Guillory wrote: > This function modifies SkPDFCatalog? Seems like it should be added as a function > on SkPDFCatalog. Perhaps SkPDFCatalog::performFontSubsetting. Done. http://codereview.appspot.com/4633050/diff/24001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:67: SkTScopedPtr<SkPDFGlyphSet> fSubset; On 2011/07/12 21:30:29, Chris Guillory wrote: > Optional: fSubset -> fGylphSet Done. http://codereview.appspot.com/4633050/diff/24001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:99: SK_API virtual bool multiByteGlyphs() = 0; On 2011/07/12 21:30:29, Chris Guillory wrote: > This can be a const function. Done. http://codereview.appspot.com/4633050/diff/24001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:132: SK_API virtual SkPDFFont* subset(SkPDFGlyphSet* usage) = 0; On 2011/07/12 21:30:29, Chris Guillory wrote: > Can we name this function getFontSubset. renamed to getSubsetFont() http://codereview.appspot.com/4633050/diff/24001/include/pdf/SkPDFPage.h File include/pdf/SkPDFPage.h (right): http://codereview.appspot.com/4633050/diff/24001/include/pdf/SkPDFPage.h#newc... include/pdf/SkPDFPage.h:95: void getGlyphUsage(SkPDFGlyphSetMap* usage); On 2011/07/12 21:30:29, Chris Guillory wrote: > getGlyphSetMap changed to getFontGlyphUsage() http://codereview.appspot.com/4633050/diff/24001/src/pdf/SkPDFCatalog.cpp File src/pdf/SkPDFCatalog.cpp (left): http://codereview.appspot.com/4633050/diff/24001/src/pdf/SkPDFCatalog.cpp#old... src/pdf/SkPDFCatalog.cpp:209: } On 2011/07/12 21:30:29, Chris Guillory wrote: > Is there a change here? If not can we remove this file from the change. Not an issue anymore after adding the performFontSubsetting() method as suggested. http://codereview.appspot.com/4633050/diff/24001/src/pdf/SkPDFFont.cpp File src/pdf/SkPDFFont.cpp (right): http://codereview.appspot.com/4633050/diff/24001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:441: SkPDFGlyphSet::SkPDFGlyphSet() : fBitSet(DIMENSION) { On 2011/07/12 21:30:29, Chris Guillory wrote: > Can we just use SK_MaxU16 + 1 or 0x10000 as the argument here instead of > creating the static variable DIMENSION? Done. http://codereview.appspot.com/4633050/diff/24001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:502: for (int i = 0; i < usage->fSet.count(); ++i) { On 2011/07/12 21:30:29, Chris Guillory wrote: > The SkPDFGlyphSet::merge function modifies the current instance while this > function modifies the argument. We should make this consistent. I think we > should modify the current instance and make the usage argument const. Done.
Sign in to reply to this message.
Why do we have SkPDFFontImpl.h? Can't this be a part of SkPDFFont.cpp? http://codereview.appspot.com/4633050/diff/30001/include/pdf/SkBitSet.h File include/pdf/SkBitSet.h (right): http://codereview.appspot.com/4633050/diff/30001/include/pdf/SkBitSet.h#newco... include/pdf/SkBitSet.h:26: explicit SkBitSet(const int numberOfBits); It seems unconventional to put const on the pass by value arguments. Also it seems to add clutter. http://codereview.appspot.com/4633050/diff/30001/include/pdf/SkPDFCatalog.h File include/pdf/SkPDFCatalog.h (right): http://codereview.appspot.com/4633050/diff/30001/include/pdf/SkPDFCatalog.h#n... include/pdf/SkPDFCatalog.h:26: class SkPDFGlyphSetMap; Nit: Newline after this line. http://codereview.appspot.com/4633050/diff/30001/include/pdf/SkPDFCatalog.h#n... include/pdf/SkPDFCatalog.h:96: void performFontSubsetting(SkPDFGlyphSetMap* usage); You can make the usage argument const. Look like you'll need to make SkPDFGlyphSetMap::getCount and SkPDFGlyphSetMap::at const member functions. http://codereview.appspot.com/4633050/diff/30001/include/pdf/SkPDFDevice.h File include/pdf/SkPDFDevice.h (right): http://codereview.appspot.com/4633050/diff/30001/include/pdf/SkPDFDevice.h#ne... include/pdf/SkPDFDevice.h:149: /** Returns a GlyphSetMap which represents glyph usage of every font that Nit: GlyphSetMap -> SkPDFGlyphSetMap http://codereview.appspot.com/4633050/diff/30001/include/pdf/SkPDFDevice.h#ne... include/pdf/SkPDFDevice.h:173: // Glyph ids used for each font on this device. Nit: Add a newline above this line. Also you'll need to rebase this file. http://codereview.appspot.com/4633050/diff/30001/include/pdf/SkPDFFont.h File include/pdf/SkPDFFont.h (right): http://codereview.appspot.com/4633050/diff/30001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:38: bool has(const uint16_t glyphID); This function can be const. http://codereview.appspot.com/4633050/diff/30001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:156: static SkTDArray<FontRec>& canonicalFonts(); Can you make these other static member functions begin with a capital letter to match the change to the "Find" static member function. http://codereview.appspot.com/4633050/diff/30001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:161: virtual SkAdvancedTypefaceMetrics* fontInfo(); Make these accessors const where possible. http://codereview.appspot.com/4633050/diff/30001/include/pdf/SkPDFPage.h File include/pdf/SkPDFPage.h (right): http://codereview.appspot.com/4633050/diff/30001/include/pdf/SkPDFPage.h#newc... include/pdf/SkPDFPage.h:93: /** Returns a GlyphSetMap which represents glyph usage of every font that Nit: GlyphSetMap -> SkPDFGlyphSetMap http://codereview.appspot.com/4633050/diff/30001/src/pdf/SkPDFDevice.cpp File src/pdf/SkPDFDevice.cpp (right): http://codereview.appspot.com/4633050/diff/30001/src/pdf/SkPDFDevice.cpp#newc... src/pdf/SkPDFDevice.cpp:1501: return *fFontGlyphUsage; Optional: *fFontGlyphUsage -> *fFontGlyphUsage.get() http://codereview.appspot.com/4633050/diff/30001/src/pdf/SkPDFDocument.cpp File src/pdf/SkPDFDocument.cpp (right): http://codereview.appspot.com/4633050/diff/30001/src/pdf/SkPDFDocument.cpp#ne... src/pdf/SkPDFDocument.cpp:96: SkTScopedPtr<SkPDFGlyphSetMap> usage; Perhaps performFontSubsetting can contain this logic to build usage from the pages array. http://codereview.appspot.com/4633050/diff/30001/src/pdf/SkPDFFont.cpp File src/pdf/SkPDFFont.cpp (right): http://codereview.appspot.com/4633050/diff/30001/src/pdf/SkPDFFont.cpp#newcode35 src/pdf/SkPDFFont.cpp:35: #include "SkPDFCatalog.h" Add in sort order. http://codereview.appspot.com/4633050/diff/30001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:398: SkPDFGlyphSet* subset, SkDynamicMemoryWStream* cmap) { Looks like subset can be const. http://codereview.appspot.com/4633050/diff/30001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:421: static SkPDFStream* generate_to_unicode_cmap( to_unicode -> tounicode. To match with append_tounicode_header? http://codereview.appspot.com/4633050/diff/30001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:518: } fSet.reset(); http://codereview.appspot.com/4633050/diff/30001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:605: SkRefPtr<SkAdvancedTypefaceMetrics> fontInfoPtr; Why are you renaming this variable? http://codereview.appspot.com/4633050/diff/30001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:670: SkPDFFont* newFont = NULL; Can you remove the newFont local and just return directly in the places where it's used? http://codereview.appspot.com/4633050/diff/30001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:697: return fFontInfo ? fFontInfo.get() : NULL; Looks like you can just return fFontInfo.get().
Sign in to reply to this message.
SkPDFFontImpl.h is done so per Mike's review. I've also tried to rebase SkPDFDevice.h but I don't know if I've got it done right. It's appreciated if you can help me check that. Thanks. http://codereview.appspot.com/4633050/diff/30001/include/pdf/SkBitSet.h File include/pdf/SkBitSet.h (right): http://codereview.appspot.com/4633050/diff/30001/include/pdf/SkBitSet.h#newco... include/pdf/SkBitSet.h:26: explicit SkBitSet(const int numberOfBits); On 2011/07/14 22:48:32, Chris Guillory wrote: > It seems unconventional to put const on the pass by value arguments. Also it > seems to add clutter. Done. http://codereview.appspot.com/4633050/diff/30001/include/pdf/SkPDFCatalog.h File include/pdf/SkPDFCatalog.h (right): http://codereview.appspot.com/4633050/diff/30001/include/pdf/SkPDFCatalog.h#n... include/pdf/SkPDFCatalog.h:26: class SkPDFGlyphSetMap; On 2011/07/14 22:48:32, Chris Guillory wrote: > Nit: Newline after this line. Done. http://codereview.appspot.com/4633050/diff/30001/include/pdf/SkPDFCatalog.h#n... include/pdf/SkPDFCatalog.h:96: void performFontSubsetting(SkPDFGlyphSetMap* usage); On 2011/07/14 22:48:32, Chris Guillory wrote: > You can make the usage argument const. Look like you'll need to make > SkPDFGlyphSetMap::getCount and SkPDFGlyphSetMap::at const member functions. Done. http://codereview.appspot.com/4633050/diff/30001/include/pdf/SkPDFDevice.h File include/pdf/SkPDFDevice.h (right): http://codereview.appspot.com/4633050/diff/30001/include/pdf/SkPDFDevice.h#ne... include/pdf/SkPDFDevice.h:149: /** Returns a GlyphSetMap which represents glyph usage of every font that On 2011/07/14 22:48:32, Chris Guillory wrote: > Nit: GlyphSetMap -> SkPDFGlyphSetMap Done. http://codereview.appspot.com/4633050/diff/30001/include/pdf/SkPDFDevice.h#ne... include/pdf/SkPDFDevice.h:173: // Glyph ids used for each font on this device. On 2011/07/14 22:48:32, Chris Guillory wrote: > Nit: Add a newline above this line. Also you'll need to rebase this file. Done. http://codereview.appspot.com/4633050/diff/30001/include/pdf/SkPDFFont.h File include/pdf/SkPDFFont.h (right): http://codereview.appspot.com/4633050/diff/30001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:38: bool has(const uint16_t glyphID); On 2011/07/14 22:48:32, Chris Guillory wrote: > This function can be const. Done. http://codereview.appspot.com/4633050/diff/30001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:156: static SkTDArray<FontRec>& canonicalFonts(); On 2011/07/14 22:48:32, Chris Guillory wrote: > Can you make these other static member functions begin with a capital letter to > match the change to the "Find" static member function. Can we do this in a separate CL? I think this CL is already big enough ... http://codereview.appspot.com/4633050/diff/30001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:161: virtual SkAdvancedTypefaceMetrics* fontInfo(); On 2011/07/14 22:48:32, Chris Guillory wrote: > Make these accessors const where possible. Done. http://codereview.appspot.com/4633050/diff/30001/include/pdf/SkPDFPage.h File include/pdf/SkPDFPage.h (right): http://codereview.appspot.com/4633050/diff/30001/include/pdf/SkPDFPage.h#newc... include/pdf/SkPDFPage.h:93: /** Returns a GlyphSetMap which represents glyph usage of every font that On 2011/07/14 22:48:32, Chris Guillory wrote: > Nit: GlyphSetMap -> SkPDFGlyphSetMap Done. http://codereview.appspot.com/4633050/diff/30001/src/pdf/SkPDFDevice.cpp File src/pdf/SkPDFDevice.cpp (right): http://codereview.appspot.com/4633050/diff/30001/src/pdf/SkPDFDevice.cpp#newc... src/pdf/SkPDFDevice.cpp:1501: return *fFontGlyphUsage; On 2011/07/14 22:48:32, Chris Guillory wrote: > Optional: *fFontGlyphUsage -> *fFontGlyphUsage.get() Done. http://codereview.appspot.com/4633050/diff/30001/src/pdf/SkPDFDocument.cpp File src/pdf/SkPDFDocument.cpp (right): http://codereview.appspot.com/4633050/diff/30001/src/pdf/SkPDFDocument.cpp#ne... src/pdf/SkPDFDocument.cpp:96: SkTScopedPtr<SkPDFGlyphSetMap> usage; On 2011/07/14 22:48:32, Chris Guillory wrote: > Perhaps performFontSubsetting can contain this logic to build usage from the > pages array. Pages are private members of Document, not Catalog, so we can't do that in performFontSubsetting(). http://codereview.appspot.com/4633050/diff/30001/src/pdf/SkPDFFont.cpp File src/pdf/SkPDFFont.cpp (right): http://codereview.appspot.com/4633050/diff/30001/src/pdf/SkPDFFont.cpp#newcode35 src/pdf/SkPDFFont.cpp:35: #include "SkPDFCatalog.h" On 2011/07/14 22:48:32, Chris Guillory wrote: > Add in sort order. Done. http://codereview.appspot.com/4633050/diff/30001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:398: SkPDFGlyphSet* subset, SkDynamicMemoryWStream* cmap) { On 2011/07/14 22:48:32, Chris Guillory wrote: > Looks like subset can be const. Done. http://codereview.appspot.com/4633050/diff/30001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:421: static SkPDFStream* generate_to_unicode_cmap( On 2011/07/14 22:48:32, Chris Guillory wrote: > to_unicode -> tounicode. To match with append_tounicode_header? Done. http://codereview.appspot.com/4633050/diff/30001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:518: } On 2011/07/14 22:48:32, Chris Guillory wrote: > fSet.reset(); Done. http://codereview.appspot.com/4633050/diff/30001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:605: SkRefPtr<SkAdvancedTypefaceMetrics> fontInfoPtr; On 2011/07/14 22:48:32, Chris Guillory wrote: > Why are you renaming this variable? Compiler gives warnings when variable name is the same as method name (line 501). http://codereview.appspot.com/4633050/diff/30001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:670: SkPDFFont* newFont = NULL; On 2011/07/14 22:48:32, Chris Guillory wrote: > Can you remove the newFont local and just return directly in the places where > it's used? Done. http://codereview.appspot.com/4633050/diff/30001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:697: return fFontInfo ? fFontInfo.get() : NULL; On 2011/07/14 22:48:32, Chris Guillory wrote: > Looks like you can just return fFontInfo.get(). Done.
Sign in to reply to this message.
+1 on early comment: don't add const to integral parameters. It doesn't change anything meaningful about the signature or the contract with the caller. http://codereview.appspot.com/4633050/diff/40001/include/pdf/SkPDFCatalog.h File include/pdf/SkPDFCatalog.h (right): http://codereview.appspot.com/4633050/diff/40001/include/pdf/SkPDFCatalog.h#n... include/pdf/SkPDFCatalog.h:96: */ In general, skia takes a const reference, rather than a const * reasoning: it is easier to tell at the call site if the parameter is const. If its a ptr, it usually isn't. If its a ref (or by value), it is. http://codereview.appspot.com/4633050/diff/40001/include/pdf/SkPDFFont.h File include/pdf/SkPDFFont.h (right): http://codereview.appspot.com/4633050/diff/40001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:36: void set(const uint16_t glyphID); Don't use const for integral parameters http://codereview.appspot.com/4633050/diff/40001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:38: bool has(const uint16_t glyphID) const; Dont' use const for integral parameters.
Sign in to reply to this message.
http://codereview.appspot.com/4633050/diff/40001/include/pdf/SkPDFCatalog.h File include/pdf/SkPDFCatalog.h (right): http://codereview.appspot.com/4633050/diff/40001/include/pdf/SkPDFCatalog.h#n... include/pdf/SkPDFCatalog.h:96: */ On 2011/07/15 12:36:01, reed1 wrote: > In general, skia takes a const reference, rather than a const * > > reasoning: it is easier to tell at the call site if the parameter is const. If > its a ptr, it usually isn't. If its a ref (or by value), it is. Done. http://codereview.appspot.com/4633050/diff/40001/include/pdf/SkPDFFont.h File include/pdf/SkPDFFont.h (right): http://codereview.appspot.com/4633050/diff/40001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:36: void set(const uint16_t glyphID); On 2011/07/15 12:36:01, reed1 wrote: > Don't use const for integral parameters Done. http://codereview.appspot.com/4633050/diff/40001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:38: bool has(const uint16_t glyphID) const; On 2011/07/15 12:36:01, reed1 wrote: > Dont' use const for integral parameters. Done.
Sign in to reply to this message.
LGTM for my parts
Sign in to reply to this message.
Looking good. SkPDFFontImpl.h needs to be added to gyp/pdf.gyp. SkPDFDevice.h/cpp don't look rebased. http://codereview.appspot.com/4633050/diff/44001/include/pdf/SkPDFDevice.h File include/pdf/SkPDFDevice.h (right): http://codereview.appspot.com/4633050/diff/44001/include/pdf/SkPDFDevice.h#ne... include/pdf/SkPDFDevice.h:33: class SkPDFGlyphSet; You don't need to forward declare SkPDFGlyphSet. http://codereview.appspot.com/4633050/diff/44001/include/pdf/SkPDFDevice.h#ne... include/pdf/SkPDFDevice.h:152: const SkPDFGlyphSetMap& getFontGlyphUsage() const; Nit: Can you move the implementation for this function into this header? http://codereview.appspot.com/4633050/diff/44001/include/pdf/SkPDFFont.h File include/pdf/SkPDFFont.h (left): http://codereview.appspot.com/4633050/diff/44001/include/pdf/SkPDFFont.h#oldc... include/pdf/SkPDFFont.h:95: SkRefPtr<SkPDFDict> fDescriptor; Can we keep this class member variable instead of duplicating it in the derived classes? We can create a protected method like setFontDescriptor(). http://codereview.appspot.com/4633050/diff/44001/include/pdf/SkPDFFont.h File include/pdf/SkPDFFont.h (right): http://codereview.appspot.com/4633050/diff/44001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:132: SK_API virtual SkPDFFont* getSubsetFont(SkPDFGlyphSet* usage) = 0; You can make usage const here. The fan out is not small it's possible. http://codereview.appspot.com/4633050/diff/44001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:161: virtual SkAdvancedTypefaceMetrics* fontInfo(); These accessors don't need to be virtual. http://codereview.appspot.com/4633050/diff/44001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:171: virtual bool fillFontDescriptor(int16_t defaultWidth); These member functions don't need to be virtual. http://codereview.appspot.com/4633050/diff/44001/src/pdf/SkPDFCatalog.cpp File src/pdf/SkPDFCatalog.cpp (right): http://codereview.appspot.com/4633050/diff/44001/src/pdf/SkPDFCatalog.cpp#new... src/pdf/SkPDFCatalog.cpp:213: if (subsetFont) { Looks like subsetFont should never be null so this if could be removed. We are still calling setSubstitue if subsetFont is the same as entry->fFont. Perhaps SkPDFCatalog::setSubstitute can be updated to bail out if this happens? http://codereview.appspot.com/4633050/diff/44001/src/pdf/SkPDFDocument.cpp File src/pdf/SkPDFDocument.cpp (right): http://codereview.appspot.com/4633050/diff/44001/src/pdf/SkPDFDocument.cpp#ne... src/pdf/SkPDFDocument.cpp:21: #include "SkPDFFontImpl.h" You don't need this include.
Sign in to reply to this message.
Some comments, I didn't review the changes in SkPDFFont, etc, yet. http://codereview.appspot.com/4633050/diff/44001/include/pdf/SkPDFCatalog.h File include/pdf/SkPDFCatalog.h (right): http://codereview.appspot.com/4633050/diff/44001/include/pdf/SkPDFCatalog.h#n... include/pdf/SkPDFCatalog.h:97: void performFontSubsetting(const SkPDFGlyphSetMap& usage); This doesn't belong in SkPDFCatalog. http://codereview.appspot.com/4633050/diff/44001/include/pdf/SkPDFDevice.h File include/pdf/SkPDFDevice.h (right): http://codereview.appspot.com/4633050/diff/44001/include/pdf/SkPDFDevice.h#ne... include/pdf/SkPDFDevice.h:175: SkTScopedPtr<SkPDFGlyphSetMap> fFontGlyphUsage; SkPDFGlyphSetMap fFontGlyphUsage; http://codereview.appspot.com/4633050/diff/44001/include/pdf/SkPDFFont.h File include/pdf/SkPDFFont.h (right): http://codereview.appspot.com/4633050/diff/44001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:47: class FontGlyphSetPair { consider a struct. http://codereview.appspot.com/4633050/diff/44001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:132: SK_API virtual SkPDFFont* getSubsetFont(SkPDFGlyphSet* usage) = 0; getFontSubset ? http://codereview.appspot.com/4633050/diff/44001/src/pdf/SkPDFCatalog.cpp File src/pdf/SkPDFCatalog.cpp (right): http://codereview.appspot.com/4633050/diff/44001/src/pdf/SkPDFCatalog.cpp#new... src/pdf/SkPDFCatalog.cpp:213: if (subsetFont) { Or change getSubsetFont to return NULL if subsetting is not supported. http://codereview.appspot.com/4633050/diff/44001/src/pdf/SkPDFDevice.cpp File src/pdf/SkPDFDevice.cpp (right): http://codereview.appspot.com/4633050/diff/44001/src/pdf/SkPDFDevice.cpp#newc... src/pdf/SkPDFDevice.cpp:553: fFontGlyphUsage.reset(new SkPDFGlyphSetMap()); Remove http://codereview.appspot.com/4633050/diff/44001/src/pdf/SkPDFDevice.cpp#newc... src/pdf/SkPDFDevice.cpp:560: fShaderResources.unrefAll(); fFontGlyphUsage.reset(); http://codereview.appspot.com/4633050/diff/44001/src/pdf/SkPDFDevice.cpp#newc... src/pdf/SkPDFDevice.cpp:1115: void SkPDFDevice::createFormXObjectFromDevice( This needs to copy the glyph sets across cleanUp/init. http://codereview.appspot.com/4633050/diff/44001/src/pdf/SkPDFDocument.cpp File src/pdf/SkPDFDocument.cpp (right): http://codereview.appspot.com/4633050/diff/44001/src/pdf/SkPDFDocument.cpp#ne... src/pdf/SkPDFDocument.cpp:96: SkTScopedPtr<SkPDFGlyphSetMap> usage; SkPDFGlyphSetMap usage;
Sign in to reply to this message.
http://codereview.appspot.com/4633050/diff/44001/include/pdf/SkPDFCatalog.h File include/pdf/SkPDFCatalog.h (right): http://codereview.appspot.com/4633050/diff/44001/include/pdf/SkPDFCatalog.h#n... include/pdf/SkPDFCatalog.h:97: void performFontSubsetting(const SkPDFGlyphSetMap& usage); On 2011/07/18 23:11:12, Steve VanDeBogart wrote: > This doesn't belong in SkPDFCatalog. Where does it belong?
Sign in to reply to this message.
http://codereview.appspot.com/4633050/diff/44001/include/pdf/SkPDFDevice.h File include/pdf/SkPDFDevice.h (right): http://codereview.appspot.com/4633050/diff/44001/include/pdf/SkPDFDevice.h#ne... include/pdf/SkPDFDevice.h:33: class SkPDFGlyphSet; On 2011/07/18 19:11:47, Chris Guillory wrote: > You don't need to forward declare SkPDFGlyphSet. Done. http://codereview.appspot.com/4633050/diff/44001/include/pdf/SkPDFDevice.h#ne... include/pdf/SkPDFDevice.h:152: const SkPDFGlyphSetMap& getFontGlyphUsage() const; On 2011/07/18 19:11:47, Chris Guillory wrote: > Nit: Can you move the implementation for this function into this header? Done. http://codereview.appspot.com/4633050/diff/44001/include/pdf/SkPDFDevice.h#ne... include/pdf/SkPDFDevice.h:175: SkTScopedPtr<SkPDFGlyphSetMap> fFontGlyphUsage; On 2011/07/18 23:11:12, Steve VanDeBogart wrote: > SkPDFGlyphSetMap fFontGlyphUsage; Doing so requires SkPDFGlyphSetMap being fully defined, which implies including SkPDFFont.h, and will cause cross inclusion problems. http://codereview.appspot.com/4633050/diff/44001/include/pdf/SkPDFFont.h File include/pdf/SkPDFFont.h (left): http://codereview.appspot.com/4633050/diff/44001/include/pdf/SkPDFFont.h#oldc... include/pdf/SkPDFFont.h:95: SkRefPtr<SkPDFDict> fDescriptor; On 2011/07/18 19:11:47, Chris Guillory wrote: > Can we keep this class member variable instead of duplicating it in the derived > classes? We can create a protected method like setFontDescriptor(). It was moved to the derived class because the object is instantiated there and the ref count are manipulated. The parent class's fillFontDescriptor() does not involve any ref counting. http://codereview.appspot.com/4633050/diff/44001/include/pdf/SkPDFFont.h File include/pdf/SkPDFFont.h (right): http://codereview.appspot.com/4633050/diff/44001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:47: class FontGlyphSetPair { On 2011/07/18 23:11:12, Steve VanDeBogart wrote: > consider a struct. Done. http://codereview.appspot.com/4633050/diff/44001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:132: SK_API virtual SkPDFFont* getSubsetFont(SkPDFGlyphSet* usage) = 0; On 2011/07/18 19:11:47, Chris Guillory wrote: > You can make usage const here. The fan out is not small it's possible. Done. http://codereview.appspot.com/4633050/diff/44001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:132: SK_API virtual SkPDFFont* getSubsetFont(SkPDFGlyphSet* usage) = 0; On 2011/07/18 23:11:12, Steve VanDeBogart wrote: > getFontSubset ? I think getSubsetFont() is better since it's returning an SkPDFFont*. http://codereview.appspot.com/4633050/diff/44001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:161: virtual SkAdvancedTypefaceMetrics* fontInfo(); On 2011/07/18 19:11:47, Chris Guillory wrote: > These accessors don't need to be virtual. Done. http://codereview.appspot.com/4633050/diff/44001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:171: virtual bool fillFontDescriptor(int16_t defaultWidth); On 2011/07/18 19:11:47, Chris Guillory wrote: > These member functions don't need to be virtual. Done. http://codereview.appspot.com/4633050/diff/44001/src/pdf/SkPDFCatalog.cpp File src/pdf/SkPDFCatalog.cpp (right): http://codereview.appspot.com/4633050/diff/44001/src/pdf/SkPDFCatalog.cpp#new... src/pdf/SkPDFCatalog.cpp:213: if (subsetFont) { On 2011/07/18 19:11:47, Chris Guillory wrote: > Looks like subsetFont should never be null so this if could be removed. We are > still calling setSubstitue if subsetFont is the same as entry->fFont. Perhaps > SkPDFCatalog::setSubstitute can be updated to bail out if this happens? Done. Fonts not supporting subsetting will return NULL. http://codereview.appspot.com/4633050/diff/44001/src/pdf/SkPDFCatalog.cpp#new... src/pdf/SkPDFCatalog.cpp:213: if (subsetFont) { On 2011/07/18 23:11:12, Steve VanDeBogart wrote: > Or change getSubsetFont to return NULL if subsetting is not supported. Done. http://codereview.appspot.com/4633050/diff/44001/src/pdf/SkPDFDevice.cpp File src/pdf/SkPDFDevice.cpp (right): http://codereview.appspot.com/4633050/diff/44001/src/pdf/SkPDFDevice.cpp#newc... src/pdf/SkPDFDevice.cpp:553: fFontGlyphUsage.reset(new SkPDFGlyphSetMap()); On 2011/07/18 23:11:12, Steve VanDeBogart wrote: > Remove Can't remove because can't change type in SkPDFDevice.h. http://codereview.appspot.com/4633050/diff/44001/src/pdf/SkPDFDevice.cpp#newc... src/pdf/SkPDFDevice.cpp:560: fShaderResources.unrefAll(); On 2011/07/18 23:11:12, Steve VanDeBogart wrote: > fFontGlyphUsage.reset(); Done. http://codereview.appspot.com/4633050/diff/44001/src/pdf/SkPDFDevice.cpp#newc... src/pdf/SkPDFDevice.cpp:1115: void SkPDFDevice::createFormXObjectFromDevice( On 2011/07/18 23:11:12, Steve VanDeBogart wrote: > This needs to copy the glyph sets across cleanUp/init. Done. http://codereview.appspot.com/4633050/diff/44001/src/pdf/SkPDFDocument.cpp File src/pdf/SkPDFDocument.cpp (right): http://codereview.appspot.com/4633050/diff/44001/src/pdf/SkPDFDocument.cpp#ne... src/pdf/SkPDFDocument.cpp:21: #include "SkPDFFontImpl.h" On 2011/07/18 19:11:47, Chris Guillory wrote: > You don't need this include. Done. http://codereview.appspot.com/4633050/diff/44001/src/pdf/SkPDFDocument.cpp#ne... src/pdf/SkPDFDocument.cpp:96: SkTScopedPtr<SkPDFGlyphSetMap> usage; On 2011/07/18 23:11:12, Steve VanDeBogart wrote: > SkPDFGlyphSetMap usage; Done.
Sign in to reply to this message.
LGTM. I have some comments and I'll defer to Steve. I think he still wants to look over the font code. http://codereview.appspot.com/4633050/diff/40003/gyp/pdf.gyp File gyp/pdf.gyp (right): http://codereview.appspot.com/4633050/diff/40003/gyp/pdf.gyp#newcode20 gyp/pdf.gyp:20: '../include/pdf/SkPDFFont.h', Can you add SkPDFFontImpl.h? http://codereview.appspot.com/4633050/diff/40003/include/pdf/SkPDFDevice.h File include/pdf/SkPDFDevice.h (right): http://codereview.appspot.com/4633050/diff/40003/include/pdf/SkPDFDevice.h#ne... include/pdf/SkPDFDevice.h:163: return *fFontGlyphUsage.get(); Optional: return *(fFontGlyphUsage.get()); http://codereview.appspot.com/4633050/diff/40003/src/pdf/SkPDFDevice.cpp File src/pdf/SkPDFDevice.cpp (right): http://codereview.appspot.com/4633050/diff/40003/src/pdf/SkPDFDevice.cpp#newc... src/pdf/SkPDFDevice.cpp:27: #include "SkPDFFontImpl.h" I don't think you need this include. I think it's only needed in SkPDFFont.cpp. http://codereview.appspot.com/4633050/diff/40003/src/pdf/SkPDFDevice.cpp#newc... src/pdf/SkPDFDevice.cpp:1166: SkPDFGlyphSetMap glyphUsage; Can you save/restore the current instance pointed to by fFontGlyphUsage instead of creating a new instance here? http://codereview.appspot.com/4633050/diff/40003/src/pdf/SkPDFDevice.cpp#newc... src/pdf/SkPDFDevice.cpp:1170: mergeGlyphUsage(glyphUsage); I'm not seeing the need for adding the mergeGlyphUsage function. You can simply do fFontGlyphUsage->merge(glyphUsage) directly here. http://codereview.appspot.com/4633050/diff/40003/src/pdf/SkPDFFont.cpp File src/pdf/SkPDFFont.cpp (right): http://codereview.appspot.com/4633050/diff/40003/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:815: addToResources((SkPDFFont*)newCIDFont); Please remove the cast to SkPDFFont* on this line and on the next line.
Sign in to reply to this message.
http://codereview.appspot.com/4633050/diff/40003/gyp/pdf.gyp File gyp/pdf.gyp (right): http://codereview.appspot.com/4633050/diff/40003/gyp/pdf.gyp#newcode20 gyp/pdf.gyp:20: '../include/pdf/SkPDFFont.h', On 2011/07/19 01:23:15, Chris Guillory wrote: > Can you add SkPDFFontImpl.h? Done. http://codereview.appspot.com/4633050/diff/40003/include/pdf/SkPDFDevice.h File include/pdf/SkPDFDevice.h (right): http://codereview.appspot.com/4633050/diff/40003/include/pdf/SkPDFDevice.h#ne... include/pdf/SkPDFDevice.h:163: return *fFontGlyphUsage.get(); On 2011/07/19 01:23:15, Chris Guillory wrote: > Optional: return *(fFontGlyphUsage.get()); Done. http://codereview.appspot.com/4633050/diff/40003/src/pdf/SkPDFDevice.cpp File src/pdf/SkPDFDevice.cpp (right): http://codereview.appspot.com/4633050/diff/40003/src/pdf/SkPDFDevice.cpp#newc... src/pdf/SkPDFDevice.cpp:27: #include "SkPDFFontImpl.h" On 2011/07/19 01:23:15, Chris Guillory wrote: > I don't think you need this include. I think it's only needed in SkPDFFont.cpp. Done. http://codereview.appspot.com/4633050/diff/40003/src/pdf/SkPDFDevice.cpp#newc... src/pdf/SkPDFDevice.cpp:1166: SkPDFGlyphSetMap glyphUsage; On 2011/07/19 01:23:15, Chris Guillory wrote: > Can you save/restore the current instance pointed to by fFontGlyphUsage instead > of creating a new instance here? cleanUp() will destroy the instance. http://codereview.appspot.com/4633050/diff/40003/src/pdf/SkPDFDevice.cpp#newc... src/pdf/SkPDFDevice.cpp:1170: mergeGlyphUsage(glyphUsage); On 2011/07/19 01:23:15, Chris Guillory wrote: > I'm not seeing the need for adding the mergeGlyphUsage function. You can simply > do fFontGlyphUsage->merge(glyphUsage) directly here. Done. http://codereview.appspot.com/4633050/diff/40003/src/pdf/SkPDFFont.cpp File src/pdf/SkPDFFont.cpp (right): http://codereview.appspot.com/4633050/diff/40003/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:815: addToResources((SkPDFFont*)newCIDFont); On 2011/07/19 01:23:15, Chris Guillory wrote: > Please remove the cast to SkPDFFont* on this line and on the next line. Done.
Sign in to reply to this message.
http://codereview.appspot.com/4633050/diff/44001/include/pdf/SkPDFFont.h File include/pdf/SkPDFFont.h (right): http://codereview.appspot.com/4633050/diff/44001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:132: SK_API virtual SkPDFFont* getSubsetFont(SkPDFGlyphSet* usage) = 0; On 2011/07/19 00:29:41, arthurhsu wrote: > On 2011/07/18 23:11:12, Steve VanDeBogart wrote: > > getFontSubset ? > > I think getSubsetFont() is better since it's returning an SkPDFFont*. I don't follow... I think getFontSubset is better because you want a subset of the font. Maybe getSubsettedFont would make sense, but that's longer. http://codereview.appspot.com/4633050/diff/59001/include/pdf/SkPDFCatalog.h File include/pdf/SkPDFCatalog.h (right): http://codereview.appspot.com/4633050/diff/59001/include/pdf/SkPDFCatalog.h#n... include/pdf/SkPDFCatalog.h:97: void performFontSubsetting(const SkPDFGlyphSetMap& usage); This doesn't belong in SkPDFCatalog, probably SkPDFDevice. http://codereview.appspot.com/4633050/diff/59001/include/pdf/SkPDFFont.h File include/pdf/SkPDFFont.h (right): http://codereview.appspot.com/4633050/diff/59001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:36: void set(uint16_t glyphID); I think this method isn't used. http://codereview.appspot.com/4633050/diff/59001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:48: FontGlyphSetPair(); This constructor isn't used, remove it. http://codereview.appspot.com/4633050/diff/59001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:59: int getCount() const; count() Is this used? http://codereview.appspot.com/4633050/diff/59001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:60: SkPDFGlyphSet* get(SkPDFFont* font); getGlyphSetForFont ? http://codereview.appspot.com/4633050/diff/59001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:60: SkPDFGlyphSet* get(SkPDFFont* font); If this is only used internally, you can make it (as well as FontGlyphSetPair) private. http://codereview.appspot.com/4633050/diff/59001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:61: FontGlyphSetPair* at(int index) const; Is this method used? Maps don't have order, consider adding an iterator if you want to iterate through the data in the map. http://codereview.appspot.com/4633050/diff/59001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:69: SkTDArray<FontGlyphSetPair*> fSet; fMap ? http://codereview.appspot.com/4633050/diff/59001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:69: SkTDArray<FontGlyphSetPair*> fSet; With SkTDArray<FontGlyphSetPair*> fSet, you end up having to do two allocations for each entry, the FontGlyphSetPair and the SkPDFGlyphSet. Instead, consider just SkTDArray<FontGlyphSetPair> fSet. That means you can't use the destructor to do the cleanup, but you'll only have one allocation... http://codereview.appspot.com/4633050/diff/59001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:128: * @return this pointer if font does not support subsetting, a new I think you changed this to return NULL if the font doesn't support subsetting. http://codereview.appspot.com/4633050/diff/59001/src/pdf/SkPDFDevice.cpp File src/pdf/SkPDFDevice.cpp (right): http://codereview.appspot.com/4633050/diff/59001/src/pdf/SkPDFDevice.cpp#newc... src/pdf/SkPDFDevice.cpp:557: fFontGlyphUsage.reset(new SkPDFGlyphSetMap()); with rest() in cleanUp, move this to the constructor. http://codereview.appspot.com/4633050/diff/59001/src/pdf/SkPDFDevice.cpp#newc... src/pdf/SkPDFDevice.cpp:1165: SkPDFGlyphSetMap glyphUsage; This makes two copies, can we do better, maybe steal the pointer and then put it back? http://codereview.appspot.com/4633050/diff/59001/src/pdf/SkPDFFont.cpp File src/pdf/SkPDFFont.cpp (right): http://codereview.appspot.com/4633050/diff/59001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:441: SkPDFGlyphSet::~SkPDFGlyphSet() { remove http://codereview.appspot.com/4633050/diff/59001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:475: SkPDFGlyphSetMap::FontGlyphSetPair::~FontGlyphSetPair() { Remove http://codereview.appspot.com/4633050/diff/59001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:490: int index = fSet.count(); Remove http://codereview.appspot.com/4633050/diff/59001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:491: for (int i = 0; i < index; ++i) { index -> fSet.count() http://codereview.appspot.com/4633050/diff/59001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:497: fSet.append(1, &item); fSet.push(item) http://codereview.appspot.com/4633050/diff/59001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:498: return fSet[index]->fGlyphSet.get(); return item->fGlyphSet.get(); http://codereview.appspot.com/4633050/diff/59001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:570: return fFontInfo->fType; This method is for UMA, so we want it to note the type we treat the font as. For multimaster, the old code changed the type to other, we should continue to get that same result. Also we got not embeddable if fFontInfo is NULL. http://codereview.appspot.com/4633050/diff/59001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:570: return fFontInfo->fType; This won't work correctly after releaseFontInfo is called. http://codereview.appspot.com/4633050/diff/59001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:580: if (multiByteGlyphs()) { this-> http://codereview.appspot.com/4633050/diff/59001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:607: SkRefPtr<SkAdvancedTypefaceMetrics> fontInfoPtr; no Ptr http://codereview.appspot.com/4633050/diff/59001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:681: return new SkPDFType0Font(info, typeface, glyphID, fontDescriptor); fontDescriptor should be NULL for a Type0 font, assert that, and don't pass it down. http://codereview.appspot.com/4633050/diff/59001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:681: return new SkPDFType0Font(info, typeface, glyphID, fontDescriptor); Type 0 fonts don't care about glyphID, if I recall correctly. Don't pass it down if that's true. http://codereview.appspot.com/4633050/diff/59001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:698: void SkPDFFont::releaseFontInfo() { release implies a transfer of ownership, removeFontInfo ? That's not much better... http://codereview.appspot.com/4633050/diff/59001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:720: SkPDFDict* descriptor = fontDescriptor(); Would it make more sense to just pass in descriptor? http://codereview.appspot.com/4633050/diff/59001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:791: : SkPDFFont(info, typeface, glyphID, false), fDescriptor(fontDescriptor) { fDescriptor goes on the next line. http://codereview.appspot.com/4633050/diff/59001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:808: SkRefPtr<SkPDFArray> descendantFonts = new SkPDFArray(); Push these two lines down to line 816. http://codereview.appspot.com/4633050/diff/59001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:816: descendantFonts->append(new SkPDFObjRef((SkPDFFont*)newCIDFont))->unref(); Why are you casting this? http://codereview.appspot.com/4633050/diff/59001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:840: : SkPDFFont(info, typeface, 0, true), fSubset(subset), fDescriptor(NULL) { fSubset and fDescriptor go on new lines. http://codereview.appspot.com/4633050/diff/59001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:847: // Always create new desciptor because subset will change This comments seems incorrect: an instance of SkPDFCIDFont will only see one subset. http://codereview.appspot.com/4633050/diff/59001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:894: return fillFontDescriptor(defaultWidth); Seems like there might be a better name for this method? addCommonFontDescriptorEntries? http://codereview.appspot.com/4633050/diff/59001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:897: bool SkPDFCIDFont::populate() { this method is always called after creating SkPDFCIDFont, should it just be called from the constructor? Do we need fSubset beyond populate()? Let's not hold on to things that are only needed for a short period. http://codereview.appspot.com/4633050/diff/59001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:966: : SkPDFFont(info, typeface, glyphID, false), fDescriptor(fontDescriptor) { fDescriptor goes on a new line. http://codereview.appspot.com/4633050/diff/59001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:966: : SkPDFFont(info, typeface, glyphID, false), fDescriptor(fontDescriptor) { indent of intializers should be 8 in skia, as normal indent is 4. http://codereview.appspot.com/4633050/diff/59001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:972: bool SkPDFType1Font::addFontDescriptor(int16_t defaultWidth, SkPDFGlyphSet*) { We don't use SkPDFGlyphSet here, and just pass NULL, so remove it. http://codereview.appspot.com/4633050/diff/59001/src/pdf/SkPDFFontImpl.h File src/pdf/SkPDFFontImpl.h (right): http://codereview.appspot.com/4633050/diff/59001/src/pdf/SkPDFFontImpl.h#newc... src/pdf/SkPDFFontImpl.h:28: protected: Why protected? These seem to be private. http://codereview.appspot.com/4633050/diff/59001/src/pdf/SkPDFFontImpl.h#newc... src/pdf/SkPDFFontImpl.h:32: virtual SkPDFDict* fontDescriptor() { return fDescriptor.get(); } I'm not sure this method is needed, just pass fDecscriptor into fillFontDescriptor http://codereview.appspot.com/4633050/diff/59001/src/pdf/SkPDFFontImpl.h#newc... src/pdf/SkPDFFontImpl.h:44: virtual SkPDFFont* getSubsetFont(const SkPDFGlyphSet* usage) { Maybe just make SkPDFFont return NULL for getSubsetFont, then SkPDFType0Font can override it. http://codereview.appspot.com/4633050/diff/59001/src/pdf/SkPDFFontImpl.h#newc... src/pdf/SkPDFFontImpl.h:48: protected: Same. http://codereview.appspot.com/4633050/diff/59001/src/pdf/SkPDFFontImpl.h#newc... src/pdf/SkPDFFontImpl.h:85: class SkPDFType3Font : public SkPDFFont { It should be pretty easy to subset a type 3 font, but that can be done later.
Sign in to reply to this message.
http://codereview.appspot.com/4633050/diff/59001/include/pdf/SkPDFCatalog.h File include/pdf/SkPDFCatalog.h (right): http://codereview.appspot.com/4633050/diff/59001/include/pdf/SkPDFCatalog.h#n... include/pdf/SkPDFCatalog.h:97: void performFontSubsetting(const SkPDFGlyphSetMap& usage); On 2011/07/19 18:13:58, Steve VanDeBogart wrote: > This doesn't belong in SkPDFCatalog, probably SkPDFDevice. Done. Moved back to SkPDFDocument. http://codereview.appspot.com/4633050/diff/59001/include/pdf/SkPDFFont.h File include/pdf/SkPDFFont.h (right): http://codereview.appspot.com/4633050/diff/59001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:36: void set(uint16_t glyphID); On 2011/07/19 18:13:58, Steve VanDeBogart wrote: > I think this method isn't used. Done. http://codereview.appspot.com/4633050/diff/59001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:48: FontGlyphSetPair(); On 2011/07/19 18:13:58, Steve VanDeBogart wrote: > This constructor isn't used, remove it. Done. http://codereview.appspot.com/4633050/diff/59001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:59: int getCount() const; On 2011/07/19 18:13:58, Steve VanDeBogart wrote: > count() Is this used? Removed since changed pattern below. http://codereview.appspot.com/4633050/diff/59001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:60: SkPDFGlyphSet* get(SkPDFFont* font); On 2011/07/19 18:13:58, Steve VanDeBogart wrote: > getGlyphSetForFont ? Done. http://codereview.appspot.com/4633050/diff/59001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:60: SkPDFGlyphSet* get(SkPDFFont* font); On 2011/07/19 18:13:58, Steve VanDeBogart wrote: > If this is only used internally, you can make it (as well as FontGlyphSetPair) > private. Done. http://codereview.appspot.com/4633050/diff/59001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:61: FontGlyphSetPair* at(int index) const; On 2011/07/19 18:13:58, Steve VanDeBogart wrote: > Is this method used? Maps don't have order, consider adding an iterator if you > want to iterate through the data in the map. Skia does not really have iterators (no STL), so I use begin()/next() instead. http://codereview.appspot.com/4633050/diff/59001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:69: SkTDArray<FontGlyphSetPair*> fSet; On 2011/07/19 18:13:58, Steve VanDeBogart wrote: > With SkTDArray<FontGlyphSetPair*> fSet, you end up having to do two allocations > for each entry, the FontGlyphSetPair and the SkPDFGlyphSet. Instead, consider > just SkTDArray<FontGlyphSetPair> fSet. That means you can't use the destructor > to do the cleanup, but you'll only have one allocation... Changing it to object will result in memory copying in get() and dtor can't be used for cleanup. I wonder the benefit of doing that. http://codereview.appspot.com/4633050/diff/59001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:69: SkTDArray<FontGlyphSetPair*> fSet; On 2011/07/19 18:13:58, Steve VanDeBogart wrote: > fMap ? Done. http://codereview.appspot.com/4633050/diff/59001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:128: * @return this pointer if font does not support subsetting, a new On 2011/07/19 18:13:58, Steve VanDeBogart wrote: > I think you changed this to return NULL if the font doesn't support subsetting. Done. http://codereview.appspot.com/4633050/diff/59001/src/pdf/SkPDFDevice.cpp File src/pdf/SkPDFDevice.cpp (right): http://codereview.appspot.com/4633050/diff/59001/src/pdf/SkPDFDevice.cpp#newc... src/pdf/SkPDFDevice.cpp:557: fFontGlyphUsage.reset(new SkPDFGlyphSetMap()); On 2011/07/19 18:13:58, Steve VanDeBogart wrote: > with rest() in cleanUp, move this to the constructor. There are multiple ctors. I changed to check NULL and do not deallocate memory in cleanUp(). http://codereview.appspot.com/4633050/diff/59001/src/pdf/SkPDFDevice.cpp#newc... src/pdf/SkPDFDevice.cpp:1165: SkPDFGlyphSetMap glyphUsage; On 2011/07/19 18:13:58, Steve VanDeBogart wrote: > This makes two copies, can we do better, maybe steal the pointer and then put it > back? Can't do that. New implementation in cleanUp() clears the content of fFontGlyphUsage instead of deallocating it. http://codereview.appspot.com/4633050/diff/59001/src/pdf/SkPDFFont.cpp File src/pdf/SkPDFFont.cpp (right): http://codereview.appspot.com/4633050/diff/59001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:441: SkPDFGlyphSet::~SkPDFGlyphSet() { On 2011/07/19 18:13:58, Steve VanDeBogart wrote: > remove Done. http://codereview.appspot.com/4633050/diff/59001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:475: SkPDFGlyphSetMap::FontGlyphSetPair::~FontGlyphSetPair() { On 2011/07/19 18:13:58, Steve VanDeBogart wrote: > Remove Done. http://codereview.appspot.com/4633050/diff/59001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:490: int index = fSet.count(); On 2011/07/19 18:13:58, Steve VanDeBogart wrote: > Remove Style changed to begin()/next(). http://codereview.appspot.com/4633050/diff/59001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:491: for (int i = 0; i < index; ++i) { On 2011/07/19 18:13:58, Steve VanDeBogart wrote: > index -> fSet.count() Style changed to begin()/next(). http://codereview.appspot.com/4633050/diff/59001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:497: fSet.append(1, &item); On 2011/07/19 18:13:58, Steve VanDeBogart wrote: > fSet.push(item) Done. http://codereview.appspot.com/4633050/diff/59001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:498: return fSet[index]->fGlyphSet.get(); On 2011/07/19 18:13:58, Steve VanDeBogart wrote: > return item->fGlyphSet.get(); Done. http://codereview.appspot.com/4633050/diff/59001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:570: return fFontInfo->fType; On 2011/07/19 18:13:58, Steve VanDeBogart wrote: > This method is for UMA, so we want it to note the type we treat the font as. > For multimaster, the old code changed the type to other, we should continue to > get that same result. Also we got not embeddable if fFontInfo is NULL. Done. http://codereview.appspot.com/4633050/diff/59001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:570: return fFontInfo->fType; On 2011/07/19 18:13:58, Steve VanDeBogart wrote: > This won't work correctly after releaseFontInfo is called. Done. http://codereview.appspot.com/4633050/diff/59001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:580: if (multiByteGlyphs()) { On 2011/07/19 18:13:58, Steve VanDeBogart wrote: > this-> Done. http://codereview.appspot.com/4633050/diff/59001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:607: SkRefPtr<SkAdvancedTypefaceMetrics> fontInfoPtr; On 2011/07/19 18:13:58, Steve VanDeBogart wrote: > no Ptr Compiler giving warnings because fontInfo has the same name as fontInfo(). http://codereview.appspot.com/4633050/diff/59001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:681: return new SkPDFType0Font(info, typeface, glyphID, fontDescriptor); On 2011/07/19 18:13:58, Steve VanDeBogart wrote: > fontDescriptor should be NULL for a Type0 font, assert that, and don't pass it > down. Done. http://codereview.appspot.com/4633050/diff/59001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:681: return new SkPDFType0Font(info, typeface, glyphID, fontDescriptor); On 2011/07/19 18:13:58, Steve VanDeBogart wrote: > Type 0 fonts don't care about glyphID, if I recall correctly. Don't pass it > down if that's true. Done. http://codereview.appspot.com/4633050/diff/59001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:698: void SkPDFFont::releaseFontInfo() { On 2011/07/19 18:13:58, Steve VanDeBogart wrote: > release implies a transfer of ownership, removeFontInfo ? That's not much > better... freeFontInfo() it is. http://codereview.appspot.com/4633050/diff/59001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:720: SkPDFDict* descriptor = fontDescriptor(); On 2011/07/19 18:13:58, Steve VanDeBogart wrote: > Would it make more sense to just pass in descriptor? Done. http://codereview.appspot.com/4633050/diff/59001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:791: : SkPDFFont(info, typeface, glyphID, false), fDescriptor(fontDescriptor) { On 2011/07/19 18:13:58, Steve VanDeBogart wrote: > fDescriptor goes on the next line. Done. http://codereview.appspot.com/4633050/diff/59001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:808: SkRefPtr<SkPDFArray> descendantFonts = new SkPDFArray(); On 2011/07/19 18:13:58, Steve VanDeBogart wrote: > Push these two lines down to line 816. Done. http://codereview.appspot.com/4633050/diff/59001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:816: descendantFonts->append(new SkPDFObjRef((SkPDFFont*)newCIDFont))->unref(); On 2011/07/19 18:13:58, Steve VanDeBogart wrote: > Why are you casting this? Done. http://codereview.appspot.com/4633050/diff/59001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:840: : SkPDFFont(info, typeface, 0, true), fSubset(subset), fDescriptor(NULL) { On 2011/07/19 18:13:58, Steve VanDeBogart wrote: > fSubset and fDescriptor go on new lines. Done. http://codereview.appspot.com/4633050/diff/59001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:847: // Always create new desciptor because subset will change On 2011/07/19 18:13:58, Steve VanDeBogart wrote: > This comments seems incorrect: an instance of SkPDFCIDFont will only see one > subset. Done. http://codereview.appspot.com/4633050/diff/59001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:894: return fillFontDescriptor(defaultWidth); On 2011/07/19 18:13:58, Steve VanDeBogart wrote: > Seems like there might be a better name for this method? > addCommonFontDescriptorEntries? Done. http://codereview.appspot.com/4633050/diff/59001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:897: bool SkPDFCIDFont::populate() { On 2011/07/19 18:13:58, Steve VanDeBogart wrote: > this method is always called after creating SkPDFCIDFont, should it just be > called from the constructor? > > Do we need fSubset beyond populate()? Let's not hold on to things that are only > needed for a short period. Done. http://codereview.appspot.com/4633050/diff/59001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:966: : SkPDFFont(info, typeface, glyphID, false), fDescriptor(fontDescriptor) { On 2011/07/19 18:13:58, Steve VanDeBogart wrote: > fDescriptor goes on a new line. Done. http://codereview.appspot.com/4633050/diff/59001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:966: : SkPDFFont(info, typeface, glyphID, false), fDescriptor(fontDescriptor) { On 2011/07/19 18:13:58, Steve VanDeBogart wrote: > indent of intializers should be 8 in skia, as normal indent is 4. Done. http://codereview.appspot.com/4633050/diff/59001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:972: bool SkPDFType1Font::addFontDescriptor(int16_t defaultWidth, SkPDFGlyphSet*) { On 2011/07/19 18:13:58, Steve VanDeBogart wrote: > We don't use SkPDFGlyphSet here, and just pass NULL, so remove it. Done. http://codereview.appspot.com/4633050/diff/59001/src/pdf/SkPDFFontImpl.h File src/pdf/SkPDFFontImpl.h (right): http://codereview.appspot.com/4633050/diff/59001/src/pdf/SkPDFFontImpl.h#newc... src/pdf/SkPDFFontImpl.h:28: protected: On 2011/07/19 18:13:58, Steve VanDeBogart wrote: > Why protected? These seem to be private. Done. http://codereview.appspot.com/4633050/diff/59001/src/pdf/SkPDFFontImpl.h#newc... src/pdf/SkPDFFontImpl.h:32: virtual SkPDFDict* fontDescriptor() { return fDescriptor.get(); } On 2011/07/19 18:13:58, Steve VanDeBogart wrote: > I'm not sure this method is needed, just pass fDecscriptor into > fillFontDescriptor fillFontDescriptor() is changed to pass the parameter. However, this function is also used in getFontResource(). http://codereview.appspot.com/4633050/diff/59001/src/pdf/SkPDFFontImpl.h#newc... src/pdf/SkPDFFontImpl.h:44: virtual SkPDFFont* getSubsetFont(const SkPDFGlyphSet* usage) { On 2011/07/19 18:13:58, Steve VanDeBogart wrote: > Maybe just make SkPDFFont return NULL for getSubsetFont, then SkPDFType0Font can > override it. Done. http://codereview.appspot.com/4633050/diff/59001/src/pdf/SkPDFFontImpl.h#newc... src/pdf/SkPDFFontImpl.h:48: protected: On 2011/07/19 18:13:58, Steve VanDeBogart wrote: > Same. Done.
Sign in to reply to this message.
http://codereview.appspot.com/4633050/diff/59001/include/pdf/SkPDFFont.h File include/pdf/SkPDFFont.h (right): http://codereview.appspot.com/4633050/diff/59001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:69: SkTDArray<FontGlyphSetPair*> fSet; On 2011/07/20 00:53:38, arthurhsu wrote: > On 2011/07/19 18:13:58, Steve VanDeBogart wrote: > > With SkTDArray<FontGlyphSetPair*> fSet, you end up having to do two > allocations > > for each entry, the FontGlyphSetPair and the SkPDFGlyphSet. Instead, consider > > just SkTDArray<FontGlyphSetPair> fSet. That means you can't use the > destructor > > to do the cleanup, but you'll only have one allocation... > > Changing it to object will result in memory copying in get() and dtor can't be > used for cleanup. I wonder the benefit of doing that. Get could return a const ref and memory allocations can be surprisingly expensive. http://codereview.appspot.com/4633050/diff/59001/src/pdf/SkPDFDevice.cpp File src/pdf/SkPDFDevice.cpp (right): http://codereview.appspot.com/4633050/diff/59001/src/pdf/SkPDFDevice.cpp#newc... src/pdf/SkPDFDevice.cpp:1165: SkPDFGlyphSetMap glyphUsage; On 2011/07/20 00:53:38, arthurhsu wrote: > On 2011/07/19 18:13:58, Steve VanDeBogart wrote: > > This makes two copies, can we do better, maybe steal the pointer and then put > it > > back? > > Can't do that. New implementation in cleanUp() clears the content of > fFontGlyphUsage instead of deallocating it. That's a shame. Can you figure out a clean way to not have to copy it in this case? http://codereview.appspot.com/4633050/diff/59001/src/pdf/SkPDFFont.cpp File src/pdf/SkPDFFont.cpp (right): http://codereview.appspot.com/4633050/diff/59001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:607: SkRefPtr<SkAdvancedTypefaceMetrics> fontInfoPtr; On 2011/07/20 00:53:38, arthurhsu wrote: > On 2011/07/19 18:13:58, Steve VanDeBogart wrote: > > no Ptr > > Compiler giving warnings because fontInfo has the same name as fontInfo(). Ok, fontMetrics ? http://codereview.appspot.com/4633050/diff/59001/src/pdf/SkPDFFontImpl.h File src/pdf/SkPDFFontImpl.h (right): http://codereview.appspot.com/4633050/diff/59001/src/pdf/SkPDFFontImpl.h#newc... src/pdf/SkPDFFontImpl.h:28: protected: On 2011/07/20 00:53:38, arthurhsu wrote: > On 2011/07/19 18:13:58, Steve VanDeBogart wrote: > > Why protected? These seem to be private. > > Done. I meant that more generally. I'm not sure any of these classes need protected sections at all. http://codereview.appspot.com/4633050/diff/69001/include/pdf/SkPDFFont.h File include/pdf/SkPDFFont.h (right): http://codereview.appspot.com/4633050/diff/69001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:161: static SkTDArray<FontRec>& canonicalFonts(); These two methods can be private, and therefore FontRec can be private. http://codereview.appspot.com/4633050/diff/69001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:162: static SkMutex& canonicalFontsMutex(); When you rebase, these will be capitalized. http://codereview.appspot.com/4633050/diff/69001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:163: static bool Find(uint32_t fontID, uint16_t glyphID, int* index); Skia style seems to be to put static methods at the end of the section (public, private, etc). http://codereview.appspot.com/4633050/diff/69001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:167: void freeFontInfo(); I think we won't ever actually free the font info... Because we can generate subsets at arbitrary points, the SkPDFType0Font never frees its fontInfo. If you agree then, there's no reason to have this method. http://codereview.appspot.com/4633050/diff/69001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:171: void addToResources(SkPDFObject* object); nit: addResource http://codereview.appspot.com/4633050/diff/69001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:175: virtual SkPDFDict* fontDescriptor() = 0; On second look, I don't see why this needs to be virtual. We can host the descriptor in SkPDFFont, and add a setter. http://codereview.appspot.com/4633050/diff/69001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:176: bool addCommonFontDescriptorEntries(int16_t defaultWidth, With the above change, this doesn't need the descriptor argument. http://codereview.appspot.com/4633050/diff/69001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:176: bool addCommonFontDescriptorEntries(int16_t defaultWidth, Add a comment and pull above the accessors. http://codereview.appspot.com/4633050/diff/69001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:178: void adjustGlyphRangeForSingleByteEncoding(int16_t glyphID); You dropped the comment for this, add it back and pull up above the acessors. http://codereview.appspot.com/4633050/diff/69001/src/pdf/SkPDFCatalog.cpp File src/pdf/SkPDFCatalog.cpp (right): http://codereview.appspot.com/4633050/diff/69001/src/pdf/SkPDFCatalog.cpp#new... src/pdf/SkPDFCatalog.cpp:18: #include "SkPDFFont.h" This isn't needed any more http://codereview.appspot.com/4633050/diff/69001/src/pdf/SkPDFDocument.cpp File src/pdf/SkPDFDocument.cpp (right): http://codereview.appspot.com/4633050/diff/69001/src/pdf/SkPDFDocument.cpp#ne... src/pdf/SkPDFDocument.cpp:94: // Build font subsetting info before proceeding. You could pull this into a helper method. http://codereview.appspot.com/4633050/diff/69001/src/pdf/SkPDFFont.cpp File src/pdf/SkPDFFont.cpp (right): http://codereview.appspot.com/4633050/diff/69001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:660: if (info->fType == NULL) { info == NULL http://codereview.appspot.com/4633050/diff/69001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:664: fFontType = info->fMultiMaster ? SkAdvancedTypefaceMetrics::kOther_Font : Make this an else if + else http://codereview.appspot.com/4633050/diff/69001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:791: : SkPDFFont(info, typeface, 0, false) { Maybe we should add SK_DEBUGCODE to getOutputSize and emitObject to ensure that populate(subset) has been called? http://codereview.appspot.com/4633050/diff/69001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:1057: if (fontInfo()->fLastGlyphID <= 255) This (and the symmetrical case in type3) seems like the one place where we can actually drop the font info... but it's small anyway, so it doesn't matter much.
Sign in to reply to this message.
http://codereview.appspot.com/4633050/diff/59001/include/pdf/SkPDFFont.h File include/pdf/SkPDFFont.h (right): http://codereview.appspot.com/4633050/diff/59001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:69: SkTDArray<FontGlyphSetPair*> fSet; On 2011/07/21 19:07:02, Steve VanDeBogart wrote: > On 2011/07/20 00:53:38, arthurhsu wrote: > > On 2011/07/19 18:13:58, Steve VanDeBogart wrote: > > > With SkTDArray<FontGlyphSetPair*> fSet, you end up having to do two > > allocations > > > for each entry, the FontGlyphSetPair and the SkPDFGlyphSet. Instead, > consider > > > just SkTDArray<FontGlyphSetPair> fSet. That means you can't use the > > destructor > > > to do the cleanup, but you'll only have one allocation... > > > > Changing it to object will result in memory copying in get() and dtor can't be > > used for cleanup. I wonder the benefit of doing that. > > Get could return a const ref and memory allocations can be surprisingly > expensive. Done. http://codereview.appspot.com/4633050/diff/59001/src/pdf/SkPDFDevice.cpp File src/pdf/SkPDFDevice.cpp (right): http://codereview.appspot.com/4633050/diff/59001/src/pdf/SkPDFDevice.cpp#newc... src/pdf/SkPDFDevice.cpp:1165: SkPDFGlyphSetMap glyphUsage; On 2011/07/21 19:07:02, Steve VanDeBogart wrote: > On 2011/07/20 00:53:38, arthurhsu wrote: > > On 2011/07/19 18:13:58, Steve VanDeBogart wrote: > > > This makes two copies, can we do better, maybe steal the pointer and then > put > > it > > > back? > > > > Can't do that. New implementation in cleanUp() clears the content of > > fFontGlyphUsage instead of deallocating it. > > That's a shame. Can you figure out a clean way to not have to copy it in this > case? Done. http://codereview.appspot.com/4633050/diff/59001/src/pdf/SkPDFFont.cpp File src/pdf/SkPDFFont.cpp (right): http://codereview.appspot.com/4633050/diff/59001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:607: SkRefPtr<SkAdvancedTypefaceMetrics> fontInfoPtr; On 2011/07/21 19:07:02, Steve VanDeBogart wrote: > On 2011/07/20 00:53:38, arthurhsu wrote: > > On 2011/07/19 18:13:58, Steve VanDeBogart wrote: > > > no Ptr > > > > Compiler giving warnings because fontInfo has the same name as fontInfo(). > > Ok, fontMetrics ? Done. http://codereview.appspot.com/4633050/diff/59001/src/pdf/SkPDFFontImpl.h File src/pdf/SkPDFFontImpl.h (right): http://codereview.appspot.com/4633050/diff/59001/src/pdf/SkPDFFontImpl.h#newc... src/pdf/SkPDFFontImpl.h:28: protected: On 2011/07/21 19:07:02, Steve VanDeBogart wrote: > On 2011/07/20 00:53:38, arthurhsu wrote: > > On 2011/07/19 18:13:58, Steve VanDeBogart wrote: > > > Why protected? These seem to be private. > > > > Done. > > I meant that more generally. I'm not sure any of these classes need protected > sections at all. Done. http://codereview.appspot.com/4633050/diff/69001/include/pdf/SkPDFFont.h File include/pdf/SkPDFFont.h (right): http://codereview.appspot.com/4633050/diff/69001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:161: static SkTDArray<FontRec>& canonicalFonts(); On 2011/07/21 19:07:02, Steve VanDeBogart wrote: > These two methods can be private, and therefore FontRec can be private. Done. http://codereview.appspot.com/4633050/diff/69001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:162: static SkMutex& canonicalFontsMutex(); On 2011/07/21 19:07:02, Steve VanDeBogart wrote: > When you rebase, these will be capitalized. Done. http://codereview.appspot.com/4633050/diff/69001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:163: static bool Find(uint32_t fontID, uint16_t glyphID, int* index); On 2011/07/21 19:07:02, Steve VanDeBogart wrote: > Skia style seems to be to put static methods at the end of the section (public, > private, etc). Done. http://codereview.appspot.com/4633050/diff/69001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:167: void freeFontInfo(); On 2011/07/21 19:07:02, Steve VanDeBogart wrote: > I think we won't ever actually free the font info... Because we can generate > subsets at arbitrary points, the SkPDFType0Font never frees its fontInfo. If > you agree then, there's no reason to have this method. Done. http://codereview.appspot.com/4633050/diff/69001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:171: void addToResources(SkPDFObject* object); On 2011/07/21 19:07:02, Steve VanDeBogart wrote: > nit: addResource Done. http://codereview.appspot.com/4633050/diff/69001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:175: virtual SkPDFDict* fontDescriptor() = 0; On 2011/07/21 19:07:02, Steve VanDeBogart wrote: > On second look, I don't see why this needs to be virtual. We can host the > descriptor in SkPDFFont, and add a setter. Done. http://codereview.appspot.com/4633050/diff/69001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:176: bool addCommonFontDescriptorEntries(int16_t defaultWidth, On 2011/07/21 19:07:02, Steve VanDeBogart wrote: > With the above change, this doesn't need the descriptor argument. Done. http://codereview.appspot.com/4633050/diff/69001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:176: bool addCommonFontDescriptorEntries(int16_t defaultWidth, On 2011/07/21 19:07:02, Steve VanDeBogart wrote: > Add a comment and pull above the accessors. Done. http://codereview.appspot.com/4633050/diff/69001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:178: void adjustGlyphRangeForSingleByteEncoding(int16_t glyphID); On 2011/07/21 19:07:02, Steve VanDeBogart wrote: > You dropped the comment for this, add it back and pull up above the acessors. Done. http://codereview.appspot.com/4633050/diff/69001/src/pdf/SkPDFCatalog.cpp File src/pdf/SkPDFCatalog.cpp (right): http://codereview.appspot.com/4633050/diff/69001/src/pdf/SkPDFCatalog.cpp#new... src/pdf/SkPDFCatalog.cpp:18: #include "SkPDFFont.h" On 2011/07/21 19:07:02, Steve VanDeBogart wrote: > This isn't needed any more Done. http://codereview.appspot.com/4633050/diff/69001/src/pdf/SkPDFDocument.cpp File src/pdf/SkPDFDocument.cpp (right): http://codereview.appspot.com/4633050/diff/69001/src/pdf/SkPDFDocument.cpp#ne... src/pdf/SkPDFDocument.cpp:94: // Build font subsetting info before proceeding. On 2011/07/21 19:07:02, Steve VanDeBogart wrote: > You could pull this into a helper method. Done. http://codereview.appspot.com/4633050/diff/69001/src/pdf/SkPDFFont.cpp File src/pdf/SkPDFFont.cpp (right): http://codereview.appspot.com/4633050/diff/69001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:660: if (info->fType == NULL) { On 2011/07/21 19:07:02, Steve VanDeBogart wrote: > info == NULL Done. http://codereview.appspot.com/4633050/diff/69001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:664: fFontType = info->fMultiMaster ? SkAdvancedTypefaceMetrics::kOther_Font : On 2011/07/21 19:07:02, Steve VanDeBogart wrote: > Make this an else if + else Done. http://codereview.appspot.com/4633050/diff/69001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:791: : SkPDFFont(info, typeface, 0, false) { On 2011/07/21 19:07:02, Steve VanDeBogart wrote: > Maybe we should add SK_DEBUGCODE to getOutputSize and emitObject to ensure that > populate(subset) has been called? Done. http://codereview.appspot.com/4633050/diff/69001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:1057: if (fontInfo()->fLastGlyphID <= 255) On 2011/07/21 19:07:02, Steve VanDeBogart wrote: > This (and the symmetrical case in type3) seems like the one place where we can > actually drop the font info... but it's small anyway, so it doesn't matter much. Done.
Sign in to reply to this message.
Here are a couple comments, I'll look through SkPDFFont.cpp and send any more. http://codereview.appspot.com/4633050/diff/73001/include/pdf/SkPDFFont.h File include/pdf/SkPDFFont.h (right): http://codereview.appspot.com/4633050/diff/73001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:144: // Accessors for subclass. Brief documentation (one line) for each non obvious method in the protected section would be great. http://codereview.appspot.com/4633050/diff/73001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:150: SkPDFDict* getDescriptor(); nit: Descriptor is a bit too generic... how about FontDescriptorDict ? http://codereview.appspot.com/4633050/diff/73001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:153: bool addCommonFontDescriptorEntries(int16_t defaultWidth); apply name here too http://codereview.appspot.com/4633050/diff/73001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:191: // The font info can be released early, therefore we need to cache the This comment is out of date. http://codereview.appspot.com/4633050/diff/73001/src/pdf/SkPDFDevice.cpp File src/pdf/SkPDFDevice.cpp (right): http://codereview.appspot.com/4633050/diff/73001/src/pdf/SkPDFDevice.cpp#newc... src/pdf/SkPDFDevice.cpp:1169: cleanUp(false); // Reset this device to have no content. This needs an explicit comment.... // We always draw the form xobjects that we create back into the device, so we simply preserve the font usage instead of pulling it out and merging it back in later. http://codereview.appspot.com/4633050/diff/73001/src/pdf/SkPDFDocument.cpp File src/pdf/SkPDFDocument.cpp (right): http://codereview.appspot.com/4633050/diff/73001/src/pdf/SkPDFDocument.cpp#ne... src/pdf/SkPDFDocument.cpp:53: catalog->setSubstitute(entry->fFont, subsetFont); I've already forgotten the ownership details of setSubstitute and it's not in the header... It looks like the catalog doesn't take ownership (please add that to the header if you agree). But that means we'll leak the substitute object. We need to store a copy somewhere.... in the original font doesn't seem right since there can be multiple substitutes. I think we have to add a list of substitute objects to document, that we delete in the destructor.
Sign in to reply to this message.
http://codereview.appspot.com/4633050/diff/73001/src/pdf/SkPDFFont.cpp File src/pdf/SkPDFFont.cpp (right): http://codereview.appspot.com/4633050/diff/73001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:665: SkPDFDict* fontDescriptor) { relatedFontDescriptor http://codereview.appspot.com/4633050/diff/73001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:725: fDescriptor->insert("FontName", new SkPDFName( Looks like you still need to rebase... http://codereview.appspot.com/4633050/diff/73001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:804: return SkPDFDict::emitObject(stream, catalog, indirect); Skia convention is to typdef INHERITED to the parent and use that... http://codereview.appspot.com/4633050/diff/73001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:1178: insert("FirstChar", new SkPDFInt(firstGlyphID()))->unref(); Oh, you did rebase, but you need to search through all the insert('s and append('s and convert them to the new helpers if applicable... insertInt, insertName, etc. http://codereview.appspot.com/4633050/diff/73001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:1187: void SkPDFType3Font::populateToUnicodeTable() { This is the same as the one in SkPDFType0Font... move it to SkPDFFont. http://codereview.appspot.com/4633050/diff/73001/src/pdf/SkPDFFontImpl.h File src/pdf/SkPDFFontImpl.h (right): http://codereview.appspot.com/4633050/diff/73001/src/pdf/SkPDFFontImpl.h#newc... src/pdf/SkPDFFontImpl.h:68: uint16_t glyphID, SkPDFDict* fontDescriptor); Let's call this relatedFontDescriptor or similar http://codereview.appspot.com/4633050/diff/73001/src/pdf/SkPDFFontImpl.h#newc... src/pdf/SkPDFFontImpl.h:85: uint16_t glyphID, SkPDFDict* fontDescriptor); And here.
Sign in to reply to this message.
http://codereview.appspot.com/4633050/diff/73001/include/pdf/SkPDFFont.h File include/pdf/SkPDFFont.h (right): http://codereview.appspot.com/4633050/diff/73001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:144: // Accessors for subclass. On 2011/07/23 01:10:35, Steve VanDeBogart wrote: > Brief documentation (one line) for each non obvious method in the protected > section would be great. Done. http://codereview.appspot.com/4633050/diff/73001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:150: SkPDFDict* getDescriptor(); On 2011/07/23 01:10:35, Steve VanDeBogart wrote: > nit: Descriptor is a bit too generic... how about FontDescriptorDict ? Changed to FontDescriptor since this is the name used within PDF stream and we use similar schemes throughout the code. http://codereview.appspot.com/4633050/diff/73001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:153: bool addCommonFontDescriptorEntries(int16_t defaultWidth); On 2011/07/23 01:10:35, Steve VanDeBogart wrote: > apply name here too Did not change this one since it's already consistent with the new name. http://codereview.appspot.com/4633050/diff/73001/include/pdf/SkPDFFont.h#newc... include/pdf/SkPDFFont.h:191: // The font info can be released early, therefore we need to cache the On 2011/07/23 01:10:35, Steve VanDeBogart wrote: > This comment is out of date. Done. http://codereview.appspot.com/4633050/diff/73001/src/pdf/SkPDFDevice.cpp File src/pdf/SkPDFDevice.cpp (right): http://codereview.appspot.com/4633050/diff/73001/src/pdf/SkPDFDevice.cpp#newc... src/pdf/SkPDFDevice.cpp:1169: cleanUp(false); // Reset this device to have no content. On 2011/07/23 01:10:35, Steve VanDeBogart wrote: > This needs an explicit comment.... > > // We always draw the form xobjects that we create back into the device, so we > simply preserve the font usage instead of pulling it out and merging it back in > later. Done. http://codereview.appspot.com/4633050/diff/73001/src/pdf/SkPDFDocument.cpp File src/pdf/SkPDFDocument.cpp (right): http://codereview.appspot.com/4633050/diff/73001/src/pdf/SkPDFDocument.cpp#ne... src/pdf/SkPDFDocument.cpp:53: catalog->setSubstitute(entry->fFont, subsetFont); On 2011/07/23 01:10:35, Steve VanDeBogart wrote: > I've already forgotten the ownership details of setSubstitute and it's not in > the header... It looks like the catalog doesn't take ownership (please add that > to the header if you agree). But that means we'll leak the substitute object. > We need to store a copy somewhere.... in the original font doesn't seem right > since there can be multiple substitutes. I think we have to add a list of > substitute objects to document, that we delete in the destructor. Done. http://codereview.appspot.com/4633050/diff/73001/src/pdf/SkPDFFont.cpp File src/pdf/SkPDFFont.cpp (right): http://codereview.appspot.com/4633050/diff/73001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:665: SkPDFDict* fontDescriptor) { On 2011/07/23 01:31:20, Steve VanDeBogart wrote: > relatedFontDescriptor Done. http://codereview.appspot.com/4633050/diff/73001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:725: fDescriptor->insert("FontName", new SkPDFName( On 2011/07/23 01:31:20, Steve VanDeBogart wrote: > Looks like you still need to rebase... Done. http://codereview.appspot.com/4633050/diff/73001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:804: return SkPDFDict::emitObject(stream, catalog, indirect); On 2011/07/23 01:31:20, Steve VanDeBogart wrote: > Skia convention is to typdef INHERITED to the parent and use that... Done. http://codereview.appspot.com/4633050/diff/73001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:1178: insert("FirstChar", new SkPDFInt(firstGlyphID()))->unref(); On 2011/07/23 01:31:20, Steve VanDeBogart wrote: > Oh, you did rebase, but you need to search through all the insert('s and > append('s and convert them to the new helpers if applicable... insertInt, > insertName, etc. Done. http://codereview.appspot.com/4633050/diff/73001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:1187: void SkPDFType3Font::populateToUnicodeTable() { On 2011/07/23 01:31:20, Steve VanDeBogart wrote: > This is the same as the one in SkPDFType0Font... move it to SkPDFFont. Done. http://codereview.appspot.com/4633050/diff/73001/src/pdf/SkPDFFontImpl.h File src/pdf/SkPDFFontImpl.h (right): http://codereview.appspot.com/4633050/diff/73001/src/pdf/SkPDFFontImpl.h#newc... src/pdf/SkPDFFontImpl.h:68: uint16_t glyphID, SkPDFDict* fontDescriptor); On 2011/07/23 01:31:20, Steve VanDeBogart wrote: > Let's call this relatedFontDescriptor or similar Done. http://codereview.appspot.com/4633050/diff/73001/src/pdf/SkPDFFontImpl.h#newc... src/pdf/SkPDFFontImpl.h:85: uint16_t glyphID, SkPDFDict* fontDescriptor); On 2011/07/23 01:31:20, Steve VanDeBogart wrote: > And here. Done.
Sign in to reply to this message.
Just a couple minor nits left, so I addressed those and committed this change as r1943. http://codereview.appspot.com/4633050/diff/80001/src/pdf/SkPDFDocument.cpp File src/pdf/SkPDFDocument.cpp (right): http://codereview.appspot.com/4633050/diff/80001/src/pdf/SkPDFDocument.cpp#ne... src/pdf/SkPDFDocument.cpp:58: substitutes->append(); nit: just use substitutes->push(subsetFont); http://codereview.appspot.com/4633050/diff/80001/src/pdf/SkPDFFont.cpp File src/pdf/SkPDFFont.cpp (right): http://codereview.appspot.com/4633050/diff/80001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:584: int index; nit: relatedFontIndex http://codereview.appspot.com/4633050/diff/80001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:591: SkPDFDict* fontDescriptor = NULL; nit: relatedFontDescriptor
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
