Closed Bug 1046416 Opened 6 years ago Closed 6 years ago

Shut down the gfxPlatformFontList on all platforms

Categories

(Core :: Layout: Text and Fonts, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: khuey, Assigned: khuey)

References

Details

(Keywords: memory-leak, Whiteboard: [MemShrink])

Attachments

(1 file)

Attached patch PatchSplinter Review
No description provided.
Attachment #8465038 - Flags: review?(jfkthame)
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
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
(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.