Shut down the gfxPlatformFontList on all platforms

RESOLVED FIXED in mozilla34

Status

()

Core
Layout: Text
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: khuey, Assigned: khuey)

Tracking

({memory-leak})

unspecified
mozilla34
memory-leak
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MemShrink])

Attachments

(1 attachment)

Comment on attachment 8465038 [details] [diff] [review]
Patch

Review of attachment 8465038 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxPlatform.cpp
@@ -448,2 @@
>      gfxPlatformFontList::Shutdown();
> -#endif

Hmm, yes - this is a bit stale, we should also be doing this on platforms that use gfxFT2FontList (i.e. Android and B2G). However, on desktop Linux systems (i.e. where fonts are managed by fontconfig) we don't use the gfxPlatformFontList code at all, so this still isn't needed there.

Looking at gfxPlatformFontList::Shutdown(), I guess it should be safe to call it even there, as all it does is delete a static pointer that will be null if we never created the object in the first place. Still, I doubt the compiler will be able to figure out that it's always null, so it'll end up linking in extra dead code to delete the potential object.

So on the whole, I think I'd prefer to keep a condition here, and just extend it to include the Android/B2G platforms. Adding "|| defined(ANDROID)" should be sufficient, right?
(In reply to Jonathan Kew (:jfkthame) from comment #1)
> So on the whole, I think I'd prefer to keep a condition here, and just
> extend it to include the Android/B2G platforms. Adding "|| defined(ANDROID)"
> should be sufficient, right?

Yes.  This feels like over-optimizing, but I'll do it if you insist.  The next platform to implement gfxPlatformFontList backends will hit this again, of course.
Flags: needinfo?(jfkthame)
Comment on attachment 8465038 [details] [diff] [review]
Patch

Review of attachment 8465038 [details] [diff] [review]:
-----------------------------------------------------------------

I suppose I won't insist; it must be pretty insignificant overall.

(I'd love to replace all the fontconfig stuff with a gfxPlatformFontList-based backend on desktop Linux, but doing that without affecting behavior in potentially undesired ways looks hard, so my guess is that we won't get around to it for ages, if at all.)
Attachment #8465038 - Flags: review?(jfkthame) → review+
https://hg.mozilla.org/mozilla-central/rev/4d0e517b8d2b
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34

Comment 6

4 years ago
(In reply to Jonathan Kew (:jfkthame) from comment #3)

> (I'd love to replace all the fontconfig stuff with a
> gfxPlatformFontList-based backend on desktop Linux, but doing that without
> affecting behavior in potentially undesired ways looks hard, so my guess is
> that we won't get around to it for ages, if at all.)

I actually think this is something we need to do to keep the pango font group stuff from becoming a boat anchor. I think we can respect fontconfig to a limited extent without the need for something too complicated. But I do think we need to get rid of the "give the fontlist to fontconfig and let it decide what's best" approach.
You need to log in before you can comment on or make changes to this bug.