Clean up CPDF_FileSpec and add unit tests

Change implicit conversion to accessor function, remove unused function
and unused parameter, add a couple checks for null pointers, and some
cleanup.

BUG=pdfium:247
R=thestig@chromium.org

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

8 files changed, 228 insertions, 77 deletions

diff --git a/BUILD.gn b/BUILD.gn
index de3570f6d1..85ecd98c60 100644
--- a/BUILD.gn
+++ b/BUILD.gn
@@ -1447,6 +1447,7 @@ test("pdfium_unittests") {
     "core/src/fpdfapi/fpdf_parser/fpdf_parser_objects_unittest.cpp",
     "core/src/fpdfapi/fpdf_parser/fpdf_parser_parser_unittest.cpp",
     "core/src/fpdfapi/fpdf_parser/fpdf_parser_utility_unittest.cpp",
+    "core/src/fpdfdoc/doc_basic_unittest.cpp",
     "core/src/fpdftext/fpdf_text_int_unittest.cpp",
     "core/src/fxcodec/codec/fx_codec_jpx_unittest.cpp",
     "core/src/fxcrt/fx_basic_bstring_unittest.cpp",
diff --git a/core/include/fpdfdoc/fpdf_doc.h b/core/include/fpdfdoc/fpdf_doc.h
index 17f2c824c6..15616be4d5 100644
--- a/core/include/fpdfdoc/fpdf_doc.h
+++ b/core/include/fpdfdoc/fpdf_doc.h
@@ -317,21 +317,23 @@ class CPDF_DocJSActions {
  protected:
   CPDF_Document* const m_pDocument;
 };
+
 class CPDF_FileSpec {
  public:
   CPDF_FileSpec();
-
   explicit CPDF_FileSpec(CPDF_Object* pObj) { m_pObj = pObj; }
 
-  operator CPDF_Object*() const { return m_pObj; }
-
-  FX_BOOL IsURL() const;
+  // Convert a platform dependent file name into pdf format.
+  static CFX_WideString EncodeFileName(const CFX_WideStringC& filepath);
 
-  FX_BOOL GetFileName(CFX_WideString& wsFileName) const;
+  // Convert a pdf file name into platform dependent format.
+  static CFX_WideString DecodeFileName(const CFX_WideStringC& filepath);
 
-  CPDF_Stream* GetFileStream() const;
+  CPDF_Object* GetObj() const { return m_pObj; }
+  bool GetFileName(CFX_WideString* wsFileName) const;
 
-  void SetFileName(const CFX_WideStringC& wsFileName, FX_BOOL bURL = FALSE);
+  // Set this file spec to refer to a file name (not a url).
+  void SetFileName(const CFX_WideStringC& wsFileName);
 
  protected:
   CPDF_Object* m_pObj;
diff --git a/core/src/fpdfdoc/doc_action.cpp b/core/src/fpdfdoc/doc_action.cpp
index 97ca046bb9..94235d2aee 100644
--- a/core/src/fpdfdoc/doc_action.cpp
+++ b/core/src/fpdfdoc/doc_action.cpp
@@ -66,7 +66,7 @@ CFX_WideString CPDF_Action::GetFilePath() const {
     return path;
   }
   CPDF_FileSpec filespec(pFile);
-  filespec.GetFileName(path);
+  filespec.GetFileName(&path);
   return path;
 }
 CFX_ByteString CPDF_Action::GetURI(CPDF_Document* pDoc) const {
diff --git a/core/src/fpdfdoc/doc_basic.cpp b/core/src/fpdfdoc/doc_basic.cpp
index 85a6c0f7c8..bc88ddc828 100644
--- a/core/src/fpdfdoc/doc_basic.cpp
+++ b/core/src/fpdfdoc/doc_basic.cpp
@@ -237,6 +237,7 @@ CPDF_Array* CPDF_NameTree::LookupNamedDest(CPDF_Document* pDoc,
     return pDict->GetArrayBy("D");
   return nullptr;
 }
+
 #if _FXM_PLATFORM_ == _FXM_PLATFORM_APPLE_ || \
     _FXM_PLATFORM_ == _FXM_PLATFORM_WINDOWS_
 static CFX_WideString ChangeSlashToPlatform(const FX_WCHAR* str) {
@@ -255,6 +256,7 @@ static CFX_WideString ChangeSlashToPlatform(const FX_WCHAR* str) {
   }
   return result;
 }
+
 static CFX_WideString ChangeSlashToPDF(const FX_WCHAR* str) {
   CFX_WideString result;
   while (*str) {
@@ -267,23 +269,21 @@ static CFX_WideString ChangeSlashToPDF(const FX_WCHAR* str) {
   }
   return result;
 }
-#endif
-static CFX_WideString FILESPEC_DecodeFileName(const CFX_WideStringC& filepath) {
-  if (filepath.GetLength() <= 1) {
+#endif  // _FXM_PLATFORM_APPLE_ || _FXM_PLATFORM_WINDOWS_
+
+CFX_WideString CPDF_FileSpec::DecodeFileName(const CFX_WideStringC& filepath) {
+  if (filepath.GetLength() <= 1)
     return CFX_WideString();
-  }
+
 #if _FXM_PLATFORM_ == _FXM_PLATFORM_APPLE_
-  if (filepath.Left(sizeof("/Mac") - 1) == CFX_WideStringC(L"/Mac")) {
+  if (filepath.Left(sizeof("/Mac") - 1) == CFX_WideStringC(L"/Mac"))
     return ChangeSlashToPlatform(filepath.GetPtr() + 1);
-  }
   return ChangeSlashToPlatform(filepath.GetPtr());
 #elif _FXM_PLATFORM_ == _FXM_PLATFORM_WINDOWS_
-  if (filepath.GetAt(0) != '/') {
+  if (filepath.GetAt(0) != '/')
     return ChangeSlashToPlatform(filepath.GetPtr());
-  }
-  if (filepath.GetAt(1) == '/') {
+  if (filepath.GetAt(1) == '/')
     return ChangeSlashToPlatform(filepath.GetPtr() + 1);
-  }
   if (filepath.GetAt(2) == '/') {
     CFX_WideString result;
     result += filepath.GetAt(1);
@@ -299,48 +299,41 @@ static CFX_WideString FILESPEC_DecodeFileName(const CFX_WideStringC& filepath) {
   return filepath;
 #endif
 }
-FX_BOOL CPDF_FileSpec::GetFileName(CFX_WideString& csFileName) const {
-  if (!m_pObj) {
-    return FALSE;
-  }
+
+bool CPDF_FileSpec::GetFileName(CFX_WideString* csFileName) const {
   if (CPDF_Dictionary* pDict = m_pObj->AsDictionary()) {
-    csFileName = pDict->GetUnicodeTextBy("UF");
-    if (csFileName.IsEmpty()) {
-      csFileName = CFX_WideString::FromLocal(pDict->GetStringBy("F"));
+    *csFileName = pDict->GetUnicodeTextBy("UF");
+    if (csFileName->IsEmpty()) {
+      *csFileName = CFX_WideString::FromLocal(pDict->GetStringBy("F"));
     }
-    if (pDict->GetStringBy("FS") == "URL") {
-      return TRUE;
-    }
-    if (csFileName.IsEmpty()) {
+    if (pDict->GetStringBy("FS") == "URL")
+      return true;
+    if (csFileName->IsEmpty()) {
       if (pDict->KeyExist("DOS")) {
-        csFileName = CFX_WideString::FromLocal(pDict->GetStringBy("DOS"));
+        *csFileName = CFX_WideString::FromLocal(pDict->GetStringBy("DOS"));
       } else if (pDict->KeyExist("Mac")) {
-        csFileName = CFX_WideString::FromLocal(pDict->GetStringBy("Mac"));
+        *csFileName = CFX_WideString::FromLocal(pDict->GetStringBy("Mac"));
       } else if (pDict->KeyExist("Unix")) {
-        csFileName = CFX_WideString::FromLocal(pDict->GetStringBy("Unix"));
+        *csFileName = CFX_WideString::FromLocal(pDict->GetStringBy("Unix"));
       } else {
-        return FALSE;
+        return false;
       }
     }
+  } else if (m_pObj->IsString()) {
+    *csFileName = CFX_WideString::FromLocal(m_pObj->GetString());
   } else {
-    csFileName = CFX_WideString::FromLocal(m_pObj->GetString());
+    return false;
   }
-  csFileName = FILESPEC_DecodeFileName(csFileName);
-  return TRUE;
+  *csFileName = DecodeFileName(*csFileName);
+  return true;
 }
+
 CPDF_FileSpec::CPDF_FileSpec() {
   m_pObj = new CPDF_Dictionary;
-  if (CPDF_Dictionary* pDict = ToDictionary(m_pObj)) {
-    pDict->SetAtName("Type", "Filespec");
-  }
+  m_pObj->AsDictionary()->SetAtName("Type", "Filespec");
 }
-FX_BOOL CPDF_FileSpec::IsURL() const {
-  if (CPDF_Dictionary* pDict = ToDictionary(m_pObj)) {
-    return pDict->GetStringBy("FS") == "URL";
-  }
-  return FALSE;
-}
-CFX_WideString FILESPEC_EncodeFileName(const CFX_WideStringC& filepath) {
+
+CFX_WideString CPDF_FileSpec::EncodeFileName(const CFX_WideStringC& filepath) {
   if (filepath.GetLength() <= 1) {
     return CFX_WideString();
   }
@@ -377,40 +370,20 @@ CFX_WideString FILESPEC_EncodeFileName(const CFX_WideStringC& filepath) {
   return filepath;
 #endif
 }
-CPDF_Stream* CPDF_FileSpec::GetFileStream() const {
+
+void CPDF_FileSpec::SetFileName(const CFX_WideStringC& wsFileName) {
   if (!m_pObj)
-    return nullptr;
-  if (CPDF_Stream* pStream = m_pObj->AsStream())
-    return pStream;
-  if (CPDF_Dictionary* pEF = m_pObj->AsDictionary()->GetDictBy("EF"))
-    return pEF->GetStreamBy("F");
-  return nullptr;
-}
-static void FPDFDOC_FILESPEC_SetFileName(CPDF_Object* pObj,
-                                         const CFX_WideStringC& wsFileName,
-                                         FX_BOOL bURL) {
-  CFX_WideString wsStr;
-  if (bURL) {
-    wsStr = wsFileName;
-  } else {
-    wsStr = FILESPEC_EncodeFileName(wsFileName);
-  }
-  if (pObj->IsString()) {
-    pObj->SetString(CFX_ByteString::FromUnicode(wsStr));
-  } else if (CPDF_Dictionary* pDict = pObj->AsDictionary()) {
+    return;
+
+  CFX_WideString wsStr = EncodeFileName(wsFileName);
+  if (m_pObj->IsString()) {
+    m_pObj->SetString(CFX_ByteString::FromUnicode(wsStr));
+  } else if (CPDF_Dictionary* pDict = m_pObj->AsDictionary()) {
     pDict->SetAtString("F", CFX_ByteString::FromUnicode(wsStr));
     pDict->SetAtString("UF", PDF_EncodeText(wsStr));
   }
 }
-void CPDF_FileSpec::SetFileName(const CFX_WideStringC& wsFileName,
-                                FX_BOOL bURL) {
-  if (bURL) {
-    if (CPDF_Dictionary* pDict = m_pObj->AsDictionary()) {
-      pDict->SetAtName("FS", "URL");
-    }
-  }
-  FPDFDOC_FILESPEC_SetFileName(m_pObj, wsFileName, bURL);
-}
+
 static CFX_WideString _MakeRoman(int num) {
   const int arabic[] = {1000, 900, 500, 400, 100, 90, 50, 40, 10, 9, 5, 4, 1};
   const CFX_WideString roman[] = {L"m",  L"cm", L"d",  L"cd", L"c",
@@ -429,6 +402,7 @@ static CFX_WideString _MakeRoman(int num) {
   }
   return wsRomanNumber;
 }
+
 static CFX_WideString _MakeLetters(int num) {
   if (num == 0) {
     return CFX_WideString();
diff --git a/core/src/fpdfdoc/doc_basic_unittest.cpp b/core/src/fpdfdoc/doc_basic_unittest.cpp
new file mode 100644
index 0000000000..ecc603aa9e
--- /dev/null
+++ b/core/src/fpdfdoc/doc_basic_unittest.cpp
@@ -0,0 +1,168 @@
+// 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 "core/include/fpdfdoc/fpdf_doc.h"
+
+#include <memory>
+#include <vector>
+
+#include "testing/gtest/include/gtest/gtest.h"
+#include "testing/test_support.h"
+
+namespace {
+
+using ScopedObj = std::unique_ptr<CPDF_Object, ReleaseDeleter<CPDF_Object>>;
+using ScopedDict =
+    std::unique_ptr<CPDF_Dictionary, ReleaseDeleter<CPDF_Dictionary>>;
+}
+
+TEST(doc_basic_filespec, EncodeDecodeFileName) {
+  std::vector<pdfium::NullTermWstrFuncTestData> test_data = {
+    // Empty src string.
+    {L"", L""},
+    // only file name.
+    {L"test.pdf", L"test.pdf"},
+#if _FXM_PLATFORM_ == _FXM_PLATFORM_WINDOWS_
+    // With drive identifier.
+    {L"r:\\pdfdocs\\spec.pdf", L"/r/pdfdocs/spec.pdf"},
+    // Relative path.
+    {L"My Document\\test.pdf", L"My Document/test.pdf"},
+    // Absolute path without drive identifier.
+    {L"\\pdfdocs\\spec.pdf", L"//pdfdocs/spec.pdf"},
+    // Absolute path with double backslashes.
+    {L"\\\\pdfdocs\\spec.pdf", L"/pdfdocs/spec.pdf"},
+    // Network resource name. It is not supported yet.
+    // {L"pclib/eng:\\pdfdocs\\spec.pdf", L"/pclib/eng/pdfdocs/spec.pdf"},
+#elif _FXM_PLATFORM_ == _FXM_PLATFORM_APPLE_
+    // Absolute path with colon separator.
+    {L"Mac HD:PDFDocs:spec.pdf", L"/Mac HD/PDFDocs/spec.pdf"},
+    // Relative path with colon separator.
+    {L"PDFDocs:spec.pdf", L"PDFDocs/spec.pdf"},
+#else
+    // Relative path.
+    {L"./docs/test.pdf", L"./docs/test.pdf"},
+    // Relative path with parent dir.
+    {L"../test_docs/test.pdf", L"../test_docs/test.pdf"},
+    // Absolute path.
+    {L"/usr/local/home/test.pdf", L"/usr/local/home/test.pdf"},
+#endif
+  };
+  for (const auto& data : test_data) {
+    CFX_WideString encoded_str = CPDF_FileSpec::EncodeFileName(data.input);
+    EXPECT_TRUE(encoded_str.Equal(data.expected));
+    // DecodeFileName is the reverse procedure of EncodeFileName.
+    CFX_WideString decoded_str = CPDF_FileSpec::DecodeFileName(data.expected);
+    EXPECT_TRUE(decoded_str.Equal(data.input));
+  }
+}
+
+TEST(doc_basic_filespec, GetFileName) {
+  {
+    // String object.
+    pdfium::NullTermWstrFuncTestData test_data = {
+#if _FXM_PLATFORM_ == _FXM_PLATFORM_WINDOWS_
+      L"/C/docs/test.pdf",
+      L"C:\\docs\\test.pdf"
+#elif _FXM_PLATFORM_ == _FXM_PLATFORM_APPLE_
+      L"/Mac HD/docs/test.pdf",
+      L"Mac HD:docs:test.pdf"
+#else
+      L"/docs/test.pdf",
+      L"/docs/test.pdf"
+#endif
+    };
+    ScopedObj str_obj(new CPDF_String(test_data.input));
+    CPDF_FileSpec file_spec(str_obj.get());
+    CFX_WideString file_name;
+    EXPECT_TRUE(file_spec.GetFileName(&file_name));
+    EXPECT_TRUE(file_name.Equal(test_data.expected));
+  }
+  {
+    // Dictionary object.
+    pdfium::NullTermWstrFuncTestData test_data[5] = {
+#if _FXM_PLATFORM_ == _FXM_PLATFORM_WINDOWS_
+      {L"/C/docs/test.pdf", L"C:\\docs\\test.pdf"},
+      {L"/D/docs/test.pdf", L"D:\\docs\\test.pdf"},
+      {L"/E/docs/test.pdf", L"E:\\docs\\test.pdf"},
+      {L"/F/docs/test.pdf", L"F:\\docs\\test.pdf"},
+      {L"/G/docs/test.pdf", L"G:\\docs\\test.pdf"},
+#elif _FXM_PLATFORM_ == _FXM_PLATFORM_APPLE_
+      {L"/Mac HD/docs1/test.pdf", L"Mac HD:docs1:test.pdf"},
+      {L"/Mac HD/docs2/test.pdf", L"Mac HD:docs2:test.pdf"},
+      {L"/Mac HD/docs3/test.pdf", L"Mac HD:docs3:test.pdf"},
+      {L"/Mac HD/docs4/test.pdf", L"Mac HD:docs4:test.pdf"},
+      {L"/Mac HD/docs5/test.pdf", L"Mac HD:docs5:test.pdf"},
+#else
+      {L"/docs/a/test.pdf", L"/docs/a/test.pdf"},
+      {L"/docs/b/test.pdf", L"/docs/b/test.pdf"},
+      {L"/docs/c/test.pdf", L"/docs/c/test.pdf"},
+      {L"/docs/d/test.pdf", L"/docs/d/test.pdf"},
+      {L"/docs/e/test.pdf", L"/docs/e/test.pdf"},
+#endif
+    };
+    // Keyword fields in reverse order of precedence to retrieve the file name.
+    const char* const keywords[5] = {"Unix", "Mac", "DOS", "F", "UF"};
+    ScopedDict dict_obj(new CPDF_Dictionary);
+    CPDF_FileSpec file_spec(dict_obj.get());
+    CFX_WideString file_name;
+    for (int i = 0; i < 5; ++i) {
+      dict_obj->SetAt(keywords[i], new CPDF_String(test_data[i].input));
+      EXPECT_TRUE(file_spec.GetFileName(&file_name));
+      EXPECT_TRUE(file_name.Equal(test_data[i].expected));
+    }
+
+    // With all the former fields and 'FS' field suggests 'URL' type.
+    dict_obj->SetAtString("FS", "URL");
+    EXPECT_TRUE(file_spec.GetFileName(&file_name));
+    // Url string is not decoded.
+    EXPECT_TRUE(file_name.Equal(test_data[4].input));
+  }
+  {
+    // Invalid object.
+    ScopedObj name_obj(new CPDF_Name("test.pdf"));
+    CPDF_FileSpec file_spec(name_obj.get());
+    CFX_WideString file_name;
+    EXPECT_FALSE(file_spec.GetFileName(&file_name));
+  }
+}
+
+TEST(doc_basic_filespec, SetFileName) {
+  pdfium::NullTermWstrFuncTestData test_data = {
+#if _FXM_PLATFORM_ == _FXM_PLATFORM_WINDOWS_
+    L"C:\\docs\\test.pdf",
+    L"/C/docs/test.pdf"
+#elif _FXM_PLATFORM_ == _FXM_PLATFORM_APPLE_
+    L"Mac HD:docs:test.pdf",
+    L"/Mac HD/docs/test.pdf"
+#else
+    L"/docs/test.pdf",
+    L"/docs/test.pdf"
+#endif
+  };
+  // String object.
+  ScopedObj str_obj(new CPDF_String(L"babababa"));
+  CPDF_FileSpec file_spec1(str_obj.get());
+  file_spec1.SetFileName(test_data.input);
+  // Check internal object value.
+  CFX_ByteString str;
+  str.ConvertFrom(test_data.expected);
+  EXPECT_TRUE(str == str_obj->GetString());
+  // Check we can get the file name back.
+  CFX_WideString file_name;
+  EXPECT_TRUE(file_spec1.GetFileName(&file_name));
+  EXPECT_TRUE(file_name.Equal(test_data.input));
+
+  // Dictionary object.
+  ScopedDict dict_obj(new CPDF_Dictionary);
+  CPDF_FileSpec file_spec2(dict_obj.get());
+  file_spec2.SetFileName(test_data.input);
+  // Check internal object value.
+  file_name = dict_obj->GetUnicodeTextBy("F");
+  EXPECT_TRUE(file_name.Equal(test_data.expected));
+  file_name = dict_obj->GetUnicodeTextBy("UF");
+  EXPECT_TRUE(file_name.Equal(test_data.expected));
+  // Check we can get the file name back.
+  EXPECT_TRUE(file_spec2.GetFileName(&file_name));
+  EXPECT_TRUE(file_name.Equal(test_data.input));
+}
diff --git a/core/src/fpdfdoc/doc_form.cpp b/core/src/fpdfdoc/doc_form.cpp
index aeddb28e96..460a44a7df 100644
--- a/core/src/fpdfdoc/doc_form.cpp
+++ b/core/src/fpdfdoc/doc_form.cpp
@@ -1041,13 +1041,13 @@ CFDF_Document* CPDF_InterForm::ExportToFDF(
   CPDF_Dictionary* pMainDict = pDoc->GetRoot()->GetDictBy("FDF");
   if (!pdf_path.IsEmpty()) {
     if (bSimpleFileSpec) {
-      CFX_WideString wsFilePath = FILESPEC_EncodeFileName(pdf_path);
+      CFX_WideString wsFilePath = CPDF_FileSpec::EncodeFileName(pdf_path);
       pMainDict->SetAtString("F", CFX_ByteString::FromUnicode(wsFilePath));
       pMainDict->SetAtString("UF", PDF_EncodeText(wsFilePath));
     } else {
       CPDF_FileSpec filespec;
       filespec.SetFileName(pdf_path);
-      pMainDict->SetAt("F", static_cast<CPDF_Object*>(filespec));
+      pMainDict->SetAt("F", filespec.GetObj());
     }
   }
   CPDF_Array* pFields = new CPDF_Array;
diff --git a/pdfium.gyp b/pdfium.gyp
index ffebc4f52d..9dd3d889a5 100644
--- a/pdfium.gyp
+++ b/pdfium.gyp
@@ -755,6 +755,7 @@
         'core/src/fpdfapi/fpdf_parser/fpdf_parser_objects_unittest.cpp',
         'core/src/fpdfapi/fpdf_parser/fpdf_parser_parser_unittest.cpp',
         'core/src/fpdfapi/fpdf_parser/fpdf_parser_utility_unittest.cpp',
+        'core/src/fpdfdoc/doc_basic_unittest.cpp',
         'core/src/fpdftext/fpdf_text_int_unittest.cpp',
         'core/src/fxcodec/codec/fx_codec_jpx_unittest.cpp',
         'core/src/fxcrt/fx_basic_bstring_unittest.cpp',
diff --git a/testing/test_support.h b/testing/test_support.h
index f135425b51..4b716ae772 100644
--- a/testing/test_support.h
+++ b/testing/test_support.h
@@ -47,6 +47,11 @@ struct DecodeTestData {
   unsigned int processed_size;
 };
 
+struct NullTermWstrFuncTestData {
+  const wchar_t* input;
+  const wchar_t* expected;
+};
+
 // Used with std::unique_ptr to free() objects that can't be deleted.
 struct FreeDeleter {
   inline void operator()(void* ptr) const { free(ptr); }