Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2278703002:
SkPDF: Glyph validation change (Closed)
|
Created:
4 years, 4 months ago by hal.canary Modified:
4 years, 3 months ago Reviewers:
bungeman-skia CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/skia.git@master Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
DescriptionSkPDF: Glyph validation change
Instead of mapping invaid glyphIDs to zero or maxGlyphID,
don't draw them at all.
Validate glyphs when glyph is written, not ahead of time.
Don't allocate array to copy user-provided glyphs.
Easy early exit from SkPDFDevice::internalDrawText()
GlyphPositioner::flush() called ~GlyphPositioner()
SkScopeExit class now exists.
Assume SkTypeface* pointers are now never null in more
places.
precalculate alignmentFactor to clean up code.
SkPDFDevice::updateFont must be called with validated
glyphID. Skip bad glyphs to make this true.
SkPDFDevice::updateFont always succeeds.
SkPDFFont::GetFontResource always succeeds (preconditions are
asserted). If GetMetrics fails, don't call GetFontResource.
SkPDFFont::glyphsToPDFFontEncodingCount() becomes
SkPDFFont::countStretch() and is inlined.
SkPDFFont::glyphsToPDFFontEncoding now works one Glyph at a
time and is inlined.
SkPDFFont::noteGlyphUsage() operates one glyph at a time.
Add SkScopeExit.h; also a unit test for it.
SkPostConfig: Fix SK_UNUSED for Win32.
No public API changes.
TBR=reed@google.com
BUG=625995
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2278703002
Committed: https://skia.googlesource.com/skia/+/4871f2277738fa7e9232d25424c008b36dae4711
Patch Set 1 #Patch Set 2 : handle error better #Patch Set 3 : major refactor #
Total comments: 20
Patch Set 4 : reply to bungemena@ #Patch Set 5 : fix compile #
Total comments: 6
Patch Set 6 : 3 nits #
Total comments: 8
Patch Set 7 : rename, fix win #Patch Set 8 : unit test for SK_AT_SCOPE_EXIT #
Total comments: 2
Patch Set 9 : fix comment #
Messages
Total messages: 60 (42 generated)
Description was changed from ========== SkPDF: Glyph validation change Instead of mapping invaid glyphIDs to zero or maxGlyphID, don't draw them at all. BUG=625995 ========== to ========== SkPDF: Glyph validation change Instead of mapping invaid glyphIDs to zero or maxGlyphID, don't draw them at all. BUG=625995 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2278703002 ==========
The CQ bit was checked by halcanary@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Note for Reviewers: The CQ is waiting for an approval. If you believe that the CL is not ready yet, or if you would like to L-G-T-M with comments then please uncheck the CQ checkbox. Waiting for LGTM from valid reviewer(s) till 2016-08-25 02:39 UTC
halcanary@google.com changed reviewers: + bungeman@google.com
PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Build-Ubuntu-GCC-x86_64-Release-GN-Trybot on master.client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-x86...)
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_64-Release-Vulkan-Trybot on master.client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86_6...)
Patchset #4 (id:60001) has been deleted
Patchset #3 (id:40001) 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: Glyph validation change Instead of mapping invaid glyphIDs to zero or maxGlyphID, don't draw them at all. BUG=625995 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2278703002 ========== to ========== SkPDF: Glyph validation change Instead of mapping invaid glyphIDs to zero or maxGlyphID, don't draw them at all. Validate glyphs when glyph is written, not ahead of time. Don't allocate array to copy user-provided glyphs. Easy early exit from SkPDFDevice::internalDrawText() GlyphPositioner::flush() called ~GlyphPositioner() TextObject class now exists. SkPDFFont::GetTypefaceInfo as central place to validate Typefaces. Call it early for early exit. Assume SkTypeface* pointers are now never null in more places. precalculate alignmentFactor to clean up code. SkPDFDevice::updateFont must be called with validated glyphID. Skip bad glyphs to make this true. SkPDFDevice::updateFont always succeeds. SkPDFFont::GetFontResource always succeeds (preconditions are asserted). SkPDFFont::glyphsToPDFFontEncodingCount() becomes SkPDFFont::countStretch() and is inlined. SkPDFFont::glyphsToPDFFontEncoding now works one Glyph at a time and is inlined. SkPDFFont::noteGlyphUsage() operates one glyph at a time. BUG=625995 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2278703002 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Ubuntu-GCC-Mips-Debug-Android-Trybot on master.client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Mip...)
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.
take another look
https://codereview.chromium.org/2278703002/diff/80001/src/pdf/SkPDFDevice.cpp File src/pdf/SkPDFDevice.cpp (right): https://codereview.chromium.org/2278703002/diff/80001/src/pdf/SkPDFDevice.cpp... src/pdf/SkPDFDevice.cpp:975: ~GlyphPositioner() {this->flush(); } nit: probably want a space between '{' and 'this'. https://codereview.chromium.org/2278703002/diff/80001/src/pdf/SkPDFDevice.cpp... src/pdf/SkPDFDevice.cpp:994: SkScalar yPosition = xy.y() - fCurrentMatrixY; This handling seems almost painful to read. Is there any reason why fCurrentMatrixX/Y and x/yPosition aren't also SkPoint? SkPoint::operator- exists as expected to do exactly this sort of operation. https://codereview.chromium.org/2278703002/diff/80001/src/pdf/SkPDFDevice.cpp... src/pdf/SkPDFDevice.cpp:1037: device->getCanon())) { nit: Since you're touching this line anyway, will it all fit on one line now? https://codereview.chromium.org/2278703002/diff/80001/src/pdf/SkPDFDevice.cpp... src/pdf/SkPDFDevice.cpp:1078: } // namespace It seems that this struct is only used by internalDrawText. Can we just declare/define it inside that method? https://codereview.chromium.org/2278703002/diff/80001/src/pdf/SkPDFDevice.cpp... src/pdf/SkPDFDevice.cpp:1167: TextObject textObject(out); // Must be in scope for this->updateFont(). I'm not sure this name clearly conveys the side effects. Something like 'ScopedTextObject' maybe? This would actually be a good place to use mtklein's proposed sk_at_scope_exit, since this isn't actually handling any resources at all. out->writeText("BT\n"); sk_at_scope_exit(out->writeText("ET\n")); https://codereview.chromium.org/2278703002/diff/80001/src/pdf/SkPDFFont.cpp File src/pdf/SkPDFFont.cpp (right): https://codereview.chromium.org/2278703002/diff/80001/src/pdf/SkPDFFont.cpp#n... src/pdf/SkPDFFont.cpp:150: // returns null only when typeface is bad /** Returns .... */ https://codereview.chromium.org/2278703002/diff/80001/src/pdf/SkPDFFont.cpp#n... src/pdf/SkPDFFont.cpp:157: sk_sp<SkTypeface> defaultFace; not used anymore? https://codereview.chromium.org/2278703002/diff/80001/src/pdf/SkPDFFont.cpp#n... src/pdf/SkPDFFont.cpp:226: 254 + (int)subsetCode)); maybe fits on one line now? https://codereview.chromium.org/2278703002/diff/80001/src/pdf/SkPDFFont.h File src/pdf/SkPDFFont.h (right): https://codereview.chromium.org/2278703002/diff/80001/src/pdf/SkPDFFont.h#new... src/pdf/SkPDFFont.h:85: SkASSERT(!this->multiByteGlyphs()); This seems like a really odd thing to put here. Why not just if (this->multiByteGlyphs) { return numGlyphs; } This would also mean you wouldn't need the get_stretch function in SkPDFDevice.cpp. With this assert, you need to document a really constraining pre-condition. https://codereview.chromium.org/2278703002/diff/80001/src/pdf/SkPDFFont.h#new... src/pdf/SkPDFFont.h:106: * @param typeface The typeface to find, non-NULL. nit: what's NULL? Oh, it's that old name for nullptr.
Patchset #4 (id:100001) 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...
https://codereview.chromium.org/2278703002/diff/80001/src/pdf/SkPDFDevice.cpp File src/pdf/SkPDFDevice.cpp (right): https://codereview.chromium.org/2278703002/diff/80001/src/pdf/SkPDFDevice.cpp... src/pdf/SkPDFDevice.cpp:975: ~GlyphPositioner() {this->flush(); } On 2016/08/25 19:28:43, bungeman-skia wrote: > nit: probably want a space between '{' and 'this'. Done. And removed the extra space before '}' https://codereview.chromium.org/2278703002/diff/80001/src/pdf/SkPDFDevice.cpp... src/pdf/SkPDFDevice.cpp:994: SkScalar yPosition = xy.y() - fCurrentMatrixY; On 2016/08/25 19:28:43, bungeman-skia wrote: > This handling seems almost painful to read. Is there any reason why > fCurrentMatrixX/Y and x/yPosition aren't also SkPoint? SkPoint::operator- exists > as expected to do exactly this sort of operation. Excellent point. Done. I also used SkPoint::operator!=() https://codereview.chromium.org/2278703002/diff/80001/src/pdf/SkPDFDevice.cpp... src/pdf/SkPDFDevice.cpp:1037: device->getCanon())) { On 2016/08/25 19:28:43, bungeman-skia wrote: > nit: Since you're touching this line anyway, will it all fit on one line now? Done. https://codereview.chromium.org/2278703002/diff/80001/src/pdf/SkPDFDevice.cpp... src/pdf/SkPDFDevice.cpp:1078: } // namespace On 2016/08/25 19:28:43, bungeman-skia wrote: > It seems that this struct is only used by internalDrawText. Can we just > declare/define it inside that method? Done. https://codereview.chromium.org/2278703002/diff/80001/src/pdf/SkPDFDevice.cpp... src/pdf/SkPDFDevice.cpp:1167: TextObject textObject(out); // Must be in scope for this->updateFont(). On 2016/08/25 19:28:43, bungeman-skia wrote: > I'm not sure this name clearly conveys the side effects. Something like > 'ScopedTextObject' maybe? > > This would actually be a good place to use mtklein's proposed sk_at_scope_exit, > since this isn't actually handling any resources at all. > > out->writeText("BT\n"); > sk_at_scope_exit(out->writeText("ET\n")); Done. https://codereview.chromium.org/2278703002/diff/80001/src/pdf/SkPDFFont.cpp File src/pdf/SkPDFFont.cpp (right): https://codereview.chromium.org/2278703002/diff/80001/src/pdf/SkPDFFont.cpp#n... src/pdf/SkPDFFont.cpp:150: // returns null only when typeface is bad On 2016/08/25 19:28:44, bungeman-skia wrote: > /** Returns .... */ Done. https://codereview.chromium.org/2278703002/diff/80001/src/pdf/SkPDFFont.cpp#n... src/pdf/SkPDFFont.cpp:157: sk_sp<SkTypeface> defaultFace; On 2016/08/25 19:28:43, bungeman-skia wrote: > not used anymore? Done. https://codereview.chromium.org/2278703002/diff/80001/src/pdf/SkPDFFont.cpp#n... src/pdf/SkPDFFont.cpp:226: 254 + (int)subsetCode)); On 2016/08/25 19:28:43, bungeman-skia wrote: > maybe fits on one line now? Done. https://codereview.chromium.org/2278703002/diff/80001/src/pdf/SkPDFFont.h File src/pdf/SkPDFFont.h (right): https://codereview.chromium.org/2278703002/diff/80001/src/pdf/SkPDFFont.h#new... src/pdf/SkPDFFont.h:85: SkASSERT(!this->multiByteGlyphs()); On 2016/08/25 19:28:44, bungeman-skia wrote: > This seems like a really odd thing to put here. Why not just > > if (this->multiByteGlyphs) { return numGlyphs; } > > This would also mean you wouldn't need the get_stretch function in > SkPDFDevice.cpp. With this assert, you need to document a really constraining > pre-condition. Done. https://codereview.chromium.org/2278703002/diff/80001/src/pdf/SkPDFFont.h#new... src/pdf/SkPDFFont.h:106: * @param typeface The typeface to find, non-NULL. On 2016/08/25 19:28:44, bungeman-skia wrote: > nit: what's NULL? Oh, it's that old name for nullptr. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Ubuntu-Clang-x86_64-Debug-GN-Trybot on master.client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-Clang-x...)
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.
https://codereview.chromium.org/2278703002/diff/140001/src/pdf/SkPDFDevice.cpp File src/pdf/SkPDFDevice.cpp (right): https://codereview.chromium.org/2278703002/diff/140001/src/pdf/SkPDFDevice.cp... src/pdf/SkPDFDevice.cpp:1070: SK_MACRO_APPEND_LINE(at_scope_exit_){std::move(fn)} It would be nice for this to look more like https://codereview.chromium.org/2274863002/diff/40001/include/private/SkTempl... with a sk_make_scope_exit to infer the type. https://codereview.chromium.org/2278703002/diff/140001/src/pdf/SkPDFDevice.cp... src/pdf/SkPDFDevice.cpp:1157: while (glyphs[index] > maxGlyphID) { // Invalid glyphID for this font. Should this while loop go before writing out "BT"? https://codereview.chromium.org/2278703002/diff/140001/src/pdf/SkPDFDevice.cp... src/pdf/SkPDFDevice.cpp:1211: : SkPoint{pos[index], 0}; nit: probably the silliest nit ever, but it makes me feel better when the '=', '?', and ':' all end up in the same column.
Patchset #6 (id:160001) has been deleted
The CQ bit was checked by halcanary@google.com to run a CQ dry run
https://codereview.chromium.org/2278703002/diff/140001/src/pdf/SkPDFDevice.cpp File src/pdf/SkPDFDevice.cpp (right): https://codereview.chromium.org/2278703002/diff/140001/src/pdf/SkPDFDevice.cp... src/pdf/SkPDFDevice.cpp:1070: SK_MACRO_APPEND_LINE(at_scope_exit_){std::move(fn)} On 2016/08/26 17:47:36, bungeman-skia wrote: > It would be nice for this to look more like > > https://codereview.chromium.org/2274863002/diff/40001/include/private/SkTempl... > > with a sk_make_scope_exit to infer the type. Done. https://codereview.chromium.org/2278703002/diff/140001/src/pdf/SkPDFDevice.cp... src/pdf/SkPDFDevice.cpp:1157: while (glyphs[index] > maxGlyphID) { // Invalid glyphID for this font. On 2016/08/26 17:47:36, bungeman-skia wrote: > Should this while loop go before writing out "BT"? Done. Good catch. https://codereview.chromium.org/2278703002/diff/140001/src/pdf/SkPDFDevice.cp... src/pdf/SkPDFDevice.cpp:1211: : SkPoint{pos[index], 0}; On 2016/08/26 17:47:36, bungeman-skia wrote: > nit: probably the silliest nit ever, but it makes me feel better when the '=', > '?', and ':' all end up in the same column. Done.
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_64-Debug-Trybot on master.client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86_6...)
https://codereview.chromium.org/2278703002/diff/180001/src/pdf/SkPDFFont.cpp File src/pdf/SkPDFFont.cpp (right): https://codereview.chromium.org/2278703002/diff/180001/src/pdf/SkPDFFont.cpp#... src/pdf/SkPDFFont.cpp:159: // Cache nullptr to skip this check. use SkSafeUnref. s/use/Use since there is a period at the end https://codereview.chromium.org/2278703002/diff/180001/src/pdf/SkScoped.h File src/pdf/SkScoped.h (right): https://codereview.chromium.org/2278703002/diff/180001/src/pdf/SkScoped.h#new... src/pdf/SkScoped.h:24: class SkScoped { SkScopeExit https://codereview.chromium.org/2278703002/diff/180001/src/pdf/SkScoped.h#new... src/pdf/SkScoped.h:39: inline SkScoped<Fn> SkMakeScoped(Fn&& fn) { SkMakeScopeExit https://codereview.chromium.org/2278703002/diff/180001/src/pdf/SkScoped.h#new... src/pdf/SkScoped.h:43: #define sk_at_scope_end(stmt) \ SK_AT_SCOPE_EXIT https://codereview.chromium.org/2278703002/diff/180001/src/pdf/SkScoped.h#new... src/pdf/SkScoped.h:44: SK_UNUSED auto&& SK_MACRO_APPEND_LINE(at_scope_exit_) = \ You'll also need https://codereview.chromium.org/2274863002/diff/40001/include/core/SkPostConf... to get SK_UNUSED hooked up on msvc.
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.
https://codereview.chromium.org/2278703002/diff/180001/src/pdf/SkPDFFont.cpp File src/pdf/SkPDFFont.cpp (right): https://codereview.chromium.org/2278703002/diff/180001/src/pdf/SkPDFFont.cpp#... src/pdf/SkPDFFont.cpp:159: // Cache nullptr to skip this check. use SkSafeUnref. On 2016/08/26 18:24:42, bungeman-skia wrote: > s/use/Use > > since there is a period at the end Done. https://codereview.chromium.org/2278703002/diff/180001/src/pdf/SkScoped.h File src/pdf/SkScoped.h (right): https://codereview.chromium.org/2278703002/diff/180001/src/pdf/SkScoped.h#new... src/pdf/SkScoped.h:24: class SkScoped { On 2016/08/26 18:24:42, bungeman-skia wrote: > SkScopeExit Done. https://codereview.chromium.org/2278703002/diff/180001/src/pdf/SkScoped.h#new... src/pdf/SkScoped.h:39: inline SkScoped<Fn> SkMakeScoped(Fn&& fn) { On 2016/08/26 18:24:42, bungeman-skia wrote: > SkMakeScopeExit Done.
Description was changed from
==========
SkPDF: Glyph validation change
Instead of mapping invaid glyphIDs to zero or maxGlyphID,
don't draw them at all.
Validate glyphs when glyph is written, not ahead of time.
Don't allocate array to copy user-provided glyphs.
Easy early exit from SkPDFDevice::internalDrawText()
GlyphPositioner::flush() called ~GlyphPositioner()
TextObject class now exists.
SkPDFFont::GetTypefaceInfo as central place to validate
Typefaces. Call it early for early exit.
Assume SkTypeface* pointers are now never null in more
places.
precalculate alignmentFactor to clean up code.
SkPDFDevice::updateFont must be called with validated
glyphID. Skip bad glyphs to make this true.
SkPDFDevice::updateFont always succeeds.
SkPDFFont::GetFontResource always succeeds (preconditions are
asserted).
SkPDFFont::glyphsToPDFFontEncodingCount() becomes
SkPDFFont::countStretch() and is inlined.
SkPDFFont::glyphsToPDFFontEncoding now works one Glyph at a
time and is inlined.
SkPDFFont::noteGlyphUsage() operates one glyph at a time.
BUG=625995
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2278703002
==========
to
==========
SkPDF: Glyph validation change
Instead of mapping invaid glyphIDs to zero or maxGlyphID,
don't draw them at all.
Validate glyphs when glyph is written, not ahead of time.
Don't allocate array to copy user-provided glyphs.
Easy early exit from SkPDFDevice::internalDrawText()
GlyphPositioner::flush() called ~GlyphPositioner()
TextObject class now exists.
SkPDFFont::GetTypefaceInfo as central place to validate
Typefaces. Call it early for early exit.
Assume SkTypeface* pointers are now never null in more
places.
precalculate alignmentFactor to clean up code.
SkPDFDevice::updateFont must be called with validated
glyphID. Skip bad glyphs to make this true.
SkPDFDevice::updateFont always succeeds.
SkPDFFont::GetFontResource always succeeds (preconditions are
asserted).
SkPDFFont::glyphsToPDFFontEncodingCount() becomes
SkPDFFont::countStretch() and is inlined.
SkPDFFont::glyphsToPDFFontEncoding now works one Glyph at a
time and is inlined.
SkPDFFont::noteGlyphUsage() operates one glyph at a time.
No public API changes.
TBR=reed@google.com
BUG=625995
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2278703002
==========
Description was changed from
==========
SkPDF: Glyph validation change
Instead of mapping invaid glyphIDs to zero or maxGlyphID,
don't draw them at all.
Validate glyphs when glyph is written, not ahead of time.
Don't allocate array to copy user-provided glyphs.
Easy early exit from SkPDFDevice::internalDrawText()
GlyphPositioner::flush() called ~GlyphPositioner()
TextObject class now exists.
SkPDFFont::GetTypefaceInfo as central place to validate
Typefaces. Call it early for early exit.
Assume SkTypeface* pointers are now never null in more
places.
precalculate alignmentFactor to clean up code.
SkPDFDevice::updateFont must be called with validated
glyphID. Skip bad glyphs to make this true.
SkPDFDevice::updateFont always succeeds.
SkPDFFont::GetFontResource always succeeds (preconditions are
asserted).
SkPDFFont::glyphsToPDFFontEncodingCount() becomes
SkPDFFont::countStretch() and is inlined.
SkPDFFont::glyphsToPDFFontEncoding now works one Glyph at a
time and is inlined.
SkPDFFont::noteGlyphUsage() operates one glyph at a time.
No public API changes.
TBR=reed@google.com
BUG=625995
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2278703002
==========
to
==========
SkPDF: Glyph validation change
Instead of mapping invaid glyphIDs to zero or maxGlyphID,
don't draw them at all.
Validate glyphs when glyph is written, not ahead of time.
Don't allocate array to copy user-provided glyphs.
Easy early exit from SkPDFDevice::internalDrawText()
GlyphPositioner::flush() called ~GlyphPositioner()
SkScopeExit class now exists.
Assume SkTypeface* pointers are now never null in more
places.
precalculate alignmentFactor to clean up code.
SkPDFDevice::updateFont must be called with validated
glyphID. Skip bad glyphs to make this true.
SkPDFDevice::updateFont always succeeds.
SkPDFFont::GetFontResource always succeeds (preconditions are
asserted). If GetMetrics fails, don't call GetFontResource.
SkPDFFont::glyphsToPDFFontEncodingCount() becomes
SkPDFFont::countStretch() and is inlined.
SkPDFFont::glyphsToPDFFontEncoding now works one Glyph at a
time and is inlined.
SkPDFFont::noteGlyphUsage() operates one glyph at a time.
Add SkScopeExit.h; fix SK_UNUSED
No public API changes.
TBR=reed@google.com
BUG=625995
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2278703002
==========
Description was changed from
==========
SkPDF: Glyph validation change
Instead of mapping invaid glyphIDs to zero or maxGlyphID,
don't draw them at all.
Validate glyphs when glyph is written, not ahead of time.
Don't allocate array to copy user-provided glyphs.
Easy early exit from SkPDFDevice::internalDrawText()
GlyphPositioner::flush() called ~GlyphPositioner()
SkScopeExit class now exists.
Assume SkTypeface* pointers are now never null in more
places.
precalculate alignmentFactor to clean up code.
SkPDFDevice::updateFont must be called with validated
glyphID. Skip bad glyphs to make this true.
SkPDFDevice::updateFont always succeeds.
SkPDFFont::GetFontResource always succeeds (preconditions are
asserted). If GetMetrics fails, don't call GetFontResource.
SkPDFFont::glyphsToPDFFontEncodingCount() becomes
SkPDFFont::countStretch() and is inlined.
SkPDFFont::glyphsToPDFFontEncoding now works one Glyph at a
time and is inlined.
SkPDFFont::noteGlyphUsage() operates one glyph at a time.
Add SkScopeExit.h; fix SK_UNUSED
No public API changes.
TBR=reed@google.com
BUG=625995
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2278703002
==========
to
==========
SkPDF: Glyph validation change
Instead of mapping invaid glyphIDs to zero or maxGlyphID,
don't draw them at all.
Validate glyphs when glyph is written, not ahead of time.
Don't allocate array to copy user-provided glyphs.
Easy early exit from SkPDFDevice::internalDrawText()
GlyphPositioner::flush() called ~GlyphPositioner()
SkScopeExit class now exists.
Assume SkTypeface* pointers are now never null in more
places.
precalculate alignmentFactor to clean up code.
SkPDFDevice::updateFont must be called with validated
glyphID. Skip bad glyphs to make this true.
SkPDFDevice::updateFont always succeeds.
SkPDFFont::GetFontResource always succeeds (preconditions are
asserted). If GetMetrics fails, don't call GetFontResource.
SkPDFFont::glyphsToPDFFontEncodingCount() becomes
SkPDFFont::countStretch() and is inlined.
SkPDFFont::glyphsToPDFFontEncoding now works one Glyph at a
time and is inlined.
SkPDFFont::noteGlyphUsage() operates one glyph at a time.
Add SkScopeExit.h; also a unit test for it.
SkPostConfig: Fix SK_UNUSED for Win32.
No public API changes.
TBR=reed@google.com
BUG=625995
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2278703002
==========
unit test for SK_AT_SCOPE_EXIT added. description updated
https://codereview.chromium.org/2278703002/diff/220001/src/pdf/SkScopeExit.h File src/pdf/SkScopeExit.h (right): https://codereview.chromium.org/2278703002/diff/220001/src/pdf/SkScopeExit.h#... src/pdf/SkScopeExit.h:14: * sk_at_scope_end(stmt) evaluates stmt when the current scope ends. SK_AT_SCOPE_EXIT
On 2016/08/26 19:47:05, bungeman-skia wrote: > https://codereview.chromium.org/2278703002/diff/220001/src/pdf/SkScopeExit.h > File src/pdf/SkScopeExit.h (right): > > https://codereview.chromium.org/2278703002/diff/220001/src/pdf/SkScopeExit.h#... > src/pdf/SkScopeExit.h:14: * sk_at_scope_end(stmt) evaluates stmt when the > current scope ends. > SK_AT_SCOPE_EXIT done
https://codereview.chromium.org/2278703002/diff/220001/src/pdf/SkScopeExit.h File src/pdf/SkScopeExit.h (right): https://codereview.chromium.org/2278703002/diff/220001/src/pdf/SkScopeExit.h#... src/pdf/SkScopeExit.h:14: * sk_at_scope_end(stmt) evaluates stmt when the current scope ends. On 2016/08/26 19:47:05, bungeman-skia wrote: > SK_AT_SCOPE_EXIT Done.
The CQ bit was checked by halcanary@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
Message was sent while issue was closed.
Description was changed from
==========
SkPDF: Glyph validation change
Instead of mapping invaid glyphIDs to zero or maxGlyphID,
don't draw them at all.
Validate glyphs when glyph is written, not ahead of time.
Don't allocate array to copy user-provided glyphs.
Easy early exit from SkPDFDevice::internalDrawText()
GlyphPositioner::flush() called ~GlyphPositioner()
SkScopeExit class now exists.
Assume SkTypeface* pointers are now never null in more
places.
precalculate alignmentFactor to clean up code.
SkPDFDevice::updateFont must be called with validated
glyphID. Skip bad glyphs to make this true.
SkPDFDevice::updateFont always succeeds.
SkPDFFont::GetFontResource always succeeds (preconditions are
asserted). If GetMetrics fails, don't call GetFontResource.
SkPDFFont::glyphsToPDFFontEncodingCount() becomes
SkPDFFont::countStretch() and is inlined.
SkPDFFont::glyphsToPDFFontEncoding now works one Glyph at a
time and is inlined.
SkPDFFont::noteGlyphUsage() operates one glyph at a time.
Add SkScopeExit.h; also a unit test for it.
SkPostConfig: Fix SK_UNUSED for Win32.
No public API changes.
TBR=reed@google.com
BUG=625995
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2278703002
==========
to
==========
SkPDF: Glyph validation change
Instead of mapping invaid glyphIDs to zero or maxGlyphID,
don't draw them at all.
Validate glyphs when glyph is written, not ahead of time.
Don't allocate array to copy user-provided glyphs.
Easy early exit from SkPDFDevice::internalDrawText()
GlyphPositioner::flush() called ~GlyphPositioner()
SkScopeExit class now exists.
Assume SkTypeface* pointers are now never null in more
places.
precalculate alignmentFactor to clean up code.
SkPDFDevice::updateFont must be called with validated
glyphID. Skip bad glyphs to make this true.
SkPDFDevice::updateFont always succeeds.
SkPDFFont::GetFontResource always succeeds (preconditions are
asserted). If GetMetrics fails, don't call GetFontResource.
SkPDFFont::glyphsToPDFFontEncodingCount() becomes
SkPDFFont::countStretch() and is inlined.
SkPDFFont::glyphsToPDFFontEncoding now works one Glyph at a
time and is inlined.
SkPDFFont::noteGlyphUsage() operates one glyph at a time.
Add SkScopeExit.h; also a unit test for it.
SkPostConfig: Fix SK_UNUSED for Win32.
No public API changes.
TBR=reed@google.com
BUG=625995
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2278703002
Committed:
https://skia.googlesource.com/skia/+/4871f2277738fa7e9232d25424c008b36dae4711
==========
Message was sent while issue was closed.
Committed patchset #9 (id:240001) as https://skia.googlesource.com/skia/+/4871f2277738fa7e9232d25424c008b36dae4711 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
