Cleanup: Refactor some code into its own function in fpdf_text_int.cpp.

core/src/fpdftext/fpdf_text_int.cpp

 // Copyright 2014 PDFium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
  
 // Original code copyright 2014 Foxit Software Inc. http://www.foxitsoftware.com
 
 #include "../../include/fpdfapi/fpdf_resource.h"
 #include "../../include/fpdfapi/fpdf_pageobj.h"
 #include "../../include/fpdftext/fpdf_text.h"
 #include "../../include/fpdfapi/fpdf_page.h"
 #include "../../include/fpdfapi/fpdf_module.h"
 #include <ctype.h>
+#include <algorithm>
 #include "text_int.h"
+
+namespace {
+
 FX_BOOL _IsIgnoreSpaceCharacter(FX_WCHAR curChar)
 {
     if(curChar < 255 ) {
         return FALSE;
     }
     if ( (curChar >= 0x0600 && curChar <= 0x06FF)
             || (curChar >= 0xFE70 && curChar <= 0xFEFF)
             || (curChar >= 0xFB50 && curChar <= 0xFDFF)
             || (curChar >= 0x0400 && curChar <= 0x04FF)
             || (curChar >= 0x0500 && curChar <= 0x052F)
             || (curChar >= 0xA640 && curChar <= 0xA69F)
             || (curChar >= 0x2DE0 && curChar <= 0x2DFF)
             || curChar == 8467
             || (curChar >= 0x2000 && curChar <= 0x206F)) {
         return FALSE;
     }
     return TRUE;
 }
+
+FX_FLOAT _NormalizeThreshold(FX_FLOAT threshold)

[Tom Sepez, 2014/12/18 19:33:36]

Ok, lets just follow their naming convention even if identifiers arting with
underbar capital-letter are reserved for the compiler in the same way __ is.

[Lei Zhang, 2014/12/18 21:07:10]

I'm a bit unclear what the style guide is exactly, so just trying to stay
consistent as much as I can. Maybe we'll just clang-format the whole repo one
day. ;)

+{
+    int nDivide = 6;

[Tom Sepez, 2014/12/18 19:33:36]

Why we need nDivide local?  Why is it int? I realize you're just copying code,
but

if (threshold < 300 {
  return threshold / 2.0
}
...
return threshold / 6.0;

seems clearer.

[Lei Zhang, 2014/12/18 21:07:09]

Done. I was just shuffling code around.

+    if (threshold < 300) {
+        nDivide = 2;
+    } else if (threshold < 500) {
+        nDivide = 4;
+    } else if (threshold < 700) {
+        nDivide = 5;
+    }
+    return threshold / nDivide;
+}
+
+FX_FLOAT _CalculateBaseSpace(const CPDF_TextObject* pTextObj,
+                             const CFX_AffineMatrix& matrix)
+{
+    FX_FLOAT baseSpace = 0.0;
+    const int nItems = pTextObj->CountItems();
+    if (pTextObj->m_TextState.GetObject()->m_CharSpace && nItems >= 3) {
+        FX_BOOL bAllChar = TRUE;
+        FX_FLOAT spacing = matrix.TransformDistance(
+            pTextObj->m_TextState.GetObject()->m_CharSpace);
+        baseSpace = spacing;
+        for (int i = 0; i < nItems; i++) {
+            CPDF_TextObjectItem item;
+            pTextObj->GetItemInfo(i, &item);
+            if (item.m_CharCode == (FX_DWORD) - 1) {
+                FX_FLOAT fontsize_h = pTextObj->m_TextState.GetFontSizeH();
+                FX_FLOAT kerning = -fontsize_h * item.m_OriginX / 1000;
+                baseSpace = std::min(baseSpace, kerning + spacing);
+                bAllChar = FALSE;
+            }
+        }
+        if (baseSpace < 0.0 || (nItems == 3 && !bAllChar)) {
+            baseSpace = 0.0;
+        }
+    }
+    return baseSpace;
+}
+
+}  // namespace
+
 CPDFText_ParseOptions::CPDFText_ParseOptions()
     : m_bGetCharCodeOnly(FALSE), m_bNormalizeObjs(TRUE), m_bOutputHyphen(FALSE)
 {
 }
 IPDF_TextPage* IPDF_TextPage::CreateTextPage(const CPDF_Page* pPage, CPDFText_ParseOptions ParserOptions)
 {
     CPDF_TextPage* pTextPageEx = FX_NEW CPDF_TextPage(pPage, ParserOptions);
     return pTextPageEx;
 }
 IPDF_TextPage* IPDF_TextPage::CreateTextPage(const CPDF_Page* pPage, int flags)
