Fix int16_t for glyphs, const glyphs, and clarify glyph loop.

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

[bungeman-skia, 2014-10-02 20:44:37]

bungeman@google.com changed reviewers:
+ mtklein@google.com

[bungeman-skia, 2014-10-02 20:44:37]

Testing this locally, it seems to have the exact same set of issues with dm as
it did before this change.

[mtklein, 2014-10-02 21:10:11]

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#newc...
src/pdf/SkPDFDevice.cpp:145: *glyphIDs = reinterpret_cast<const
uint16_t*>(text);
this can be a static_cast<const uint16_t*>, right?

https://codereview.chromium.org/626613002/diff/1/src/pdf/SkPDFDevice.cpp#newc...
src/pdf/SkPDFDevice.cpp:1131: //TODO: the const_cast here is a bug if the
encoding started out as glyph encoding.
Can we assert it wasn't, or is this really happening?

[bungeman-skia, 2014-10-02 22:28:17]

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#newc...
src/pdf/SkPDFDevice.cpp:145: *glyphIDs = reinterpret_cast<const
uint16_t*>(text);
On 2014/10/02 21:10:11, mtklein wrote:
> this can be a static_cast<const uint16_t*>, right?

Done. Here and above.

https://codereview.chromium.org/626613002/diff/1/src/pdf/SkPDFDevice.cpp#newc...
src/pdf/SkPDFDevice.cpp:1131: //TODO: the const_cast here is a bug if the
encoding started out as glyph encoding.
On 2014/10/02 21:10:11, mtklein wrote:
> Can we assert it wasn't, or is this really happening?

This will still 'work' here, but will mutate the 'const void* text' which was
passed in.

I was going to say that we had no tests for this code path (drawText with glyph
encoding) because if you have glyphs then you've probably already done the work
to figure out where to put them.

However, textBlob is using this, and this code will change the glyphs the
textBlob is pointing at when the font is CFF (will be turned into Type3). This
really needs to be fixed. Writing the assertion here

SkASSERT(glyphIDs != text || font->multiByteGlyphs());

currently fails on my machine, as one of the fonts turns out to be Type1 (Nimbus
Roman). If the blob test verified that it serialized the same before and after
drawing, that test would fail.

[mtklein, 2014-10-03 13:48:58]

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#newc...
src/pdf/SkPDFDevice.cpp:1131: //TODO: the const_cast here is a bug if the
encoding started out as glyph encoding.
On 2014/10/02 22:28:16, bungeman1 wrote:
> On 2014/10/02 21:10:11, mtklein wrote:
> > Can we assert it wasn't, or is this really happening?
> 
> This will still 'work' here, but will mutate the 'const void* text' which was
> passed in.
> 
> I was going to say that we had no tests for this code path (drawText with
glyph
> encoding) because if you have glyphs then you've probably already done the
work
> to figure out where to put them.
> 
> However, textBlob is using this, and this code will change the glyphs the
> textBlob is pointing at when the font is CFF (will be turned into Type3). This
> really needs to be fixed. Writing the assertion here
> 
> SkASSERT(glyphIDs != text || font->multiByteGlyphs());
> 
> currently fails on my machine, as one of the fonts turns out to be Type1
(Nimbus
> Roman). If the blob test verified that it serialized the same before and after
> drawing, that test would fail.

Bloody hell.

[bungeman-skia, 2014-10-03 14:48:48]

The CQ bit was checked by bungeman@google.com

[commit-bot: I haz the power, 2014-10-03 14:49:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/626613002/20001

[commit-bot: I haz the power, 2014-10-03 14:56:01]

Committed patchset #2 (id:20001) as 22edc8310cd57ab02155bfa6b2ddaf830556bcaf