1046416 - Shut down the gfxPlatformFontList on all platforms

Status: RESOLVED FIXED

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

Description

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

Comment 1

[Jonathan Kew [: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?

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(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.

Comment 3

[Jonathan Kew [: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.)

Comment 4

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4d0e517b8d2b

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4d0e517b8d2b

Comment 6

[John Daggett (:jtd)]

(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.