1458301 - Harmonize methods for computing the difference between a given font resource and a requested style

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, enhancement, P3)

Description

[Jonathan Kew [:jfkthame]]

Currently, we have two separate helper functions that calculate a "similarity" or "distance" between the properties of a given font resource (gfxFontEntry) and a style as requested by CSS (gfxFontStyle):

(1) There's WeightStyleStretchDistance(), called by gfxFontFamily::FindAllFontsForStyle when the family needs to choose the best match (or matches) among its faces for a given style;

(2) There's also CalcStyleMatch(), called by gfxFontFamily::FindFontForChar and gfxFontFamily::SearchAllFontsForChar, which are used when font fallback (either within the family or globally) is looking for an alternative face that supports a character not found in the initially-chosen face.

These two are performing essentially equivalent jobs, although their return values work in opposite ways: for WeightStyleStretchDistance(), a large return value means the resource is a poor match for the requested style, while for CalcStyleMatch() the larger the result, the better the match is considered to be.

In addition, CalcStyleMatch() is rather incomplete: in particular, it does not take any account of font-stretch. It's just a rather ad hoc method used to choose a face during fallback. WeightStyleStretchDistance(), OTOH, tries more carefully to respect the CSS font-matching algorithm's rules about which face should be used, with font-stretch, font-style and font-weight being considered in that order.

So I think we should eliminate CalcStyleMatch() and revise its callers such that they work with the result of WeightStyleStretchDistance() instead, to simplify maintenance and (incidentally) to potentially give a better result during fallback (although it may be hard to find a real-world example where the difference is visible).

Comment 1

[Jonathan Kew [:jfkthame]]

Attachment: Unify font face selection methods to consistently use WeightStyleStretchDistance to evaluate the "closeness" of an available resource to a requested style

Here's what I suggest we do here (but let's not do it until the Fx62 cycle starts, given that it's not entirely risk-free).

Comment 2

[Jonathan Kew [:jfkthame]]

Attachment: Remove failure annotations for WPT tests that now pass, due to improved font fallback choices

Here's a small bonus: as the patch here has the potential to affect the font choices made by fallback, it's not entirely surprising that it affects a few test results. Sure enough, we now get 'unexpected' passes on a few WPT tests that use obscure Unicode characters, so we can remove the failure annotations.

Comment 3

[Jonathan Watt [:jwatt]]

Comment on attachment 8973168 [details] [diff] [review]
Unify font face selection methods to consistently use WeightStyleStretchDistance to evaluate the "closeness" of an available resource to a requested style

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

::: gfx/thebes/gfxFontEntry.cpp
@@ +1721,5 @@
>          // so bail out immediately
>          return;
>      }
>  
> +    gfxFontEntry* fe = FindFontForStyle(aMatchData->mStyle, true);

While you're here, can you put a comment before the boolean argument, as in:

  /*aIgnoreSizeTolerance*/ true

It makes call sites more readable.

::: gfx/thebes/gfxFontEntry.h
@@ +695,2 @@
>      const uint32_t         mCh;          // codepoint to be matched
> +    float                  mMatchDistance; // metric indicating closest match

It likely makes no difference to packing in this case, but just as good practice maybe put the like types together? I.e. move the 'float' member up a line.

Comment 4

[Jonathan Watt [:jwatt]]

Comment on attachment 8973169 [details] [diff] [review]
Remove failure annotations for WPT tests that now pass, due to improved font fallback choices

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

\o/

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/62be2b3915b4
Unify font face selection methods to consistently use WeightStyleStretchDistance to evaluate the closeness of an available resource to a requested style. r=jwatt
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a875278bca9
Remove failure annotations for WPT tests that now pass, due to improved font fallback choices. r=jwatt

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/aad9816c24f8
followup, use explicit namespace for std::isfinite to fix build failure on some platforms. r=bustage fix on CLOSED TREE

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e214255e32f
followup #2, also use explicit namespace for std::isinf. r=preemptive bustage fix on CLOSED TREE

Comment 8

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/62be2b3915b4
https://hg.mozilla.org/mozilla-central/rev/7a875278bca9
https://hg.mozilla.org/mozilla-central/rev/aad9816c24f8
https://hg.mozilla.org/mozilla-central/rev/8e214255e32f