commit 3eaad184c4c4f179d226352bf055d5956425c20d
author Tom Sepez <tsepez@chromium.org> Wed Nov 07 21:52:21 2018 +0000
committer Chromium commit bot <commit-bot@chromium.org> Wed Nov 07 21:52:21 2018 +0000
tree b9da1ecb97722423d390c2a32ab7953e2c51d4f2
parent cd9a7f0a75185267a9086f84397ab99f29b315a4

Tidy some small TODOs in CFX_GlobalData.

Change-Id: Ic23dd80d5f553bb5665b85b6a87f62dfed62bc8f
Reviewed-on: https://pdfium-review.googlesource.com/c/45230
Reviewed-by: Lei Zhang <thestig@chromium.org>
Commit-Queue: Tom Sepez <tsepez@chromium.org>

fxjs/cfx_globaldata.cpp

diff --git a/fxjs/cfx_globaldata.cpp b/fxjs/cfx_globaldata.cpp
index 120a875..db0d125 100644
--- a/fxjs/cfx_globaldata.cpp
+++ b/fxjs/cfx_globaldata.cpp
@@ -76,7 +76,7 @@
       MakeNameTypeString(name, pData.nType, result);
       return true;
     }
-    // TODO(tsepez): persist these array objects.
+    // Arrays don't get persisted per JS spec page 484.
     case CFX_KeyValue::DataType::OBJECT:
     default:
       break;
@@ -260,25 +260,27 @@
   return m_arrayGlobalData[index].get();
 }
 
-void CFX_GlobalData::LoadGlobalPersistentVariables() {
+bool CFX_GlobalData::LoadGlobalPersistentVariables() {
   if (!m_pDelegate)
-    return;
+    return false;
 
+  bool ret;
   {
     // Span can't outlive call to BufferDone().
     Optional<pdfium::span<uint8_t>> buffer = m_pDelegate->LoadBuffer();
     if (!buffer.has_value() || buffer.value().empty())
-      return;
+      return false;
 
-    LoadGlobalPersistentVariablesFromBuffer(buffer.value());
+    ret = LoadGlobalPersistentVariablesFromBuffer(buffer.value());
   }
   m_pDelegate->BufferDone();
+  return ret;
 }
 
-void CFX_GlobalData::LoadGlobalPersistentVariablesFromBuffer(
+bool CFX_GlobalData::LoadGlobalPersistentVariablesFromBuffer(
     pdfium::span<uint8_t> buffer) {
   if (buffer.size() < kMinGlobalDataBytes)
-    return;
+    return false;
 
   CRYPT_ArcFourCryptBlock(buffer.data(), buffer.size(), kRC4KEY,
                           sizeof(kRC4KEY));
@@ -287,12 +289,12 @@
   uint16_t wType = *((uint16_t*)p);
   p += sizeof(uint16_t);
   if (wType != kMagic)
-    return;
+    return false;
 
   uint16_t wVersion = *((uint16_t*)p);
   p += sizeof(uint16_t);
   if (wVersion > kMaxVersion)
-    return;
+    return false;
 
   uint32_t dwCount = *((uint32_t*)p);
   p += sizeof(uint32_t);
@@ -301,7 +303,7 @@
   p += sizeof(uint32_t);
 
   if (dwSize != buffer.size() - sizeof(uint16_t) * 2 - sizeof(uint32_t) * 2)
-    return;
+    return false;
 
   for (int32_t i = 0, sz = dwCount; i < sz; i++) {
     if (p > buffer.end())
@@ -357,14 +359,17 @@
         SetGlobalVariablePersistent(sEntry, true);
       } break;
       case CFX_KeyValue::DataType::OBJECT:
-        break;
+      default:
+        // Arrays aren't allowed in these buffers, nor are unrecoginzed tags.
+        return false;
     }
   }
+  return true;
 }
 
-void CFX_GlobalData::SaveGlobalPersisitentVariables() {
+bool CFX_GlobalData::SaveGlobalPersisitentVariables() {
   if (!m_pDelegate)
-    return;
+    return false;
 
   uint32_t nCount = 0;
   CFX_BinaryBuf sData;
@@ -385,18 +390,19 @@
 
   CFX_BinaryBuf sFile;
   uint16_t wType = kMagic;
-  sFile.AppendBlock(&wType, sizeof(uint16_t));
   uint16_t wVersion = 2;
+  sFile.AppendBlock(&wType, sizeof(uint16_t));
   sFile.AppendBlock(&wVersion, sizeof(uint16_t));
   sFile.AppendBlock(&nCount, sizeof(uint32_t));
+
   uint32_t dwSize = sData.GetSize();
   sFile.AppendBlock(&dwSize, sizeof(uint32_t));
   sFile.AppendBlock(sData.GetBuffer(), sData.GetSize());
+
   CRYPT_ArcFourCryptBlock(sFile.GetBuffer(), sFile.GetSize(), kRC4KEY,
                           sizeof(kRC4KEY));
 
-  // TODO(tsepez): check return value?
-  m_pDelegate->StoreBuffer({sFile.GetBuffer(), sFile.GetSize()});
+  return m_pDelegate->StoreBuffer({sFile.GetBuffer(), sFile.GetSize()});
 }
 
 CFX_GlobalData::Element::Element() = default;

fxjs/cfx_globaldata.h

diff --git a/fxjs/cfx_globaldata.h b/fxjs/cfx_globaldata.h
index 3a79a6e..66dc0bd 100644
--- a/fxjs/cfx_globaldata.h
+++ b/fxjs/cfx_globaldata.h
@@ -62,9 +62,9 @@
   explicit CFX_GlobalData(Delegate* pDelegate);
   ~CFX_GlobalData();
 
-  void LoadGlobalPersistentVariables();
-  void LoadGlobalPersistentVariablesFromBuffer(pdfium::span<uint8_t> buffer);
-  void SaveGlobalPersisitentVariables();
+  bool LoadGlobalPersistentVariables();
+  bool LoadGlobalPersistentVariablesFromBuffer(pdfium::span<uint8_t> buffer);
+  bool SaveGlobalPersisitentVariables();
 
   iterator FindGlobalVariable(const ByteString& sPropname);
   const_iterator FindGlobalVariable(const ByteString& sPropname) const;

fxjs/cfx_globaldata_unittest.cpp

diff --git a/fxjs/cfx_globaldata_unittest.cpp b/fxjs/cfx_globaldata_unittest.cpp
index 52a13dc..8f6cbce 100644
--- a/fxjs/cfx_globaldata_unittest.cpp
+++ b/fxjs/cfx_globaldata_unittest.cpp
@@ -93,7 +93,7 @@
   EXPECT_EQ("null", element->data.sKey);
   EXPECT_EQ(CFX_KeyValue::DataType::NULLOBJ, element->data.nType);
 
-  // TODO(tsepez): arrays don't get persisted.
+  // Arrays don't get persisted.
   element = pInstance->GetAt(4);
   ASSERT_FALSE(element);