Make CPDF_Dictionary use unique pointers.

Make CPDF_Dictionary use unique pointers.

Some changes were required to match underlying ctors
as invoked by the templated methods.

Many release() calls go away, a few WrapUniques() are
introduced to avoid going deeper into other code.

Committed: https://pdfium.googlesource.com/pdfium/+/0e606b5ecd6e45f74391f110cc1fe0cce0e80c64

[Tom Sepez, 2016-11-17 22:01:28]

The CQ bit was checked by tsepez@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-17 22:01:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-17 22:05:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-17 22:05:03]

Dry run: Try jobs failed on following builders:
  win_xfa_rel on master.tryserver.client.pdfium (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.pdfium/builders/win_xfa_rel/bui...)

[Tom Sepez, 2016-11-17 22:13:27]

The CQ bit was checked by tsepez@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-17 22:13:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Tom Sepez, 2016-11-17 22:13:47]

tsepez@chromium.org changed reviewers:
+ dsinclair@chromium.org, thestig@chromium.org

[Tom Sepez, 2016-11-17 22:15:22]

Description was changed from

==========
Make CPDF_Dictionary use unique pointers.
==========

to

==========
Make CPDF_Dictionary use unique pointers.

Some changes were required to match underlying ctors
as invoked by the templated methods.

Many release() calls go away, a few WrapUniques() are
introduced to avoid going deeper into other code.
==========

[Tom Sepez, 2016-11-17 22:15:53]

Lei, Dan, please review.  This is the one that gets us to 95% of the way there
with unique_ptrs and CPDF_Objects.

[Tom Sepez, 2016-11-17 23:04:16]

The CQ bit was checked by tsepez@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-17 23:04:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-17 23:33:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-17 23:33:41]

Dry run: This issue passed the CQ dry run.

[Lei Zhang, 2016-11-18 04:14:20]

This CL is a bit big. I looked through maybe 1/3 of it, but I had other things
to work on today. Will finish looking tomorrow.

https://codereview.chromium.org/2510223002/diff/40001/core/fpdfapi/page/cpdf_...
File core/fpdfapi/page/cpdf_streamparser.cpp (right):

https://codereview.chromium.org/2510223002/diff/40001/core/fpdfapi/page/cpdf_...
core/fpdfapi/page/cpdf_streamparser.cpp:351: CPDF_Dictionary* pDict = new
CPDF_Dictionary(m_pPool);
For later: unique_ptr.

https://codereview.chromium.org/2510223002/diff/40001/testing/libfuzzer/pdf_h...
File testing/libfuzzer/pdf_hint_table_fuzzer.cc (right):

https://codereview.chromium.org/2510223002/diff/40001/testing/libfuzzer/pdf_h...
testing/libfuzzer/pdf_hint_table_fuzzer.cc:82: linearized_dict->SetFor("H",
std::move(hint_info));
#include <utility>, maybe also check other places. "git cl lint" can find them
for you.

[Tom Sepez, 2016-11-18 17:20:27]

On 2016/11/18 04:14:20, Lei Zhang (Mostly OOO) wrote:
> This CL is a bit big. I looked through maybe 1/3 of it, but I had other things
> to work on today. Will finish looking tomorrow.

