Use a FontPlatformData pointer for FontDataCache's key

Use a FontPlatformData pointer for FontDataCache's key

FontDataCache's key data type was a full FontPlatformData object, not a
pointer to one The value type of that cache is a SimpleFontData object,
which in itself stores a FontPlatformData object type.

If we want to manage memory related to font blobs (OpenType tables, web
font file buffers) we need to come to a situation where the lifecycle of
FontPlatformData objects is absolutely clear.

Previously, adding to the FontDataCache meant one copy of
FontPlatformData for the cache key, and one for the value as an owned
member of SimpleFontData.

In this situation, it is much harder to keep track of where we keep
FontPlatformData objects.

FontPlatformData objects keep a RefPtr<HarfBuzzFace>, which in turn
keeps hb_font objects, which keep references to OpenType tables copied
from Skia. If we cannot destroy FontPlatformData objects after use, we
cannot free the corresponding copied OpenType tables - so over the
lifetime of the renderer we're leaking OpenType table data for stale
FontPlatformData objects.

This is a first step towards fixing this situation and gaining better
control over the lifecylce of FontPlatformData objects.

BUG=617568
R=eae,tzik,bashi

Committed: https://crrev.com/414107a81fdf7231eebc30c45aa14a437561bbbd
Cr-Commit-Position: refs/heads/master@{#398079}

[drott, 2016-06-03 15:51:47]

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

[commit-bot: I haz the power, 2016-06-03 15:52:07]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2031363002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2031363002/1

[commit-bot: I haz the power, 2016-06-03 16:23:36]

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

[commit-bot: I haz the power, 2016-06-03 16:23:37]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[drott, 2016-06-06 11:11:13]

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

[commit-bot: I haz the power, 2016-06-06 11:11:21]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2031363002/20001

[drott, 2016-06-06 11:11:36]

Description was changed from

==========
FontDataCache to use a FontPlatformData pointer.

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

to

==========
Use a FontPlatformData pointer for FontDataCache's key

FontDataCache's key data type was a full FontPlatformData object, not as
pointer to one The value type of that cache is a SimpleFontData object,
which in itself stores a FontPlatformData object type.

If we want to manage memory related to font blobs (OpenType tables, web
font file buffers) we need to come to a situation where the lifecycle of
FontPlatformData objects is absolutely clear.

Previously, adding to the FontDataCache meant one copy of
FontPlatformData for the cache key, and one for the value as an owned
member of SimpleFontData.

In this situation, it is much harder to keep track of where we keep
FontPlatformData objects.

FontPlatformData objects keep a RefPtr<HarfBuzzFace>, which in turn
keeps hb_font objects, which keep references to OpenType tables copied
from Skia. If we cannot destroy FontPlatformData objects after use, we
cannot free the corresponding copied OpenType tables - so over the
lifetime of the renderer we're leaking OpenType table data for stale
FontPlatformData objects.

This is the first step towards fixing this situation and gaining better
control over the lifecylce of FontPlatformData objects.

BUG=617568
R=eae,tzik,bashi
==========

[commit-bot: I haz the power, 2016-06-06 12:45:31]

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

[commit-bot: I haz the power, 2016-06-06 12:45:32]

Dry run: This issue passed the CQ dry run.

[drott, 2016-06-06 12:59:00]

drott@chromium.org changed reviewers:
+ eae@chromium.org, kojii@chromium.org, tzik@chromium.org

[drott, 2016-06-06 12:59:01]

PTAL, I think this is ready for review and discussion now.

[drott, 2016-06-06 12:59:22]

Description was changed from

==========
Use a FontPlatformData pointer for FontDataCache's key

FontDataCache's key data type was a full FontPlatformData object, not as
pointer to one The value type of that cache is a SimpleFontData object,
which in itself stores a FontPlatformData object type.

If we want to manage memory related to font blobs (OpenType tables, web
font file buffers) we need to come to a situation where the lifecycle of
FontPlatformData objects is absolutely clear.

Previously, adding to the FontDataCache meant one copy of
FontPlatformData for the cache key, and one for the value as an owned
member of SimpleFontData.

In this situation, it is much harder to keep track of where we keep
FontPlatformData objects.

FontPlatformData objects keep a RefPtr<HarfBuzzFace>, which in turn
keeps hb_font objects, which keep references to OpenType tables copied
from Skia. If we cannot destroy FontPlatformData objects after use, we
cannot free the corresponding copied OpenType tables - so over the
lifetime of the renderer we're leaking OpenType table data for stale
FontPlatformData objects.

This is the first step towards fixing this situation and gaining better
control over the lifecylce of FontPlatformData objects.

BUG=617568
R=eae,tzik,bashi
==========

to

==========
Use a FontPlatformData pointer for FontDataCache's key

FontDataCache's key data type was a full FontPlatformData object, not a
pointer to one The value type of that cache is a SimpleFontData object,
which in itself stores a FontPlatformData object type.

If we want to manage memory related to font blobs (OpenType tables, web
font file buffers) we need to come to a situation where the lifecycle of
FontPlatformData objects is absolutely clear.

Previously, adding to the FontDataCache meant one copy of
FontPlatformData for the cache key, and one for the value as an owned
member of SimpleFontData.

In this situation, it is much harder to keep track of where we keep
FontPlatformData objects.

FontPlatformData objects keep a RefPtr<HarfBuzzFace>, which in turn
keeps hb_font objects, which keep references to OpenType tables copied
from Skia. If we cannot destroy FontPlatformData objects after use, we
cannot free the corresponding copied OpenType tables - so over the
lifetime of the renderer we're leaking OpenType table data for stale
FontPlatformData objects.

This is the first step towards fixing this situation and gaining better
control over the lifecylce of FontPlatformData objects.

BUG=617568
R=eae,tzik,bashi
==========

[drott, 2016-06-06 12:59:54]

Description was changed from

==========
Use a FontPlatformData pointer for FontDataCache's key

FontDataCache's key data type was a full FontPlatformData object, not a
pointer to one The value type of that cache is a SimpleFontData object,
which in itself stores a FontPlatformData object type.

If we want to manage memory related to font blobs (OpenType tables, web
font file buffers) we need to come to a situation where the lifecycle of
FontPlatformData objects is absolutely clear.

Previously, adding to the FontDataCache meant one copy of
FontPlatformData for the cache key, and one for the value as an owned
member of SimpleFontData.

In this situation, it is much harder to keep track of where we keep
FontPlatformData objects.

FontPlatformData objects keep a RefPtr<HarfBuzzFace>, which in turn
keeps hb_font objects, which keep references to OpenType tables copied
from Skia. If we cannot destroy FontPlatformData objects after use, we
cannot free the corresponding copied OpenType tables - so over the
lifetime of the renderer we're leaking OpenType table data for stale
FontPlatformData objects.

This is the first step towards fixing this situation and gaining better
control over the lifecylce of FontPlatformData objects.

BUG=617568
R=eae,tzik,bashi
==========

to

==========
Use a FontPlatformData pointer for FontDataCache's key

FontDataCache's key data type was a full FontPlatformData object, not a
pointer to one The value type of that cache is a SimpleFontData object,
which in itself stores a FontPlatformData object type.

If we want to manage memory related to font blobs (OpenType tables, web
font file buffers) we need to come to a situation where the lifecycle of
FontPlatformData objects is absolutely clear.

Previously, adding to the FontDataCache meant one copy of
FontPlatformData for the cache key, and one for the value as an owned
member of SimpleFontData.

In this situation, it is much harder to keep track of where we keep
FontPlatformData objects.

FontPlatformData objects keep a RefPtr<HarfBuzzFace>, which in turn
keeps hb_font objects, which keep references to OpenType tables copied
from Skia. If we cannot destroy FontPlatformData objects after use, we
cannot free the corresponding copied OpenType tables - so over the
lifetime of the renderer we're leaking OpenType table data for stale
FontPlatformData objects.

This is a first step towards fixing this situation and gaining better
control over the lifecylce of FontPlatformData objects.

BUG=617568
R=eae,tzik,bashi
==========

[eae, 2016-06-06 17:26:53]

Thanks for tackling this drott!

https://codereview.chromium.org/2031363002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/fonts/FontDataCache.h (right):

https://codereview.chromium.org/2031363002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/fonts/FontDataCache.h:54: if (a ==
emptyValue)
What's the difference between emptyValue (-1) and null?

