Properly ref-count CFGAS_GEFont with CFX_RetainPtr.

Properly ref-count CFGAS_GEFont with CFX_RetainPtr.

We worry about cyclical references, but no leaks found.

Review-Url: https://codereview.chromium.org/2609423003
Committed: https://pdfium.googlesource.com/pdfium/+/e647799f6a2f7f747c9f55d9f0ce08dcdfbd53f4

[Tom Sepez, 2017-01-04 19:54:07]

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

[commit-bot: I haz the power, 2017-01-04 19:54:13]

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, 2017-01-04 20:02:02]

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

[commit-bot: I haz the power, 2017-01-04 20:02:03]

Dry run: Try jobs failed on following builders:
  win_xfa_rel on master.tryserver.client.pdfium (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.pdfium/builders/win_xfa_rel/bui...)

[Tom Sepez, 2017-01-04 21:07:00]

Description was changed from

==========
Properly ref-count CFGASS_GEFont with CFX_RetainPtr.

WIP.  Worry about cyclical references.
==========

to

==========
Properly ref-count CFGAS_GEFont with CFX_RetainPtr.

WIP.  Worry about cyclical references.
==========

[Tom Sepez, 2017-01-04 21:07:39]

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

[commit-bot: I haz the power, 2017-01-04 21:07:47]

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, 2017-01-04 21:14:16]

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

[commit-bot: I haz the power, 2017-01-04 21:14:17]

Dry run: Try jobs failed on following builders:
  win_xfa_rel on master.tryserver.client.pdfium (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.pdfium/builders/win_xfa_rel/bui...)

[Tom Sepez, 2017-01-04 21:22:40]

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

[commit-bot: I haz the power, 2017-01-04 21:22:49]

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, 2017-01-04 21:35:26]

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

[commit-bot: I haz the power, 2017-01-04 21:35:26]

Dry run: Try jobs failed on following builders:
  win_xfa_clang_32 on master.tryserver.client.pdfium (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.pdfium/builders/win_xfa_clang_3...)

[Tom Sepez, 2017-01-04 21:38:11]

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

[commit-bot: I haz the power, 2017-01-04 21:38:16]

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, 2017-01-04 21:43:46]

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

[commit-bot: I haz the power, 2017-01-04 21:43:47]

Dry run: Try jobs failed on following builders:
  win_xfa_32 on master.tryserver.client.pdfium (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.pdfium/builders/win_xfa_32/buil...)

[Tom Sepez, 2017-01-04 21:55:02]

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

[commit-bot: I haz the power, 2017-01-04 21:55:06]

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, 2017-01-04 22:06:36]

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

[commit-bot: I haz the power, 2017-01-04 22:06:37]

Dry run: This issue passed the CQ dry run.

[Tom Sepez, 2017-01-04 22:29:41]

Description was changed from

==========
Properly ref-count CFGAS_GEFont with CFX_RetainPtr.

WIP.  Worry about cyclical references.
==========

to

==========
Properly ref-count CFGAS_GEFont with CFX_RetainPtr.

We worry about cyclical references, but no leaks found.
==========

[Tom Sepez, 2017-01-04 22:29:41]

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

[dsinclair, 2017-01-05 17:01:20]

https://codereview.chromium.org/2609423003/diff/90001/xfa/fde/fde_gedevice.h
File xfa/fde/fde_gedevice.h (right):

https://codereview.chromium.org/2609423003/diff/90001/xfa/fde/fde_gedevice.h#...
xfa/fde/fde_gedevice.h:103: const CFX_RetainPtr<CFGAS_GEFont>& pFont,
Do we expect these draw methods to store the pointer? If not, isn't it clearer
to pass the * then a const retainptr reference?

https://codereview.chromium.org/2609423003/diff/90001/xfa/fgas/font/cfgas_fon...
File xfa/fgas/font/cfgas_fontmgr.h (right):

https://codereview.chromium.org/2609423003/diff/90001/xfa/fgas/font/cfgas_fon...
xfa/fgas/font/cfgas_fontmgr.h:212: void RemoveFont(const
CFX_RetainPtr<CFGAS_GEFont>& pFont);
This signature is a bit confusing. Will the retainPtr I pass in still be valid
after removeFont has completed? Or will this function decrement the retention?

[Tom Sepez, 2017-01-05 17:35:50]

https://codereview.chromium.org/2609423003/diff/90001/xfa/fde/fde_gedevice.h
File xfa/fde/fde_gedevice.h (right):

https://codereview.chromium.org/2609423003/diff/90001/xfa/fde/fde_gedevice.h#...
xfa/fde/fde_gedevice.h:103: const CFX_RetainPtr<CFGAS_GEFont>& pFont,
On 2017/01/05 17:01:20, dsinclair wrote:
> Do we expect these draw methods to store the pointer? If not, isn't it clearer
> to pass the * then a const retainptr reference?
It allows us to be unconcerned about what the method does under the covers, and
since its a const-ref, there isn't ref-count churn on each call, and is cheap.

https://codereview.chromium.org/2609423003/diff/90001/xfa/fgas/font/cfgas_fon...
File xfa/fgas/font/cfgas_fontmgr.h (right):

https://codereview.chromium.org/2609423003/diff/90001/xfa/fgas/font/cfgas_fon...
xfa/fgas/font/cfgas_fontmgr.h:212: void RemoveFont(const
CFX_RetainPtr<CFGAS_GEFont>& pFont);
On 2017/01/05 17:01:20, dsinclair wrote:
> This signature is a bit confusing. Will the retainPtr I pass in still be valid
> after removeFont has completed? Or will this function decrement the retention?

RemoveFont removes the font from the Manager.  Since it's a const ref, the
object pointed to by the pointer will still be alive since the caller holds a
pointer to it (it must somewhere, in order to be able to call this).  If the
caller's ptr is the last one, when that goes out of scope it deletes the object.

[dsinclair, 2017-01-05 18:54:38]

The CQ bit was checked by dsinclair@chromium.org

[dsinclair, 2017-01-05 18:54:38]

lgtm

[commit-bot: I haz the power, 2017-01-05 18:54:49]

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, 2017-01-05 20:57:01]

CQ is committing da patch.

Bot data: {"patchset_id": 90001, "attempt_start_ts": 1483642478288060,
"parent_rev": "6bb3b894488fd6f38c096b708980a9f08286ac5c", "commit_rev":
"e647799f6a2f7f747c9f55d9f0ce08dcdfbd53f4"}

[commit-bot: I haz the power, 2017-01-05 20:57:04]

Description was changed from

==========
Properly ref-count CFGAS_GEFont with CFX_RetainPtr.

We worry about cyclical references, but no leaks found.
==========

to

==========
Properly ref-count CFGAS_GEFont with CFX_RetainPtr.

We worry about cyclical references, but no leaks found.

Review-Url: https://codereview.chromium.org/2609423003
Committed:
https://pdfium.googlesource.com/pdfium/+/e647799f6a2f7f747c9f55d9f0ce08dcdfbd...
==========

[commit-bot: I haz the power, 2017-01-05 20:57:05]

Committed patchset #6 (id:90001) as
https://pdfium.googlesource.com/pdfium/+/e647799f6a2f7f747c9f55d9f0ce08dcdfbd...