1452466 - Get rid of gfxFontStyle::ComputeWeight

Status: RESOLVED FIXED

(Core :: Graphics: Text, enhancement)

Description

[Jonathan Watt [:jwatt]]

Dealing with this function separately from the other changes for bug 1436048 to keep any potential tangential discussion from cluttering that bug.

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 2

[Jonathan Watt [:jwatt]]

Comment on attachment 8966028 [details] [diff] [review]
patch

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

::: gfx/thebes/gfxFontEntry.cpp
@@ -1307,5 @@
>  
>      aNeedsSyntheticBold = false;
>  
> -    int8_t baseWeight = aFontStyle.ComputeWeight();
> -    bool wantBold = baseWeight >= 6;

To be strictly equivalent with the old code, this should really use 550 instead of 600, but it's not clear to me that the old behavior is actually desirable.

@@ +1500,5 @@
>               rank += 10;
>           }
>  
>          // measure of closeness of weight to the desired value
> +        rank += 9 - DeprecatedAbs((aFontEntry->Weight() - aStyle->weight) / 100);

Again, the algorithm would now be slightly different. Specifically there would be no clamping of the computed font-weight [0, 950] or the rounding when calculating the distance. Neither of those things seem to be part of CSS Fonts 3 or 4 though, so this looks more like what we should be doing anyway.

::: gfx/thebes/gfxTextRun.cpp
@@ +3443,5 @@
>      gfxFontEntry *fe =
>          gfxPlatformFontList::PlatformFontList()->
>              SystemFindFontForChar(aCh, aNextCh, aRunScript, &mStyle);
>      if (fe) {
> +        bool wantBold = mStyle.weight >= 600;

Again, using 550 would be required for equivalence.

Comment 3

[Jonathan Kew [:jfkthame]]

Comment on attachment 8966028 [details] [diff] [review]
patch

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

I'm fine with doing this, if it doesn't break existing font-matching tests. Given that font-weight currently works in multiples of 100, the changes here may not actually be user-visible.

(Maybe fine even if it does break something, but in that case we'd need to look at what behavior changed and see if we're OK with it.)

::: gfx/thebes/gfxFontEntry.cpp
@@ +1500,5 @@
>               rank += 10;
>           }
>  
>          // measure of closeness of weight to the desired value
> +        rank += 9 - DeprecatedAbs((aFontEntry->Weight() - aStyle->weight) / 100);

While you're touching this line, maybe also replace DeprecatedAbs with Abs?

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e7478c64d4d9
Get rid of gfxFontStyle::ComputeWeight. r=jfkthame

Comment 5

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/e7478c64d4d9