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

Issue 1433503002: Add test for CPDF_SyntaxParser::ReadHexString. (Closed)

Created:
5 years, 1 month ago by dsinclair
Modified:
5 years, 1 month ago
Reviewers:
Tom Sepez
CC:
pdfium-reviews_googlegroups.com
Base URL:
https://pdfium.googlesource.com/pdfium.git@master
Target Ref:
refs/heads/master
Visibility:
Public.

Description

Add test for CPDF_SyntaxParser::ReadHexString. This CL adds tests for the ReadHexString method of the syntax parser. R=tsepez@chromium.org Committed: https://pdfium.googlesource.com/pdfium/+/71ad9a0028a183605fba734c48b4756d6caa7754

Patch Set 1 #

Total comments: 13

Patch Set 2 : #

Total comments: 4

Patch Set 3 : #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+176 lines, -2 lines) Patch
M BUILD.gn View 1 chunk +1 line, -0 lines 0 comments Download
M core/include/fpdfapi/fpdf_parser.h View 1 2 chunks +4 lines, -2 lines 0 comments Download
A core/src/fpdfapi/fpdf_parser/fpdf_parser_parser_unittest.cpp View 1 2 1 chunk +170 lines, -0 lines 2 comments Download
M pdfium.gyp View 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 9 (1 generated)
dsinclair
PTAL.
5 years, 1 month ago (2015-11-03 15:03:45 UTC) #2
Tom Sepez
https://codereview.chromium.org/1433503002/diff/1/core/include/fpdfapi/fpdf_parser.h File core/include/fpdfapi/fpdf_parser.h (right): https://codereview.chromium.org/1433503002/diff/1/core/include/fpdfapi/fpdf_parser.h#newcode359 core/include/fpdfapi/fpdf_parser.h:359: private: Is there anything private in this class that ...
5 years, 1 month ago (2015-11-03 17:41:08 UTC) #3
dsinclair
We may need to re-visit this method as it doesn't really do what I'd expect ...
5 years, 1 month ago (2015-11-03 18:29:49 UTC) #4
Tom Sepez
lgtm https://codereview.chromium.org/1433503002/diff/1/core/src/fpdfapi/fpdf_parser/fpdf_parser_parser_unittest.cpp File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser_unittest.cpp (right): https://codereview.chromium.org/1433503002/diff/1/core/src/fpdfapi/fpdf_parser/fpdf_parser_parser_unittest.cpp#newcode17 core/src/fpdfapi/fpdf_parser/fpdf_parser_parser_unittest.cpp:17: uint8_t data[] = "1A2b>abcd"; On 2015/11/03 18:29:49, dsinclair ...
5 years, 1 month ago (2015-11-03 18:39:56 UTC) #5
dsinclair
https://codereview.chromium.org/1433503002/diff/20001/core/src/fpdfapi/fpdf_parser/fpdf_parser_parser_unittest.cpp File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser_unittest.cpp (right): https://codereview.chromium.org/1433503002/diff/20001/core/src/fpdfapi/fpdf_parser/fpdf_parser_parser_unittest.cpp#newcode61 core/src/fpdfapi/fpdf_parser/fpdf_parser_parser_unittest.cpp:61: uint8_t data[] = "1ab"; On 2015/11/03 18:39:56, Tom Sepez ...
5 years, 1 month ago (2015-11-03 19:08:06 UTC) #6
dsinclair
Committed patchset #3 (id:40001) manually as 71ad9a0028a183605fba734c48b4756d6caa7754 (presubmit successful).
5 years, 1 month ago (2015-11-03 19:08:36 UTC) #7
Tom Sepez
https://codereview.chromium.org/1433503002/diff/40001/core/src/fpdfapi/fpdf_parser/fpdf_parser_parser_unittest.cpp File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser_unittest.cpp (right): https://codereview.chromium.org/1433503002/diff/40001/core/src/fpdfapi/fpdf_parser/fpdf_parser_parser_unittest.cpp#newcode108 core/src/fpdfapi/fpdf_parser/fpdf_parser_parser_unittest.cpp:108: parser.RestorePos(0xfffffffffff); Nit: ULL suffix.
5 years, 1 month ago (2015-11-03 19:10:15 UTC) #8
Tom Sepez
5 years, 1 month ago (2015-11-03 19:12:10 UTC) #9
Message was sent while issue was closed.
https://codereview.chromium.org/1433503002/diff/40001/core/src/fpdfapi/fpdf_p...
File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser_unittest.cpp (right):

https://codereview.chromium.org/1433503002/diff/40001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser_unittest.cpp:108:
parser.RestorePos(0xfffffffffff);
On 2015/11/03 19:10:15, Tom Sepez wrote:
> Nit: ULL suffix.

It also dawns on me that 0xff00000001 might be a better test for trunctation.

Powered by Google App Engine
This is Rietveld 408576698