Split off CPDF_Parser and CPDF_SimpleParser into .h/.cpp files

Split off CPDF_Parser and CPDF_SimpleParser into .h/.cpp files

R=dsinclair@chromium.org

Committed: https://pdfium.googlesource.com/pdfium/+/8388037a5c58d60043b11c03a8efe78c54c65a4b

[Tom Sepez, 2016-03-08 23:12:46]

tsepez@chromium.org changed reviewers:
+ dsinclair@chromium.org

[Tom Sepez, 2016-03-08 23:12:46]

Dan, next one.

[dsinclair, 2016-03-09 15:24:58]

https://codereview.chromium.org/1773103003/diff/1/core/include/fpdfapi/cpdf_s...
File core/include/fpdfapi/cpdf_simple_parser.h (right):

https://codereview.chromium.org/1773103003/diff/1/core/include/fpdfapi/cpdf_s...
core/include/fpdfapi/cpdf_simple_parser.h:25: FX_DWORD GetCurPos() const {
return m_dwCurPos; }
As an aside, what about renaming these to ForTesting, so GetCurPosForTesting()
which makes it obvious from the calling side it's test only.

Not for this CL, but add a tracking bug?

https://codereview.chromium.org/1773103003/diff/1/core/include/fpdfapi/cpdf_s...
core/include/fpdfapi/cpdf_simple_parser.h:28: void ParseWord(const uint8_t*&
pStart, FX_DWORD& dwSize);
uint8_t*& ? A reference to a pointer?

https://codereview.chromium.org/1773103003/diff/1/core/src/fpdfapi/fpdf_parse...
File core/src/fpdfapi/fpdf_parser/cpdf_parser.cpp (right):

https://codereview.chromium.org/1773103003/diff/1/core/src/fpdfapi/fpdf_parse...
core/src/fpdfapi/fpdf_parser/cpdf_parser.cpp:10: #include
"core/include/fpdfapi/cpdf_document.h"
nit: Sorting

https://codereview.chromium.org/1773103003/diff/1/core/src/fpdfapi/fpdf_parse...
core/src/fpdfapi/fpdf_parser/cpdf_parser.cpp:871: if (byte == '\r' || byte ==
'\n')
PDFCharIsLineEnding(byte)

https://codereview.chromium.org/1773103003/diff/1/core/src/fpdfapi/fpdf_parse...
File core/src/fpdfapi/fpdf_parser/cpdf_simple_parser.cpp (right):

https://codereview.chromium.org/1773103003/diff/1/core/src/fpdfapi/fpdf_parse...
core/src/fpdfapi/fpdf_parser/cpdf_simple_parser.cpp:12: m_pData = pData;
Move these to : m_pData(pData), m_dwSize(dwSize), m_dwCurPos(0) ?

