[webkit-dev] FontCache refactoring proposal

[Julien Chaffraix]

Hi Dan,

On Thu, Dec 11, 2008 at 6:40 PM, Dan Bernstein <mitz at apple.com> wrote:
> Hi Julien,
> On Dec 11, 2008, at 2:14 AM, Julien Chaffraix wrote:
>
> Hi everyone,
>
> while working on memory leaks inside WebCore, the Pleyo team has found
> that the FontCache was responsible for a few of them.
>
> It would be good to have a bug filed about each leak.
>

Sure. I was not the one to look for leaks but I will get the
information and file the bugs.

> In order to
> solve those leaks and prevent future ones, we have done a refactoring
> of the FontCache and its internal working (mainly making the
> SimpleFontData Refcounted and change several sites inside WebCore to
> hold RefPtr). The modification are done and they are too big for
> integration right now so we would like to split them and contribute
> them back to WebKit. You will find below the different parts that we
> have worked on (this will follow more or less the order in which they
> will be contributed back):
>
> - initial clean-up
>    * share some methods that are the same in all implementations
>    * add a 'platform' suffix for those that should be implemented per
> platform
>    * avoid using FontPlatformData ouside the few font files inside platform
>    * make FontCache a singleton and remove all the current static methods
>
> - Add leaks probe using RefCountedLeakCounter to track our progress as
> well as see where the leaks occurs.
>
> - Make the FontCache mechanism use smart pointers
>    * have SimpleFontData derive from RefCounted
>    * use internally RefPtr
>    * use RefPtr for external reference to SimpleFontData all over WebCore
>
> - FontCache HashMap refactoring: currently some HashMaps return
> FontPlatformData internally and there is a mapping between those and
> SimpleFontData so it should be better to use the SimpleFontData
> instead.
>
> How do you plan to maintain the constructor
> Font::Font(const FontPlatformData& fontData, bool isPrinterFont)
> without a mapping from FontPlatformData to FontData?

I may have not explained well. A mapping between FontPlatformData and
FontData is required (as you were pointing out at least this
constructor requires this). However once you have made most of the
methods in FontCache return FontData, it seems relevant to also switch
to using FontData internally (and wrap the FontPlatformData in a
FontData if necessary). This switch is for FontPlatformDataCache which
currently maps a custom key to a FontPlatformData (but changing it
means to change all the methods that rely on the current mapping).

Best regards,
Julien