Fix CJS_PublicMethods::IsNumber() with unit test and some cleanup

R=tsepez@chromium.org

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

3 files changed, 73 insertions, 27 deletions

diff --git a/fpdfsdk/javascript/PublicMethods.cpp b/fpdfsdk/javascript/PublicMethods.cpp
index d154fc1766..6097369430 100644
--- a/fpdfsdk/javascript/PublicMethods.cpp
+++ b/fpdfsdk/javascript/PublicMethods.cpp
@@ -62,44 +62,44 @@ static const FX_WCHAR* const fullmonths[] = {
     L"May",       L"June",     L"July",     L"August",
     L"September", L"October",  L"November", L"December"};
 
-FX_BOOL CJS_PublicMethods::IsNumber(const FX_WCHAR* str) {
+bool CJS_PublicMethods::IsNumber(const FX_WCHAR* str) {
   CFX_WideString sTrim = StrTrim(str);
   const FX_WCHAR* pTrim = sTrim.c_str();
   const FX_WCHAR* p = pTrim;
 
-  FX_BOOL bDot = FALSE;
-  FX_BOOL bKXJS = FALSE;
+  bool bDot = false;
+  bool bKXJS = false;
 
   wchar_t c;
-  while ((c = *p)) {
-    if (c == '.' || c == ',') {
+  while ((c = *p) != L'\0') {
+    if (c == L'.' || c == L',') {
       if (bDot)
-        return FALSE;
-      bDot = TRUE;
-    } else if (c == '-' || c == '+') {
+        return false;
+      bDot = true;
+    } else if (c == L'-' || c == L'+') {
       if (p != pTrim)
-        return FALSE;
-    } else if (c == 'e' || c == 'E') {
+        return false;
+    } else if (c == L'e' || c == L'E') {
       if (bKXJS)
-        return FALSE;
+        return false;
 
       p++;
       c = *p;
-      if (c == '+' || c == '-') {
-        bKXJS = TRUE;
+      if (c == L'+' || c == L'-') {
+        bKXJS = true;
       } else {
-        return FALSE;
+        return false;
       }
     } else if (!FXSYS_iswdigit(c)) {
-      return FALSE;
+      return false;
     }
     p++;
   }
 
-  return TRUE;
+  return true;
 }
 
-FX_BOOL CJS_PublicMethods::maskSatisfied(wchar_t c_Change, wchar_t c_Mask) {
+bool CJS_PublicMethods::maskSatisfied(wchar_t c_Change, wchar_t c_Mask) {
   switch (c_Mask) {
     case L'9':
       return FXSYS_iswdigit(c_Change);
@@ -108,13 +108,13 @@ FX_BOOL CJS_PublicMethods::maskSatisfied(wchar_t c_Change, wchar_t c_Mask) {
     case L'O':
       return FXSYS_iswalnum(c_Change);
     case L'X':
-      return TRUE;
+      return true;
     default:
       return (c_Change == c_Mask);
   }
 }
 
-FX_BOOL CJS_PublicMethods::isReservedMaskChar(wchar_t ch) {
+bool CJS_PublicMethods::isReservedMaskChar(wchar_t ch) {
   return ch == L'9' || ch == L'A' || ch == L'O' || ch == L'X';
 }
 
diff --git a/fpdfsdk/javascript/PublicMethods.h b/fpdfsdk/javascript/PublicMethods.h
index 8961c5abc8..16cc5bc21a 100644
--- a/fpdfsdk/javascript/PublicMethods.h
+++ b/fpdfsdk/javascript/PublicMethods.h
@@ -18,7 +18,6 @@ class CJS_PublicMethods : public CJS_Object {
       : CJS_Object(pObject) {}
   ~CJS_PublicMethods() override {}
 
- public:
   static FX_BOOL AFNumber_Format(IJS_Context* cc,
                                  const std::vector<CJS_Value>& params,
                                  CJS_Value& vRet,
@@ -108,7 +107,6 @@ class CJS_PublicMethods : public CJS_Object {
                                CJS_Value& vRet,
                                CFX_WideString& sError);
 
- public:
   JS_STATIC_GLOBAL_FUN(AFNumber_Format);
   JS_STATIC_GLOBAL_FUN(AFNumber_Keystroke);
   JS_STATIC_GLOBAL_FUN(AFPercent_Format);
@@ -134,7 +132,6 @@ class CJS_PublicMethods : public CJS_Object {
 
   JS_STATIC_DECLARE_GLOBAL_FUN();
 
- public:
   static int ParseStringInteger(const CFX_WideString& string,
                                 int nStart,
                                 int& nSkip,
@@ -151,7 +148,6 @@ class CJS_PublicMethods : public CJS_Object {
                                 bool* bWrongFormat);
   static double MakeInterDate(CFX_WideString strValue);
 
- public:
   static CFX_WideString StrLTrim(const FX_WCHAR* pStr);
   static CFX_WideString StrRTrim(const FX_WCHAR* pStr);
   static CFX_WideString StrTrim(const FX_WCHAR* pStr);
@@ -160,11 +156,10 @@ class CJS_PublicMethods : public CJS_Object {
   static CFX_ByteString StrRTrim(const FX_CHAR* pStr);
   static CFX_ByteString StrTrim(const FX_CHAR* pStr);
 
-  static FX_BOOL IsNumber(const FX_CHAR* string);
-  static FX_BOOL IsNumber(const FX_WCHAR* string);
+  static bool IsNumber(const FX_WCHAR* string);
 
-  static FX_BOOL maskSatisfied(wchar_t c_Change, wchar_t c_Mask);
-  static FX_BOOL isReservedMaskChar(wchar_t ch);
+  static bool maskSatisfied(wchar_t c_Change, wchar_t c_Mask);
+  static bool isReservedMaskChar(wchar_t ch);
 
   static double AF_Simple(const FX_WCHAR* sFuction,
                           double dValue1,
diff --git a/fpdfsdk/javascript/public_methods_unittest.cpp b/fpdfsdk/javascript/public_methods_unittest.cpp
new file mode 100644
index 0000000000..ace0920e8f
--- /dev/null
+++ b/fpdfsdk/javascript/public_methods_unittest.cpp
@@ -0,0 +1,51 @@
+// Copyright 2016 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.
+
+#include "fpdfsdk/javascript/PublicMethods.h"
+
+#include "testing/gtest/include/gtest/gtest.h"
+#include "testing/test_support.h"
+
+TEST(CJS_PublicMethods, IsNumber) {
+  // TODO(weili): Check whether results from case 0, 1, 10, 15 are intended.
+  struct {
+    const wchar_t* input;
+    bool expected;
+  } test_data[] = {
+      // Empty string.
+      {L"", true},
+      // Only whitespaces.
+      {L"  ", true},
+      // Content with invalid characters.
+      {L"xyz00", false},
+      {L"1%", false},
+      // Hex string.
+      {L"0x234", false},
+      // Signed numbers.
+      {L"+123", true},
+      {L"-98765", true},
+      // Numbers with whitespaces.
+      {L"  345 ", true},
+      // Float numbers.
+      {L"-1e5", false},
+      {L"-2e", false},
+      {L"e-5", true},
+      {L"0.023", true},
+      {L".356089", true},
+      {L"1e-9", true},
+      {L"-1.23e+23", true},
+      // Numbers with commas.
+      {L"1,000,000", false},
+      {L"560,024", true},
+      // Regular numbers.
+      {L"0", true},
+      {L"0123", true},
+      {L"9876123", true},
+  };
+  for (size_t i = 0; i < FX_ArraySize(test_data); ++i) {
+    EXPECT_EQ(test_data[i].expected,
+              CJS_PublicMethods::IsNumber(test_data[i].input))
+        << "for case " << i;
+  }
+}