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.

Review URL: https://codereview.chromium.org/626613002

4 files changed, 22 insertions, 19 deletions

diff --git a/src/pdf/SkPDFDevice.cpp b/src/pdf/SkPDFDevice.cpp
index bd9b1d7f7e..564fdc2123 100644
--- a/src/pdf/SkPDFDevice.cpp
+++ b/src/pdf/SkPDFDevice.cpp
@@ -118,7 +118,7 @@ typedef SkAutoSTMalloc<128, uint16_t> SkGlyphStorage;
 
 static int force_glyph_encoding(const SkPaint& paint, const void* text,
                                 size_t len, SkGlyphStorage* storage,
-                                uint16_t** glyphIDs) {
+                                const uint16_t** glyphIDs) {
     // Make sure we have a glyph id encoding.
     if (paint.getTextEncoding() != SkPaint::kGlyphID_TextEncoding) {
         int numGlyphs = paint.textToGlyphs(text, len, NULL);
@@ -131,8 +131,7 @@ static int force_glyph_encoding(const SkPaint& paint, const void* text,
     // For user supplied glyph ids we need to validate them.
     SkASSERT((len & 1) == 0);
     int numGlyphs = SkToInt(len / 2);
-    const uint16_t* input =
-        reinterpret_cast<uint16_t*>(const_cast<void*>((text)));
+    const uint16_t* input = static_cast<const uint16_t*>(text);
 
     int maxGlyphID = max_glyphid_for_typeface(paint.getTypeface());
     int validated;
@@ -142,7 +141,7 @@ static int force_glyph_encoding(const SkPaint& paint, const void* text,
         }
     }
     if (validated >= numGlyphs) {
-        *glyphIDs = reinterpret_cast<uint16_t*>(const_cast<void*>((text)));
+        *glyphIDs = static_cast<const uint16_t*>(text);
         return numGlyphs;
     }
 
@@ -1115,7 +1114,7 @@ void SkPDFDevice::drawText(const SkDraw& d, const void* text, size_t len,
     }
 
     SkGlyphStorage storage(0);
-    uint16_t* glyphIDs = NULL;
+    const uint16_t* glyphIDs = NULL;
     int numGlyphs = force_glyph_encoding(paint, text, len, &storage, &glyphIDs);
     textPaint.setTextEncoding(SkPaint::kGlyphID_TextEncoding);
 
@@ -1128,8 +1127,9 @@ void SkPDFDevice::drawText(const SkDraw& d, const void* text, size_t len,
     while (numGlyphs > consumedGlyphCount) {
         updateFont(textPaint, glyphIDs[consumedGlyphCount], content.entry());
         SkPDFFont* font = content.entry()->fState.fFont;
+        //TODO: the const_cast here is a bug if the encoding started out as glyph encoding.
         int availableGlyphs =
-            font->glyphsToPDFFontEncoding(glyphIDs + consumedGlyphCount,
+            font->glyphsToPDFFontEncoding(const_cast<uint16_t*>(glyphIDs) + consumedGlyphCount,
                                           numGlyphs - consumedGlyphCount);
         fFontGlyphUsage->noteGlyphUsage(font, glyphIDs + consumedGlyphCount,
                                         availableGlyphs);
@@ -1160,9 +1160,8 @@ void SkPDFDevice::drawPosText(const SkDraw& d, const void* text, size_t len,
     }
 
     SkGlyphStorage storage(0);
-    uint16_t* glyphIDs = NULL;
-    size_t numGlyphs = force_glyph_encoding(paint, text, len, &storage,
-                                            &glyphIDs);
+    const uint16_t* glyphIDs = NULL;
+    size_t numGlyphs = force_glyph_encoding(paint, text, len, &storage, &glyphIDs);
     textPaint.setTextEncoding(SkPaint::kGlyphID_TextEncoding);
 
     SkDrawCacheProc glyphCacheProc = textPaint.getDrawCacheProc();
@@ -1172,20 +1171,24 @@ void SkPDFDevice::drawPosText(const SkDraw& d, const void* text, size_t len,
         SkPDFFont* font = content.entry()->fState.fFont;
         uint16_t encodedValue = glyphIDs[i];
         if (font->glyphsToPDFFontEncoding(&encodedValue, 1) != 1) {
+            // The current pdf font cannot encode the current glyph.
+            // Try to get a pdf font which can encode the current glyph.
             updateFont(textPaint, glyphIDs[i], content.entry());
-            i--;
-            continue;
+            font = content.entry()->fState.fFont;
+            if (font->glyphsToPDFFontEncoding(&encodedValue, 1) != 1) {
+                SkDEBUGFAIL("PDF could not encode glyph.");
+                continue;
+            }
         }
+
         fFontGlyphUsage->noteGlyphUsage(font, &encodedValue, 1);
         SkScalar x = offset.x() + pos[i * scalarsPerPos];
         SkScalar y = offset.y() + (2 == scalarsPerPos ? pos[i * scalarsPerPos + 1] : 0);
 
         align_text(glyphCacheProc, textPaint, glyphIDs + i, 1, &x, &y);
-        set_text_transform(x, y, textPaint.getTextSkewX(),
-                           &content.entry()->fContent);
+        set_text_transform(x, y, textPaint.getTextSkewX(), &content.entry()->fContent);
         SkString encodedString =
-            SkPDFString::FormatString(&encodedValue, 1,
-                                      font->multiByteGlyphs());
+            SkPDFString::FormatString(&encodedValue, 1, font->multiByteGlyphs());
         content.entry()->fContent.writeText(encodedString.c_str());
         content.entry()->fContent.writeText(" Tj\n");
     }
diff --git a/src/pdf/SkPDFFont.cpp b/src/pdf/SkPDFFont.cpp
index b4e6d98722..7d8b29bfc5 100644
--- a/src/pdf/SkPDFFont.cpp
+++ b/src/pdf/SkPDFFont.cpp
@@ -1027,7 +1027,7 @@ bool SkPDFFont::addCommonFontDescriptorEntries(int16_t defaultWidth) {
     return true;
 }
 
-void SkPDFFont::adjustGlyphRangeForSingleByteEncoding(int16_t glyphID) {
+void SkPDFFont::adjustGlyphRangeForSingleByteEncoding(uint16_t glyphID) {
     // Single byte glyph encoding supports a max of 255 glyphs.
     fFirstGlyphID = glyphID - (glyphID - 1) % 255;
     if (fLastGlyphID > fFirstGlyphID + 255 - 1) {
@@ -1424,7 +1424,7 @@ SkPDFType3Font::SkPDFType3Font(const SkAdvancedTypefaceMetrics* info,
 
 SkPDFType3Font::~SkPDFType3Font() {}
 
-bool SkPDFType3Font::populate(int16_t glyphID) {
+bool SkPDFType3Font::populate(uint16_t glyphID) {
     SkPaint paint;
     paint.setTypeface(typeface());
     paint.setTextSize(1000);
diff --git a/src/pdf/SkPDFFont.h b/src/pdf/SkPDFFont.h
index 48d9c30b81..27f1b5bfe3 100644
--- a/src/pdf/SkPDFFont.h
+++ b/src/pdf/SkPDFFont.h
@@ -164,7 +164,7 @@ protected:
     /** Set fFirstGlyphID and fLastGlyphID to span at most 255 glyphs,
      *  including the passed glyphID.
      */
-    void adjustGlyphRangeForSingleByteEncoding(int16_t glyphID);
+    void adjustGlyphRangeForSingleByteEncoding(uint16_t glyphID);
 
     // Generate ToUnicode table according to glyph usage subset.
     // If subset is NULL, all available glyph ids will be used.
diff --git a/src/pdf/SkPDFFontImpl.h b/src/pdf/SkPDFFontImpl.h
index 99f1941a8c..842e1c9547 100644
--- a/src/pdf/SkPDFFontImpl.h
+++ b/src/pdf/SkPDFFontImpl.h
@@ -78,7 +78,7 @@ private:
     SkPDFType3Font(const SkAdvancedTypefaceMetrics* info,
                    SkTypeface* typeface, uint16_t glyphID);
 
-    bool populate(int16_t glyphID);
+    bool populate(uint16_t glyphID);
 };
 
 #endif