Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(3)

Issue 626613002: Fix int16_t for glyphs, const glyphs, and clarify glyph loop. (Closed)

Created:
4 years, 9 months ago by bungeman-skia
Modified:
4 years, 9 months ago
Reviewers:
mtklein
CC:
reviews_skia.org, hal.canary
Base URL:
https://skia.googlesource.com/skia.git@master
Project:
skia
Visibility:
Public.

Description

Fix int16_t for glyphs, const glyphs, and clarify glyph loop. Several places in the PDF code are using int16_t for glyphs. With newer NotoSans fonts, all possible glyph ids are being used, so this can lead to problems. The PDF glyphs from text code returns the text for the glyphs if the encoding is for glyphs. However, it returns this using an unsafe const cast which is hiding possible bugs and preventing correct use of const in other places. The way the glyph loop in SkPDFDevice::drawPosText is written uses a '--i' in the loop, which makes it appear this can loop forever. I don't believe it can, but it is an unecessary code folding. We should also at least assert the forward progress correctness here. Committed: https://skia.googlesource.com/skia/+/22edc8310cd57ab02155bfa6b2ddaf830556bcaf

Patch Set 1 #

Total comments: 5

Patch Set 2 : Address comment. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+22 lines, -19 lines) Patch
M src/pdf/SkPDFDevice.cpp View 1 7 chunks +18 lines, -15 lines 0 comments Download
M src/pdf/SkPDFFont.h View 1 chunk +1 line, -1 line 0 comments Download
M src/pdf/SkPDFFont.cpp View 2 chunks +2 lines, -2 lines 0 comments Download
M src/pdf/SkPDFFontImpl.h View 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 8 (2 generated)
bungeman-skia
Testing this locally, it seems to have the exact same set of issues with dm ...
4 years, 9 months ago (2014-10-02 20:44:37 UTC) #2
mtklein
https://codereview.chromium.org/626613002/diff/1/src/pdf/SkPDFDevice.cpp File src/pdf/SkPDFDevice.cpp (right): https://codereview.chromium.org/626613002/diff/1/src/pdf/SkPDFDevice.cpp#newcode145 src/pdf/SkPDFDevice.cpp:145: *glyphIDs = reinterpret_cast<const uint16_t*>(text); this can be a static_cast<const ...
4 years, 9 months ago (2014-10-02 21:10:11 UTC) #3
bungeman-skia
https://codereview.chromium.org/626613002/diff/1/src/pdf/SkPDFDevice.cpp File src/pdf/SkPDFDevice.cpp (right): https://codereview.chromium.org/626613002/diff/1/src/pdf/SkPDFDevice.cpp#newcode145 src/pdf/SkPDFDevice.cpp:145: *glyphIDs = reinterpret_cast<const uint16_t*>(text); On 2014/10/02 21:10:11, mtklein wrote: ...
4 years, 9 months ago (2014-10-02 22:28:17 UTC) #4
mtklein
lgtm https://codereview.chromium.org/626613002/diff/1/src/pdf/SkPDFDevice.cpp File src/pdf/SkPDFDevice.cpp (right): https://codereview.chromium.org/626613002/diff/1/src/pdf/SkPDFDevice.cpp#newcode1131 src/pdf/SkPDFDevice.cpp:1131: //TODO: the const_cast here is a bug if ...
4 years, 9 months ago (2014-10-03 13:48:58 UTC) #5
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/626613002/20001
4 years, 9 months ago (2014-10-03 14:49:21 UTC) #7
commit-bot: I haz the power
4 years, 9 months ago (2014-10-03 14:56:01 UTC) #8
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as 22edc8310cd57ab02155bfa6b2ddaf830556bcaf

Powered by Google App Engine
This is Rietveld 408576698