From 2aa4b2f3928ccd8393f60db8f7b740c75f4e8a8d Mon Sep 17 00:00:00 2001 From: Tom Sepez Date: Fri, 25 May 2018 22:38:49 +0000 Subject: Make CPDF_Page retainable. Small step to reducing the differences between XFA and non-XFA. We still use the RetainPtr pretty much as if it were an unique_ptr, in that we're not yet caching pages and handing out multiple pointers to the same page in the non-XFA case. The one change is in page view cleanup, where we no longer need a boolean and can take (sufficient) page ownership with a RetainPtr. Tidy up some document.h -> page.h -> document.h circular inclusion while we're at it. NOTE: Wait for imminent branch to pass before landing. We'll want this to bake a while. Change-Id: I64a2f12ac3424ece1063d40583995b834117cf34 Reviewed-on: https://pdfium-review.googlesource.com/32790 Reviewed-by: dsinclair Reviewed-by: Lei Zhang Commit-Queue: Tom Sepez --- .../edit/cpdf_pagecontentgenerator_unittest.cpp | 20 ++++----- core/fpdfapi/page/cpdf_page.h | 20 ++++----- core/fpdfapi/parser/cpdf_document.h | 3 +- fpdfsdk/cpdfsdk_pageview.cpp | 15 +------ fpdfsdk/cpdfsdk_pageview.h | 13 +++--- fpdfsdk/fpdf_editpage.cpp | 4 +- fpdfsdk/fpdf_flatten.cpp | 2 +- fpdfsdk/fpdf_ppo.cpp | 4 +- fpdfsdk/fpdf_view.cpp | 49 ++++++++++------------ fpdfsdk/fpdfxfa/cpdfxfa_page.cpp | 4 +- fpdfsdk/fpdfxfa/cpdfxfa_page.h | 7 ++-- fxjs/cjs_document.cpp | 12 +++--- 12 files changed, 65 insertions(+), 88 deletions(-) diff --git a/core/fpdfapi/edit/cpdf_pagecontentgenerator_unittest.cpp b/core/fpdfapi/edit/cpdf_pagecontentgenerator_unittest.cpp index d97e8abf0b..1244b12331 100644 --- a/core/fpdfapi/edit/cpdf_pagecontentgenerator_unittest.cpp +++ b/core/fpdfapi/edit/cpdf_pagecontentgenerator_unittest.cpp @@ -54,8 +54,8 @@ TEST_F(CPDF_PageContentGeneratorTest, ProcessRect) { pPathObj->m_FillType = FXFILL_ALTERNATE; pPathObj->m_bStroke = true; - auto pTestPage = pdfium::MakeUnique(nullptr, nullptr, false); - CPDF_PageContentGenerator generator(pTestPage.get()); + auto pTestPage = pdfium::MakeRetain(nullptr, nullptr, false); + CPDF_PageContentGenerator generator(pTestPage.Get()); std::ostringstream buf; TestProcessPath(&generator, &buf, pPathObj.get()); EXPECT_EQ("q 1 0 0 1 0 0 cm 10 5 3 25 re B* Q\n", ByteString(buf)); @@ -98,8 +98,8 @@ TEST_F(CPDF_PageContentGeneratorTest, ProcessPath) { pPathObj->m_FillType = FXFILL_WINDING; pPathObj->m_bStroke = false; - auto pTestPage = pdfium::MakeUnique(nullptr, nullptr, false); - CPDF_PageContentGenerator generator(pTestPage.get()); + auto pTestPage = pdfium::MakeRetain(nullptr, nullptr, false); + CPDF_PageContentGenerator generator(pTestPage.Get()); std::ostringstream buf; TestProcessPath(&generator, &buf, pPathObj.get()); EXPECT_EQ( @@ -129,8 +129,8 @@ TEST_F(CPDF_PageContentGeneratorTest, ProcessGraphics) { auto pDoc = pdfium::MakeUnique(nullptr); pDoc->CreateNewDoc(); CPDF_Dictionary* pPageDict = pDoc->CreateNewPage(0); - auto pTestPage = pdfium::MakeUnique(pDoc.get(), pPageDict, false); - CPDF_PageContentGenerator generator(pTestPage.get()); + auto pTestPage = pdfium::MakeRetain(pDoc.get(), pPageDict, false); + CPDF_PageContentGenerator generator(pTestPage.Get()); std::ostringstream buf; TestProcessPath(&generator, &buf, pPathObj.get()); ByteString pathString(buf); @@ -168,8 +168,8 @@ TEST_F(CPDF_PageContentGeneratorTest, ProcessStandardText) { auto pDoc = pdfium::MakeUnique(nullptr); pDoc->CreateNewDoc(); CPDF_Dictionary* pPageDict = pDoc->CreateNewPage(0); - auto pTestPage = pdfium::MakeUnique(pDoc.get(), pPageDict, false); - CPDF_PageContentGenerator generator(pTestPage.get()); + auto pTestPage = pdfium::MakeRetain(pDoc.get(), pPageDict, false); + CPDF_PageContentGenerator generator(pTestPage.Get()); auto pTextObj = pdfium::MakeUnique(); CPDF_Font* pFont = CPDF_Font::GetStockFont(pDoc.get(), "Times-Roman"); pTextObj->m_TextState.SetFont(pFont); @@ -231,8 +231,8 @@ TEST_F(CPDF_PageContentGeneratorTest, ProcessText) { auto pDoc = pdfium::MakeUnique(nullptr); pDoc->CreateNewDoc(); CPDF_Dictionary* pPageDict = pDoc->CreateNewPage(0); - auto pTestPage = pdfium::MakeUnique(pDoc.get(), pPageDict, false); - CPDF_PageContentGenerator generator(pTestPage.get()); + auto pTestPage = pdfium::MakeRetain(pDoc.get(), pPageDict, false); + CPDF_PageContentGenerator generator(pTestPage.Get()); std::ostringstream buf; { diff --git a/core/fpdfapi/page/cpdf_page.h b/core/fpdfapi/page/cpdf_page.h index 28a12f45b5..0bb99b73f1 100644 --- a/core/fpdfapi/page/cpdf_page.h +++ b/core/fpdfapi/page/cpdf_page.h @@ -10,7 +10,6 @@ #include #include "core/fpdfapi/page/cpdf_pageobjectholder.h" -#include "core/fpdfapi/parser/cpdf_document.h" #include "core/fxcrt/fx_coordinates.h" #include "core/fxcrt/fx_system.h" #include "core/fxcrt/retain_ptr.h" @@ -23,19 +22,13 @@ class CPDF_Object; class CPDF_PageRenderCache; class CPDF_PageRenderContext; -class CPDF_Page : public CPDF_PageObjectHolder { +class CPDF_Page : public Retainable, public CPDF_PageObjectHolder { public: class View {}; // Caller implements as desired, empty here due to layering. + class Extension : public Retainable {}; // XFA page parent class, layering. - // XFA page parent class, layering. - class Extension : public Retainable { - virtual CPDF_Document::Extension* GetDocumentExtension() const = 0; - }; - - CPDF_Page(CPDF_Document* pDocument, - CPDF_Dictionary* pPageDict, - bool bPageCache); - ~CPDF_Page() override; + template + friend RetainPtr pdfium::MakeRetain(Args&&... args); // CPDF_PageObjectHolder: bool IsPage() const override; @@ -71,6 +64,11 @@ class CPDF_Page : public CPDF_PageObjectHolder { void SetPageExtension(Extension* pExt) { m_pPageExtension = pExt; } private: + CPDF_Page(CPDF_Document* pDocument, + CPDF_Dictionary* pPageDict, + bool bPageCache); + ~CPDF_Page() override; + void StartParse(); CPDF_Object* GetPageAttr(const ByteString& name) const; diff --git a/core/fpdfapi/parser/cpdf_document.h b/core/fpdfapi/parser/cpdf_document.h index 356d341302..013eded765 100644 --- a/core/fpdfapi/parser/cpdf_document.h +++ b/core/fpdfapi/parser/cpdf_document.h @@ -14,6 +14,7 @@ #include #include "core/fpdfapi/page/cpdf_image.h" +#include "core/fpdfapi/page/cpdf_page.h" #include "core/fpdfapi/parser/cpdf_indirect_object_holder.h" #include "core/fpdfapi/parser/cpdf_object.h" #include "core/fpdfdoc/cpdf_linklist.h" @@ -165,7 +166,7 @@ class CPDF_Document : public CPDF_IndirectObjectHolder { std::unique_ptr m_pDocRender; std::unique_ptr m_pCodecContext; std::unique_ptr m_pLinksContext; - std::vector m_PageList; + std::vector m_PageList; // Page number to page's dict objnum. UnownedPtr m_pExtension; }; diff --git a/fpdfsdk/cpdfsdk_pageview.cpp b/fpdfsdk/cpdfsdk_pageview.cpp index 2eab6ca32b..c86adf323d 100644 --- a/fpdfsdk/cpdfsdk_pageview.cpp +++ b/fpdfsdk/cpdfsdk_pageview.cpp @@ -32,15 +32,7 @@ CPDFSDK_PageView::CPDFSDK_PageView(CPDFSDK_FormFillEnvironment* pFormFillEnv, UnderlyingPageType* page) - : m_page(page), - m_pFormFillEnv(pFormFillEnv), -#ifndef PDF_ENABLE_XFA - m_bOwnsPage(false), -#endif // PDF_ENABLE_XFA - m_bOnWidget(false), - m_bValid(false), - m_bLocked(false), - m_bBeingDestroyed(false) { + : m_page(page), m_pFormFillEnv(pFormFillEnv) { CPDFSDK_InterForm* pInterForm = pFormFillEnv->GetInterForm(); CPDF_InterForm* pPDFInterForm = pInterForm->GetInterForm(); #ifdef PDF_ENABLE_XFA @@ -67,11 +59,6 @@ CPDFSDK_PageView::~CPDFSDK_PageView() { m_SDKAnnotArray.clear(); m_pAnnotList.reset(); - -#ifndef PDF_ENABLE_XFA - if (m_bOwnsPage) - delete m_page; -#endif // PDF_ENABLE_XFA } void CPDFSDK_PageView::PageView_OnDraw(CFX_RenderDevice* pDevice, diff --git a/fpdfsdk/cpdfsdk_pageview.h b/fpdfsdk/cpdfsdk_pageview.h index e85b32aa78..afb200e43b 100644 --- a/fpdfsdk/cpdfsdk_pageview.h +++ b/fpdfsdk/cpdfsdk_pageview.h @@ -102,8 +102,7 @@ class CPDFSDK_PageView final : public CPDF_Page::View { bool IsBeingDestroyed() const { return m_bBeingDestroyed; } #ifndef PDF_ENABLE_XFA - bool OwnsPage() const { return m_bOwnsPage; } - void TakePageOwnership() { m_bOwnsPage = true; } + void TakePageOwnership() { m_pOwnsPage.Reset(m_page); } #endif // PDF_ENABLE_XFA private: @@ -126,12 +125,12 @@ class CPDFSDK_PageView final : public CPDF_Page::View { UnownedPtr const m_pFormFillEnv; CPDFSDK_Annot::ObservedPtr m_pCaptureWidget; #ifndef PDF_ENABLE_XFA - bool m_bOwnsPage; + RetainPtr m_pOwnsPage; #endif // PDF_ENABLE_XFA - bool m_bOnWidget; - bool m_bValid; - bool m_bLocked; - bool m_bBeingDestroyed; + bool m_bOnWidget = false; + bool m_bValid = false; + bool m_bLocked = false; + bool m_bBeingDestroyed = false; }; #endif // FPDFSDK_CPDFSDK_PAGEVIEW_H_ diff --git a/fpdfsdk/fpdf_editpage.cpp b/fpdfsdk/fpdf_editpage.cpp index ea4659f54a..39cf85fb95 100644 --- a/fpdfsdk/fpdf_editpage.cpp +++ b/fpdfsdk/fpdf_editpage.cpp @@ -196,9 +196,9 @@ FPDF_EXPORT FPDF_PAGE FPDF_CALLCONV FPDFPage_New(FPDF_DOCUMENT document, // Eventually, fallthru into non-XFA case once page type is consistent. return nullptr; #else // PDF_ENABLE_XFA - auto pPage = pdfium::MakeUnique(pDoc, pPageDict, true); + auto pPage = pdfium::MakeRetain(pDoc, pPageDict, true); pPage->ParseContent(); - return FPDFPageFromUnderlying(pPage.release()); // Caller takes ownership. + return FPDFPageFromUnderlying(pPage.Leak()); // Caller takes ownership. #endif // PDF_ENABLE_XFA } diff --git a/fpdfsdk/fpdf_flatten.cpp b/fpdfsdk/fpdf_flatten.cpp index 03a4f60b5c..720fa9ff55 100644 --- a/fpdfsdk/fpdf_flatten.cpp +++ b/fpdfsdk/fpdf_flatten.cpp @@ -47,7 +47,7 @@ bool IsValidRect(const CFX_FloatRect& rect, const CFX_FloatRect& rcPage) { void GetContentsRect(CPDF_Document* pDoc, CPDF_Dictionary* pDict, std::vector* pRectArray) { - auto pPDFPage = pdfium::MakeUnique(pDoc, pDict, false); + auto pPDFPage = pdfium::MakeRetain(pDoc, pDict, false); pPDFPage->ParseContent(); for (const auto& pPageObject : *pPDFPage->GetPageObjectList()) { diff --git a/fpdfsdk/fpdf_ppo.cpp b/fpdfsdk/fpdf_ppo.cpp index 1790e17fb3..b73042466b 100644 --- a/fpdfsdk/fpdf_ppo.cpp +++ b/fpdfsdk/fpdf_ppo.cpp @@ -618,9 +618,9 @@ bool CPDF_NPageToOneExporter::ExportNPagesToOne( if (!pSrcPageDict) return false; - CPDF_Page srcPage(src(), pSrcPageDict, true); + auto srcPage = pdfium::MakeRetain(src(), pSrcPageDict, true); NupPageSettings settings = - nupState.CalculateNewPagePosition(srcPage.GetPageSize()); + nupState.CalculateNewPagePosition(srcPage->GetPageSize()); AddSubPage(pSrcPageDict, settings, &objectNumberMap, &pageXObjectMap, &xObjNameNumberMap, &bsContent); } diff --git a/fpdfsdk/fpdf_view.cpp b/fpdfsdk/fpdf_view.cpp index 6c4ad6020e..593a092fd3 100644 --- a/fpdfsdk/fpdf_view.cpp +++ b/fpdfsdk/fpdf_view.cpp @@ -355,9 +355,9 @@ FPDF_EXPORT FPDF_PAGE FPDF_CALLCONV FPDF_LoadPage(FPDF_DOCUMENT document, if (!pDict) return nullptr; - CPDF_Page* pPage = new CPDF_Page(pDoc, pDict, true); + auto pPage = pdfium::MakeRetain(pDoc, pDict, true); pPage->ParseContent(); - return FPDFPageFromUnderlying(pPage); + return FPDFPageFromUnderlying(pPage.Leak()); #endif // PDF_ENABLE_XFA } @@ -723,35 +723,28 @@ FPDF_EXPORT FPDF_RECORDER FPDF_CALLCONV FPDF_RenderPageSkp(FPDF_PAGE page, #endif FPDF_EXPORT void FPDF_CALLCONV FPDF_ClosePage(FPDF_PAGE page) { - UnderlyingPageType* pPage = UnderlyingFromFPDFPage(page); if (!page) return; -#ifdef PDF_ENABLE_XFA - // Take it back across the API and throw it away. - RetainPtr().Unleak(pPage); -#else // PDF_ENABLE_XFA + + // Take it back across the API and hold for duration of this function. + RetainPtr pPage; + pPage.Unleak(UnderlyingFromFPDFPage(page)); + +#ifndef PDF_ENABLE_XFA CPDFSDK_PageView* pPageView = static_cast(pPage->GetView()); - if (pPageView) { - // We're already destroying the pageview, so bail early. - if (pPageView->IsBeingDestroyed()) - return; - - if (pPageView->IsLocked()) { - pPageView->TakePageOwnership(); - return; - } + if (!pPageView || pPageView->IsBeingDestroyed()) + return; - bool owned = pPageView->OwnsPage(); - // This will delete the |pPageView| object. We must cleanup the PageView - // first because it will attempt to reset the View on the |pPage| during - // destruction. - pPageView->GetFormFillEnv()->RemovePageView(pPage); - // If the page was owned then the pageview will have deleted the page. - if (owned) - return; + if (pPageView->IsLocked()) { + pPageView->TakePageOwnership(); + return; } - delete pPage; + + // This will delete the |pPageView| object. We must cleanup the PageView + // first because it will attempt to reset the View on the |pPage| during + // destruction. + pPageView->GetFormFillEnv()->RemovePageView(pPage.Get()); #endif // PDF_ENABLE_XFA } @@ -970,9 +963,9 @@ FPDF_EXPORT int FPDF_CALLCONV FPDF_GetPageSizeByIndex(FPDF_DOCUMENT document, if (!pDict) return false; - CPDF_Page page(pDoc, pDict, true); - *width = page.GetPageWidth(); - *height = page.GetPageHeight(); + auto page = pdfium::MakeRetain(pDoc, pDict, true); + *width = page->GetPageWidth(); + *height = page->GetPageHeight(); return true; } diff --git a/fpdfsdk/fpdfxfa/cpdfxfa_page.cpp b/fpdfsdk/fpdfxfa/cpdfxfa_page.cpp index 170f17ea7a..4d7e3bc222 100644 --- a/fpdfsdk/fpdfxfa/cpdfxfa_page.cpp +++ b/fpdfsdk/fpdfxfa/cpdfxfa_page.cpp @@ -35,7 +35,7 @@ bool CPDFXFA_Page::LoadPDFPage() { return false; if (!m_pPDFPage || m_pPDFPage->GetFormDict() != pDict) { - m_pPDFPage = pdfium::MakeUnique(pPDFDoc, pDict, true); + m_pPDFPage = pdfium::MakeRetain(pPDFDoc, pDict, true); m_pPDFPage->ParseContent(); } return true; @@ -81,7 +81,7 @@ bool CPDFXFA_Page::LoadPDFPage(CPDF_Dictionary* pageDict) { return false; m_pPDFPage = - pdfium::MakeUnique(m_pContext->GetPDFDoc(), pageDict, true); + pdfium::MakeRetain(m_pContext->GetPDFDoc(), pageDict, true); m_pPDFPage->ParseContent(); return true; } diff --git a/fpdfsdk/fpdfxfa/cpdfxfa_page.h b/fpdfsdk/fpdfxfa/cpdfxfa_page.h index 4f4d6b0a22..2573b9cebd 100644 --- a/fpdfsdk/fpdfxfa/cpdfxfa_page.h +++ b/fpdfsdk/fpdfxfa/cpdfxfa_page.h @@ -29,11 +29,10 @@ class CPDFXFA_Page : public CPDF_Page::Extension { bool LoadPage(); bool LoadPDFPage(CPDF_Dictionary* pageDict); - // CPDF_Page::Extension: - CPDF_Document::Extension* GetDocumentExtension() const override; + CPDF_Document::Extension* GetDocumentExtension() const; int GetPageIndex() const { return m_iPageIndex; } - CPDF_Page* GetPDFPage() const { return m_pPDFPage.get(); } + CPDF_Page* GetPDFPage() const { return m_pPDFPage.Get(); } CXFA_FFPageView* GetXFAPageView() const { return m_pXFAPageView; } void SetXFAPageView(CXFA_FFPageView* pPageView) { @@ -61,7 +60,7 @@ class CPDFXFA_Page : public CPDF_Page::Extension { bool LoadXFAPageView(); private: - std::unique_ptr m_pPDFPage; + RetainPtr m_pPDFPage; CXFA_FFPageView* m_pXFAPageView; UnownedPtr const m_pContext; const int m_iPageIndex; diff --git a/fxjs/cjs_document.cpp b/fxjs/cjs_document.cpp index b90794a88d..66577062a6 100644 --- a/fxjs/cjs_document.cpp +++ b/fxjs/cjs_document.cpp @@ -1256,12 +1256,12 @@ CJS_Return CJS_Document::getPageNthWord( if (!pPageDict) return CJS_Return(false); - CPDF_Page page(pDocument, pPageDict, true); - page.ParseContent(); + auto page = pdfium::MakeRetain(pDocument, pPageDict, true); + page->ParseContent(); int nWords = 0; WideString swRet; - for (auto& pPageObj : *page.GetPageObjectList()) { + for (auto& pPageObj : *page->GetPageObjectList()) { if (pPageObj->IsText()) { CPDF_TextObject* pTextObj = pPageObj->AsText(); int nObjWords = CountWords(pTextObj); @@ -1305,11 +1305,11 @@ CJS_Return CJS_Document::getPageNumWords( if (!pPageDict) return CJS_Return(false); - CPDF_Page page(pDocument, pPageDict, true); - page.ParseContent(); + auto page = pdfium::MakeRetain(pDocument, pPageDict, true); + page->ParseContent(); int nWords = 0; - for (auto& pPageObj : *page.GetPageObjectList()) { + for (auto& pPageObj : *page->GetPageObjectList()) { if (pPageObj->IsText()) nWords += CountWords(pPageObj->AsText()); } -- cgit v1.2.3