Make CPDF_TextState have a CPDF_TextStateData rather than inheriting one.

Make CPDF_TextState have a CPDF_TextStateData rather than inheriting one.

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

[Tom Sepez, 2016-08-29 23:23:34]

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

[Tom Sepez, 2016-08-29 23:23:35]

Dan, for review.  This is what the code will look like as we get callers out of
the GetObject/GetPrivateCopy game.

[Tom Sepez, 2016-08-29 23:28:29]

The CQ bit was checked by tsepez@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-29 23:28:39]

Dry run: 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-08-29 23:42:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-29 23:42:11]

Dry run: This issue passed the CQ dry run.

[dsinclair, 2016-08-30 13:22:39]

Description was changed from

==========
Make CPDF_TextState have a CPDF_TextStateData rather than inheriting one.

BUG=
==========

to

==========
Make CPDF_TextState have a CPDF_TextStateData rather than inheriting one.
==========

[dsinclair, 2016-08-30 13:36:11]

https://codereview.chromium.org/2287313004/diff/40001/core/fpdfapi/fpdf_page/...
File core/fpdfapi/fpdf_page/cpdf_textstate.cpp (right):

https://codereview.chromium.org/2287313004/diff/40001/core/fpdfapi/fpdf_page/...
core/fpdfapi/fpdf_page/cpdf_textstate.cpp:15: void CPDF_TextState::New() {
Can we call this Initialize()? Or, something other then New()? New makes me
think new/delete of the CPDF_TextState, not the internal ref.

https://codereview.chromium.org/2287313004/diff/40001/core/fpdfapi/fpdf_page/...
File core/fpdfapi/fpdf_page/cpdf_textstatedata.cpp (right):

https://codereview.chromium.org/2287313004/diff/40001/core/fpdfapi/fpdf_page/...
core/fpdfapi/fpdf_page/cpdf_textstatedata.cpp:88: FX_FLOAT size = unit *
m_FontSize;
No need to store size. (and below)

https://codereview.chromium.org/2287313004/diff/40001/core/fpdfapi/fpdf_page/...
core/fpdfapi/fpdf_page/cpdf_textstatedata.cpp:89: return
(FX_FLOAT)FXSYS_fabs(size);
static_cast?  (and below)

[Tom Sepez, 2016-08-30 16:59:37]

The CQ bit was checked by tsepez@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-30 16:59:39]

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

[Tom Sepez, 2016-08-30 16:59:48]

https://codereview.chromium.org/2287313004/diff/40001/core/fpdfapi/fpdf_page/...
File core/fpdfapi/fpdf_page/cpdf_textstate.cpp (right):

https://codereview.chromium.org/2287313004/diff/40001/core/fpdfapi/fpdf_page/...
core/fpdfapi/fpdf_page/cpdf_textstate.cpp:15: void CPDF_TextState::New() {
On 2016/08/30 13:36:10, dsinclair wrote:
> Can we call this Initialize()? Or, something other then New()? New makes me
> think new/delete of the CPDF_TextState, not the internal ref.

The way I'm thinking about these is that they are like a Maybe<SomeObject> or
base::optional<SomeObject> rather than a pointer, and they behave as if each
object were unique with the sharing of storage just an implementation detail to
save space.  So you're right about New() being a mismatch in that we don't want
to think in ptrs ... hmmm.

The m_Ref.New() call should really be m_Ref.emplace() as it forwards args to the
(textstatedata) ctor.  Should this be emplace() as well?

https://codereview.chromium.org/2287313004/diff/40001/core/fpdfapi/fpdf_page/...
File core/fpdfapi/fpdf_page/cpdf_textstatedata.cpp (right):

https://codereview.chromium.org/2287313004/diff/40001/core/fpdfapi/fpdf_page/...
core/fpdfapi/fpdf_page/cpdf_textstatedata.cpp:88: FX_FLOAT size = unit *
m_FontSize;
On 2016/08/30 13:36:10, dsinclair wrote:
> No need to store size. (and below)

Done.

https://codereview.chromium.org/2287313004/diff/40001/core/fpdfapi/fpdf_page/...
core/fpdfapi/fpdf_page/cpdf_textstatedata.cpp:89: return
(FX_FLOAT)FXSYS_fabs(size);
On 2016/08/30 13:36:10, dsinclair wrote:
> static_cast?  (and below)
FXSYS_fabs already includes this cast.  Removed.

[commit-bot: I haz the power, 2016-08-30 17:20:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-30 17:20:09]

Dry run: This issue passed the CQ dry run.

[dsinclair, 2016-08-30 17:32:11]

The CQ bit was checked by dsinclair@chromium.org

[dsinclair, 2016-08-30 17:32:11]

lgtm

[commit-bot: I haz the power, 2016-08-30 17:32:19]

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-08-30 17:32:39]

Description was changed from

==========
Make CPDF_TextState have a CPDF_TextStateData rather than inheriting one.
==========

to

==========
Make CPDF_TextState have a CPDF_TextStateData rather than inheriting one.

Committed:
https://pdfium.googlesource.com/pdfium/+/fbda17d61de1e02799f5d77dceb23df3688b...
==========

[commit-bot: I haz the power, 2016-08-30 17:32:40]

Committed patchset #5 (id:80001) as
https://pdfium.googlesource.com/pdfium/+/fbda17d61de1e02799f5d77dceb23df3688b...