@@ skipping 1593 matching lines @@
     }
     if (FPDFTEXT_MC_DELAY == bPreMKC) {
         ProcessMarkedContent(Obj);
         m_pPreTextObj = pTextObj;
         m_perMatrix.Copy(formMatrix);
         return;
     }
     m_pPreTextObj = pTextObj;
     m_perMatrix.Copy(formMatrix);
     int nItems = pTextObj->CountItems();
-    FX_FLOAT spacing = 0;
-    FX_FLOAT baseSpace = 0.0;
-    FX_BOOL bAllChar = TRUE;
-    if (pTextObj->m_TextState.GetObject()->m_CharSpace && nItems >= 3) {
-        spacing = matrix.TransformDistance(pTextObj->m_TextState.GetObject()->m_CharSpace);
-        baseSpace = spacing;
-        for (int i = 0; i < nItems; i++) {
-            CPDF_TextObjectItem item;
-            pTextObj->GetItemInfo(i, &item);
-            if (item.m_CharCode == (FX_DWORD) - 1) {
-                FX_FLOAT fontsize_h = pTextObj->m_TextState.GetFontSizeH();
-                FX_FLOAT kerning = -fontsize_h * item.m_OriginX / 1000;
-                if(kerning + spacing < baseSpace) {
-                    baseSpace = kerning + spacing;
-                }
-                bAllChar = FALSE;
-            }
-        }
-        spacing = 0;
-        if(baseSpace < 0.0 || (nItems == 3 && !bAllChar)) {
-            baseSpace = 0.0;
-        }
-    }
+    FX_FLOAT baseSpace = _CalculateBaseSpace(pTextObj, matrix);
 
     FX_BOOL bIsBidiAndMirrosInverse = FALSE;
     IFX_BidiChar* BidiChar = IFX_BidiChar::Create();
     FX_INT32 nR2L = 0;
     FX_INT32 nL2R = 0;
     FX_INT32 start = 0, count = 0;
     CPDF_TextObjectItem item;
     for (FX_INT32 i = 0; i < nItems; i++) {
         pTextObj->GetItemInfo(i, &item);
         if (item.m_CharCode == (FX_DWORD)-1) {
@@ skipping 27 matching lines @@
         }
     }
     FX_BOOL bR2L = FALSE;
     if (nR2L > 0 && nR2L >= nL2R) {
         bR2L = TRUE;
     }
     bIsBidiAndMirrosInverse = bR2L && (matrix.a * matrix.d - matrix.b * matrix.c) < 0;
     FX_INT32 iBufStartAppend = m_TempTextBuf.GetLength();
     FX_INT32 iCharListStartAppend = m_TempCharList.GetSize();
 
+    FX_FLOAT spacing = 0;

[Tom Sepez, 2014/12/18 19:33:36]

nit: 0.0 

[Lei Zhang, 2014/12/18 21:07:09]