[drott, 2016-06-06 18:39:15]

On 2016/06/06 at 17:26:53, eae wrote:
> Thanks for tackling this drott!
> 
>
https://codereview.chromium.org/2031363002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/platform/fonts/FontDataCache.h (right):
> 
>
https://codereview.chromium.org/2031363002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/platform/fonts/FontDataCache.h:54: if (a ==
emptyValue)
> What's the difference between emptyValue (-1) and null?

As far as I understand, HashTables can keep slot entries in an empty state and
overwrite them with new incoming values. The default HashTraits implementation
for pointer types defines the emptyValue as -1.

[eae, 2016-06-06 18:39:46]

LGTM

[drott, 2016-06-06 18:41:21]

The CQ bit was checked by drott@chromium.org

[commit-bot: I haz the power, 2016-06-06 18:41:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2031363002/20001

[commit-bot: I haz the power, 2016-06-06 18:47:22]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-06-06 18:49:26]

Description was changed from

==========
Use a FontPlatformData pointer for FontDataCache's key

FontDataCache's key data type was a full FontPlatformData object, not a
pointer to one The value type of that cache is a SimpleFontData object,
which in itself stores a FontPlatformData object type.

If we want to manage memory related to font blobs (OpenType tables, web
font file buffers) we need to come to a situation where the lifecycle of
FontPlatformData objects is absolutely clear.