https://codereview.chromium.org/1773103003/diff/1/core/src/fpdfapi/fpdf_parse...
core/src/fpdfapi/fpdf_parser/cpdf_simple_parser.cpp:17:
CPDF_SimpleParser::CPDF_SimpleParser(const CFX_ByteStringC& str) {
: m_pData(str.GetPtr()), m_dwSize(str.GetLength()), m_dwCurPos(0)

https://codereview.chromium.org/1773103003/diff/1/core/src/fpdfapi/fpdf_parse...
core/src/fpdfapi/fpdf_parser/cpdf_simple_parser.cpp:24: pStart = NULL;
nit: nullptr

https://codereview.chromium.org/1773103003/diff/1/core/src/fpdfapi/fpdf_parse...
core/src/fpdfapi/fpdf_parser/cpdf_simple_parser.cpp:44: if (ch == '\r' || ch ==
'\n')
PDFCharIsLineEnding(ch)

https://codereview.chromium.org/1773103003/diff/1/core/src/fpdfapi/fpdf_parse...
File core/src/fpdfapi/fpdf_parser/fpdf_parser_utility_unittest.cpp (right):

https://codereview.chromium.org/1773103003/diff/1/core/src/fpdfapi/fpdf_parse...
core/src/fpdfapi/fpdf_parser/fpdf_parser_utility_unittest.cpp:10: #include
"core/include/fpdfapi/cpdf_simple_parser.h"
nit: sort order

https://codereview.chromium.org/1773103003/diff/1/core/src/fpdfapi/fpdf_parse...
core/src/fpdfapi/fpdf_parser/fpdf_parser_utility_unittest.cpp:14:
TEST(SimpleParserTest, GetWord) {
This test file should probably be renamed to cpdf_simple_parser_unittest.cpp
yea? Or, the test cases should be renamed from SimpleParserTest

https://codereview.chromium.org/1773103003/diff/1/fpdfsdk/include/fsdk_define.h
File fpdfsdk/include/fsdk_define.h (right):

https://codereview.chromium.org/1773103003/diff/1/fpdfsdk/include/fsdk_define...
fpdfsdk/include/fsdk_define.h:13: #include "core/include/fpdfapi/fpdf_parser.h"
For the files where we added cpdf_parser or cpdf_simple_parser, can we remove
the fpdf_parser.h include or is other stuff from there still needed?

(Or, is the plan to just remove everything from fpdf_parser.h and we'll do a
pass at the end to remove the include when the header is removed?)

[Tom Sepez, 2016-03-09 18:39:30]

PTAAL.

https://codereview.chromium.org/1773103003/diff/1/core/include/fpdfapi/cpdf_s...
File core/include/fpdfapi/cpdf_simple_parser.h (right):

https://codereview.chromium.org/1773103003/diff/1/core/include/fpdfapi/cpdf_s...
core/include/fpdfapi/cpdf_simple_parser.h:25: FX_DWORD GetCurPos() const {
return m_dwCurPos; }
On 2016/03/09 15:24:57, dsinclair wrote:
> As an aside, what about renaming these to ForTesting, so GetCurPosForTesting()
> which makes it obvious from the calling side it's test only.
> 
> Not for this CL, but add a tracking bug?

Acknowledged.

https://codereview.chromium.org/1773103003/diff/1/core/include/fpdfapi/cpdf_s...
core/include/fpdfapi/cpdf_simple_parser.h:28: void ParseWord(const uint8_t*&
pStart, FX_DWORD& dwSize);
On 2016/03/09 15:24:57, dsinclair wrote:
> uint8_t*& ? A reference to a pointer?

Yeah, should be a **, I believe the idea is to update the pStart position in the
string as a result of the parse.

https://codereview.chromium.org/1773103003/diff/1/core/src/fpdfapi/fpdf_parse...
File core/src/fpdfapi/fpdf_parser/cpdf_parser.cpp (right):

https://codereview.chromium.org/1773103003/diff/1/core/src/fpdfapi/fpdf_parse...
core/src/fpdfapi/fpdf_parser/cpdf_parser.cpp:10: #include
"core/include/fpdfapi/cpdf_document.h"
On 2016/03/09 15:24:57, dsinclair wrote:
> nit: Sorting

Done.

https://codereview.chromium.org/1773103003/diff/1/core/src/fpdfapi/fpdf_parse...
core/src/fpdfapi/fpdf_parser/cpdf_parser.cpp:871: if (byte == '\r' || byte ==
'\n')
On 2016/03/09 15:24:57, dsinclair wrote:
> PDFCharIsLineEnding(byte)

Much better. Done.

https://codereview.chromium.org/1773103003/diff/1/core/src/fpdfapi/fpdf_parse...
File core/src/fpdfapi/fpdf_parser/cpdf_simple_parser.cpp (right):

https://codereview.chromium.org/1773103003/diff/1/core/src/fpdfapi/fpdf_parse...
core/src/fpdfapi/fpdf_parser/cpdf_simple_parser.cpp:12: m_pData = pData;
On 2016/03/09 15:24:57, dsinclair wrote:
> Move these to : m_pData(pData), m_dwSize(dwSize), m_dwCurPos(0) ?

Done.

https://codereview.chromium.org/1773103003/diff/1/core/src/fpdfapi/fpdf_parse...
core/src/fpdfapi/fpdf_parser/cpdf_simple_parser.cpp:17:
CPDF_SimpleParser::CPDF_SimpleParser(const CFX_ByteStringC& str) {
On 2016/03/09 15:24:57, dsinclair wrote:
> : m_pData(str.GetPtr()), m_dwSize(str.GetLength()), m_dwCurPos(0)

Done.

https://codereview.chromium.org/1773103003/diff/1/core/src/fpdfapi/fpdf_parse...
core/src/fpdfapi/fpdf_parser/cpdf_simple_parser.cpp:24: pStart = NULL;
On 2016/03/09 15:24:57, dsinclair wrote:
> nit: nullptr

Done.

https://codereview.chromium.org/1773103003/diff/1/core/src/fpdfapi/fpdf_parse...
core/src/fpdfapi/fpdf_parser/cpdf_simple_parser.cpp:44: if (ch == '\r' || ch ==
'\n')
On 2016/03/09 15:24:57, dsinclair wrote:
> PDFCharIsLineEnding(ch)

Done.

https://codereview.chromium.org/1773103003/diff/1/core/src/fpdfapi/fpdf_parse...
File core/src/fpdfapi/fpdf_parser/fpdf_parser_utility_unittest.cpp (right):

https://codereview.chromium.org/1773103003/diff/1/core/src/fpdfapi/fpdf_parse...
core/src/fpdfapi/fpdf_parser/fpdf_parser_utility_unittest.cpp:14:
TEST(SimpleParserTest, GetWord) {
On 2016/03/09 15:24:57, dsinclair wrote:
> This test file should probably be renamed to cpdf_simple_parser_unittest.cpp
> yea? Or, the test cases should be renamed from SimpleParserTest

Done.

https://codereview.chromium.org/1773103003/diff/1/fpdfsdk/include/fsdk_define.h
File fpdfsdk/include/fsdk_define.h (right):

https://codereview.chromium.org/1773103003/diff/1/fpdfsdk/include/fsdk_define...
fpdfsdk/include/fsdk_define.h:13: #include "core/include/fpdfapi/fpdf_parser.h"
On 2016/03/09 15:24:57, dsinclair wrote:
> For the files where we added cpdf_parser or cpdf_simple_parser, can we remove
> the fpdf_parser.h include or is other stuff from there still needed?
> 
> (Or, is the plan to just remove everything from fpdf_parser.h and we'll do a
> pass at the end to remove the include when the header is removed?)
Yes, exactly.  fpdf_parser.h becomes extinct in the end.

[dsinclair, 2016-03-09 18:44:46]

lgtm

[Tom Sepez, 2016-03-09 18:47:49]

Description was changed from

==========
Split off CPDF_Parser and CPDF_SimpleParser into .h/.cpp files
==========

to

==========
Split off CPDF_Parser and CPDF_SimpleParser into .h/.cpp files

R=dsinclair@chromium.org

Committed:
https://pdfium.googlesource.com/pdfium/+/8388037a5c58d60043b11c03a8efe78c54c6...
==========

[Tom Sepez, 2016-03-09 18:47:50]

Committed patchset #3 (id:40001) manually as
8388037a5c58d60043b11c03a8efe78c54c65a4b (presubmit successful).