Thanks, I couldn't find a way to split it up because it's a big API change :( .

[Lei Zhang, 2016-11-18 23:28:35]

https://codereview.chromium.org/2510223002/diff/40001/core/fpdfapi/parser/cpd...
File core/fpdfapi/parser/cpdf_document_unittest.cpp (right):

https://codereview.chromium.org/2510223002/diff/40001/core/fpdfapi/parser/cpd...
core/fpdfapi/parser/cpdf_document_unittest.cpp:42:
page->SetNewFor<CPDF_Number>("PageNumbering", static_cast<int>(number));
Maybe just pass in an int instead?

https://codereview.chromium.org/2510223002/diff/40001/core/fpdfapi/parser/cpd...
File core/fpdfapi/parser/cpdf_string.h (right):

https://codereview.chromium.org/2510223002/diff/40001/core/fpdfapi/parser/cpd...
core/fpdfapi/parser/cpdf_string.h:22:
CPDF_String(CFX_WeakPtr<CFX_ByteStringPool> pPool, const CFX_WideString& str);
If we have this new ctor, then can we change a bunch of the

pDict->SetNewFor<CPDF_String>(key, str, false) calls to just
pDict->SetNewFor<CPDF_String>(key, str) ?

[Lei Zhang, 2016-11-18 23:38:35]

lgtm, maybe simplify some of the SetNewFor<CPDF_String> calls later, assuming
it's possible?

https://codereview.chromium.org/2510223002/diff/40001/core/fpdfdoc/cpvt_gener...
File core/fpdfdoc/cpvt_generateap.cpp (right):

https://codereview.chromium.org/2510223002/diff/40001/core/fpdfdoc/cpvt_gener...
core/fpdfdoc/cpvt_generateap.cpp:701: if (!pAnnotDict->KeyExist("AS")) {
(for later) &&, how does it work?

[Tom Sepez, 2016-11-18 23:51:22]

Yes, the CPDF_String constructor arguments are for another day.  I'm not sure
what you're asking in your question, other than to say that the GetString
returns the bytestring out of the CPDF_String wrapper in one step.

[Tom Sepez, 2016-11-18 23:51:28]

The CQ bit was checked by tsepez@chromium.org

[Tom Sepez, 2016-11-18 23:51:29]

The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org
Link to the patchset: https://codereview.chromium.org/2510223002/#ps60001
(title: "Fix IWYU")

[commit-bot: I haz the power, 2016-11-18 23:51:33]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Lei Zhang, 2016-11-18 23:58:59]

https://codereview.chromium.org/2510223002/diff/40001/core/fpdfapi/parser/cpd...
File core/fpdfapi/parser/cpdf_string.h (right):

https://codereview.chromium.org/2510223002/diff/40001/core/fpdfapi/parser/cpd...
core/fpdfapi/parser/cpdf_string.h:22:
CPDF_String(CFX_WeakPtr<CFX_ByteStringPool> pPool, const CFX_WideString& str);
On 2016/11/18 23:28:35, Lei Zhang (Mostly OOO) wrote:
> If we have this new ctor, then can we change a bunch of the
> 
> pDict->SetNewFor<CPDF_String>(key, str, false) calls to just
> pDict->SetNewFor<CPDF_String>(key, str) ?

Oh, I missed ByteString vs WideString, n/m.

[commit-bot: I haz the power, 2016-11-19 00:08:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-19 00:08:31]

Failed to apply patch for core/fpdfapi/parser/cpdf_document.cpp:
While running git apply --index -p1;
  error: patch failed: core/fpdfapi/parser/cpdf_document.cpp:22
  error: core/fpdfapi/parser/cpdf_document.cpp: patch does not apply

Patch:       core/fpdfapi/parser/cpdf_document.cpp
Index: core/fpdfapi/parser/cpdf_document.cpp
diff --git a/core/fpdfapi/parser/cpdf_document.cpp
b/core/fpdfapi/parser/cpdf_document.cpp
index
c7f99fa6efd9726444236b09e1fc4a8fb6bf5cfe..67868265a9c02a8424ee6477f33629f0ce92856d
100644
--- a/core/fpdfapi/parser/cpdf_document.cpp
+++ b/core/fpdfapi/parser/cpdf_document.cpp
@@ -8,6 +8,7 @@
 
 #include <memory>
 #include <set>
+#include <utility>
 #include <vector>
 
 #include "core/fpdfapi/cpdf_modulemgr.h"
@@ -22,6 +23,7 @@
 #include "core/fpdfapi/parser/cpdf_number.h"
 #include "core/fpdfapi/parser/cpdf_parser.h"
 #include "core/fpdfapi/parser/cpdf_reference.h"
+#include "core/fpdfapi/parser/cpdf_string.h"
 #include "core/fpdfapi/parser/cpdf_stream.h"
 #include "core/fpdfapi/render/render_int.h"
 #include "core/fxcodec/JBig2_DocumentContext.h"
@@ -264,7 +266,7 @@ int CountPages(CPDF_Dictionary* pPages,
       count++;
     }
   }
-  pPages->SetIntegerFor("Count", count);
+  pPages->SetNewFor<CPDF_Number>("Count", count);
   return count;
 }
 
