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

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Markus Stange [:mstange]]

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.

Comment 1

[Markus Stange [:mstange]]

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

Comment 2

[Markus Stange [:mstange]]

Attachment: Bug 1414147 - Move fontSmoothingBackgroundColor from nsStyleUserInterface to nsStyleFont / nsFont.

Review commit: https://reviewboard.mozilla.org/r/196062/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/196062/

Comment 3

[Markus Stange [:mstange]]

https://github.com/servo/servo/pull/19105 has the corresponding style changes.

Comment 4

[Markus Stange [:mstange]]

Aaand the tests are failing. I will investigate.

Comment 5

[Markus Stange [:mstange]]

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.

Comment 6

[Markus Stange [:mstange]]

(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

Comment 7

[Markus Stange [:mstange]]

Try is green!

Comment 8

[Markus Stange [:mstange]]

Comment on attachment 8924824 [details]
Bug 1414147 - Move fontSmoothingBackgroundColor from nsStyleUserInterface to nsStyleFont / nsFont.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/196062/diff/1-2/

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 10

[Markus Stange [:mstange]]

(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 11

[Markus Stange [:mstange]]

Comment on attachment 8924824 [details]
Bug 1414147 - Move fontSmoothingBackgroundColor from nsStyleUserInterface to nsStyleFont / nsFont.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/196062/diff/2-3/

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bccbe51cd7ca
Move fontSmoothingBackgroundColor from nsStyleUserInterface to nsStyleFont / nsFont. r=dbaron

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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