Does that actually make a difference? There's too many of these everywhere.

     for (int i = 0; i < nItems; i++) {
         CPDF_TextObjectItem item;
         PAGECHAR_INFO charinfo;
         charinfo.m_OriginX = 0;
         charinfo.m_OriginY = 0;
         pTextObj->GetItemInfo(i, &item);
         if (item.m_CharCode == (FX_DWORD) - 1) {
             CFX_WideString str = m_TempTextBuf.GetWideString();
             if(str.IsEmpty()) {
                 str = m_TextBuf.GetWideString();
@@ skipping 22 matching lines @@
             }
             if (threshold > fontsize_h / 3) {
                 threshold = 0;
             } else {
                 threshold /= 2;
             }
             if (threshold == 0) {
                 threshold = fontsize_h;
                 int this_width = FXSYS_abs(GetCharWidth(item.m_CharCode, pFont));
                 threshold = this_width > last_width ? (FX_FLOAT)this_width : (FX_FLOAT)last_width;
-                int nDivide = 6;
-                if (threshold < 300) {
-                    nDivide = 2;
-                } else if (threshold < 500) {
-                    nDivide = 4;
-                } else if (threshold < 700) {
-                    nDivide = 5;
-                }
-                threshold = threshold / nDivide;
+                threshold = _NormalizeThreshold(threshold);
                 threshold = fontsize_h * threshold / 1000;
             }
             if (threshold && (spacing && spacing >= threshold) ) {
                 charinfo.m_Unicode = TEXT_BLANK_CHAR;
                 charinfo.m_Flag = FPDFTEXT_CHAR_GENERATED;
                 charinfo.m_pTextObj = pTextObj;
                 charinfo.m_Index = m_TextBuf.GetLength();
                 m_TempTextBuf.AppendChar(TEXT_BLANK_CHAR);
                 charinfo.m_CharCode = -1;
                 charinfo.m_Matrix.Copy(formMatrix);
@@ skipping 44 matching lines @@
         charinfo.m_Matrix.Copy(matrix);
         if (wstrItem.IsEmpty()) {
             charinfo.m_Unicode = 0;
             m_TempCharList.Add(charinfo);
             m_TempTextBuf.AppendChar(0xfffe);
             continue;
         } else {
             int nTotal = wstrItem.GetLength();
             int n = 0;
             FX_BOOL bDel = FALSE;
-            while (n < m_TempCharList.GetSize() && n < 7) {
+            const int count = std::min(m_TempCharList.GetSize(), 7);
+            while (n < count) {
                 n++;
-                PAGECHAR_INFO* charinfo1 = (PAGECHAR_INFO*)m_TempCharList.GetAt(m_TempCharList.GetSize() - n);
+                int index = m_TempCharList.GetSize() - n;

[Tom Sepez, 2014/12/18 19:33:36]

why not just count down instead? Is n used somewhere I didn't spot?

[Lei Zhang, 2014/12/18 21:07:09]

Done.

+                PAGECHAR_INFO* charinfo1 = (PAGECHAR_INFO*)m_TempCharList.GetAt(index);

[Tom Sepez, 2014/12/18 19:33:36]

Doesn't the first iteration do ... .GetAt(... .GetSize()) which would be out of
bounds even for their custom arrays?  I bet there's a bug lurking here.

[Lei Zhang, 2014/12/18 21:07:10]

No, it's confusing because the first thing we do when we enter the loop is to
increment |n| from 0 to 1. Thus we start with |size() - 1| here.

                 if(charinfo1->m_CharCode == charinfo.m_CharCode &&
                         charinfo1->m_pTextObj->GetFont() == charinfo.m_pTextObj->GetFont()  &&
                         FXSYS_fabs(charinfo1->m_OriginX - charinfo.m_OriginX) < TEXT_CHARRATIO_GAPDELTA * pTextObj->GetFontSize()  &&
                         FXSYS_fabs(charinfo1->m_OriginY - charinfo.m_OriginY) < TEXT_CHARRATIO_GAPDELTA * pTextObj->GetFontSize() ) {
                     bDel = TRUE;
                     break;
                 }
             }
             if(!bDel) {
                 for (int nIndex = 0; nIndex < nTotal; nIndex++) {
@@ skipping 12 matching lines @@
                     m_TempTextBuf.Delete(m_TempTextBuf.GetLength() - 1, 1);
                     m_TempCharList.Delete(m_TempCharList.GetSize() - 1);
                 }
             }
         }
     }
     if (bIsBidiAndMirrosInverse) {
         FX_INT32 i, j;
         i = iCharListStartAppend;
         j = m_TempCharList.GetSize() - 1;
-        PAGECHAR_INFO tempCharInfo;
-        FX_INT32 tempIndex = 0;
         for (; i < j; i++, j--) {
-            tempCharInfo = m_TempCharList[i];
-            m_TempCharList[i] = m_TempCharList[j];
-            m_TempCharList[j] = tempCharInfo;
-            tempIndex = m_TempCharList[i].m_Index;
-            m_TempCharList[i].m_Index = m_TempCharList[j].m_Index;
-            m_TempCharList[j].m_Index = tempIndex;
+            std::swap(m_TempCharList[i], m_TempCharList[j]);
+            std::swap(m_TempCharList[i].m_Index, m_TempCharList[j].m_Index);
         }
         FX_WCHAR * pTempBuffer = m_TempTextBuf.GetBuffer();
         i = iBufStartAppend;
         j = m_TempTextBuf.GetLength() - 1;
         FX_WCHAR wTemp;
         for (; i < j; i++, j--) {
-            wTemp = pTempBuffer[i];
-            pTempBuffer[i] = pTempBuffer[j];
-            pTempBuffer[j] = wTemp;
+            std::swap(pTempBuffer[i], pTempBuffer[j]);
         }
     }
 }
 FX_INT32 CPDF_TextPage::GetTextObjectWritingMode(const CPDF_TextObject* pTextObj)
 {
     FX_INT32 nChars = pTextObj->CountChars();
     if (nChars == 1) {
         return m_TextlineDir;
     }
     CPDF_TextObjectItem first, last;
@@ skipping 937 matching lines @@
     if (!m_IsParserd || index < 0 || index >= m_LinkList.GetSize()) {
         return;
     }
     CPDF_LinkExt* link = NULL;
     link = m_LinkList.GetAt(index);
     if (!link) {
         return ;
     }
     m_pTextPage->GetRectArray(link->m_Start, link->m_Count, rects);
 }