With DirectWrite enabled, synthetic variations reftest fails. Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a2pre) Gecko/20100228 Minefield/3.7a2pre Steps to reproduce: 1. Enable DirectWrite (gfx.font_rendering.directwrite.enabled to true, restart) 2. Load test below: http://people.mozilla.org/~jdaggett/font-face/synthetic-variations.html Result: several lines up from the bottom, normal and italic appear bolded in two rows. These should appear non-bolded, as on other lines. This reftest passes when DirectWrite is not enabled, when GDI is used.

So, I decided to remove my DWrite synthetic bolding logic, and start listening to aNeedsBold in CreateFontInstance. However this produced some undesired results, the bottom 4 lines of the synthetic variations test would no longer be bolded. This was because nothing was turning on aNeedsBold when in a family with multiple faces a 600/700/800/900 font was matched to a lower weight one (the 300 in this case), when the weightDistance was actually 0. I added an additional check there which I think is what we want, we should look carefully at if what I did was right. All font-face reftests pass on both GDI and DWrite with this patch. I verified GDI still works (not surprising since that ignores aNeedsBold), but Mac does use it, so I'm not sure if that will be alright.

Comment on attachment 430127 [details] [diff] [review] Fix & use cross platform synthetic bolding logic with DWrite This boldness stuff confuses me every time I try to read it, but I think this is OK! It also seems to behave correctly with updated GDI code I'm working on that does make use of the aNeedsBold flag.

Comment on attachment 430127 [details] [diff] [review] Fix & use cross platform synthetic bolding logic with DWrite - if (((weightDistance == 0 && baseWeight >= 6) - || (weightDistance > 0)) && !fe->IsBold()) { + if (aNeedsBold) { - if (weightDistance > 0 && wghtSteps <= absDistance) { + if ((weightDistance > 0 && wghtSteps <= absDistance) || + (baseWeight >= 6 && !matchFE->IsBold() && + (wghtSteps - 1) <= weightDistance)) { The current code only sets aNeedsBold when bolder is used, it doesn't set it in cases where the weight is >= 600 but the font is actually < 600. That's why the gfxMacFont constructor has the logic below: if (!aFontEntry->IsBold() && ((weightDistance == 0 && targetWeight >= 600) || (weightDistance > 0 && aNeedsBold))) { mSyntheticBoldOffset = 1; // devunit offset when double-striking text to fake boldness } The simplest solution is to tweak the original code in gfxDWriteFont::gfxDWriteFont and leave the code in gfxFontFamily::FindFontForStyle as is: if (!fe->IsBold() && ((weightDistance == 0 && baseWeight >= 6) || (weightDistance > 0 && aNeedsBold))) { sims |= DWRITE_FONT_SIMULATIONS_BOLD; }

Hmm, looks like Jonathan's patch for GDI fonts on bug 502906 makes the same assumption you're making so maybe this logic would fit better in FindFontForStyle. If so, then you'll also need to update the logic in gfxMacFont.

(In reply to comment #4) > Hmm, looks like Jonathan's patch for GDI fonts on bug 502906 makes the same > assumption you're making so maybe this logic would fit better in > FindFontForStyle. If so, then you'll also need to update the logic in > gfxMacFont. I was assuming it wouldn't matter, since I thought it would just be redundant, but not harmful. (I couldn't check since I have no Mac to test)

Well, it doesn't cause any unit-test failures (as I've been running with this patch in place, and pushing tryserver tests), but it can presumably be tidied up a bit if we take this approach.

Comment on attachment 430127 [details] [diff] [review] Fix & use cross platform synthetic bolding logic with DWrite Plusing the review and I'll add a follow-on patch to tweak things a bit.

Always use the needsBold when synthetic bolding is necessary, not just in the 'bolder' case.

Landed on trunk http://hg.mozilla.org/mozilla-central/rev/232629d34cc7 http://hg.mozilla.org/mozilla-central/rev/fbef676a2762