Use smart pointers for CFX_Font and CFX_Type3Font classes

Use smart pointers for CFX_Font and CFX_Type3Font classes

For the class owned member variables, use std::unique_ptr or
std::vector for memory management.

BUG=pdfium:518
Committed: https://pdfium.googlesource.com/pdfium/+/9b671ace8aec906683b05399cf8a2882ed8ef7b6

[Wei Li, 2016-07-21 17:09:03]

weili@chromium.org changed reviewers:
+ thestig@chromium.org

[Wei Li, 2016-07-21 17:09:04]

pls review, thanks

[Lei Zhang, 2016-07-21 19:21:03]

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

[dsinclair, 2016-07-21 19:40:00]

https://codereview.chromium.org/2169793002/diff/20001/core/fxge/ge/fx_ge_font...
File core/fxge/ge/fx_ge_font.cpp (right):

https://codereview.chromium.org/2169793002/diff/20001/core/fxge/ge/fx_ge_font...
core/fxge/ge/fx_ge_font.cpp:40: if (count == 0) {
nit: no {}'s

https://codereview.chromium.org/2169793002/diff/20001/core/fxge/ge/fx_ge_font...
core/fxge/ge/fx_ge_font.cpp:43: IFX_FileRead* pFile =
(IFX_FileRead*)stream->descriptor.pointer;
nit: static_cast

https://codereview.chromium.org/2169793002/diff/20001/core/fxge/ge/fx_ge_font...
core/fxge/ge/fx_ge_font.cpp:45: if (res) {
ditto

https://codereview.chromium.org/2169793002/diff/20001/core/fxge/ge/fx_ge_font...
core/fxge/ge/fx_ge_font.cpp:48: return 0;
return res ? count : 0; ?

https://codereview.chromium.org/2169793002/diff/20001/core/fxge/ge/fx_ge_font...
core/fxge/ge/fx_ge_font.cpp:60: stream1->size = (unsigned long)pFile->GetSize();
nit: static_cast

https://codereview.chromium.org/2169793002/diff/20001/core/fxge/ge/fx_ge_font...
core/fxge/ge/fx_ge_font.cpp:232: std::vector<uint8_t> temp(data, data + size /
sizeof(uint8_t));
Why does this divide by uint8_t? Size is the number of elements in data, so
won't last just be data + size?

https://codereview.chromium.org/2169793002/diff/20001/xfa/fde/fde_gedevice.cpp
File xfa/fde/fde_gedevice.cpp (left):

https://codereview.chromium.org/2169793002/diff/20001/xfa/fde/fde_gedevice.cp...
xfa/fde/fde_gedevice.cpp:178: FxFont.SetSubstFont(nullptr);
Isn't this still needed to clear the FxFont substitution font?

https://codereview.chromium.org/2169793002/diff/20001/xfa/fde/fde_gedevice.cp...
xfa/fde/fde_gedevice.cpp:189: FxFont.SetSubstFont(nullptr);
ditto

[Wei Li, 2016-07-21 21:15:03]

ptal, thanks

https://codereview.chromium.org/2169793002/diff/20001/core/fxge/ge/fx_ge_font...
File core/fxge/ge/fx_ge_font.cpp (right):

https://codereview.chromium.org/2169793002/diff/20001/core/fxge/ge/fx_ge_font...
core/fxge/ge/fx_ge_font.cpp:40: if (count == 0) {
On 2016/07/21 19:39:59, dsinclair wrote:
> nit: no {}'s

Done.

https://codereview.chromium.org/2169793002/diff/20001/core/fxge/ge/fx_ge_font...
core/fxge/ge/fx_ge_font.cpp:43: IFX_FileRead* pFile =
(IFX_FileRead*)stream->descriptor.pointer;
On 2016/07/21 19:39:59, dsinclair wrote:
> nit: static_cast

Done.

https://codereview.chromium.org/2169793002/diff/20001/core/fxge/ge/fx_ge_font...
core/fxge/ge/fx_ge_font.cpp:45: if (res) {
On 2016/07/21 19:39:59, dsinclair wrote:
> ditto

Removed this.

https://codereview.chromium.org/2169793002/diff/20001/core/fxge/ge/fx_ge_font...
core/fxge/ge/fx_ge_font.cpp:48: return 0;
On 2016/07/21 19:39:59, dsinclair wrote:
> return res ? count : 0; ?

Done.

https://codereview.chromium.org/2169793002/diff/20001/core/fxge/ge/fx_ge_font...
core/fxge/ge/fx_ge_font.cpp:60: stream1->size = (unsigned long)pFile->GetSize();
On 2016/07/21 19:39:59, dsinclair wrote:
> nit: static_cast

Done.

https://codereview.chromium.org/2169793002/diff/20001/core/fxge/ge/fx_ge_font...
core/fxge/ge/fx_ge_font.cpp:232: std::vector<uint8_t> temp(data, data + size /
sizeof(uint8_t));
On 2016/07/21 19:39:59, dsinclair wrote:
> Why does this divide by uint8_t? Size is the number of elements in data, so
> won't last just be data + size?

Right, thanks

https://codereview.chromium.org/2169793002/diff/20001/xfa/fde/fde_gedevice.cpp
File xfa/fde/fde_gedevice.cpp (left):

https://codereview.chromium.org/2169793002/diff/20001/xfa/fde/fde_gedevice.cp...
xfa/fde/fde_gedevice.cpp:178: FxFont.SetSubstFont(nullptr);
On 2016/07/21 19:39:59, dsinclair wrote:
> Isn't this still needed to clear the FxFont substitution font?

Substitution font in FxFont is stored as a unique_ptr now (fx_font.h:144). It
will be cleared when FxFont is out of scope. So we do not need to explicitly
clear it now.

https://codereview.chromium.org/2169793002/diff/20001/xfa/fde/fde_gedevice.cp...
xfa/fde/fde_gedevice.cpp:189: FxFont.SetSubstFont(nullptr);
On 2016/07/21 19:39:59, dsinclair wrote:
> ditto

same here.

[dsinclair, 2016-07-25 14:02:07]

The CQ bit was checked by dsinclair@chromium.org

[dsinclair, 2016-07-25 14:02:07]

lgtm

[commit-bot: I haz the power, 2016-07-25 14:02:13]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-25 14:40:30]

Description was changed from

==========
Use smart pointers for CFX_Font and CFX_Type3Font classes

For the class owned member variables, use std::unique_ptr or
std::vector for memory management.

BUG=pdfium:518
==========

to

==========
Use smart pointers for CFX_Font and CFX_Type3Font classes

For the class owned member variables, use std::unique_ptr or
std::vector for memory management.

BUG=pdfium:518

Committed:
https://pdfium.googlesource.com/pdfium/+/9b671ace8aec906683b05399cf8a2882ed8e...
==========

[commit-bot: I haz the power, 2016-07-25 14:40:31]

Committed patchset #4 (id:60001) as
https://pdfium.googlesource.com/pdfium/+/9b671ace8aec906683b05399cf8a2882ed8e...