|
|
DescriptionAdd CPDF_Document::GetPage() unittests
Added a nontrivial page tree and a test that pages are being fetched
properly, both when requested in order and in reverse order. This will
help prevent introducing bugs while changing the way the page tree is
processed.
BUG=chromium:638513
Committed: https://pdfium.googlesource.com/pdfium/+/3cad596c55cd4db64e002aba118904f65c385168
Patch Set 1 #Patch Set 2 : Try fix win #
Total comments: 15
Patch Set 3 : Comments #
Total comments: 2
Patch Set 4 : Use unique_ptr #
Messages
Total messages: 30 (20 generated)
The CQ bit was checked by npm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win on master.tryserver.client.pdfium (JOB_FAILED, https://build.chromium.org/p/tryserver.client.pdfium/builders/win/builds/2541)
The CQ bit was checked by npm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Add CPDF_Document::GetPage() unittests Added a nontrivial page tree and a test that pages are being fetched properly, both when requested in order and in reverse order. This will help prevent introducing bugs while changing the way the page tree is processed. ========== to ========== Add CPDF_Document::GetPage() unittests Added a nontrivial page tree and a test that pages are being fetched properly, both when requested in order and in reverse order. This will help prevent introducing bugs while changing the way the page tree is processed. BUG=chromium:638513 ==========
npm@chromium.org changed reviewers: + thestig@chromium.org, tsepez@chromium.org, weili@chromium.org
great https://codereview.chromium.org/2435783006/diff/20001/core/fpdfapi/parser/cpd... File core/fpdfapi/parser/cpdf_document.h (right): https://codereview.chromium.org/2435783006/diff/20001/core/fpdfapi/parser/cpd... core/fpdfapi/parser/cpdf_document.h:107: friend class CPDF_DocumentTest; nit: can we call this class CPDF_TestDocumentForPages ? https://codereview.chromium.org/2435783006/diff/20001/core/fpdfapi/parser/cpd... File core/fpdfapi/parser/cpdf_document_unittest.cpp (right): https://codereview.chromium.org/2435783006/diff/20001/core/fpdfapi/parser/cpd... core/fpdfapi/parser/cpdf_document_unittest.cpp:44: CPDF_Document* testDoc = new CPDF_Document(std::unique_ptr<CPDF_Parser>()); Are you sure you don't want to inherit from CPDF_Document instead? Then all this can happen in its ctor. I think you can just pass nullptr here and the unique_ptr ctor is implicit. https://codereview.chromium.org/2435783006/diff/20001/core/fpdfapi/parser/cpd... core/fpdfapi/parser/cpdf_document_unittest.cpp:46: CPDF_Dictionary* pages[7]; nit: probably don't need this local, later on just testDoc->AddIndirectObject(CreateNumberedPage(0)); https://codereview.chromium.org/2435783006/diff/20001/core/fpdfapi/parser/cpd... core/fpdfapi/parser/cpdf_document_unittest.cpp:89: ASSERT_TRUE(page); probably don't need this one, segv on next line etc. https://codereview.chromium.org/2435783006/diff/20001/core/fpdfapi/parser/cpd... core/fpdfapi/parser/cpdf_document_unittest.cpp:90: ASSERT_TRUE(page->GetObjectFor("PageNumbering")); probably should be expect_true.
https://codereview.chromium.org/2435783006/diff/20001/core/fpdfapi/parser/cpd... File core/fpdfapi/parser/cpdf_document.h (right): https://codereview.chromium.org/2435783006/diff/20001/core/fpdfapi/parser/cpd... core/fpdfapi/parser/cpdf_document.h:105: private: maybe this just becomes protected:, as we have subclasses for testing.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2435783006/diff/20001/core/fpdfapi/parser/cpd... File core/fpdfapi/parser/cpdf_document.h (right): https://codereview.chromium.org/2435783006/diff/20001/core/fpdfapi/parser/cpd... core/fpdfapi/parser/cpdf_document.h:105: private: On 2016/10/21 21:33:38, Tom Sepez wrote: > maybe this just becomes protected:, as we have subclasses for testing. protected and no friends is preferred then? https://codereview.chromium.org/2435783006/diff/20001/core/fpdfapi/parser/cpd... File core/fpdfapi/parser/cpdf_document_unittest.cpp (right): https://codereview.chromium.org/2435783006/diff/20001/core/fpdfapi/parser/cpd... core/fpdfapi/parser/cpdf_document_unittest.cpp:89: ASSERT_TRUE(page); On 2016/10/21 21:31:40, Tom Sepez wrote: > probably don't need this one, segv on next line etc. I thought we prefer not to crash if possible (ie to run all the tests) https://codereview.chromium.org/2435783006/diff/20001/core/fpdfapi/parser/cpd... core/fpdfapi/parser/cpdf_document_unittest.cpp:90: ASSERT_TRUE(page->GetObjectFor("PageNumbering")); On 2016/10/21 21:31:40, Tom Sepez wrote: > probably should be expect_true. Ditto: if this is expect, then it will crash on the next expect_eq. In that case, might as well get rid of the assert_true altogether.
https://codereview.chromium.org/2435783006/diff/20001/core/fpdfapi/parser/cpd... File core/fpdfapi/parser/cpdf_document.h (right): https://codereview.chromium.org/2435783006/diff/20001/core/fpdfapi/parser/cpd... core/fpdfapi/parser/cpdf_document.h:105: private: On 2016/10/21 21:46:00, npm wrote: > On 2016/10/21 21:33:38, Tom Sepez wrote: > > maybe this just becomes protected:, as we have subclasses for testing. > > protected and no friends is preferred then? either way. https://codereview.chromium.org/2435783006/diff/20001/core/fpdfapi/parser/cpd... File core/fpdfapi/parser/cpdf_document_unittest.cpp (right): https://codereview.chromium.org/2435783006/diff/20001/core/fpdfapi/parser/cpd... core/fpdfapi/parser/cpdf_document_unittest.cpp:89: ASSERT_TRUE(page); On 2016/10/21 21:46:00, npm wrote: > On 2016/10/21 21:31:40, Tom Sepez wrote: > > probably don't need this one, segv on next line etc. > > I thought we prefer not to crash if possible (ie to run all the tests) Acknowledged. Let's leave it then
The CQ bit was checked by npm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL https://codereview.chromium.org/2435783006/diff/20001/core/fpdfapi/parser/cpd... File core/fpdfapi/parser/cpdf_document.h (right): https://codereview.chromium.org/2435783006/diff/20001/core/fpdfapi/parser/cpd... core/fpdfapi/parser/cpdf_document.h:105: private: On 2016/10/21 21:48:33, Tom Sepez wrote: > On 2016/10/21 21:46:00, npm wrote: > > On 2016/10/21 21:33:38, Tom Sepez wrote: > > > maybe this just becomes protected:, as we have subclasses for testing. > > > > protected and no friends is preferred then? > > either way. Done. https://codereview.chromium.org/2435783006/diff/20001/core/fpdfapi/parser/cpd... core/fpdfapi/parser/cpdf_document.h:107: friend class CPDF_DocumentTest; On 2016/10/21 21:31:40, Tom Sepez wrote: > nit: can we call this class CPDF_TestDocumentForPages ? Done. https://codereview.chromium.org/2435783006/diff/20001/core/fpdfapi/parser/cpd... File core/fpdfapi/parser/cpdf_document_unittest.cpp (right): https://codereview.chromium.org/2435783006/diff/20001/core/fpdfapi/parser/cpd... core/fpdfapi/parser/cpdf_document_unittest.cpp:44: CPDF_Document* testDoc = new CPDF_Document(std::unique_ptr<CPDF_Parser>()); On 2016/10/21 21:31:40, Tom Sepez wrote: > Are you sure you don't want to inherit from CPDF_Document instead? > Then all this can happen in its ctor. > > I think you can just pass nullptr here and the unique_ptr ctor is implicit. Done. https://codereview.chromium.org/2435783006/diff/20001/core/fpdfapi/parser/cpd... core/fpdfapi/parser/cpdf_document_unittest.cpp:46: CPDF_Dictionary* pages[7]; On 2016/10/21 21:31:40, Tom Sepez wrote: > nit: probably don't need this local, later on just > > testDoc->AddIndirectObject(CreateNumberedPage(0)); Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2435783006/diff/40001/core/fpdfapi/parser/cpd... File core/fpdfapi/parser/cpdf_document_unittest.cpp (right): https://codereview.chromium.org/2435783006/diff/40001/core/fpdfapi/parser/cpd... core/fpdfapi/parser/cpdf_document_unittest.cpp:79: CPDF_TestDocumentForPages* document = new CPDF_TestDocumentForPages(); Why a pointer? wouldn't this leak?
The CQ bit was checked by npm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2435783006/diff/40001/core/fpdfapi/parser/cpd... File core/fpdfapi/parser/cpdf_document_unittest.cpp (right): https://codereview.chromium.org/2435783006/diff/40001/core/fpdfapi/parser/cpd... core/fpdfapi/parser/cpdf_document_unittest.cpp:79: CPDF_TestDocumentForPages* document = new CPDF_TestDocumentForPages(); On 2016/10/21 22:25:33, Tom Sepez wrote: > Why a pointer? wouldn't this leak? Using unique_ptr instead of pointer
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by npm@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Add CPDF_Document::GetPage() unittests Added a nontrivial page tree and a test that pages are being fetched properly, both when requested in order and in reverse order. This will help prevent introducing bugs while changing the way the page tree is processed. BUG=chromium:638513 ========== to ========== Add CPDF_Document::GetPage() unittests Added a nontrivial page tree and a test that pages are being fetched properly, both when requested in order and in reverse order. This will help prevent introducing bugs while changing the way the page tree is processed. BUG=chromium:638513 Committed: https://pdfium.googlesource.com/pdfium/+/3cad596c55cd4db64e002aba118904f65c38... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://pdfium.googlesource.com/pdfium/+/3cad596c55cd4db64e002aba118904f65c38... |