169.10 KB, image/png
Fix handling of platform ID in gfxFontUtils::ReadNames, so that fallback to Windows-platform names on Mac works as intended
1.86 KB, patch
|Details | Diff | Splinter Review|
[Notes]: - This is not reproducible on Win 10 64-bit and Ubuntu 16.04 64-bit. [Affected versions]: - Firefox 49 beta 2 - latest Aurora 50.0a2 2016-08-11 - latest Nightly 51.0a1 2016-08-11 [Affected platforms]: - Mac OS X 10.9.5 - Mac OS X 10.12 beta [Steps to reproduce]: 1. Open a website with heavy content(eg https://goo.gl/GS0h5t) 2. Open the Developer Tools - Inspector and go to Fonts tab 3. Check the fonts names [Expected result]: - All the fonts are displayed [Actual result]: - Some of the remote font names are shown as "(MISSING NAME)" [Regression range]: - This a Firefox 44 regression: - moz-central: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=2387ada864282880d3a498d643abe3d8b887ee59&tochange=e193b4da0a8c1025aa76a403c64663ff1cd41709 - moz-inbound: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=15104d2224f4d1c794994941e880d616cf9aa486&tochange=5baceaa01feee58e5dee795f9ef3bd2a596c8448 According to m-i, this was caused by bug 1211867, bug 1193050 or bug 1211141. Adding Jonathan and Frederic to CC.
3 years ago
QA Whiteboard: [qe-dthtml]
I guess this must be a result of bug 1193050 changing some details of how the 'name' table gets processed. Depending what is going on, it might need to be reported upstream as an OTS issue; but first we should understand exactly what changed.
OK, I see what's going on; it's a Gecko bug, not OTS. Note that prior to bug 1193050, Firefox on OS X showed "fake" names ("OTS derived font") for those faces. This is something that OTS was inserting where the font lacked Mac-platform name table entries. It no longer does this if the font contains valid Windows-platform names -- which I think is OK. So the decoded font resource ends up having _only_ Windows-platform names, whereas previously it also contained (fake) Mac-platform names. Now, when we get a font with only Windows names, we have code that was intended to use those as a fallback (on the Mac platform; we'd already use them by default on other platforms). However, it turns out that gfxFontUtils::ReadNames has a bug in how it handles the platform ID, such that this fallback-to-Windows-names-on-Mac never actually works. And so we end up with the "(MISSING NAME)" string. By fixing that code, we'll not only get rid of the "(MISSING NAME)" junk, we'll actually show the correct face names for these resources, instead of the generic "OTS derived font" that we used to get.
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
Comment on attachment 8780157 [details] [diff] [review] Fix handling of platform ID in gfxFontUtils::ReadNames, so that fallback to Windows-platform names on Mac works as intended Review of attachment 8780157 [details] [diff] [review]: ----------------------------------------------------------------- This would've been easier to review with cosmetics separated out into a separate patch. It was a bit of a struggle to find out what was actually changing. It's probably worth making the commit message a bit more specific about what's being fixed if you intend to keep it as a single commit.
Attachment #8780157 - Flags: review?(jmuizelaar) → review+
Fair point, sorry! I've split off the cosmetic fixes into a separate patch for landing, so the first one contains just the single functional change. Carrying over your r+, as the total effect is the same.
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e5c6bab845670af3b30c310145f3e9c8640d050 Bug 1294448 - Fix handling of platform ID in gfxFontUtils::ReadNames, so that fallback to Windows-platform names on Mac works as intended. r=jrmuizel https://hg.mozilla.org/integration/mozilla-inbound/rev/0307f3b495ed7cc0105b338bcf1eb8b6175009ef Bug 1294448 followup - Cosmetic fixes to code style, no functional change. r=jrmuizel
not critical enough for a 48 dot release, however, tracking to make sure we take a fix in 49
Hi :jfkthame, Since this bug is a regression and also affects 49/50, are you also considering to uplift this patch to 49/50?
Considering this is a long-standing bug -- the visible "regression" in Firefox 44 was only that the placeholder "OTS derived font" was replaced by the more obviously broken "(MISSING NAME)", but even prior to that the correct font names were not being shown, AFAICT -- I don't think it really merits tracking for FF49/beta. The patch is trivial enough that there's minimal risk to it, IMO, so I'll nominate it for aurora uplift.
Target Milestone: mozilla51 → ---
Comment on attachment 8780157 [details] [diff] [review] Fix handling of platform ID in gfxFontUtils::ReadNames, so that fallback to Windows-platform names on Mac works as intended Approval Request Comment [Feature/regressing bug #]: bug 1193050 (but as noted above, even prior to that we were not actually showing the proper names) [User impact if declined]: "MISSING NAME" for some remote fonts in Dev Tools inspector [Describe test coverage new/current, TreeHerder]: tested manually with example from comment 0 [Risks and why]: minimal, trivial patch to font name-reading code [String/UUID change made/needed]: none
Attachment #8780157 - Flags: approval-mozilla-aurora?
Based on comment #10, it's a long-standing bug. Mark 49 as won't fix.
Comment on attachment 8780157 [details] [diff] [review] Fix handling of platform ID in gfxFontUtils::ReadNames, so that fallback to Windows-platform names on Mac works as intended The patch fixes a regression. Take it in 50 aurora.
Attachment #8780157 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Verified that using Aurora 50.0a2 and Nightly 51.0a1 both from 2016-08-17, the font names are displayed under Mac OS X 10.9.5.
You need to log in before you can comment on or make changes to this bug.