Closed Bug 1452466 Opened 2 years ago Closed 2 years ago

Get rid of gfxFontStyle::ComputeWeight

Categories

(Core :: Graphics: Text, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: jwatt, Assigned: jwatt)

References

Details

Attachments

(1 file)

Dealing with this function separately from the other changes for bug 1436048 to keep any potential tangential discussion from cluttering that bug.
Attached patch patchSplinter Review
Attachment #8966028 - Flags: review?(jfkthame)
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 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?
Attachment #8966028 - Flags: review?(jfkthame) → review+
Pushed by jwatt@jwatt.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e7478c64d4d9
Get rid of gfxFontStyle::ComputeWeight. r=jfkthame
https://hg.mozilla.org/mozilla-central/rev/e7478c64d4d9
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.