Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1409013005:
Add base test for RebuildCrossRef function. (Closed)
|
Created:
5 years, 1 month ago by Wei Li Modified:
5 years, 1 month ago CC:
pdfium-reviews_googlegroups.com Base URL:
https://pdfium.googlesource.com/pdfium.git@master Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionAdd base test for RebuildCrossRef function.
This tests whether RebuildCrossRef could handle well-formatted pdf file.
R=thestig@chromium.org
Committed: https://pdfium.googlesource.com/pdfium/+/4544797e8998a31e7bc3f5439a5982f7f66dff26
Patch Set 1 #
Total comments: 16
Patch Set 2 : address comments #
Total comments: 2
Patch Set 3 : fix file detection #
Total comments: 2
Patch Set 4 : pdf format #Patch Set 5 : one more comment #
Messages
Total messages: 16 (2 generated)
weili@chromium.org changed reviewers: + thestig@chromium.org
Pls take a look, thanks.
dsinclair@chromium.org changed reviewers: + dsinclair@chromium.org
https://codereview.chromium.org/1409013005/diff/1/core/src/fpdfapi/fpdf_parse... File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser_unittests.cpp (right): https://codereview.chromium.org/1409013005/diff/1/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser_unittests.cpp:1: // Copyright 2015 PDFium Authors. All rights reserved. There is an fpdf_parser_parser_unittest.cpp file that was created, can these be added to that one? https://codereview.chromium.org/1409013005/diff/1/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser_unittests.cpp:36: ASSERT_TRUE(parser.RebuildCrossRef()); I think some of these can be EXPECT_ versions? https://codereview.chromium.org/1409013005/diff/1/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser_unittests.cpp:39: for (int i = 0; i < kNumObjs; i++) { nit: ++i nit: don't need {]'s for single line bodies.
https://codereview.chromium.org/1409013005/diff/1/core/src/fpdfapi/fpdf_parse... File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser_unittests.cpp (right): https://codereview.chromium.org/1409013005/diff/1/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser_unittests.cpp:1: // Copyright 2015 PDFium Authors. All rights reserved. On 2015/11/04 02:07:14, dsinclair wrote: > There is an fpdf_parser_parser_unittest.cpp file that was created, can these be > added to that one? Dan added it in https://codereview.chromium.org/1414793010 earlier today. https://codereview.chromium.org/1409013005/diff/1/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser_unittests.cpp:17: FRIEND_TEST(CPDF_Parser_Test, RebuildCrossRefCorrectly); Are these actually needed? https://codereview.chromium.org/1409013005/diff/1/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser_unittests.cpp:18: FRIEND_TEST(CPDF_Parser_Test, RebuildCrossRefHandleErrors); This test doesn to exist yet. https://codereview.chromium.org/1409013005/diff/1/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser_unittests.cpp:23: ASSERT_TRUE(pFileAccess); You can return false on failure, and the caller won't need ASSERT_NO_FATAL_FAILURE(). https://codereview.chromium.org/1409013005/diff/1/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser_unittests.cpp:30: const int kNumObjs = 7; You can declare |offsets| and |versions|, and use static_assert to make sure FX_ArraySize(offsets) == FX_ArraySize(versions). Then you can use FX_ArraySize() below in place of |kNumObjs|.
PTAL, thanks. https://codereview.chromium.org/1409013005/diff/1/core/src/fpdfapi/fpdf_parse... File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser_unittests.cpp (right): https://codereview.chromium.org/1409013005/diff/1/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser_unittests.cpp:1: // Copyright 2015 PDFium Authors. All rights reserved. On 2015/11/04 02:07:14, dsinclair wrote: > There is an fpdf_parser_parser_unittest.cpp file that was created, can these be > added to that one? oh, I need to follow code updates more closely. Done. https://codereview.chromium.org/1409013005/diff/1/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser_unittests.cpp:1: // Copyright 2015 PDFium Authors. All rights reserved. On 2015/11/04 02:12:39, Lei Zhang wrote: > On 2015/11/04 02:07:14, dsinclair wrote: > > There is an fpdf_parser_parser_unittest.cpp file that was created, can these > be > > added to that one? > > Dan added it in https://codereview.chromium.org/1414793010 earlier today. Done. https://codereview.chromium.org/1409013005/diff/1/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser_unittests.cpp:17: FRIEND_TEST(CPDF_Parser_Test, RebuildCrossRefCorrectly); On 2015/11/04 02:12:39, Lei Zhang wrote: > Are these actually needed? Yes, it is needed for test cases to access protected/private members. An alternative way is to expose those members through public wrapper in this class. But the tests may not seem less direct. https://codereview.chromium.org/1409013005/diff/1/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser_unittests.cpp:18: FRIEND_TEST(CPDF_Parser_Test, RebuildCrossRefHandleErrors); On 2015/11/04 02:12:39, Lei Zhang wrote: > This test doesn to exist yet. Done. https://codereview.chromium.org/1409013005/diff/1/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser_unittests.cpp:23: ASSERT_TRUE(pFileAccess); On 2015/11/04 02:12:39, Lei Zhang wrote: > You can return false on failure, and the caller won't need > ASSERT_NO_FATAL_FAILURE(). Done. https://codereview.chromium.org/1409013005/diff/1/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser_unittests.cpp:30: const int kNumObjs = 7; On 2015/11/04 02:12:39, Lei Zhang wrote: > You can declare |offsets| and |versions|, and use static_assert to make sure > FX_ArraySize(offsets) == FX_ArraySize(versions). Then you can use FX_ArraySize() > below in place of |kNumObjs|. Done. https://codereview.chromium.org/1409013005/diff/1/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser_unittests.cpp:36: ASSERT_TRUE(parser.RebuildCrossRef()); On 2015/11/04 02:07:14, dsinclair wrote: > I think some of these can be EXPECT_ versions? There is no point to continue if RebuildCrossRef could not be executed or successful. Changed the final results comparison to EXPECT_ versions. https://codereview.chromium.org/1409013005/diff/1/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser_unittests.cpp:39: for (int i = 0; i < kNumObjs; i++) { On 2015/11/04 02:07:14, dsinclair wrote: > nit: ++i > nit: don't need {]'s for single line bodies. Done.
The diff for the PDF test file looks funny. Can you try upload with a different --similarity= ? Maybe set it to 100?
https://codereview.chromium.org/1409013005/diff/20001/core/src/fpdfapi/fpdf_p... File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser_unittest.cpp (right): https://codereview.chromium.org/1409013005/diff/20001/core/src/fpdfapi/fpdf_p... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser_unittest.cpp:26: FRIEND_TEST(fpdf_parser_parser, RebuildCrossRefCorrectly); If you provide public CPDF_TestParser getter methods to return |m_CrossRef| and |m_ObjVersion|, then you don't need to be friends.
On 2015/11/04 18:01:41, Lei Zhang wrote: > The diff for the PDF test file looks funny. Can you try upload with a different > --similarity= ? Maybe set it to 100? Works, thanks.
On 2015/11/04 18:40:43, Wei wrote: > On 2015/11/04 18:01:41, Lei Zhang wrote: > > The diff for the PDF test file looks funny. Can you try upload with a > different > > --similarity= ? Maybe set it to 100? > > Works, thanks. So the PDF is just a text file?
https://codereview.chromium.org/1409013005/diff/20001/core/src/fpdfapi/fpdf_p... File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser_unittest.cpp (right): https://codereview.chromium.org/1409013005/diff/20001/core/src/fpdfapi/fpdf_p... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser_unittest.cpp:26: FRIEND_TEST(fpdf_parser_parser, RebuildCrossRefCorrectly); On 2015/11/04 18:04:26, Lei Zhang wrote: > If you provide public CPDF_TestParser getter methods to return |m_CrossRef| and > |m_ObjVersion|, then you don't need to be friends. Yes, I also need to provide a public wrapper for BuildCrossRefs(). Later, if we decide to check other protected/private variables or functions, we need to add all of them. WDYT?
On 2015/11/04 18:47:03, Wei wrote: > Yes, I also need to provide a public wrapper for BuildCrossRefs(). Later, if we > decide to check other protected/private variables or functions, we need to add > all of them. WDYT? If you are planning on adding more tests and examining more member variables, then we can stick with friends. Code looks good but I'm a bit confused about the PDF still.
lg to me. https://codereview.chromium.org/1409013005/diff/40001/core/src/fpdfapi/fpdf_p... File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser_unittest.cpp (right): https://codereview.chromium.org/1409013005/diff/40001/core/src/fpdfapi/fpdf_p... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser_unittest.cpp:26: FRIEND_TEST(fpdf_parser_parser, RebuildCrossRefCorrectly); Can you add a comment that this is private friends so we can access RebuildCrossRef in CPDF_Parser?
Turned out windows git decided to do smart diff for PDFs. :p Now fixed. https://codereview.chromium.org/1409013005/diff/40001/core/src/fpdfapi/fpdf_p... File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser_unittest.cpp (right): https://codereview.chromium.org/1409013005/diff/40001/core/src/fpdfapi/fpdf_p... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser_unittest.cpp:26: FRIEND_TEST(fpdf_parser_parser, RebuildCrossRefCorrectly); On 2015/11/04 20:19:36, dsinclair wrote: > Can you add a comment that this is private friends so we can access > RebuildCrossRef in CPDF_Parser? Done.
lgtm
Message was sent while issue was closed.
Committed patchset #5 (id:80001) manually as 4544797e8998a31e7bc3f5439a5982f7f66dff26 (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
