Issue 4247043: PDF Type3 Support (Closed)
|
Created:
14 years, 3 months ago by Chris Guillory Modified:
14 years, 3 months ago CC:
skia-review_googlegroups.com Base URL:
http://skia.googlecode.com/svn/trunk/ Visibility:
Public. |
DescriptionPDF Type3 Support.
Committed: http://code.google.com/p/skia/source/detail?r=892
Committed: http://code.google.com/p/skia/source/detail?r=893
Patch Set 1 #Patch Set 2 : Windows is working with noticeable performance issues. #Patch Set 3 : Works on linux now. Ready for review. Investigating performance issues. #
Total comments: 24
Patch Set 4 : Updated with comments from Steve. #
Total comments: 47
Patch Set 5 : Using Google copyright for new files. #Patch Set 6 : Updated with more comments from Steve. #
Total comments: 23
Patch Set 7 : Updated with more comments from Steve. #
Total comments: 32
Patch Set 8 : Updated with more comments from Steve. #
Total comments: 11
Patch Set 9 : Rebased. Cleaned up fLastGlyphID usage. #Patch Set 10 : Rebased. #
Total comments: 4
MessagesTotal messages: 19
Sign in to reply to this message.
Sign in to reply to this message.
http://codereview.appspot.com/4247043/diff/4001/src/pdf/SkPDFFont.cpp File src/pdf/SkPDFFont.cpp (right): http://codereview.appspot.com/4247043/diff/4001/src/pdf/SkPDFFont.cpp#newcode426 src/pdf/SkPDFFont.cpp:426: populateType3Font(fFirstGlyphID, fLastGlyphID); Maybe this should just set font type to other or not embeddable and remove the else so there's only one call to populateType3Font ? http://codereview.appspot.com/4247043/diff/4001/src/pdf/SkPDFFont.cpp#newcode446 src/pdf/SkPDFFont.cpp:446: // if ((fontInfo->fType == You don't want to commit this as comments. http://codereview.appspot.com/4247043/diff/4001/src/pdf/SkPDFFont.cpp#newcode448 src/pdf/SkPDFFont.cpp:448: // populateType1Font(firstGlyphID, lastGlyphID)) { Making this two levels of if might make the error -> fallback to type 3 more explicit, but up to you. http://codereview.appspot.com/4247043/diff/4001/src/pdf/SkPDFFont.cpp#newcode454 src/pdf/SkPDFFont.cpp:454: case SkAdvancedTypefaceMetrics::kCFF_Font: I guess this should go above as well and fall into the Type3 constructor. http://codereview.appspot.com/4247043/diff/4001/src/pdf/SkPDFFont.cpp#newcode617 src/pdf/SkPDFFont.cpp:617: fMultiByteGlyphs = false; It looks large chunks of this code have been copied from other functions. Probably best to pull them out into functions where possible to reduce code duplication. http://codereview.appspot.com/4247043/diff/4001/src/pdf/SkPDFFont.cpp#newcode662 src/pdf/SkPDFFont.cpp:662: SkRefPtr<SkPDFArray> fontMatrix = new SkPDFArray; I plan to add the following code snippet into SkPDFShader. Maybe I should create SkPDFUtil and put it in there, then you could just do: SkMatrix fontMatrix; fontMatrix.setScale(1000, 1000); insert("FontMatrix", matrixToArray(fontMatrix))->unref() SkPDFArray* matrixToArray(const SkMatrix& matrix) { SkScalar values[6]; SkAssertResult(matrix.pdfTransform(values)); SkPDFArray* result = new SkPDFArray; result->reserve(6); for (size_t i = 0; i < SK_ARRAY_COUNT(values); i++) { result->append(new SkPDFScalar(values[i]))->unref(); } return result; } http://codereview.appspot.com/4247043/diff/4001/src/pdf/SkPDFFont.cpp#newcode678 src/pdf/SkPDFFont.cpp:678: paint.setTypeface(SkFontHost::FindTypefaceByUniqueID(fFontID)); Instead of adding this method, we should probably just plumb the SkTypeface in from SkPDFDevice instead of plumbing the fontID. http://codereview.appspot.com/4247043/diff/4001/src/pdf/SkPDFFont.cpp#newcode685 src/pdf/SkPDFFont.cpp:685: SkString characterName("uni"); I'm not sure we want to name the glyphs as uniXXX, that implies that XXX is the unicode value, maybe gidXXX? Also, you might find the following construct easier to read: SkString characterName; characterName.printf("gid%d", gID); http://codereview.appspot.com/4247043/diff/4001/src/pdf/SkPDFFont.cpp#newcode694 src/pdf/SkPDFFont.cpp:694: scaleFromFontUnits(-glyph.fTop - glyph.fHeight, emSize), Why negative glyph.fTop? Would it be better to account for this in the fontMatrix? At the least we should have a comment here. http://codereview.appspot.com/4247043/diff/4001/src/pdf/SkPDFFont.cpp#newcode698 src/pdf/SkPDFFont.cpp:698: header.appendf("%f 0 0 -%f 0 0 cm\n", Instead of putting this in each glyph, you can put it in the fontMatrix. Is it needed because of the -glyph.fTop above? http://codereview.appspot.com/4247043/diff/4001/src/pdf/SkPDFFont.cpp#newcode707 src/pdf/SkPDFFont.cpp:707: SkRefPtr<SkStream> content = pattern.content(); Ugg. Since you only need to draw a path, with no resources, no clip/transform, etc, plus you need to prepend a header, it seems like the best thing to do is to move the emitPath code (any anything needed to support it into an SkPDFUtils file. What do you think? http://codereview.appspot.com/4247043/diff/4001/src/pdf/SkPDFFont.cpp#newcode729 src/pdf/SkPDFFont.cpp:729: for (int gID = fFirstGlyphID; gID <= fLastGlyphID; gID++) { You can build this up at the same time as you generate the paths.
Sign in to reply to this message.
http://codereview.appspot.com/4247043/diff/4001/src/pdf/SkPDFFont.cpp File src/pdf/SkPDFFont.cpp (right): http://codereview.appspot.com/4247043/diff/4001/src/pdf/SkPDFFont.cpp#newcode426 src/pdf/SkPDFFont.cpp:426: populateType3Font(fFirstGlyphID, fLastGlyphID); On 2011/03/01 01:22:01, Steve VanDeBogart wrote: > Maybe this should just set font type to other or not embeddable and remove the > else so there's only one call to populateType3Font ? Done. http://codereview.appspot.com/4247043/diff/4001/src/pdf/SkPDFFont.cpp#newcode446 src/pdf/SkPDFFont.cpp:446: // if ((fontInfo->fType == On 2011/03/01 01:22:01, Steve VanDeBogart wrote: > You don't want to commit this as comments. Done. http://codereview.appspot.com/4247043/diff/4001/src/pdf/SkPDFFont.cpp#newcode448 src/pdf/SkPDFFont.cpp:448: // populateType1Font(firstGlyphID, lastGlyphID)) { On 2011/03/01 01:22:01, Steve VanDeBogart wrote: > Making this two levels of if might make the error -> fallback to type 3 more > explicit, but up to you. Done. http://codereview.appspot.com/4247043/diff/4001/src/pdf/SkPDFFont.cpp#newcode454 src/pdf/SkPDFFont.cpp:454: case SkAdvancedTypefaceMetrics::kCFF_Font: On 2011/03/01 01:22:01, Steve VanDeBogart wrote: > I guess this should go above as well and fall into the Type3 constructor. Done. http://codereview.appspot.com/4247043/diff/4001/src/pdf/SkPDFFont.cpp#newcode617 src/pdf/SkPDFFont.cpp:617: fMultiByteGlyphs = false; On 2011/03/01 01:22:01, Steve VanDeBogart wrote: > It looks large chunks of this code have been copied from other functions. > Probably best to pull them out into functions where possible to reduce code > duplication. Done. http://codereview.appspot.com/4247043/diff/4001/src/pdf/SkPDFFont.cpp#newcode662 src/pdf/SkPDFFont.cpp:662: SkRefPtr<SkPDFArray> fontMatrix = new SkPDFArray; On 2011/03/01 01:22:01, Steve VanDeBogart wrote: > I plan to add the following code snippet into SkPDFShader. Maybe I should > create SkPDFUtil and put it in there, then you could just do: > > SkMatrix fontMatrix; > fontMatrix.setScale(1000, 1000); > insert("FontMatrix", matrixToArray(fontMatrix))->unref() > > SkPDFArray* matrixToArray(const SkMatrix& matrix) { > SkScalar values[6]; > SkAssertResult(matrix.pdfTransform(values)); > > SkPDFArray* result = new SkPDFArray; > result->reserve(6); > for (size_t i = 0; i < SK_ARRAY_COUNT(values); i++) { > result->append(new SkPDFScalar(values[i]))->unref(); > } > return result; > } That could work. Done. http://codereview.appspot.com/4247043/diff/4001/src/pdf/SkPDFFont.cpp#newcode678 src/pdf/SkPDFFont.cpp:678: paint.setTypeface(SkFontHost::FindTypefaceByUniqueID(fFontID)); On 2011/03/01 01:22:01, Steve VanDeBogart wrote: > Instead of adding this method, we should probably just plumb the SkTypeface in > from SkPDFDevice instead of plumbing the fontID. Done. http://codereview.appspot.com/4247043/diff/4001/src/pdf/SkPDFFont.cpp#newcode685 src/pdf/SkPDFFont.cpp:685: SkString characterName("uni"); On 2011/03/01 01:22:01, Steve VanDeBogart wrote: > I'm not sure we want to name the glyphs as uniXXX, that implies that XXX is the > unicode value, maybe gidXXX? Also, you might find the following construct > easier to read: > > SkString characterName; > characterName.printf("gid%d", gID); Done. http://codereview.appspot.com/4247043/diff/4001/src/pdf/SkPDFFont.cpp#newcode694 src/pdf/SkPDFFont.cpp:694: scaleFromFontUnits(-glyph.fTop - glyph.fHeight, emSize), On 2011/03/01 01:22:01, Steve VanDeBogart wrote: > Why negative glyph.fTop? Would it be better to account for this in the > fontMatrix? At the least we should have a comment here. Using positive glyph.fTop per your comment below. http://codereview.appspot.com/4247043/diff/4001/src/pdf/SkPDFFont.cpp#newcode698 src/pdf/SkPDFFont.cpp:698: header.appendf("%f 0 0 -%f 0 0 cm\n", On 2011/03/01 01:22:01, Steve VanDeBogart wrote: > Instead of putting this in each glyph, you can put it in the fontMatrix. Is it > needed because of the -glyph.fTop above? Yes, this can go in the fontMatrix! Now I can use positive glyph.fTop. To make the cursor appear correctly I had to flip the bbox vertically. http://codereview.appspot.com/4247043/diff/4001/src/pdf/SkPDFFont.cpp#newcode707 src/pdf/SkPDFFont.cpp:707: SkRefPtr<SkStream> content = pattern.content(); On 2011/03/01 01:22:01, Steve VanDeBogart wrote: > Ugg. Since you only need to draw a path, with no resources, no clip/transform, > etc, plus you need to prepend a header, it seems like the best thing to do is to > move the emitPath code (any anything needed to support it into an SkPDFUtils > file. What do you think? Agree. Done. http://codereview.appspot.com/4247043/diff/4001/src/pdf/SkPDFFont.cpp#newcode729 src/pdf/SkPDFFont.cpp:729: for (int gID = fFirstGlyphID; gID <= fLastGlyphID; gID++) { On 2011/03/01 01:22:01, Steve VanDeBogart wrote: > You can build this up at the same time as you generate the paths. Done.
Sign in to reply to this message.
For new files, lets standardize on the Google one:
/*
Copyright 2010 Google Inc.
Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/
Sign in to reply to this message.
On 2011/03/01 18:38:51, reed1 wrote: > For new files, lets standardize on the Google one: > > > /* > Copyright 2010 Google Inc. > > Licensed under the Apache License, Version 2.0 (the "License"); > you may not use this file except in compliance with the License. > You may obtain a copy of the License at > > http://www.apache.org/licenses/LICENSE-2.0 > > Unless required by applicable law or agreed to in writing, software > distributed under the License is distributed on an "AS IS" BASIS, > WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. > See the License for the specific language governing permissions and > limitations under the License. > */ Done.
Sign in to reply to this message.
http://codereview.appspot.com/4247043/diff/8001/include/pdf/SkPDFDevice.h File include/pdf/SkPDFDevice.h (right): http://codereview.appspot.com/4247043/diff/8001/include/pdf/SkPDFDevice.h#new... include/pdf/SkPDFDevice.h:181: void appendRectangle(SkScalar x, SkScalar y, SkScalar w, SkScalar h); Might as well move appendRectangle and strokePath as well. http://codereview.appspot.com/4247043/diff/8001/include/pdf/SkPDFFont.h File include/pdf/SkPDFFont.h (right): http://codereview.appspot.com/4247043/diff/8001/include/pdf/SkPDFFont.h#newco... include/pdf/SkPDFFont.h:42: SkTypeface* typeFace(); The code generally considers typeface as a single word (caps) http://codereview.appspot.com/4247043/diff/8001/include/pdf/SkPDFFont.h#newco... include/pdf/SkPDFFont.h:125: void addFontWidthInfo(int16_t defaultWidth = 0, addSimpleFontWidthInfo? Type0 fonts have a different format. http://codereview.appspot.com/4247043/diff/8001/include/pdf/SkPDFUtils.h File include/pdf/SkPDFUtils.h (right): http://codereview.appspot.com/4247043/diff/8001/include/pdf/SkPDFUtils.h#newc... include/pdf/SkPDFUtils.h:24: class SkPDFUtils { Thanks for doing this. http://codereview.appspot.com/4247043/diff/8001/include/pdf/SkPDFUtils.h#newc... include/pdf/SkPDFUtils.h:28: static void MoveTo(SkScalar x, SkScalar y, SkString& content); References can be confusing, any reason to not make these pointers? Any thoughts on first vs. last argument for content? http://codereview.appspot.com/4247043/diff/8001/src/pdf/SkPDFDevice.cpp File src/pdf/SkPDFDevice.cpp (right): http://codereview.appspot.com/4247043/diff/8001/src/pdf/SkPDFDevice.cpp#newco... src/pdf/SkPDFDevice.cpp:637: uint32_t fontID = SkTypeface::UniqueID(paint.getTypeface()); fontID isn't used anymore... maybe get the face to a variable and shorten up some of the lines? http://codereview.appspot.com/4247043/diff/8001/src/pdf/SkPDFDevice.cpp#newco... src/pdf/SkPDFDevice.cpp:641: != paint.getTypeface()->uniqueID() || 4 spaces? http://codereview.appspot.com/4247043/diff/8001/src/pdf/SkPDFFont.cpp File src/pdf/SkPDFFont.cpp (right): http://codereview.appspot.com/4247043/diff/8001/src/pdf/SkPDFFont.cpp#newcode19 src/pdf/SkPDFFont.cpp:19: #include "SkDraw.h" I think SkDraw and SkPDFDevice are no longer used. http://codereview.appspot.com/4247043/diff/8001/src/pdf/SkPDFFont.cpp#newcode258 src/pdf/SkPDFFont.cpp:258: void addFontBBox( I'd make this makeFontBBox and return the SkPDFArray* http://codereview.appspot.com/4247043/diff/8001/src/pdf/SkPDFFont.cpp#newcode459 src/pdf/SkPDFFont.cpp:459: SkPDFFont::SkPDFFont(class SkAdvancedTypefaceMetrics* fontInfo, SkTypeface* typeFace, 80 columns http://codereview.appspot.com/4247043/diff/8001/src/pdf/SkPDFFont.cpp#newcode492 src/pdf/SkPDFFont.cpp:492: } You lost the clamp to fontInfo->fLastGlyphID http://codereview.appspot.com/4247043/diff/8001/src/pdf/SkPDFFont.cpp#newcode501 src/pdf/SkPDFFont.cpp:501: fFirstGlyphID = glyphID - (glyphID - 1) % 255; It'd be nice if this code didn't get repeated, but maybe there isn't a reasonable control flow which allows that? http://codereview.appspot.com/4247043/diff/8001/src/pdf/SkPDFFont.cpp#newcode516 src/pdf/SkPDFFont.cpp:516: insert("BaseFont", new SkPDFName(fFontInfo->fFontName))->unref(); Thanks for cleaning these up. http://codereview.appspot.com/4247043/diff/8001/src/pdf/SkPDFFont.cpp#newcode630 src/pdf/SkPDFFont.cpp:630: // Flip about the x-axis. "and scale by emsize." Do we know why you have to flip? http://codereview.appspot.com/4247043/diff/8001/src/pdf/SkPDFFont.cpp#newcode638 src/pdf/SkPDFFont.cpp:638: insert("CharProcs", new SkPDFObjRef(charProcs.get()))->unref(); I don't think this needs to be an object ref. http://codereview.appspot.com/4247043/diff/8001/src/pdf/SkPDFFont.cpp#newcode643 src/pdf/SkPDFFont.cpp:643: insert("Encoding", new SkPDFObjRef(encoding.get()))->unref(); Or this. http://codereview.appspot.com/4247043/diff/8001/src/pdf/SkPDFFont.cpp#newcode667 src/pdf/SkPDFFont.cpp:667: scaleFromFontUnits(SkFixedRound(glyph.fAdvanceX), emSize), Why round? http://codereview.appspot.com/4247043/diff/8001/src/pdf/SkPDFFont.cpp#newcode675 src/pdf/SkPDFFont.cpp:675: SkPDFUtils::PaintPath(paint.getStyle(), path->getFillType(), shape); I think you may be able to use strokePath, but I'm not sure. http://codereview.appspot.com/4247043/diff/8001/src/pdf/SkPDFFont.cpp#newcode677 src/pdf/SkPDFFont.cpp:677: SkRefPtr<SkStream> glyphStream = new SkMemoryStream(shape.c_str(), shape.size(), true); 80 col http://codereview.appspot.com/4247043/diff/8001/src/pdf/SkPDFFont.cpp#newcode679 src/pdf/SkPDFFont.cpp:679: SkRefPtr<SkPDFStream> glyphDescription = new SkPDFStream(glyphStream.get()); 80 col http://codereview.appspot.com/4247043/diff/8001/src/pdf/SkPDFFont.cpp#newcode682 src/pdf/SkPDFFont.cpp:682: charProcs->insert(characterName.c_str(), new SkPDFObjRef(glyphDescription.get()))->unref(); 80 col
Sign in to reply to this message.
PDF viewer is having trouble when SkString::appendScalar appends values using scientific notation. http://codereview.appspot.com/4247043/diff/8001/include/pdf/SkPDFDevice.h File include/pdf/SkPDFDevice.h (right): http://codereview.appspot.com/4247043/diff/8001/include/pdf/SkPDFDevice.h#new... include/pdf/SkPDFDevice.h:181: void appendRectangle(SkScalar x, SkScalar y, SkScalar w, SkScalar h); On 2011/03/01 21:27:47, Steve VanDeBogart wrote: > Might as well move appendRectangle and strokePath as well. Done. http://codereview.appspot.com/4247043/diff/8001/include/pdf/SkPDFFont.h File include/pdf/SkPDFFont.h (right): http://codereview.appspot.com/4247043/diff/8001/include/pdf/SkPDFFont.h#newco... include/pdf/SkPDFFont.h:42: SkTypeface* typeFace(); On 2011/03/01 21:27:47, Steve VanDeBogart wrote: > The code generally considers typeface as a single word (caps) Done. http://codereview.appspot.com/4247043/diff/8001/include/pdf/SkPDFFont.h#newco... include/pdf/SkPDFFont.h:125: void addFontWidthInfo(int16_t defaultWidth = 0, On 2011/03/01 21:27:47, Steve VanDeBogart wrote: > addSimpleFontWidthInfo? Type0 fonts have a different format. Done. http://codereview.appspot.com/4247043/diff/8001/include/pdf/SkPDFUtils.h File include/pdf/SkPDFUtils.h (right): http://codereview.appspot.com/4247043/diff/8001/include/pdf/SkPDFUtils.h#newc... include/pdf/SkPDFUtils.h:24: class SkPDFUtils { On 2011/03/01 21:27:47, Steve VanDeBogart wrote: > Thanks for doing this. Thanks for suggesting it. http://codereview.appspot.com/4247043/diff/8001/include/pdf/SkPDFUtils.h#newc... include/pdf/SkPDFUtils.h:28: static void MoveTo(SkScalar x, SkScalar y, SkString& content); On 2011/03/01 21:27:47, Steve VanDeBogart wrote: > References can be confusing, any reason to not make these pointers? Any > thoughts on first vs. last argument for content? Agree. I think I'd like to remove content from the argument list somehow. No preference on first vs last. Would you like first? http://codereview.appspot.com/4247043/diff/8001/src/pdf/SkPDFDevice.cpp File src/pdf/SkPDFDevice.cpp (right): http://codereview.appspot.com/4247043/diff/8001/src/pdf/SkPDFDevice.cpp#newco... src/pdf/SkPDFDevice.cpp:637: uint32_t fontID = SkTypeface::UniqueID(paint.getTypeface()); On 2011/03/01 21:27:47, Steve VanDeBogart wrote: > fontID isn't used anymore... maybe get the face to a variable and shorten up > some of the lines? Done. http://codereview.appspot.com/4247043/diff/8001/src/pdf/SkPDFDevice.cpp#newco... src/pdf/SkPDFDevice.cpp:641: != paint.getTypeface()->uniqueID() || On 2011/03/01 21:27:47, Steve VanDeBogart wrote: > 4 spaces? Don't need this indentation anymore thanks to the comment above. http://codereview.appspot.com/4247043/diff/8001/src/pdf/SkPDFFont.cpp File src/pdf/SkPDFFont.cpp (right): http://codereview.appspot.com/4247043/diff/8001/src/pdf/SkPDFFont.cpp#newcode19 src/pdf/SkPDFFont.cpp:19: #include "SkDraw.h" On 2011/03/01 21:27:47, Steve VanDeBogart wrote: > I think SkDraw and SkPDFDevice are no longer used. Done. http://codereview.appspot.com/4247043/diff/8001/src/pdf/SkPDFFont.cpp#newcode258 src/pdf/SkPDFFont.cpp:258: void addFontBBox( On 2011/03/01 21:27:47, Steve VanDeBogart wrote: > I'd make this makeFontBBox and return the SkPDFArray* Done. http://codereview.appspot.com/4247043/diff/8001/src/pdf/SkPDFFont.cpp#newcode459 src/pdf/SkPDFFont.cpp:459: SkPDFFont::SkPDFFont(class SkAdvancedTypefaceMetrics* fontInfo, SkTypeface* typeFace, On 2011/03/01 21:27:47, Steve VanDeBogart wrote: > 80 columns Done. http://codereview.appspot.com/4247043/diff/8001/src/pdf/SkPDFFont.cpp#newcode492 src/pdf/SkPDFFont.cpp:492: } On 2011/03/01 21:27:47, Steve VanDeBogart wrote: > You lost the clamp to fontInfo->fLastGlyphID fLastGlyphID is set to fontInfo->fLastGlyphID above: fLastGlyphID(fontInfo->fLastGlyphID) http://codereview.appspot.com/4247043/diff/8001/src/pdf/SkPDFFont.cpp#newcode501 src/pdf/SkPDFFont.cpp:501: fFirstGlyphID = glyphID - (glyphID - 1) % 255; On 2011/03/01 21:27:47, Steve VanDeBogart wrote: > It'd be nice if this code didn't get repeated, but maybe there isn't a > reasonable control flow which allows that? Changed to an early return control flow. http://codereview.appspot.com/4247043/diff/8001/src/pdf/SkPDFFont.cpp#newcode516 src/pdf/SkPDFFont.cpp:516: insert("BaseFont", new SkPDFName(fFontInfo->fFontName))->unref(); On 2011/03/01 21:27:47, Steve VanDeBogart wrote: > Thanks for cleaning these up. No problem. http://codereview.appspot.com/4247043/diff/8001/src/pdf/SkPDFFont.cpp#newcode630 src/pdf/SkPDFFont.cpp:630: // Flip about the x-axis. On 2011/03/01 21:27:47, Steve VanDeBogart wrote: > "and scale by emsize." Do we know why you have to flip? Looks like it might not to be flipped if SkPDFDevice::setTextTransform didn't already do a flip. http://codereview.appspot.com/4247043/diff/8001/src/pdf/SkPDFFont.cpp#newcode638 src/pdf/SkPDFFont.cpp:638: insert("CharProcs", new SkPDFObjRef(charProcs.get()))->unref(); On 2011/03/01 21:27:47, Steve VanDeBogart wrote: > I don't think this needs to be an object ref. Done. http://codereview.appspot.com/4247043/diff/8001/src/pdf/SkPDFFont.cpp#newcode643 src/pdf/SkPDFFont.cpp:643: insert("Encoding", new SkPDFObjRef(encoding.get()))->unref(); On 2011/03/01 21:27:47, Steve VanDeBogart wrote: > Or this. Done. http://codereview.appspot.com/4247043/diff/8001/src/pdf/SkPDFFont.cpp#newcode667 src/pdf/SkPDFFont.cpp:667: scaleFromFontUnits(SkFixedRound(glyph.fAdvanceX), emSize), On 2011/03/01 21:27:47, Steve VanDeBogart wrote: > Why round? Hmm. I wanted to convert to out of fixed and on windows fAdvanceX is set from a short. Changing to SkFixedToFloat, although scaleFromFontUnits will convert the float to int16_t. http://codereview.appspot.com/4247043/diff/8001/src/pdf/SkPDFFont.cpp#newcode675 src/pdf/SkPDFFont.cpp:675: SkPDFUtils::PaintPath(paint.getStyle(), path->getFillType(), shape); On 2011/03/01 21:27:47, Steve VanDeBogart wrote: > I think you may be able to use strokePath, but I'm not sure. Doesn't look right with the 'S' from SrokePath. http://codereview.appspot.com/4247043/diff/8001/src/pdf/SkPDFFont.cpp#newcode677 src/pdf/SkPDFFont.cpp:677: SkRefPtr<SkStream> glyphStream = new SkMemoryStream(shape.c_str(), shape.size(), true); On 2011/03/01 21:27:47, Steve VanDeBogart wrote: > 80 col Done. http://codereview.appspot.com/4247043/diff/8001/src/pdf/SkPDFFont.cpp#newcode679 src/pdf/SkPDFFont.cpp:679: SkRefPtr<SkPDFStream> glyphDescription = new SkPDFStream(glyphStream.get()); On 2011/03/01 21:27:47, Steve VanDeBogart wrote: > 80 col Done. http://codereview.appspot.com/4247043/diff/8001/src/pdf/SkPDFFont.cpp#newcode682 src/pdf/SkPDFFont.cpp:682: charProcs->insert(characterName.c_str(), new SkPDFObjRef(glyphDescription.get()))->unref(); On 2011/03/01 21:27:47, Steve VanDeBogart wrote: > 80 col Done.
Sign in to reply to this message.
http://codereview.appspot.com/4247043/diff/8001/include/pdf/SkPDFUtils.h File include/pdf/SkPDFUtils.h (right): http://codereview.appspot.com/4247043/diff/8001/include/pdf/SkPDFUtils.h#newc... include/pdf/SkPDFUtils.h:28: static void MoveTo(SkScalar x, SkScalar y, SkString& content); On 2011/03/02 00:02:25, Chris Guillory wrote: > On 2011/03/01 21:27:47, Steve VanDeBogart wrote: > > References can be confusing, any reason to not make these pointers? Any > > thoughts on first vs. last argument for content? > > Agree. I think I'd like to remove content from the argument list somehow. No > preference on first vs last. Would you like first? Making it the return value would be nice, but would necessitate copying the data. Chrome style says all output arguments at the end, which makes sense. So I guess pointers at the end is best. http://codereview.appspot.com/4247043/diff/8001/src/pdf/SkPDFFont.cpp File src/pdf/SkPDFFont.cpp (right): http://codereview.appspot.com/4247043/diff/8001/src/pdf/SkPDFFont.cpp#newcode492 src/pdf/SkPDFFont.cpp:492: } On 2011/03/02 00:02:25, Chris Guillory wrote: > On 2011/03/01 21:27:47, Steve VanDeBogart wrote: > > You lost the clamp to fontInfo->fLastGlyphID > fLastGlyphID is set to fontInfo->fLastGlyphID above: > fLastGlyphID(fontInfo->fLastGlyphID) Der, I see now. http://codereview.appspot.com/4247043/diff/8001/src/pdf/SkPDFFont.cpp#newcode630 src/pdf/SkPDFFont.cpp:630: // Flip about the x-axis. On 2011/03/02 00:02:25, Chris Guillory wrote: > On 2011/03/01 21:27:47, Steve VanDeBogart wrote: > > "and scale by emsize." Do we know why you have to flip? > Looks like it might not to be flipped if SkPDFDevice::setTextTransform didn't > already do a flip. We flip the origin to make Skia consumers happy. This makes PDF renderers draw the text (and images) upside down. I guess that doesn't apply when the font is drawn as a shape. Of course double flipping it does the right thing. A comment to that effect wouldn't hurt (why the flip). http://codereview.appspot.com/4247043/diff/8001/src/pdf/SkPDFFont.cpp#newcode667 src/pdf/SkPDFFont.cpp:667: scaleFromFontUnits(SkFixedRound(glyph.fAdvanceX), emSize), On 2011/03/02 00:02:25, Chris Guillory wrote: > On 2011/03/01 21:27:47, Steve VanDeBogart wrote: > > Why round? > Hmm. I wanted to convert to out of fixed and on windows fAdvanceX is set from a > short. Changing to SkFixedToFloat, although scaleFromFontUnits will convert the > float to int16_t. Oh, I see, it's a Fixed because it's been scaled up for setTextSize.... This reveals some other stuff. See other comments about emSize. http://codereview.appspot.com/4247043/diff/8001/src/pdf/SkPDFFont.cpp#newcode675 src/pdf/SkPDFFont.cpp:675: SkPDFUtils::PaintPath(paint.getStyle(), path->getFillType(), shape); On 2011/03/02 00:02:25, Chris Guillory wrote: > On 2011/03/01 21:27:47, Steve VanDeBogart wrote: > > I think you may be able to use strokePath, but I'm not sure. > > Doesn't look right with the 'S' from SrokePath. Hmm? I don't understand. Or is that a joke on the strokePath -> StrokePath change? http://codereview.appspot.com/4247043/diff/2015/include/pdf/SkPDFUtils.h File include/pdf/SkPDFUtils.h (right): http://codereview.appspot.com/4247043/diff/2015/include/pdf/SkPDFUtils.h#newc... include/pdf/SkPDFUtils.h:17: #ifndef SkPDFUTILS_DEFINED SkPDFUtils http://codereview.appspot.com/4247043/diff/2015/src/pdf/SkPDFDevice.cpp File src/pdf/SkPDFDevice.cpp (right): http://codereview.appspot.com/4247043/diff/2015/src/pdf/SkPDFDevice.cpp#newco... src/pdf/SkPDFDevice.cpp:36: #define NOT_IMPLEMENTED(condition, assert) \ Delete this, and move the one from SkPDFUtil.cpp to SkPDFUtil.h http://codereview.appspot.com/4247043/diff/2015/src/pdf/SkPDFDevice.cpp#newco... src/pdf/SkPDFDevice.cpp:224: SkPDFUtils::MoveTo( Wrapping http://codereview.appspot.com/4247043/diff/2015/src/pdf/SkPDFDevice.cpp#newco... src/pdf/SkPDFDevice.cpp:275: SkPDFUtils::AppendRectangle( Wrapping http://codereview.appspot.com/4247043/diff/2015/src/pdf/SkPDFFont.cpp File src/pdf/SkPDFFont.cpp (right): http://codereview.appspot.com/4247043/diff/2015/src/pdf/SkPDFFont.cpp#newcode258 src/pdf/SkPDFFont.cpp:258: bool flipVertical = false) { Mike wants to move away from boolean parameters in favor of enums. There's already SkPDFDevice::OriginTransform. It seems silly to include SkPDFDevice just to get that enum though. An alternative would be to have two wrappers to this method just above it and only use those wrappers. Your call. http://codereview.appspot.com/4247043/diff/2015/src/pdf/SkPDFFont.cpp#newcode503 src/pdf/SkPDFFont.cpp:503: populateType3Font(); Because of kOther_Font here, populateType3Font has to be careful, or at least explicit about what it used from fontInfo. Can you figure out what is used (transitively) and add it as a comment at the top of populateType3Font(). http://codereview.appspot.com/4247043/diff/2015/src/pdf/SkPDFFont.cpp#newcode626 src/pdf/SkPDFFont.cpp:626: fontMatrix.setScale(SkScalarInvert(1000), -SkScalarInvert(1000)); This needs to be the same as emSize below. http://codereview.appspot.com/4247043/diff/2015/src/pdf/SkPDFFont.cpp#newcode659 src/pdf/SkPDFFont.cpp:659: scaleFromFontUnits(SkFixedToFloat(glyph.fAdvanceX), emSize), But if emSize is anything other than 1000, we're going to scale all these values. So lets just paint.setTextSize(1000); and not have to scale metrics. i.e. completely ignore fFontInfo->fEmSize. http://codereview.appspot.com/4247043/diff/2015/src/pdf/SkPDFFont.cpp#newcode680 src/pdf/SkPDFFont.cpp:680: addSimpleFontWidthInfo(); Since we calculate all the width information above anyways, we should get it from there, instead of from fFontInfo. http://codereview.appspot.com/4247043/diff/2015/src/pdf/SkPDFFont.cpp#newcode781 src/pdf/SkPDFFont.cpp:781: getDefaultWidthInfo(fFontInfo.get(), &defaultWidth, widthRangeEntry); I'd rather just have the other caller of this method call getDefaultWidthInfo before calling this method instead of the default/magic combination of parameters.
Sign in to reply to this message.
http://codereview.appspot.com/4247043/diff/2015/include/pdf/SkPDFUtils.h File include/pdf/SkPDFUtils.h (right): http://codereview.appspot.com/4247043/diff/2015/include/pdf/SkPDFUtils.h#newc... include/pdf/SkPDFUtils.h:17: #ifndef SkPDFUTILS_DEFINED On 2011/03/02 02:41:55, Steve VanDeBogart wrote: > SkPDFUtils Done. http://codereview.appspot.com/4247043/diff/2015/src/pdf/SkPDFDevice.cpp File src/pdf/SkPDFDevice.cpp (right): http://codereview.appspot.com/4247043/diff/2015/src/pdf/SkPDFDevice.cpp#newco... src/pdf/SkPDFDevice.cpp:36: #define NOT_IMPLEMENTED(condition, assert) \ On 2011/03/02 02:41:55, Steve VanDeBogart wrote: > Delete this, and move the one from SkPDFUtil.cpp to SkPDFUtil.h Done. http://codereview.appspot.com/4247043/diff/2015/src/pdf/SkPDFDevice.cpp#newco... src/pdf/SkPDFDevice.cpp:224: SkPDFUtils::MoveTo( On 2011/03/02 02:41:55, Steve VanDeBogart wrote: > Wrapping Unsure of wrapping rules. Using: SkPDFUtils::MoveTo(points[i * 2].fX, points[i * 2].fY, &fContent); http://codereview.appspot.com/4247043/diff/2015/src/pdf/SkPDFDevice.cpp#newco... src/pdf/SkPDFDevice.cpp:275: SkPDFUtils::AppendRectangle( On 2011/03/02 02:41:55, Steve VanDeBogart wrote: > Wrapping Done. http://codereview.appspot.com/4247043/diff/2015/src/pdf/SkPDFDevice.cpp#newco... src/pdf/SkPDFDevice.cpp:638: SkTypeface* typeface = paint.getTypeface(); Some updates need to be made here. Looks like this will return NULL when the SkPaint isn't explicitly set to a typeface. SkTypeface::UniqueID actually creates the default font. http://codereview.appspot.com/4247043/diff/2015/src/pdf/SkPDFFont.cpp File src/pdf/SkPDFFont.cpp (right): http://codereview.appspot.com/4247043/diff/2015/src/pdf/SkPDFFont.cpp#newcode258 src/pdf/SkPDFFont.cpp:258: bool flipVertical = false) { On 2011/03/02 02:41:55, Steve VanDeBogart wrote: > Mike wants to move away from boolean parameters in favor of enums. There's > already SkPDFDevice::OriginTransform. It seems silly to include SkPDFDevice just > to get that enum though. An alternative would be to have two wrappers to this > method just above it and only use those wrappers. Your call. Concept seems the same as SkPDFDevice::OriginTransform. Including SkPDFDevice. http://codereview.appspot.com/4247043/diff/2015/src/pdf/SkPDFFont.cpp#newcode503 src/pdf/SkPDFFont.cpp:503: populateType3Font(); On 2011/03/02 02:41:55, Steve VanDeBogart wrote: > Because of kOther_Font here, populateType3Font has to be careful, or at least > explicit about what it used from fontInfo. Can you figure out what is used > (transitively) and add it as a comment at the top of populateType3Font(). Done. http://codereview.appspot.com/4247043/diff/2015/src/pdf/SkPDFFont.cpp#newcode582 src/pdf/SkPDFFont.cpp:582: SkASSERT(!fFontInfo->fVerticalMetrics.get()); Perhaps I should remove getDefaultWidthInfo and addSimpleFontWidthInfo since there's only one caller now? http://codereview.appspot.com/4247043/diff/2015/src/pdf/SkPDFFont.cpp#newcode626 src/pdf/SkPDFFont.cpp:626: fontMatrix.setScale(SkScalarInvert(1000), -SkScalarInvert(1000)); On 2011/03/02 02:41:55, Steve VanDeBogart wrote: > This needs to be the same as emSize below. I used to handle the emSize scaling with a matrix in the glyph stream. Using font size 1000 now. http://codereview.appspot.com/4247043/diff/2015/src/pdf/SkPDFFont.cpp#newcode659 src/pdf/SkPDFFont.cpp:659: scaleFromFontUnits(SkFixedToFloat(glyph.fAdvanceX), emSize), On 2011/03/02 02:41:55, Steve VanDeBogart wrote: > But if emSize is anything other than 1000, we're going to scale all these > values. > > So lets just paint.setTextSize(1000); and not have to scale metrics. i.e. > completely ignore fFontInfo->fEmSize. Done. Look the same on simple tests. If we see problems we may have to use the "magic" font size of emSize on windows gives us font design units. http://codereview.appspot.com/4247043/diff/2015/src/pdf/SkPDFFont.cpp#newcode680 src/pdf/SkPDFFont.cpp:680: addSimpleFontWidthInfo(); On 2011/03/02 02:41:55, Steve VanDeBogart wrote: > Since we calculate all the width information above anyways, we should get it > from there, instead of from fFontInfo. Done. http://codereview.appspot.com/4247043/diff/2015/src/pdf/SkPDFFont.cpp#newcode781 src/pdf/SkPDFFont.cpp:781: getDefaultWidthInfo(fFontInfo.get(), &defaultWidth, widthRangeEntry); On 2011/03/02 02:41:55, Steve VanDeBogart wrote: > I'd rather just have the other caller of this method call getDefaultWidthInfo > before calling this method instead of the default/magic combination of > parameters. Done.
Sign in to reply to this message.
I'm sorry for the death by a thousand papercuts! http://codereview.appspot.com/4247043/diff/2015/src/pdf/SkPDFFont.cpp File src/pdf/SkPDFFont.cpp (right): http://codereview.appspot.com/4247043/diff/2015/src/pdf/SkPDFFont.cpp#newcode582 src/pdf/SkPDFFont.cpp:582: SkASSERT(!fFontInfo->fVerticalMetrics.get()); On 2011/03/02 05:13:35, Chris Guillory wrote: > Perhaps I should remove getDefaultWidthInfo and addSimpleFontWidthInfo since > there's only one caller now? Some code modularity doesn't hurt. Do you see any reason to undo it? http://codereview.appspot.com/4247043/diff/9003/include/pdf/SkPDFFont.h File include/pdf/SkPDFFont.h (right): http://codereview.appspot.com/4247043/diff/9003/include/pdf/SkPDFFont.h#newco... include/pdf/SkPDFFont.h:40: /* Returns the typeface represented by this class. nit: note, returns null for the default face. http://codereview.appspot.com/4247043/diff/9003/src/pdf/SkPDFDevice.cpp File src/pdf/SkPDFDevice.cpp (right): http://codereview.appspot.com/4247043/diff/9003/src/pdf/SkPDFDevice.cpp#newco... src/pdf/SkPDFDevice.cpp:216: SkPDFUtils::MoveTo(points[i * 2].fX, points[i * 2].fY, nit: trailing whitespace http://codereview.appspot.com/4247043/diff/9003/src/pdf/SkPDFDevice.cpp#newco... src/pdf/SkPDFDevice.cpp:218: SkPDFUtils::AppendLine(points[i * 2 + 1].fX, nit: trailing whitespace http://codereview.appspot.com/4247043/diff/9003/src/pdf/SkPDFFont.cpp File src/pdf/SkPDFFont.cpp (right): http://codereview.appspot.com/4247043/diff/9003/src/pdf/SkPDFFont.cpp#newcode235 src/pdf/SkPDFFont.cpp:235: int16_t* defaultWidth, nit: trailing whitespace http://codereview.appspot.com/4247043/diff/9003/src/pdf/SkPDFFont.cpp#newcode349 src/pdf/SkPDFFont.cpp:349: if (find(fTypeface->uniqueID(), fFirstGlyphID, &index)) { to handle the NULL fTypeface, it looks like all calls to fTypeface->uniqueID should be changed to SkTypeface::UniqueID(fTypeface) ? http://codereview.appspot.com/4247043/diff/9003/src/pdf/SkPDFFont.cpp#newcode552 src/pdf/SkPDFFont.cpp:552: &defaultWidth); nit: up to previous line. http://codereview.appspot.com/4247043/diff/9003/src/pdf/SkPDFFont.cpp#newcode569 src/pdf/SkPDFFont.cpp:569: &appendVerticalAdvance, &defaultAdvance); nit: up to previous line. http://codereview.appspot.com/4247043/diff/9003/src/pdf/SkPDFFont.cpp#newcode623 src/pdf/SkPDFFont.cpp:623: insert("FontBBox", nit: trailing whitespace http://codereview.appspot.com/4247043/diff/9003/src/pdf/SkPDFFont.cpp#newcode624 src/pdf/SkPDFFont.cpp:624: makeFontBBox(fFontInfo.get(), nit: trailing whitespace http://codereview.appspot.com/4247043/diff/9003/src/pdf/SkPDFFont.cpp#newcode624 src/pdf/SkPDFFont.cpp:624: makeFontBBox(fFontInfo.get(), This is the last real dependence on fFontInfo in populateType3Font, but it should be easy to get rid of it. Just start with SkRect bbox = SkRect::MakeEmpty(), and then as you draw each glyph, do bbox->join(left, top, right, bottom). This may yield a different bbox for each subset of a large font, but it will still be correct. You'll then want to change makeFontBBox to take a SkRect and emSize. http://codereview.appspot.com/4247043/diff/9003/src/pdf/SkPDFFont.cpp#newcode665 src/pdf/SkPDFFont.cpp:665: content.appendf("%f 0 %hd %hd %hd %hd d1\n", nit: trailing whitespace http://codereview.appspot.com/4247043/diff/9003/src/pdf/SkPDFFont.cpp#newcode665 src/pdf/SkPDFFont.cpp:665: content.appendf("%f 0 %hd %hd %hd %hd d1\n", SkScalar can be either a float or a fixed, so you have to use appendScalar. http://codereview.appspot.com/4247043/diff/9003/src/pdf/SkPDFFont.cpp#newcode665 src/pdf/SkPDFFont.cpp:665: content.appendf("%f 0 %hd %hd %hd %hd d1\n", nit: Also, can you pull this line into a function? http://codereview.appspot.com/4247043/diff/9003/src/pdf/SkPDFFont.cpp#newcode666 src/pdf/SkPDFFont.cpp:666: SkFixedToFloat(glyph.fAdvanceX), nit: trailing whitespace http://codereview.appspot.com/4247043/diff/9003/src/pdf/SkPDFFont.cpp#newcode672 src/pdf/SkPDFFont.cpp:672: SkPDFUtils::PaintPath(paint.getStyle(), path->getFillType(), nit: trailing whitespace http://codereview.appspot.com/4247043/diff/9003/src/pdf/SkPDFFont.cpp#newcode785 src/pdf/SkPDFFont.cpp:785: void SkPDFFont::addSimpleFontWidthInfo( nit: Now that type 3 isn't using this, maybe addWidthInfoFromRange ?
Sign in to reply to this message.
Change is now dependent on http://codereview.appspot.com/4261042. http://codereview.appspot.com/4247043/diff/9003/include/pdf/SkPDFFont.h File include/pdf/SkPDFFont.h (right): http://codereview.appspot.com/4247043/diff/9003/include/pdf/SkPDFFont.h#newco... include/pdf/SkPDFFont.h:40: /* Returns the typeface represented by this class. On 2011/03/02 19:43:10, Steve VanDeBogart wrote: > nit: note, returns null for the default face. Done. http://codereview.appspot.com/4247043/diff/9003/src/pdf/SkPDFDevice.cpp File src/pdf/SkPDFDevice.cpp (right): http://codereview.appspot.com/4247043/diff/9003/src/pdf/SkPDFDevice.cpp#newco... src/pdf/SkPDFDevice.cpp:216: SkPDFUtils::MoveTo(points[i * 2].fX, points[i * 2].fY, On 2011/03/02 19:43:10, Steve VanDeBogart wrote: > nit: trailing whitespace Done. http://codereview.appspot.com/4247043/diff/9003/src/pdf/SkPDFDevice.cpp#newco... src/pdf/SkPDFDevice.cpp:218: SkPDFUtils::AppendLine(points[i * 2 + 1].fX, On 2011/03/02 19:43:10, Steve VanDeBogart wrote: > nit: trailing whitespace Done. http://codereview.appspot.com/4247043/diff/9003/src/pdf/SkPDFFont.cpp File src/pdf/SkPDFFont.cpp (right): http://codereview.appspot.com/4247043/diff/9003/src/pdf/SkPDFFont.cpp#newcode235 src/pdf/SkPDFFont.cpp:235: int16_t* defaultWidth, On 2011/03/02 19:43:10, Steve VanDeBogart wrote: > nit: trailing whitespace Done. http://codereview.appspot.com/4247043/diff/9003/src/pdf/SkPDFFont.cpp#newcode349 src/pdf/SkPDFFont.cpp:349: if (find(fTypeface->uniqueID(), fFirstGlyphID, &index)) { On 2011/03/02 19:43:10, Steve VanDeBogart wrote: > to handle the NULL fTypeface, it looks like all calls to fTypeface->uniqueID > should be changed to SkTypeface::UniqueID(fTypeface) ? Done. http://codereview.appspot.com/4247043/diff/9003/src/pdf/SkPDFFont.cpp#newcode552 src/pdf/SkPDFFont.cpp:552: &defaultWidth); On 2011/03/02 19:43:10, Steve VanDeBogart wrote: > nit: up to previous line. Done. http://codereview.appspot.com/4247043/diff/9003/src/pdf/SkPDFFont.cpp#newcode569 src/pdf/SkPDFFont.cpp:569: &appendVerticalAdvance, &defaultAdvance); On 2011/03/02 19:43:10, Steve VanDeBogart wrote: > nit: up to previous line. Done. http://codereview.appspot.com/4247043/diff/9003/src/pdf/SkPDFFont.cpp#newcode623 src/pdf/SkPDFFont.cpp:623: insert("FontBBox", On 2011/03/02 19:43:10, Steve VanDeBogart wrote: > nit: trailing whitespace Done. http://codereview.appspot.com/4247043/diff/9003/src/pdf/SkPDFFont.cpp#newcode624 src/pdf/SkPDFFont.cpp:624: makeFontBBox(fFontInfo.get(), On 2011/03/02 19:43:10, Steve VanDeBogart wrote: > nit: trailing whitespace Done. http://codereview.appspot.com/4247043/diff/9003/src/pdf/SkPDFFont.cpp#newcode624 src/pdf/SkPDFFont.cpp:624: makeFontBBox(fFontInfo.get(), On 2011/03/02 19:43:10, Steve VanDeBogart wrote: > This is the last real dependence on fFontInfo in populateType3Font, but it > should be easy to get rid of it. Just start with SkRect bbox = > SkRect::MakeEmpty(), and then as you draw each glyph, do bbox->join(left, top, > right, bottom). This may yield a different bbox for each subset of a large > font, but it will still be correct. You'll then want to change makeFontBBox to > take a SkRect and emSize. Done. http://codereview.appspot.com/4247043/diff/9003/src/pdf/SkPDFFont.cpp#newcode665 src/pdf/SkPDFFont.cpp:665: content.appendf("%f 0 %hd %hd %hd %hd d1\n", On 2011/03/02 19:43:10, Steve VanDeBogart wrote: > nit: trailing whitespace Done. http://codereview.appspot.com/4247043/diff/9003/src/pdf/SkPDFFont.cpp#newcode665 src/pdf/SkPDFFont.cpp:665: content.appendf("%f 0 %hd %hd %hd %hd d1\n", On 2011/03/02 19:43:10, Steve VanDeBogart wrote: > SkScalar can be either a float or a fixed, so you have to use appendScalar. Done. http://codereview.appspot.com/4247043/diff/9003/src/pdf/SkPDFFont.cpp#newcode665 src/pdf/SkPDFFont.cpp:665: content.appendf("%f 0 %hd %hd %hd %hd d1\n", On 2011/03/02 19:43:10, Steve VanDeBogart wrote: > nit: Also, can you pull this line into a function? Done. http://codereview.appspot.com/4247043/diff/9003/src/pdf/SkPDFFont.cpp#newcode666 src/pdf/SkPDFFont.cpp:666: SkFixedToFloat(glyph.fAdvanceX), On 2011/03/02 19:43:10, Steve VanDeBogart wrote: > nit: trailing whitespace Done. http://codereview.appspot.com/4247043/diff/9003/src/pdf/SkPDFFont.cpp#newcode672 src/pdf/SkPDFFont.cpp:672: SkPDFUtils::PaintPath(paint.getStyle(), path->getFillType(), On 2011/03/02 19:43:10, Steve VanDeBogart wrote: > nit: trailing whitespace Done. http://codereview.appspot.com/4247043/diff/9003/src/pdf/SkPDFFont.cpp#newcode785 src/pdf/SkPDFFont.cpp:785: void SkPDFFont::addSimpleFontWidthInfo( On 2011/03/02 19:43:10, Steve VanDeBogart wrote: > nit: Now that type 3 isn't using this, maybe addWidthInfoFromRange ? Done.
Sign in to reply to this message.
http://codereview.appspot.com/4247043/diff/13004/src/pdf/SkPDFFont.cpp File src/pdf/SkPDFFont.cpp (right): http://codereview.appspot.com/4247043/diff/13004/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:630: SkRefPtr<SkPDFArray> bboxArray = new SkPDFArray(); This line has been removed.
Sign in to reply to this message.
LGTM with a few minor fixes. http://codereview.appspot.com/4247043/diff/13004/src/pdf/SkPDFFont.cpp File src/pdf/SkPDFFont.cpp (right): http://codereview.appspot.com/4247043/diff/13004/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:571: const SkAdvancedTypefaceMetrics::WidthRange* widthRangeEntry = NULL; unused var http://codereview.appspot.com/4247043/diff/13004/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:624: fLastGlyphID = cache->getGlyphCount() - 1; Note that fLastGlyphID may not be correct because fFontInfo->fLastGlyphID isn't set for unknown fonts. http://codereview.appspot.com/4247043/diff/13004/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:686: insert("FontBBox", makeFontBBox(bbox, 1000)); ->unref() http://codereview.appspot.com/4247043/diff/13004/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:779: fFontInfo->fEmSize)); ->unref()
Sign in to reply to this message.
http://codereview.appspot.com/4247043/diff/13004/src/pdf/SkPDFFont.cpp File src/pdf/SkPDFFont.cpp (right): http://codereview.appspot.com/4247043/diff/13004/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:571: const SkAdvancedTypefaceMetrics::WidthRange* widthRangeEntry = NULL; On 2011/03/03 01:05:40, Steve VanDeBogart wrote: > unused var widthRangeEntry? It's used. http://codereview.appspot.com/4247043/diff/13004/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:624: fLastGlyphID = cache->getGlyphCount() - 1; On 2011/03/03 01:05:40, Steve VanDeBogart wrote: > Note that fLastGlyphID may not be correct because fFontInfo->fLastGlyphID isn't > set for unknown fonts. cache->getGlyphCount() may not be correct? The value from fFontInfo->fLastGlyphID isn't used. http://codereview.appspot.com/4247043/diff/13004/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:686: insert("FontBBox", makeFontBBox(bbox, 1000)); On 2011/03/03 01:05:40, Steve VanDeBogart wrote: > ->unref() Done. http://codereview.appspot.com/4247043/diff/13004/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:779: fFontInfo->fEmSize)); On 2011/03/03 01:05:40, Steve VanDeBogart wrote: > ->unref() Done.
Sign in to reply to this message.
http://codereview.appspot.com/4247043/diff/13004/src/pdf/SkPDFFont.cpp File src/pdf/SkPDFFont.cpp (right): http://codereview.appspot.com/4247043/diff/13004/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:571: const SkAdvancedTypefaceMetrics::WidthRange* widthRangeEntry = NULL; On 2011/03/03 01:46:31, Chris Guillory wrote: > On 2011/03/03 01:05:40, Steve VanDeBogart wrote: > > unused var > widthRangeEntry? It's used. Oops, my mistake. http://codereview.appspot.com/4247043/diff/13004/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:624: fLastGlyphID = cache->getGlyphCount() - 1; On 2011/03/03 01:46:31, Chris Guillory wrote: > On 2011/03/03 01:05:40, Steve VanDeBogart wrote: > > Note that fLastGlyphID may not be correct because fFontInfo->fLastGlyphID > isn't > > set for unknown fonts. > cache->getGlyphCount() may not be correct? The value from > fFontInfo->fLastGlyphID isn't used. Sorry, I didn't see that you re initialize it here. Probably should do this at the top level and only call cache->getGlyphCount if the one we get out of fFontInfo is zero? That would avoid calculating it twice on windows. Unless you think that the calculation is cheap enough, then we should just remove fLastGlyphID from the advanced glyph metrics.
Sign in to reply to this message.
Cleaned up fLastGlyphID usage.
Sign in to reply to this message.
LGTM with two minor things. http://codereview.appspot.com/4247043/diff/24001/src/pdf/SkPDFFont.cpp File src/pdf/SkPDFFont.cpp (right): http://codereview.appspot.com/4247043/diff/24001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:613: if (fFontInfo->fLastGlyphID <= 255) We need this if clause at the end of populateType3Font as well. http://codereview.appspot.com/4247043/diff/24001/src/pdf/pdf_files.mk File src/pdf/pdf_files.mk (right): http://codereview.appspot.com/4247043/diff/24001/src/pdf/pdf_files.mk#newcode12 src/pdf/pdf_files.mk:12: SkPDFUtils.cpp \ trailing \ without a following newline may break things. Convention for these files in no \ on the last line.
Sign in to reply to this message.
Thanks. http://codereview.appspot.com/4247043/diff/24001/src/pdf/SkPDFFont.cpp File src/pdf/SkPDFFont.cpp (right): http://codereview.appspot.com/4247043/diff/24001/src/pdf/SkPDFFont.cpp#newcod... src/pdf/SkPDFFont.cpp:613: if (fFontInfo->fLastGlyphID <= 255) On 2011/03/04 21:28:54, Steve VanDeBogart wrote: > We need this if clause at the end of populateType3Font as well. Done. http://codereview.appspot.com/4247043/diff/24001/src/pdf/pdf_files.mk File src/pdf/pdf_files.mk (right): http://codereview.appspot.com/4247043/diff/24001/src/pdf/pdf_files.mk#newcode12 src/pdf/pdf_files.mk:12: SkPDFUtils.cpp \ On 2011/03/04 21:28:54, Steve VanDeBogart wrote: > trailing \ without a following newline may break things. Convention for these > files in no \ on the last line. Done.
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
