Convert some pointers in fpdf_parser_parser to unique_ptr and std::vector.

Convert some pointers in fpdf_parser_parser to unique_ptr and std::vector.

R=thestig@chromium.org, tsepez@chromium.org

Committed: https://pdfium.googlesource.com/pdfium/+/bef190fcacb7dde4fedc1360e3019ab9968db16a

[Oliver Chang, 2015-10-23 23:25:39]

Description was changed from

==========
Convert some pointers in fpdf_parser_parser to unique_ptr and std::vector.
==========

to

==========
Convert some pointers in fpdf_parser_parser to unique_ptr and std::vector.

R=thestig@chromium.org,tsepez@chromium.org
==========

[Oliver Chang, 2015-10-23 23:25:39]

ochang@chromium.org changed reviewers:
+ thestig@chromium.org, tsepez@chromium.org

[Oliver Chang, 2015-10-23 23:26:12]

ptal

[Oliver Chang, 2015-10-23 23:27:12]

Patchset #4 (id:60001) has been deleted

[Lei Zhang, 2015-10-23 23:41:44]

There's probably more opportunities for various methods to take or return
unique_ptr instead of raw pointers. Another day perhaps?

https://codereview.chromium.org/1411043003/diff/80001/core/src/fpdfapi/fpdf_p...
File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (left):

https://codereview.chromium.org/1411043003/diff/80001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:1498:
ASSERT(m_pSecurityHandler == NULL);
This is a weird assert to have considering the next few lines.

https://codereview.chromium.org/1411043003/diff/80001/core/src/fpdfapi/fpdf_p...
File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (right):

https://codereview.chromium.org/1411043003/diff/80001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:309: if (pSecurityHandler ==
NULL) {
!pSecurityHandler

https://codereview.chromium.org/1411043003/diff/80001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:326:
m_Syntax.m_pCryptoHandler.reset(nullptr);
just reset(), ditto below

https://codereview.chromium.org/1411043003/diff/80001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:441: int32_t recordsize =
20;
If you make this a constant, I think |buf| can just be: char buf[...]

https://codereview.chromium.org/1411043003/diff/80001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:529: int32_t recordsize =
20;
ditto

[Oliver Chang, 2015-10-24 00:23:36]

yeah let's leave the opportunity to fix other things in future CLs

https://codereview.chromium.org/1411043003/diff/80001/core/src/fpdfapi/fpdf_p...
File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (left):

https://codereview.chromium.org/1411043003/diff/80001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:1498:
ASSERT(m_pSecurityHandler == NULL);
On 2015/10/23 23:41:44, Lei Zhang wrote:
> This is a weird assert to have considering the next few lines.

removed

https://codereview.chromium.org/1411043003/diff/80001/core/src/fpdfapi/fpdf_p...
File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (right):

https://codereview.chromium.org/1411043003/diff/80001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:309: if (pSecurityHandler ==
NULL) {
On 2015/10/23 23:41:44, Lei Zhang wrote:
> !pSecurityHandler

Done, and replaced all other similar instances of this.

https://codereview.chromium.org/1411043003/diff/80001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:326:
m_Syntax.m_pCryptoHandler.reset(nullptr);
On 2015/10/23 23:41:44, Lei Zhang wrote:
> just reset(), ditto below

Done.

https://codereview.chromium.org/1411043003/diff/80001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:441: int32_t recordsize =
20;
On 2015/10/23 23:41:44, Lei Zhang wrote:
> If you make this a constant, I think |buf| can just be: char buf[...]

having a 20K stack allocated buffer feels a bit scary and feels a bit better
suited for the heap... is this an issue?

https://codereview.chromium.org/1411043003/diff/80001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:529: int32_t recordsize =
20;
On 2015/10/23 23:41:44, Lei Zhang wrote:
> ditto

Acknowledged.

[Lei Zhang, 2015-10-24 00:26:04]

lgtm

https://codereview.chromium.org/1411043003/diff/80001/core/src/fpdfapi/fpdf_p...
File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (right):

https://codereview.chromium.org/1411043003/diff/80001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:441: int32_t recordsize =
20;
On 2015/10/24 00:23:35, Oliver Chang wrote:
> On 2015/10/23 23:41:44, Lei Zhang wrote:
> > If you make this a constant, I think |buf| can just be: char buf[...]
> 
> having a 20K stack allocated buffer feels a bit scary and feels a bit better
> suited for the heap... is this an issue?

Fair enough. Can we make |recordsize| a const at least?

[Oliver Chang, 2015-10-24 00:29:00]

https://codereview.chromium.org/1411043003/diff/80001/core/src/fpdfapi/fpdf_p...
File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (right):

https://codereview.chromium.org/1411043003/diff/80001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:441: int32_t recordsize =
20;
On 2015/10/24 00:26:04, Lei Zhang wrote:
> On 2015/10/24 00:23:35, Oliver Chang wrote:
> > On 2015/10/23 23:41:44, Lei Zhang wrote:
> > > If you make this a constant, I think |buf| can just be: char buf[...]
> > 
> > having a 20K stack allocated buffer feels a bit scary and feels a bit better
> > suited for the heap... is this an issue?
> 
> Fair enough. Can we make |recordsize| a const at least?

Done.

[Oliver Chang, 2015-10-24 00:55:00]

Patchset #7 (id:140001) has been deleted

[Oliver Chang, 2015-10-24 00:56:42]

Committed patchset #7 (id:160001) manually as
bef190fcacb7dde4fedc1360e3019ab9968db16a (presubmit successful).

[David Lattimore, 2015-10-27 00:24:14]

When I pull this CL into our build system, I get compile errors on the
std::vector::data() calls when compiling for Android. vector::data() is only
available in C++11. I'm not sure why I get the compilation error since we're
passing -std=gnu++11. Before I try to figure out why, I'm wondering if this is
allowed anyway since http://chromium-cpp.appspot.com/ says "All C++11 Standard
Library features are currently banned"? Or is that page out of date?

[Oliver Chang, 2015-10-27 00:33:40]

On 2015/10/27 00:24:14, David Lattimore wrote:
> When I pull this CL into our build system, I get compile errors on the
> std::vector::data() calls when compiling for Android. vector::data() is only
> available in C++11. I'm not sure why I get the compilation error since we're
> passing -std=gnu++11. Before I try to figure out why, I'm wondering if this is
> allowed anyway since http://chromium-cpp.appspot.com/ says "All C++11 Standard
> Library features are currently banned"? Or is that page out of date?

Oh my bad, I'll upload another CL to address this in a sec.

[Lei Zhang, 2015-10-30 08:03:38]

On 2015/10/27 00:24:14, David Lattimore wrote:
> When I pull this CL into our build system, I get compile errors on the
> std::vector::data() calls when compiling for Android. vector::data() is only
> available in C++11. I'm not sure why I get the compilation error since we're
> passing -std=gnu++11. Before I try to figure out why, I'm wondering if this is
> allowed anyway since http://chromium-cpp.appspot.com/ says "All C++11 Standard
> Library features are currently banned"? Or is that page out of date?

I took the Chromium Android build, hacked the gyp files, and enabled building
pdfium_test. The output binary is for ARM, but it built fine with this CL and
didn't complain. Until we have an Android bot, period, and hopefully one that
can repo this build error, you'll have to be vigilant.