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)
Core
Graphics: Text
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: myk, Assigned: myk)
References
Details
(Whiteboard: [gfx-noted])
Attachments
(4 files)
19.60 KB,
patch
|
jfkthame
:
review+
|
Details | Diff | Splinter Review |
12.33 KB,
patch
|
jrmuizel
:
review+
|
Details | Diff | Splinter Review |
patch 3 - Clean up vestigial code that existed to support setting the gfxFontEntry::mSymbolFont flag
23.84 KB,
patch
|
jrmuizel
:
review+
|
Details | Diff | Splinter Review |
7.60 KB,
patch
|
jrmuizel
:
review+
|
Details | Diff | Splinter Review |
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)
Updated•7 years ago
|
Assignee: nobody → myk
Comment 1•7 years ago
|
||
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...
Comment 2•7 years ago
|
||
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.
Comment 3•7 years ago
|
||
This replaces gfxFontEntry::IsSymbolFont with gfxFontFamily::IsSymbolFontFamily, as per comment 1 above.
Attachment #8904324 -
Flags: review?(jmuizelaar)
Comment 4•7 years ago
|
||
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 5•7 years ago
|
||
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+
Comment 6•7 years ago
|
||
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)
Updated•7 years ago
|
Priority: -- → P3
Whiteboard: [gfx-noted]
Updated•7 years ago
|
Attachment #8904324 -
Flags: review?(jmuizelaar) → review+
Updated•7 years ago
|
Attachment #8904325 -
Flags: review?(jmuizelaar) → review+
Updated•7 years ago
|
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
Comment 8•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/72a7f3392c3c https://hg.mozilla.org/mozilla-central/rev/f546b44e4549 https://hg.mozilla.org/mozilla-central/rev/395a64d67308 https://hg.mozilla.org/mozilla-central/rev/3ef47d9eabf7
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in
before you can comment on or make changes to this bug.
Description
•