Move -moz-font-smoothing-background-color style struct storage from nsStyleUserInterface to nsStyleFont / nsFont

RESOLVED FIXED in Firefox 58

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mstange, Assigned: mstange)

Tracking

Trunk
mozilla58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox58 fixed)

Details

Attachments

(1 attachment)

This will make it easier to store the color on the ScaledFontMac, which will let us get rid of a bunch of code that only exists for passing the color down into text rendering.
Try run with the property enabled for content so that it can be tested by our tests:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9461b1559ec022494297589876697c3227ce0cab
https://github.com/servo/servo/pull/19105 has the corresponding style changes.
Attachment #8924824 - Flags: review?(dbaron)
Aaand the tests are failing. I will investigate.
Here's what's happening:
 - The old Gecko implementation was wrong in how it handled the "unset" value. This is an inherited property, so "unset" should cause the value to be inherited, and not reset to the initial value.
 - The servo implementation seems to be buggy, both before and after the change in this bug.

I'm going to fix the former here and file a new bug on the latter.
(In reply to Markus Stange [:mstange] from comment #5)
>  - The servo implementation seems to be buggy, both before and after the
> change in this bug.

This is incorrect. I forgot to remove internal=True from the stylo part when I pushed to try. Of course it doesn't work if it's disabled.

New try push with hopefully all this fixed: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c71abf777a23a40254b966282e29680d763e0c33
Try is green!
Comment hidden (mozreview-request)
Comment on attachment 8924824 [details]
Bug 1414147 - Move fontSmoothingBackgroundColor from nsStyleUserInterface to nsStyleFont / nsFont.

https://reviewboard.mozilla.org/r/196062/#review201658

I presume that the old code was never run through the style system mochitests with an entry in property_database.js, but the new code has been?

::: gfx/src/nsFont.cpp:67
(Diff revision 2)
>        (variantPosition == aOther.variantPosition) &&
>        (variantWidth == aOther.variantWidth) &&
>        (alternateValues == aOther.alternateValues) &&
>        (featureValueLookup == aOther.featureValueLookup) &&
> -      (smoothing == aOther.smoothing)) {
> +      (smoothing == aOther.smoothing) &&
> +      (fontSmoothingBackgroundColor == aOther.fontSmoothingBackgroundColor)) {

You should probably add a comment either here or in nsStyleFont::CalcDifference that fontSmoothingBackgroundColor changes really only need a repaint hint rather than a reflow+repaint hint, but it's not worth optimizing.
Attachment #8924824 - Flags: review?(dbaron) → review+
(In reply to David Baron :dbaron: ⌚️UTC-7 (busy week of November 6-10) from comment #9)
> Comment on attachment 8924824 [details]
> Bug 1414147 - Move fontSmoothingBackgroundColor from nsStyleUserInterface to
> nsStyleFont / nsFont.
> 
> https://reviewboard.mozilla.org/r/196062/#review201658
> 
> I presume that the old code was never run through the style system
> mochitests with an entry in property_database.js, but the new code has been?

I did run the old code through the tests before I landed it, here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5c882504fa97e400aab463aba969be8c2f783b06&selectedJob=131040052

And that same failure happened at the time. It looks like I just didn't look through the results closely enough :(

> ::: gfx/src/nsFont.cpp:67
> (Diff revision 2)
> >        (variantPosition == aOther.variantPosition) &&
> >        (variantWidth == aOther.variantWidth) &&
> >        (alternateValues == aOther.alternateValues) &&
> >        (featureValueLookup == aOther.featureValueLookup) &&
> > -      (smoothing == aOther.smoothing)) {
> > +      (smoothing == aOther.smoothing) &&
> > +      (fontSmoothingBackgroundColor == aOther.fontSmoothingBackgroundColor)) {
> 
> You should probably add a comment either here or in
> nsStyleFont::CalcDifference that fontSmoothingBackgroundColor changes really
> only need a repaint hint rather than a reflow+repaint hint, but it's not
> worth optimizing.

Good point, I'll add the comment.
Comment hidden (mozreview-request)

Comment 12

2 years ago
Pushed by mstange@themasta.com:
https://hg.mozilla.org/integration/autoland/rev/bccbe51cd7ca
Move fontSmoothingBackgroundColor from nsStyleUserInterface to nsStyleFont / nsFont. r=dbaron
https://hg.mozilla.org/mozilla-central/rev/bccbe51cd7ca
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.