Closed
Bug 1046416
Opened 10 years ago
Closed 10 years ago
Shut down the gfxPlatformFontList on all platforms
Categories
(Core :: Layout: Text and Fonts, defect)
Core
Layout: Text and Fonts
Tracking
()
RESOLVED
FIXED
mozilla34
People
(Reporter: khuey, Assigned: khuey)
References
Details
(Keywords: memory-leak, Whiteboard: [MemShrink])
Attachments
(1 file)
(deleted),
patch
|
jfkthame
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Attachment #8465038 -
Flags: review?(jfkthame)
Comment 1•10 years ago
|
||
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?
Assignee | ||
Comment 2•10 years ago
|
||
(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.
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(jfkthame)
Comment 3•10 years ago
|
||
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+
Assignee | ||
Comment 4•10 years ago
|
||
Flags: needinfo?(jfkthame)
Comment 5•10 years ago
|
||
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Comment 6•10 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.
Description
•