Closed Bug 1414147 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: CSS Parsing and Computation, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: mstange, Assigned: mstange)

References

Details

Attachments

(1 file)

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
Blocks: 1414154
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 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.
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
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Depends on: 1414926
You need to log in before you can comment on or make changes to this bug.