Fix nits in CPDF_PageOrganizer.

fpdfsdk/fpdfppo.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 "public/fpdf_ppo.h"
 
 #include <map>
 #include <memory>
 #include <vector>
 
 #include "core/fpdfapi/parser/cpdf_array.h"
 #include "core/fpdfapi/parser/cpdf_document.h"
 #include "core/fpdfapi/parser/cpdf_name.h"
 #include "core/fpdfapi/parser/cpdf_number.h"
 #include "core/fpdfapi/parser/cpdf_reference.h"
 #include "core/fpdfapi/parser/cpdf_stream.h"
 #include "core/fpdfapi/parser/cpdf_string.h"
 #include "fpdfsdk/fsdk_define.h"
 #include "third_party/base/stl_util.h"

[Tom Sepez, 2016/11/14 17:31:35]

nit: include ptr_util.h

[Lei Zhang, 2016/11/14 19:57:01]

Done.

 
 class CPDF_PageOrganizer {
  public:
   using ObjectNumberMap = std::map<uint32_t, uint32_t>;
   CPDF_PageOrganizer();
   ~CPDF_PageOrganizer();
 
   bool PDFDocInit(CPDF_Document* pDestPDFDoc, CPDF_Document* pSrcPDFDoc);
-  bool ExportPage(CPDF_Document* pSrcPDFDoc,
-                  std::vector<uint16_t>* pPageNums,
-                  CPDF_Document* pDestPDFDoc,
+  bool ExportPage(CPDF_Document* pDestPDFDoc,
+                  CPDF_Document* pSrcPDFDoc,
+                  const std::vector<uint16_t>& pageNums,
                   int nIndex);
   CPDF_Object* PageDictGetInheritableTag(CPDF_Dictionary* pDict,
                                          const CFX_ByteString& bsSrctag);
   bool UpdateReference(CPDF_Object* pObj,
                        CPDF_Document* pDoc,
                        ObjectNumberMap* pObjNumberMap);
   uint32_t GetNewObjId(CPDF_Document* pDoc,
                        ObjectNumberMap* pObjNumberMap,
                        CPDF_Reference* pRef);
 };
 
 CPDF_PageOrganizer::CPDF_PageOrganizer() {}
 
 CPDF_PageOrganizer::~CPDF_PageOrganizer() {}
 
 bool CPDF_PageOrganizer::PDFDocInit(CPDF_Document* pDestPDFDoc,
                                     CPDF_Document* pSrcPDFDoc) {
   if (!pDestPDFDoc || !pSrcPDFDoc)
     return false;
 
   CPDF_Dictionary* pNewRoot = pDestPDFDoc->GetRoot();
   if (!pNewRoot)
     return false;
 
-  CPDF_Dictionary* DInfoDict = pDestPDFDoc->GetInfo();
-  if (!DInfoDict)
+  CPDF_Dictionary* pDocInfoDict = pDestPDFDoc->GetInfo();
+  if (!pDocInfoDict)
     return false;
 
   CFX_ByteString producerstr;
   producerstr.Format("PDFium");
-  DInfoDict->SetFor("Producer", new CPDF_String(producerstr, false));
+  pDocInfoDict->SetFor("Producer", new CPDF_String(producerstr, false));
 
   CFX_ByteString cbRootType = pNewRoot->GetStringFor("Type", "");
   if (cbRootType.IsEmpty())
     pNewRoot->SetFor("Type", new CPDF_Name("Catalog"));
 
   CPDF_Object* pElement = pNewRoot->GetObjectFor("Pages");
   CPDF_Dictionary* pNewPages =
       pElement ? ToDictionary(pElement->GetDirect()) : nullptr;
   if (!pNewPages) {
     pNewPages = new CPDF_Dictionary(pDestPDFDoc->GetByteStringPool());
     pNewRoot->SetReferenceFor("Pages", pDestPDFDoc,
                               pDestPDFDoc->AddIndirectObject(pNewPages));
   }
 
   CFX_ByteString cbPageType = pNewPages->GetStringFor("Type", "");
-  if (cbPageType == "") {
+  if (cbPageType.IsEmpty())
     pNewPages->SetFor("Type", new CPDF_Name("Pages"));
-  }
 
   if (!pNewPages->GetArrayFor("Kids")) {
     pNewPages->SetIntegerFor("Count", 0);
     pNewPages->SetReferenceFor("Kids", pDestPDFDoc,
                                pDestPDFDoc->AddIndirectObject(new CPDF_Array));
   }
 
   return true;
 }
 
-bool CPDF_PageOrganizer::ExportPage(CPDF_Document* pSrcPDFDoc,
-                                    std::vector<uint16_t>* pPageNums,
-                                    CPDF_Document* pDestPDFDoc,
+bool CPDF_PageOrganizer::ExportPage(CPDF_Document* pDestPDFDoc,
+                                    CPDF_Document* pSrcPDFDoc,
+                                    const std::vector<uint16_t>& pageNums,
                                     int nIndex) {
   int curpage = nIndex;
-  std::unique_ptr<ObjectNumberMap> pObjNumberMap(new ObjectNumberMap);
-  int nSize = pdfium::CollectionSize<int>(*pPageNums);
+  auto pObjNumberMap = pdfium::MakeUnique<ObjectNumberMap>();
+  int nSize = pdfium::CollectionSize<int>(pageNums);
   for (int i = 0; i < nSize; ++i) {
     CPDF_Dictionary* pCurPageDict = pDestPDFDoc->CreateNewPage(curpage);
-    CPDF_Dictionary* pSrcPageDict = pSrcPDFDoc->GetPage(pPageNums->at(i) - 1);
+    CPDF_Dictionary* pSrcPageDict = pSrcPDFDoc->GetPage(pageNums[i] - 1);
     if (!pSrcPageDict || !pCurPageDict)
       return false;
 
     // Clone the page dictionary
     for (const auto& it : *pSrcPageDict) {
       const CFX_ByteString& cbSrcKeyStr = it.first;
       CPDF_Object* pObj = it.second;
       if (cbSrcKeyStr.Compare(("Type")) && cbSrcKeyStr.Compare(("Parent"))) {

[Tom Sepez, 2016/11/14 17:31:35]

nit: overparenthesized, also how about
  if (cbSrcKeyStr == "Type" || cbSrcKeyStr == "Parent")
    continue;

[Lei Zhang, 2016/11/14 19:57:01]

Done.

         if (pCurPageDict->KeyExist(cbSrcKeyStr))

[Tom Sepez, 2016/11/14 17:31:35]

No need to remove as dicts clean themselves.

Wonder if we need a CPDF_Dictionary::DeepCopyExceptForKeys() method. we have
this pattern in a couple places I think.

[Lei Zhang, 2016/11/14 19:57:01]

Done.

           pCurPageDict->RemoveFor(cbSrcKeyStr);
         pCurPageDict->SetFor(cbSrcKeyStr, pObj->Clone().release());
       }
     }
 
     // inheritable item
-    CPDF_Object* pInheritable = nullptr;
-    // 1 MediaBox  //required
+    // 1 MediaBox - required
     if (!pCurPageDict->KeyExist("MediaBox")) {
-      pInheritable = PageDictGetInheritableTag(pSrcPageDict, "MediaBox");
-      if (!pInheritable) {
-        // Search the "CropBox" from source page dictionary,
-        // if not exists,we take the letter size.
+      CPDF_Object* pInheritable =
+          PageDictGetInheritableTag(pSrcPageDict, "MediaBox");
+      if (pInheritable) {
+        pCurPageDict->SetFor("MediaBox", pInheritable->Clone().release());
+      } else {
+        // Search for "CropBox" in the source page dictionary,
+        // if it does not exists, use the default letter size.
         pInheritable = PageDictGetInheritableTag(pSrcPageDict, "CropBox");
         if (pInheritable) {
           pCurPageDict->SetFor("MediaBox", pInheritable->Clone().release());
         } else {
           // Make the default size to be letter size (8.5'x11')
           CPDF_Array* pArray = new CPDF_Array;
           pArray->AddNumber(0);
           pArray->AddNumber(0);
           pArray->AddNumber(612);
           pArray->AddNumber(792);
           pCurPageDict->SetFor("MediaBox", pArray);
         }
-      } else {
-        pCurPageDict->SetFor("MediaBox", pInheritable->Clone().release());
       }
     }
-    // 2 Resources //required
+    // 2 Resources - required
     if (!pCurPageDict->KeyExist("Resources")) {
-      pInheritable = PageDictGetInheritableTag(pSrcPageDict, "Resources");
+      CPDF_Object* pInheritable =

[Tom Sepez, 2016/11/14 17:31:35]

Helper method?

[Lei Zhang, 2016/11/14 19:57:01]

Done.

+          PageDictGetInheritableTag(pSrcPageDict, "Resources");
       if (!pInheritable)
         return false;
       pCurPageDict->SetFor("Resources", pInheritable->Clone().release());
     }
-    // 3 CropBox  //Optional
+    // 3 CropBox - optional
     if (!pCurPageDict->KeyExist("CropBox")) {
-      pInheritable = PageDictGetInheritableTag(pSrcPageDict, "CropBox");
+      CPDF_Object* pInheritable =
+          PageDictGetInheritableTag(pSrcPageDict, "CropBox");
       if (pInheritable)
         pCurPageDict->SetFor("CropBox", pInheritable->Clone().release());
     }
-    // 4 Rotate  //Optional
+    // 4 Rotate - optional
     if (!pCurPageDict->KeyExist("Rotate")) {
-      pInheritable = PageDictGetInheritableTag(pSrcPageDict, "Rotate");
+      CPDF_Object* pInheritable =
+          PageDictGetInheritableTag(pSrcPageDict, "Rotate");
       if (pInheritable)
         pCurPageDict->SetFor("Rotate", pInheritable->Clone().release());
     }
 
     // Update the reference
     uint32_t dwOldPageObj = pSrcPageDict->GetObjNum();
     uint32_t dwNewPageObj = pCurPageDict->GetObjNum();
     (*pObjNumberMap)[dwOldPageObj] = dwNewPageObj;
     UpdateReference(pCurPageDict, pDestPDFDoc, pObjNumberMap.get());
     ++curpage;
@@ skipping 69 matching lines @@
         if (!pNextObj)
           return false;
         if (!UpdateReference(pNextObj, pDoc, pObjNumberMap))
           return false;
       }
       break;
     }
     case CPDF_Object::STREAM: {
       CPDF_Stream* pStream = pObj->AsStream();
       CPDF_Dictionary* pDict = pStream->GetDict();
-      if (pDict) {
-        if (!UpdateReference(pDict, pDoc, pObjNumberMap))
-          return false;
-      } else {
+      if (!pDict)
         return false;
-      }
+      if (!UpdateReference(pDict, pDoc, pObjNumberMap))
+        return false;
       break;
     }
     default:
       break;
   }
 
   return true;
 }
 
 uint32_t CPDF_PageOrganizer::GetNewObjId(CPDF_Document* pDoc,
@@ skipping 26 matching lines @@
   }
   dwNewObjNum = pDoc->AddIndirectObject(pClone.get());
   (*pObjNumberMap)[dwObjnum] = dwNewObjNum;
   if (!UpdateReference(pClone.get(), pDoc, pObjNumberMap))
     return 0;
 
   pClone.release();  // TODO(tsepez): figure out ownership.
   return dwNewObjNum;
 }
 
 FPDF_BOOL ParserPageRangeString(CFX_ByteString rangstring,

[Tom Sepez, 2016/11/14 17:31:35]

nit: bool ?

[Lei Zhang, 2016/11/14 19:57:01]

Done.

                                 std::vector<uint16_t>* pageArray,
                                 int nCount) {
-  if (rangstring.GetLength() != 0) {
-    rangstring.Remove(' ');
-    int nLength = rangstring.GetLength();
-    CFX_ByteString cbCompareString("0123456789-,");
-    for (int i = 0; i < nLength; ++i) {
-      if (cbCompareString.Find(rangstring[i]) == -1)
+  if (rangstring.IsEmpty())
+    return true;
+
+  rangstring.Remove(' ');
+  int nLength = rangstring.GetLength();
+  CFX_ByteString cbCompareString("0123456789-,");
+  for (int i = 0; i < nLength; ++i) {
+    if (cbCompareString.Find(rangstring[i]) == -1)
+      return false;
+  }
+
+  CFX_ByteString cbMidRange;
+  int nStringFrom = 0;
+  int nStringTo = 0;
+  while (nStringTo < nLength) {
+    nStringTo = rangstring.Find(',', nStringFrom);
+    if (nStringTo == -1)
+      nStringTo = nLength;
+    cbMidRange = rangstring.Mid(nStringFrom, nStringTo - nStringFrom);
+    int nMid = cbMidRange.Find('-');
+    if (nMid == -1) {
+      long lPageNum = atol(cbMidRange.c_str());
+      if (lPageNum <= 0 || lPageNum > nCount)
         return false;
+      pageArray->push_back((uint16_t)lPageNum);
+    } else {
+      int nStartPageNum = atol(cbMidRange.Mid(0, nMid).c_str());
+      if (nStartPageNum == 0)
+        return false;
+
+      ++nMid;
+      int nEnd = cbMidRange.GetLength() - nMid;
+      if (nEnd == 0)
+        return false;
+
+      int nEndPageNum = atol(cbMidRange.Mid(nMid, nEnd).c_str());
+      if (nStartPageNum < 0 || nStartPageNum > nEndPageNum ||
+          nEndPageNum > nCount) {
+        return false;
+      }
+      for (int i = nStartPageNum; i <= nEndPageNum; ++i) {
+        pageArray->push_back(i);
+      }
     }
-    CFX_ByteString cbMidRange;
-    int nStringFrom = 0;
-    int nStringTo = 0;
-    while (nStringTo < nLength) {
-      nStringTo = rangstring.Find(',', nStringFrom);
-      if (nStringTo == -1)
-        nStringTo = nLength;
-      cbMidRange = rangstring.Mid(nStringFrom, nStringTo - nStringFrom);
-      int nMid = cbMidRange.Find('-');
-      if (nMid == -1) {
-        long lPageNum = atol(cbMidRange.c_str());
-        if (lPageNum <= 0 || lPageNum > nCount)
-          return false;
-        pageArray->push_back((uint16_t)lPageNum);
-      } else {
-        int nStartPageNum = atol(cbMidRange.Mid(0, nMid).c_str());
-        if (nStartPageNum == 0)
-          return false;
-
-        ++nMid;
-        int nEnd = cbMidRange.GetLength() - nMid;
-        if (nEnd == 0)
-          return false;
-
-        int nEndPageNum = atol(cbMidRange.Mid(nMid, nEnd).c_str());
-        if (nStartPageNum < 0 || nStartPageNum > nEndPageNum ||
-            nEndPageNum > nCount) {
-          return false;
-        }
-        for (int i = nStartPageNum; i <= nEndPageNum; ++i) {
-          pageArray->push_back(i);
-        }
-      }
-      nStringFrom = nStringTo + 1;
-    }
+    nStringFrom = nStringTo + 1;
   }
   return true;
 }
 
 DLLEXPORT FPDF_BOOL STDCALL FPDF_ImportPages(FPDF_DOCUMENT dest_doc,
                                              FPDF_DOCUMENT src_doc,
                                              FPDF_BYTESTRING pagerange,
                                              int index) {
   CPDF_Document* pDestDoc = CPDFDocumentFromFPDFDocument(dest_doc);
   if (!dest_doc)
     return false;
 
   CPDF_Document* pSrcDoc = CPDFDocumentFromFPDFDocument(src_doc);
   if (!pSrcDoc)
     return false;
 
   std::vector<uint16_t> pageArray;
   int nCount = pSrcDoc->GetPageCount();
   if (pagerange) {
     if (!ParserPageRangeString(pagerange, &pageArray, nCount))
       return false;
   } else {
     for (int i = 1; i <= nCount; ++i) {
       pageArray.push_back(i);
     }
   }
 
   CPDF_PageOrganizer pageOrg;
   pageOrg.PDFDocInit(pDestDoc, pSrcDoc);
-  return pageOrg.ExportPage(pSrcDoc, &pageArray, pDestDoc, index);
+  return pageOrg.ExportPage(pDestDoc, pSrcDoc, pageArray, index);
 }
 
 DLLEXPORT FPDF_BOOL STDCALL FPDF_CopyViewerPreferences(FPDF_DOCUMENT dest_doc,
                                                        FPDF_DOCUMENT src_doc) {
   CPDF_Document* pDstDoc = CPDFDocumentFromFPDFDocument(dest_doc);
   if (!pDstDoc)
     return false;
 
   CPDF_Document* pSrcDoc = CPDFDocumentFromFPDFDocument(src_doc);
   if (!pSrcDoc)
     return false;
 
   CPDF_Dictionary* pSrcDict = pSrcDoc->GetRoot();
   pSrcDict = pSrcDict->GetDictFor("ViewerPreferences");
   if (!pSrcDict)
     return false;
 
   CPDF_Dictionary* pDstDict = pDstDoc->GetRoot();
   if (!pDstDict)
     return false;
 
   pDstDict->SetFor("ViewerPreferences",
                    pSrcDict->CloneDirectObject().release());
   return true;
 }