Closed Bug 1395061 Opened 7 years ago Closed 7 years ago

refactor gfxFontEntry::IsSymbolFont, ::SupportsLangGroup, and ::MatchesGenericFamily into gfxFontFamily

Categories

(Core :: Graphics: Text, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: myk, Assigned: myk)

References

Details

(Whiteboard: [gfx-noted])

Attachments

(4 files)

Per bug 1375978, comment 26, these gfxFontEntry methods are consistent across all faces in a family:

        fontEntry->IsSymbolFont()
        fontEntry->SupportsLangGroup(aLangGroup)
        fontEntry->MatchesGenericFamily(aGenericFamily)

So we should refactor these methods into gfxFontFamily to avoid having to call gfxFontFamily::FindFontForStyle in gfxPlatformFontList::GetFontList, which eventually calls into gfxFontFamily::FindStyleVariations (which needs to happen on the main thread on Mac because it accesses preferences).

The attached patch does this for SupportsLangGroup and MatchesGenericFamily, which are fairly straightforward.  It doesn't yet do it for IsSymbolFont, which I haven't quite figured out.

jfkthame: I may need your help there, or perhaps you could pick this up and finish it faster than me once you return from PTO.

Note: the patch does a poor job of showing the changes in gfxGDIFontList.h.  Instead of showing that I moved MatchesGenericFamily and SupportsLangGroup from GDIFontEntry to GDIFontFamily, it shows that those two methods stayed in place while the rest of GDIFontEntry moved.
Attachment #8902544 - Flags: review?(jfkthame)
Blocks: 1375978
Assignee: nobody → myk
The patch for SupportsLangGroup and MatchesGenericFamily looks good to me, thanks!

Regarding IsSymbolFont: this is meant to enable us to exclude symbol-encoded fonts from the popup, as they wouldn't be appropriate default text fonts. However, this is mostly a legacy thing from the days when fonts were specific to (pre-Unicode) charsets; currently, most fonts -- even if they're full of symbols, dingbats, emoji, or whatever -- are simply Unicode-encoded.

In particular, not all platform backends even attempt to set this flag; AFAICS, it's only the Mac and GDI font-list code that attempts it. And it's unlikely that it does anything useful on macOS, as even fonts like Symbol or Zapf Dingbats or Apple Symbols are now simply using standard Unicode encoding there. I haven't seen a font using the old Script Manager "symbols" writing system for many years (I'm not even sure the OS would still support it). So I think we can safely drop this test altogether on the Mac.

That leaves GDI. There are still some fonts on Windows that have cmap tables tagged as MS platform, Symbol charset, and it's nice to exclude these from the list we present in Preferences.

So I think we should replace this with a new method gfxFontFamily::IsSymbolFontFamily. The generic method will simply return false; and there'll be a GDI override that checks the SYMBOL_CHARSET bit. And that's all we need to do. Then we can drop the mSymbolFont flag from gfxFontEntry altogether, and avoid the need to populate the family with gfxFontEntry objects just to do this test.

Patch coming...
Hmm, actually the DWrite backend also handles IsSymbolFont, though in a different way -- it doesn't depend on setting the mSymbolFont flag, which is why I overlooked it at first.
This replaces gfxFontEntry::IsSymbolFont with gfxFontFamily::IsSymbolFontFamily, as per comment 1 above.
Attachment #8904324 - Flags: review?(jmuizelaar)
Now that gfxFontEntry::mSymbolFont is gone, we can also get rid of some more code that only existed to help set it.
Attachment #8904325 - Flags: review?(jmuizelaar)
Comment on attachment 8902544 [details] [diff] [review]
refactor font attributes

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

Note that this requires minor rebasing since bug 835204 just landed. I have an updated copy in my local queue, ready to land when the following patches are also reviewed.
Attachment #8902544 - Flags: review?(jfkthame) → review+
One more thing we can do here is to refactor these checks so that the loop in GetFontList just does a single virtual call to a platform-specific Filter method, instead of a separate virtual call for each of the three things we might (or might not, depending on platform) care to test.
Attachment #8904529 - Flags: review?(jmuizelaar)
Priority: -- → P3
Whiteboard: [gfx-noted]
Attachment #8904324 - Flags: review?(jmuizelaar) → review+
Attachment #8904325 - Flags: review?(jmuizelaar) → review+
Attachment #8904529 - Flags: review?(jmuizelaar) → review+
Pushed by jkew@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/72a7f3392c3c
patch 1 - Refactor gfxFontEntry::SupportsLangGroup and MatchesGenericFamily into gfxFontFamily. r=jfkthame
https://hg.mozilla.org/integration/mozilla-inbound/rev/f546b44e4549
patch 2 - Implement default gfxFontFamily::IsSymbolFontFamily, and provide override for GDI font backend. r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/395a64d67308
patch 3 - Clean up vestigial code that existed to support setting the gfxFontEntry::mSymbolFont flag. r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ef47d9eabf7
patch 4 - Refactor checks in the gfxPlatformFontList::GetFontList loop to use a single virtual method call instead of three separate calls. r=jrmuizel
You need to log in before you can comment on or make changes to this bug.