@@ -303,11 +305,11 @@ void ProcessNonbCJK(CPDF_Dictionary* pBaseDict,
     basefont += ",Bold";
   else if (italic)
     basefont += ",Italic";
-  pBaseDict->SetNameFor("Subtype", "TrueType");
-  pBaseDict->SetNameFor("BaseFont", basefont);
-  pBaseDict->SetNumberFor("FirstChar", 32);
-  pBaseDict->SetNumberFor("LastChar", 255);
-  pBaseDict->SetFor("Widths", pWidths);
+  pBaseDict->SetNewFor<CPDF_Name>("Subtype", "TrueType");
+  pBaseDict->SetNewFor<CPDF_Name>("BaseFont", basefont);
+  pBaseDict->SetNewFor<CPDF_Number>("FirstChar", 32);
+  pBaseDict->SetNewFor<CPDF_Number>("LastChar", 255);
+  pBaseDict->SetFor("Widths", pdfium::WrapUnique(pWidths));
 }
 
 std::unique_ptr<CPDF_Dictionary> CalculateFontDesc(CPDF_Document* pDoc,
@@ -320,14 +322,14 @@ std::unique_ptr<CPDF_Dictionary>
CalculateFontDesc(CPDF_Document* pDoc,
                                                    int32_t stemV) {
   auto pFontDesc =
       pdfium::MakeUnique<CPDF_Dictionary>(pDoc->GetByteStringPool());
-  pFontDesc->SetNameFor("Type", "FontDescriptor");
-  pFontDesc->SetNameFor("FontName", basefont);
-  pFontDesc->SetIntegerFor("Flags", flags);
-  pFontDesc->SetFor("FontBBox", bbox);
-  pFontDesc->SetIntegerFor("ItalicAngle", italicangle);
-  pFontDesc->SetIntegerFor("Ascent", ascend);
-  pFontDesc->SetIntegerFor("Descent", descend);
-  pFontDesc->SetIntegerFor("StemV", stemV);
+  pFontDesc->SetNewFor<CPDF_Name>("Type", "FontDescriptor");
+  pFontDesc->SetNewFor<CPDF_Name>("FontName", basefont);
+  pFontDesc->SetNewFor<CPDF_Number>("Flags", flags);
+  pFontDesc->SetFor("FontBBox", pdfium::WrapUnique(bbox));
+  pFontDesc->SetNewFor<CPDF_Number>("ItalicAngle", italicangle);
+  pFontDesc->SetNewFor<CPDF_Number>("Ascent", ascend);
+  pFontDesc->SetNewFor<CPDF_Number>("Descent", descend);
+  pFontDesc->SetNewFor<CPDF_Number>("StemV", stemV);
   return pFontDesc;
 }
 
@@ -648,19 +650,19 @@ CPDF_Image* CPDF_Document::LoadImageFromPageData(uint32_t
dwStreamObjNum) {
 void CPDF_Document::CreateNewDoc() {
   ASSERT(!m_pRootDict && !m_pInfoDict);
   m_pRootDict = NewIndirect<CPDF_Dictionary>();
-  m_pRootDict->SetNameFor("Type", "Catalog");
+  m_pRootDict->SetNewFor<CPDF_Name>("Type", "Catalog");
 
   CPDF_Dictionary* pPages = NewIndirect<CPDF_Dictionary>();
-  pPages->SetNameFor("Type", "Pages");
-  pPages->SetNumberFor("Count", 0);
-  pPages->SetFor("Kids", new CPDF_Array);
-  m_pRootDict->SetReferenceFor("Pages", this, pPages);
+  pPages->SetNewFor<CPDF_Name>("Type", "Pages");
+  pPages->SetNewFor<CPDF_Number>("Count", 0);
+  pPages->SetNewFor<CPDF_Array>("Kids");
+  m_pRootDict->SetNewFor<CPDF_Reference>("Pages", this, pPages->GetObjNum());
   m_pInfoDict = NewIndirect<CPDF_Dictionary>();
 }
 
 CPDF_Dictionary* CPDF_Document::CreateNewPage(int iPage) {
   CPDF_Dictionary* pDict = NewIndirect<CPDF_Dictionary>();
-  pDict->SetNameFor("Type", "Page");
+  pDict->SetNewFor<CPDF_Name>("Type", "Page");
   uint32_t dwObjNum = pDict->GetObjNum();
   if (!InsertNewPage(iPage, pDict)) {
     DeleteIndirectObject(dwObjNum);
@@ -687,11 +689,12 @@ bool CPDF_Document::InsertDeletePDFPage(CPDF_Dictionary*
pPages,
       }
       if (bInsert) {
         pKidList->InsertNewAt<CPDF_Reference>(i, this, pPageDict->GetObjNum());
-        pPageDict->SetReferenceFor("Parent", this, pPages->GetObjNum());
+        pPageDict->SetNewFor<CPDF_Reference>("Parent", this,
+                                             pPages->GetObjNum());
       } else {
         pKidList->RemoveAt(i);
       }
-      pPages->SetIntegerFor(
+      pPages->SetNewFor<CPDF_Number>(
           "Count", pPages->GetIntegerFor("Count") + (bInsert ? 1 : -1));
       ResetTraversal();
       break;
@@ -708,8 +711,8 @@ bool CPDF_Document::InsertDeletePDFPage(CPDF_Dictionary*
pPages,
     if (!InsertDeletePDFPage(pKid, nPagesToGo, pPageDict, bInsert, pVisited))
       return false;
 
-    pPages->SetIntegerFor("Count",
-                          pPages->GetIntegerFor("Count") + (bInsert ? 1 : -1));
+    pPages->SetNewFor<CPDF_Number>(
+        "Count", pPages->GetIntegerFor("Count") + (bInsert ? 1 : -1));
     break;
   }
   return true;
@@ -727,13 +730,11 @@ bool CPDF_Document::InsertNewPage(int iPage,
CPDF_Dictionary* pPageDict) {
 
   if (iPage == nPages) {
     CPDF_Array* pPagesList = pPages->GetArrayFor("Kids");
-    if (!pPagesList) {
-      pPagesList = new CPDF_Array;
-      pPages->SetFor("Kids", pPagesList);
-    }
+    if (!pPagesList)
+      pPagesList = pPages->SetNewFor<CPDF_Array>("Kids");
     pPagesList->AddNew<CPDF_Reference>(this, pPageDict->GetObjNum());
-    pPages->SetIntegerFor("Count", nPages + 1);
-    pPageDict->SetReferenceFor("Parent", this, pPages->GetObjNum());
+    pPages->SetNewFor<CPDF_Number>("Count", nPages + 1);
+    pPageDict->SetNewFor<CPDF_Reference>("Parent", this, pPages->GetObjNum());
     ResetTraversal();
   } else {
     std::set<CPDF_Dictionary*> stack = {pPages};
@@ -779,9 +780,9 @@ size_t CPDF_Document::CalculateEncodingDict(int charset,
     return i;
 
   CPDF_Dictionary* pEncodingDict = NewIndirect<CPDF_Dictionary>();
-  pEncodingDict->SetNameFor("BaseEncoding", "WinAnsiEncoding");
+  pEncodingDict->SetNewFor<CPDF_Name>("BaseEncoding", "WinAnsiEncoding");
 
-  CPDF_Array* pArray = new CPDF_Array;
+  CPDF_Array* pArray = pEncodingDict->SetNewFor<CPDF_Array>("Differences");
   pArray->AddNew<CPDF_Number>(128);
 
   const uint16_t* pUnicodes = g_FX_CharsetUnicodes[i].m_pUnicodes;
@@ -789,8 +790,8 @@ size_t CPDF_Document::CalculateEncodingDict(int charset,
     CFX_ByteString name = PDF_AdobeNameFromUnicode(pUnicodes[j]);
     pArray->AddNew<CPDF_Name>(name.IsEmpty() ? ".notdef" : name);
   }
-  pEncodingDict->SetFor("Differences", pArray);
-  pBaseDict->SetReferenceFor("Encoding", this, pEncodingDict);
+  pBaseDict->SetNewFor<CPDF_Reference>("Encoding", this,
+                                       pEncodingDict->GetObjNum());
   return i;
 }
 
@@ -804,7 +805,7 @@ CPDF_Dictionary* CPDF_Document::ProcessbCJK(
   CFX_ByteString cmap;
   CFX_ByteString ordering;
   int supplement = 0;
-  CPDF_Array* pWidthArray = new CPDF_Array;
+  CPDF_Array* pWidthArray = pFontDict->SetNewFor<CPDF_Array>("W");
   switch (charset) {
     case FXFONT_CHINESEBIG5_CHARSET:
       cmap = bVert ? "ETenms-B5-V" : "ETenms-B5-H";
@@ -843,20 +844,20 @@ CPDF_Dictionary* CPDF_Document::ProcessbCJK(
       Insert(0x7e, 0x7e, pWidthArray);
       break;
   }
-  pBaseDict->SetNameFor("Subtype", "Type0");
-  pBaseDict->SetNameFor("BaseFont", basefont);
-  pBaseDict->SetNameFor("Encoding", cmap);
-  pFontDict->SetFor("W", pWidthArray);
-  pFontDict->SetNameFor("Type", "Font");
-  pFontDict->SetNameFor("Subtype", "CIDFontType2");
-  pFontDict->SetNameFor("BaseFont", basefont);
-  CPDF_Dictionary* pCIDSysInfo = new CPDF_Dictionary(GetByteStringPool());
-  pCIDSysInfo->SetStringFor("Registry", "Adobe");
-  pCIDSysInfo->SetStringFor("Ordering", ordering);
-  pCIDSysInfo->SetIntegerFor("Supplement", supplement);
-  pFontDict->SetFor("CIDSystemInfo", pCIDSysInfo);
-  CPDF_Array* pArray = new CPDF_Array;
-  pBaseDict->SetFor("DescendantFonts", pArray);
+  pBaseDict->SetNewFor<CPDF_Name>("Subtype", "Type0");
+  pBaseDict->SetNewFor<CPDF_Name>("BaseFont", basefont);
+  pBaseDict->SetNewFor<CPDF_Name>("Encoding", cmap);
+  pFontDict->SetNewFor<CPDF_Name>("Type", "Font");
+  pFontDict->SetNewFor<CPDF_Name>("Subtype", "CIDFontType2");
+  pFontDict->SetNewFor<CPDF_Name>("BaseFont", basefont);
+
+  CPDF_Dictionary* pCIDSysInfo =
+      pFontDict->SetNewFor<CPDF_Dictionary>("CIDSystemInfo");
+  pCIDSysInfo->SetNewFor<CPDF_String>("Registry", "Adobe", false);
+  pCIDSysInfo->SetNewFor<CPDF_String>("Ordering", ordering, false);
+  pCIDSysInfo->SetNewFor<CPDF_Number>("Supplement", supplement);
+
+  CPDF_Array* pArray = pBaseDict->SetNewFor<CPDF_Array>("DescendantFonts");
   pArray->AddNew<CPDF_Reference>(this, pFontDict->GetObjNum());
   return pFontDict;
 }
@@ -876,7 +877,7 @@ CPDF_Font* CPDF_Document::AddFont(CFX_Font* pFont, int
charset, bool bVert) {
                      false, false, charset == FXFONT_SYMBOL_CHARSET);
 
   CPDF_Dictionary* pBaseDict = NewIndirect<CPDF_Dictionary>();
-  pBaseDict->SetNameFor("Type", "Font");
+  pBaseDict->SetNewFor<CPDF_Name>("Type", "Font");
   std::unique_ptr<CFX_UnicodeEncoding> pEncoding(
       new CFX_UnicodeEncoding(pFont));
   CPDF_Dictionary* pFontDict = pBaseDict;
@@ -889,7 +890,7 @@ CPDF_Font* CPDF_Document::AddFont(CFX_Font* pFont, int
charset, bool bVert) {
     }
     if (charset == FXFONT_ANSI_CHARSET || charset == FXFONT_DEFAULT_CHARSET ||
         charset == FXFONT_SYMBOL_CHARSET) {
-      pBaseDict->SetNameFor("Encoding", "WinAnsiEncoding");
+      pBaseDict->Se…
(message too large)

[Tom Sepez, 2016-11-19 00:10:57]

The CQ bit was checked by tsepez@chromium.org

[Tom Sepez, 2016-11-19 00:10:57]

The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org
Link to the patchset: https://codereview.chromium.org/2510223002/#ps80001
(title: "rebase")

[commit-bot: I haz the power, 2016-11-19 00:11:01]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-19 00:22:43]

Description was changed from

==========
Make CPDF_Dictionary use unique pointers.

Some changes were required to match underlying ctors
as invoked by the templated methods.

Many release() calls go away, a few WrapUniques() are
introduced to avoid going deeper into other code.
==========

to

==========
Make CPDF_Dictionary use unique pointers.

Some changes were required to match underlying ctors
as invoked by the templated methods.

Many release() calls go away, a few WrapUniques() are
introduced to avoid going deeper into other code.

Committed:
https://pdfium.googlesource.com/pdfium/+/0e606b5ecd6e45f74391f110cc1fe0cce0e8...
==========

[commit-bot: I haz the power, 2016-11-19 00:22:44]

Committed patchset #5 (id:80001) as
https://pdfium.googlesource.com/pdfium/+/0e606b5ecd6e45f74391f110cc1fe0cce0e8...