Previously, adding to the FontDataCache meant one copy of
FontPlatformData for the cache key, and one for the value as an owned
member of SimpleFontData.

In this situation, it is much harder to keep track of where we keep
FontPlatformData objects.

FontPlatformData objects keep a RefPtr<HarfBuzzFace>, which in turn
keeps hb_font objects, which keep references to OpenType tables copied
from Skia. If we cannot destroy FontPlatformData objects after use, we
cannot free the corresponding copied OpenType tables - so over the
lifetime of the renderer we're leaking OpenType table data for stale
FontPlatformData objects.

This is a first step towards fixing this situation and gaining better
control over the lifecylce of FontPlatformData objects.

BUG=617568
R=eae,tzik,bashi
==========

to

==========
Use a FontPlatformData pointer for FontDataCache's key

FontDataCache's key data type was a full FontPlatformData object, not a
pointer to one The value type of that cache is a SimpleFontData object,
which in itself stores a FontPlatformData object type.

If we want to manage memory related to font blobs (OpenType tables, web
font file buffers) we need to come to a situation where the lifecycle of
FontPlatformData objects is absolutely clear.

Previously, adding to the FontDataCache meant one copy of
FontPlatformData for the cache key, and one for the value as an owned
member of SimpleFontData.

In this situation, it is much harder to keep track of where we keep
FontPlatformData objects.

FontPlatformData objects keep a RefPtr<HarfBuzzFace>, which in turn
keeps hb_font objects, which keep references to OpenType tables copied
from Skia. If we cannot destroy FontPlatformData objects after use, we
cannot free the corresponding copied OpenType tables - so over the
lifetime of the renderer we're leaking OpenType table data for stale
FontPlatformData objects.

This is a first step towards fixing this situation and gaining better
control over the lifecylce of FontPlatformData objects.

BUG=617568
R=eae,tzik,bashi

Committed: https://crrev.com/414107a81fdf7231eebc30c45aa14a437561bbbd
Cr-Commit-Position: refs/heads/master@{#398079}
==========

[commit-bot: I haz the power, 2016-06-06 18:49:27]

Patchset 2 (id:??) landed as
https://crrev.com/414107a81fdf7231eebc30c45aa14a437561bbbd
Cr-Commit-Position: refs/heads/master@{#398079}