Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2231483002:
SkPDF: Subset Type3 (fallback) font (Closed)
|
Created:
4 years, 4 months ago by hal.canary Modified:
4 years, 4 months ago Reviewers:
bungeman-skia CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/skia.git@SkPDF_next3 Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
DescriptionSkPDF: Subset Type3 (fallback) font
Motivation: significant file-size reduction.
Also: SkPDFFont::subsetFont() returns a sk_sp<SkPDFObject>
rather than a SkPDFFont*.
SkPDFType3Font constructor no longer populates font info;
relies on subsetting.
SkPDFFont::Create is easier to read
Also: SkPDFType3Font are scaled by emSize rather than 1000.
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2231483002
Committed: https://skia.googlesource.com/skia/+/88b138da99328b04cae9a8ee19c3882b8847a550
Committed: https://skia.googlesource.com/skia/+/7e8d5d3519ea2d4c7f158ff9737843e20daad0cb
Patch Set 1 : 2016-08-09 (Tuesday) 22:35:28 EDT #Patch Set 2 : 2016-08-10 (Wednesday) 10:28:24 EDT #
Total comments: 16
Patch Set 3 : 2016-08-10 (Wednesday) 14:58:04 EDT #
Total comments: 4
Patch Set 4 : 2016-08-11 (Thursday) 08:39:03 EDT #Patch Set 5 : 2016-08-11 (Thursday) 13:49:34 EDT #
Total comments: 2
Patch Set 6 : 2016-08-11 (Thursday) 14:02:09 EDT #Patch Set 7 : winding number ??? 2016-08-11 (Thursday) 14:10:36 EDT #Patch Set 8 : nit #Patch Set 9 : 2016-08-12 (Friday) 08:40:07 EDT #Patch Set 10 : 2016-08-12 (Friday) 10:20:16 EDT #
Messages
Total messages: 62 (51 generated)
Description was changed from ========== SkPDF: Subset Type3 (fallback) font BUG=skia: ========== to ========== SkPDF: Subset Type3 (fallback) font BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2231483002 ==========
The CQ bit was checked by halcanary@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-GN-Trybot on master.client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)
The CQ bit was checked by halcanary@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-Shared-Trybot on master.client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)
The CQ bit was checked by halcanary@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #2 (id:20001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-GN-Trybot on master.client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)
The CQ bit was checked by halcanary@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== SkPDF: Subset Type3 (fallback) font BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2231483002 ========== to ========== SkPDF: Subset Type3 (fallback) font Motivation: significant file-size reduction. GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2231483002 ==========
Patchset #2 (id:40001) has been deleted
Patchset #1 (id:1) has been deleted
The CQ bit was checked by halcanary@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== SkPDF: Subset Type3 (fallback) font Motivation: significant file-size reduction. GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2231483002 ========== to ========== SkPDF: Subset Type3 (fallback) font Motivation: significant file-size reduction. Also: SkPDFFont::subsetFont() returns a sk_sp<SkPDFObject> rather than a SkPDFFont*. SkPDFType3Font constructor no longer populates font info; relies on subsetting. SkPDFFont::Create is easier to read SkPDFFont::Create checks kNotEmbeddable_FontFlag, and uses Type3 for that. GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2231483002 ==========
Description was changed from ========== SkPDF: Subset Type3 (fallback) font Motivation: significant file-size reduction. Also: SkPDFFont::subsetFont() returns a sk_sp<SkPDFObject> rather than a SkPDFFont*. SkPDFType3Font constructor no longer populates font info; relies on subsetting. SkPDFFont::Create is easier to read SkPDFFont::Create checks kNotEmbeddable_FontFlag, and uses Type3 for that. GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2231483002 ========== to ========== SkPDF: Subset Type3 (fallback) font Motivation: significant file-size reduction. Also: SkPDFFont::subsetFont() returns a sk_sp<SkPDFObject> rather than a SkPDFFont*. SkPDFType3Font constructor no longer populates font info; relies on subsetting. SkPDFFont::Create is easier to read SkPDFFont::Create checks kNotEmbeddable_FontFlag, and uses Type3 for that. Also: SkPDFType3Font are scaled by emSize rather than 1000. GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2231483002 ==========
halcanary@google.com changed reviewers: + bungeman@google.com
PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2231483002/diff/80001/src/pdf/SkPDFFont.cpp File src/pdf/SkPDFFont.cpp (right): https://codereview.chromium.org/2231483002/diff/80001/src/pdf/SkPDFFont.cpp#n... src/pdf/SkPDFFont.cpp:1079: SkGlyphID operator*() const { return SkToU16(fCurrent); } Doesn't this operator normally return a reference? (probably a const reference) https://codereview.chromium.org/2231483002/diff/80001/src/pdf/SkPDFFont.cpp#n... src/pdf/SkPDFFont.cpp:1083: const SkGlyphID& fFirst; It seems odd that this is a reference. If the compiler optimizes it out it makes no difference, but if not the reference will be larger than the integer. It seems strange because it makes it look like this fFirst may change based on the 'outer' fFirst changing, but the 'outer' fFirst doesn't change. https://codereview.chromium.org/2231483002/diff/80001/src/pdf/SkPDFFont.cpp#n... src/pdf/SkPDFFont.cpp:1089: SkGlyphID fLast; This is all in a limited scope, but it seems better if these fields are private. If they're made private I don't have to think about them again (same goes for fCurrent above). Also, they don't appear to ever change, so it would be good to mark them const. https://codereview.chromium.org/2231483002/diff/80001/src/pdf/SkPDFFont.cpp#n... src/pdf/SkPDFFont.cpp:1102: paint.setTextSize(emSize); Set hinting to none? https://codereview.chromium.org/2231483002/diff/80001/src/pdf/SkPDFFont.cpp#n... src/pdf/SkPDFFont.cpp:1107: // Flip about the x-axis and scale by 1/1000. comment: scale by em https://codereview.chromium.org/2231483002/diff/80001/src/pdf/SkPDFFont.cpp#n... src/pdf/SkPDFFont.cpp:1120: widthArray->reserve(lastGlyphID - firstGlyphID + 2); a short comment about these magic '2' and '3' would be nice here. https://codereview.chromium.org/2231483002/diff/80001/src/pdf/SkPDFFont.cpp#n... src/pdf/SkPDFFont.cpp:1165: font->insertObject("FontBBox", makeFontBBox(bbox, 1000)); Is this 1000 right? Should it be em? https://codereview.chromium.org/2231483002/diff/80001/src/pdf/SkPDFFont.cpp#n... src/pdf/SkPDFFont.cpp:1206: uint16_t emSize = info && info->fEmSize > 0 ? info->fEmSize : 1000; Do you sometimes get a bogus em size or no info? Seems like an odd thing to guess here.
https://codereview.chromium.org/2231483002/diff/80001/src/pdf/SkPDFFont.cpp File src/pdf/SkPDFFont.cpp (right): https://codereview.chromium.org/2231483002/diff/80001/src/pdf/SkPDFFont.cpp#n... src/pdf/SkPDFFont.cpp:1079: SkGlyphID operator*() const { return SkToU16(fCurrent); } On 2016/08/10 15:32:56, bungeman-skia wrote: > Doesn't this operator normally return a reference? (probably a const reference) It doesn't have to, but okay. Done. https://codereview.chromium.org/2231483002/diff/80001/src/pdf/SkPDFFont.cpp#n... src/pdf/SkPDFFont.cpp:1083: const SkGlyphID& fFirst; On 2016/08/10 15:32:56, bungeman-skia wrote: > It seems odd that this is a reference. If the compiler optimizes it out it makes > no difference, but if not the reference will be larger than the integer. It > seems strange because it makes it look like this fFirst may change based on the > 'outer' fFirst changing, but the 'outer' fFirst doesn't change. Done. https://codereview.chromium.org/2231483002/diff/80001/src/pdf/SkPDFFont.cpp#n... src/pdf/SkPDFFont.cpp:1089: SkGlyphID fLast; On 2016/08/10 15:32:56, bungeman-skia wrote: > This is all in a limited scope, but it seems better if these fields are private. > If they're made private I don't have to think about them again (same goes for > fCurrent above). Also, they don't appear to ever change, so it would be good to > mark them const. Done. https://codereview.chromium.org/2231483002/diff/80001/src/pdf/SkPDFFont.cpp#n... src/pdf/SkPDFFont.cpp:1102: paint.setTextSize(emSize); On 2016/08/10 15:32:56, bungeman-skia wrote: > Set hinting to none? Done. https://codereview.chromium.org/2231483002/diff/80001/src/pdf/SkPDFFont.cpp#n... src/pdf/SkPDFFont.cpp:1107: // Flip about the x-axis and scale by 1/1000. On 2016/08/10 15:32:56, bungeman-skia wrote: > comment: scale by em Done. https://codereview.chromium.org/2231483002/diff/80001/src/pdf/SkPDFFont.cpp#n... src/pdf/SkPDFFont.cpp:1120: widthArray->reserve(lastGlyphID - firstGlyphID + 2); On 2016/08/10 15:32:56, bungeman-skia wrote: > a short comment about these magic '2' and '3' would be nice here. Done. https://codereview.chromium.org/2231483002/diff/80001/src/pdf/SkPDFFont.cpp#n... src/pdf/SkPDFFont.cpp:1165: font->insertObject("FontBBox", makeFontBBox(bbox, 1000)); On 2016/08/10 15:32:56, bungeman-skia wrote: > Is this 1000 right? Should it be em? actually no. they want the glyph coordinate system. fixed https://codereview.chromium.org/2231483002/diff/80001/src/pdf/SkPDFFont.cpp#n... src/pdf/SkPDFFont.cpp:1206: uint16_t emSize = info && info->fEmSize > 0 ? info->fEmSize : 1000; On 2016/08/10 15:32:56, bungeman-skia wrote: > Do you sometimes get a bogus em size or no info? Seems like an odd thing to > guess here. I do not assume SkAdvancedTypefaceMetrics* is non-null. if it is null, I end up here. Also emSize defaults to zero. I want to future-proof this code.
Description was changed from ========== SkPDF: Subset Type3 (fallback) font Motivation: significant file-size reduction. Also: SkPDFFont::subsetFont() returns a sk_sp<SkPDFObject> rather than a SkPDFFont*. SkPDFType3Font constructor no longer populates font info; relies on subsetting. SkPDFFont::Create is easier to read SkPDFFont::Create checks kNotEmbeddable_FontFlag, and uses Type3 for that. Also: SkPDFType3Font are scaled by emSize rather than 1000. GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2231483002 ========== to ========== SkPDF: Subset Type3 (fallback) font Motivation: significant file-size reduction. Also: SkPDFFont::subsetFont() returns a sk_sp<SkPDFObject> rather than a SkPDFFont*. SkPDFType3Font constructor no longer populates font info; relies on subsetting. SkPDFFont::Create is easier to read Also: SkPDFType3Font are scaled by emSize rather than 1000. GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2231483002 ==========
https://codereview.chromium.org/2231483002/diff/100001/src/pdf/SkPDFFont.cpp File src/pdf/SkPDFFont.cpp (right): https://codereview.chromium.org/2231483002/diff/100001/src/pdf/SkPDFFont.cpp#... src/pdf/SkPDFFont.cpp:1091: int fCurrent; // must be int to make fLast+1 to fit While half open ranges have great properties on the low side, they do have this annoying property of requiring at least one otherwise unusable value to represent one past the end. I think operator* should return either a SkGlyphId values (and cast) or const int& (and update gID below). Before I was just commenting that usually operator* returns a reference, but in the case of input iterators (which this can be considered to be) operator* may return a value. For example, see std::istreambuf_iterator. As a result, it would probably be cleaner to just do this the way you were doing it before, but with at least a comment so we dont' forget this // This is an input_iterator SkGlyphID operator*() const { return static_cast<SkGlyphID>(fCurrent); } This is at least cleaner than having an extra field sitting around and the extra complexity of managing it. https://codereview.chromium.org/2231483002/diff/100001/src/pdf/SkPDFFont.cpp#... src/pdf/SkPDFFont.cpp:1125: // lenth(firstGlyphID .. lastGlyphID) == lastGlyphID - firstGlyphID + 1 s/lenth/length
https://codereview.chromium.org/2231483002/diff/100001/src/pdf/SkPDFFont.cpp File src/pdf/SkPDFFont.cpp (right): https://codereview.chromium.org/2231483002/diff/100001/src/pdf/SkPDFFont.cpp#... src/pdf/SkPDFFont.cpp:1091: int fCurrent; // must be int to make fLast+1 to fit On 2016/08/11 16:19:56, bungeman-skia wrote: > While half open ranges have great properties on the low side, they do have this > annoying property of requiring at least one otherwise unusable value to > represent one past the end. > > I think operator* should return either a SkGlyphId values (and cast) or const > int& (and update gID below). Before I was just commenting that usually operator* > returns a reference, but in the case of input iterators (which this can be > considered to be) operator* may return a value. For example, see > std::istreambuf_iterator. As a result, it would probably be cleaner to just do > this the way you were doing it before, but with at least a comment so we dont' > forget this > > // This is an input_iterator > SkGlyphID operator*() const { return static_cast<SkGlyphID>(fCurrent); } > > This is at least cleaner than having an extra field sitting around and the extra > complexity of managing it. Done. https://codereview.chromium.org/2231483002/diff/100001/src/pdf/SkPDFFont.cpp#... src/pdf/SkPDFFont.cpp:1125: // lenth(firstGlyphID .. lastGlyphID) == lastGlyphID - firstGlyphID + 1 On 2016/08/11 16:19:56, bungeman-skia wrote: > s/lenth/length Done.
The CQ bit was checked by halcanary@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Win-MSVC-x86-Debug-Trybot on master.client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86-D...)
The CQ bit was checked by halcanary@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by halcanary@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm with two nits https://codereview.chromium.org/2231483002/diff/140001/src/pdf/SkPDFFont.cpp File src/pdf/SkPDFFont.cpp (right): https://codereview.chromium.org/2231483002/diff/140001/src/pdf/SkPDFFont.cpp#... src/pdf/SkPDFFont.cpp:1086: : fFirst(f), fCurrent(c) {} nit: suppose this could fit on one line https://codereview.chromium.org/2231483002/diff/140001/src/pdf/SkPDFFont.cpp#... src/pdf/SkPDFFont.cpp:1190: //FIXME !!!
The CQ bit was checked by halcanary@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from bungeman@google.com Link to the patchset: https://codereview.chromium.org/2231483002/#ps200001 (title: "nit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== SkPDF: Subset Type3 (fallback) font Motivation: significant file-size reduction. Also: SkPDFFont::subsetFont() returns a sk_sp<SkPDFObject> rather than a SkPDFFont*. SkPDFType3Font constructor no longer populates font info; relies on subsetting. SkPDFFont::Create is easier to read Also: SkPDFType3Font are scaled by emSize rather than 1000. GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2231483002 ========== to ========== SkPDF: Subset Type3 (fallback) font Motivation: significant file-size reduction. Also: SkPDFFont::subsetFont() returns a sk_sp<SkPDFObject> rather than a SkPDFFont*. SkPDFType3Font constructor no longer populates font info; relies on subsetting. SkPDFFont::Create is easier to read Also: SkPDFType3Font are scaled by emSize rather than 1000. GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2231483002 Committed: https://skia.googlesource.com/skia/+/88b138da99328b04cae9a8ee19c3882b8847a550 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:200001) as https://skia.googlesource.com/skia/+/88b138da99328b04cae9a8ee19c3882b8847a550
Message was sent while issue was closed.
A revert of this CL (patchset #8 id:200001) has been created in https://codereview.chromium.org/2232283003/ by halcanary@google.com. The reason for reverting is: internal failing.
Message was sent while issue was closed.
Description was changed from ========== SkPDF: Subset Type3 (fallback) font Motivation: significant file-size reduction. Also: SkPDFFont::subsetFont() returns a sk_sp<SkPDFObject> rather than a SkPDFFont*. SkPDFType3Font constructor no longer populates font info; relies on subsetting. SkPDFFont::Create is easier to read Also: SkPDFType3Font are scaled by emSize rather than 1000. GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2231483002 Committed: https://skia.googlesource.com/skia/+/88b138da99328b04cae9a8ee19c3882b8847a550 ========== to ========== SkPDF: Subset Type3 (fallback) font Motivation: significant file-size reduction. Also: SkPDFFont::subsetFont() returns a sk_sp<SkPDFObject> rather than a SkPDFFont*. SkPDFType3Font constructor no longer populates font info; relies on subsetting. SkPDFFont::Create is easier to read Also: SkPDFType3Font are scaled by emSize rather than 1000. GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2231483002 Committed: https://skia.googlesource.com/skia/+/88b138da99328b04cae9a8ee19c3882b8847a550 ==========
The CQ bit was checked by halcanary@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by halcanary@google.com
The CQ bit was checked by halcanary@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by halcanary@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from bungeman@google.com Link to the patchset: https://codereview.chromium.org/2231483002/#ps240001 (title: "2016-08-12 (Friday) 10:20:16 EDT")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== SkPDF: Subset Type3 (fallback) font Motivation: significant file-size reduction. Also: SkPDFFont::subsetFont() returns a sk_sp<SkPDFObject> rather than a SkPDFFont*. SkPDFType3Font constructor no longer populates font info; relies on subsetting. SkPDFFont::Create is easier to read Also: SkPDFType3Font are scaled by emSize rather than 1000. GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2231483002 Committed: https://skia.googlesource.com/skia/+/88b138da99328b04cae9a8ee19c3882b8847a550 ========== to ========== SkPDF: Subset Type3 (fallback) font Motivation: significant file-size reduction. Also: SkPDFFont::subsetFont() returns a sk_sp<SkPDFObject> rather than a SkPDFFont*. SkPDFType3Font constructor no longer populates font info; relies on subsetting. SkPDFFont::Create is easier to read Also: SkPDFType3Font are scaled by emSize rather than 1000. GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2231483002 Committed: https://skia.googlesource.com/skia/+/88b138da99328b04cae9a8ee19c3882b8847a550 Committed: https://skia.googlesource.com/skia/+/7e8d5d3519ea2d4c7f158ff9737843e20daad0cb ==========
Message was sent while issue was closed.
Committed patchset #10 (id:240001) as https://skia.googlesource.com/skia/+/7e8d5d3519ea2d4c7f158ff9737843e20daad0cb |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
