Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(18)

Issue 2435783006: Add CPDF_Document::GetPage() unittests (Closed)

Created:
4 years, 2 months ago by npm
Modified:
4 years, 2 months ago
Reviewers:
Lei Zhang, Tom Sepez, Wei Li
CC:
pdfium-reviews_googlegroups.com
Target Ref:
refs/heads/master
Project:
pdfium
Visibility:
Public.

Description

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/+/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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+104 lines, -3 lines) Patch
M BUILD.gn View 1 chunk +1 line, -0 lines 0 comments Download
M core/fpdfapi/parser/cpdf_document.h View 1 2 1 chunk +1 line, -3 lines 0 comments Download
A core/fpdfapi/parser/cpdf_document_unittest.cpp View 1 2 3 1 chunk +102 lines, -0 lines 0 comments Download

Messages

Total messages: 30 (20 generated)
Tom Sepez
great https://codereview.chromium.org/2435783006/diff/20001/core/fpdfapi/parser/cpdf_document.h File core/fpdfapi/parser/cpdf_document.h (right): https://codereview.chromium.org/2435783006/diff/20001/core/fpdfapi/parser/cpdf_document.h#newcode107 core/fpdfapi/parser/cpdf_document.h:107: friend class CPDF_DocumentTest; nit: can we call this ...
4 years, 2 months ago (2016-10-21 21:31:40 UTC) #9
Tom Sepez
https://codereview.chromium.org/2435783006/diff/20001/core/fpdfapi/parser/cpdf_document.h File core/fpdfapi/parser/cpdf_document.h (right): https://codereview.chromium.org/2435783006/diff/20001/core/fpdfapi/parser/cpdf_document.h#newcode105 core/fpdfapi/parser/cpdf_document.h:105: private: maybe this just becomes protected:, as we have ...
4 years, 2 months ago (2016-10-21 21:33:38 UTC) #10
npm
https://codereview.chromium.org/2435783006/diff/20001/core/fpdfapi/parser/cpdf_document.h File core/fpdfapi/parser/cpdf_document.h (right): https://codereview.chromium.org/2435783006/diff/20001/core/fpdfapi/parser/cpdf_document.h#newcode105 core/fpdfapi/parser/cpdf_document.h:105: private: On 2016/10/21 21:33:38, Tom Sepez wrote: > maybe ...
4 years, 2 months ago (2016-10-21 21:46:00 UTC) #13
Tom Sepez
https://codereview.chromium.org/2435783006/diff/20001/core/fpdfapi/parser/cpdf_document.h File core/fpdfapi/parser/cpdf_document.h (right): https://codereview.chromium.org/2435783006/diff/20001/core/fpdfapi/parser/cpdf_document.h#newcode105 core/fpdfapi/parser/cpdf_document.h:105: private: On 2016/10/21 21:46:00, npm wrote: > On 2016/10/21 ...
4 years, 2 months ago (2016-10-21 21:48:33 UTC) #14
npm
PTAL https://codereview.chromium.org/2435783006/diff/20001/core/fpdfapi/parser/cpdf_document.h File core/fpdfapi/parser/cpdf_document.h (right): https://codereview.chromium.org/2435783006/diff/20001/core/fpdfapi/parser/cpdf_document.h#newcode105 core/fpdfapi/parser/cpdf_document.h:105: private: On 2016/10/21 21:48:33, Tom Sepez wrote: > ...
4 years, 2 months ago (2016-10-21 22:12:04 UTC) #17
Tom Sepez
https://codereview.chromium.org/2435783006/diff/40001/core/fpdfapi/parser/cpdf_document_unittest.cpp File core/fpdfapi/parser/cpdf_document_unittest.cpp (right): https://codereview.chromium.org/2435783006/diff/40001/core/fpdfapi/parser/cpdf_document_unittest.cpp#newcode79 core/fpdfapi/parser/cpdf_document_unittest.cpp:79: CPDF_TestDocumentForPages* document = new CPDF_TestDocumentForPages(); Why a pointer? wouldn't ...
4 years, 2 months ago (2016-10-21 22:25:33 UTC) #20
npm
https://codereview.chromium.org/2435783006/diff/40001/core/fpdfapi/parser/cpdf_document_unittest.cpp File core/fpdfapi/parser/cpdf_document_unittest.cpp (right): https://codereview.chromium.org/2435783006/diff/40001/core/fpdfapi/parser/cpdf_document_unittest.cpp#newcode79 core/fpdfapi/parser/cpdf_document_unittest.cpp:79: CPDF_TestDocumentForPages* document = new CPDF_TestDocumentForPages(); On 2016/10/21 22:25:33, Tom ...
4 years, 2 months ago (2016-10-21 22:39:25 UTC) #23
Tom Sepez
lgtm
4 years, 2 months ago (2016-10-21 22:39:36 UTC) #24
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2435783006/60001
4 years, 2 months ago (2016-10-21 23:00:39 UTC) #28
commit-bot: I haz the power
4 years, 2 months ago (2016-10-21 23:02:19 UTC) #30
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as
https://pdfium.googlesource.com/pdfium/+/3cad596c55cd4db64e002aba118904f65c38...

Powered by Google App Engine
This is Rietveld 408576698