458463 - Rename nsStyleFont::mFlags to ::mGenericID and stop treating it like a bitfield

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Zack Weinberg (:zwol)]

Attachment: (rev 1) patch [Checkin: Comment 4]

The mFlags field of nsStyleFont is misnamed; the only thing it's used for is to record a number for the CSS generic font family that was specified.  I can only assume there used to be actual flags, because all the code that manipulates it is careful to mask its value before use (with a mask that covers all the bits of the field!) and treat it as a set of one-bit flags, even though every place that sets any of the "flags" clears all the others first.

The attached patch renames it to mGenericID, deletes the useless mask operations, and switches the handful of places that look at the value to treat it as a single number, not a bit-set.  We could also flatten out the values of the kGenericFont_* constants, I suppose, but it wouldn't really gain us anything, and maybe there's some other place in the code where they *are* used as bit flags.

(I noticed this while updating the patch in bug 456150.  This was responsible for a handful of "overflow in constant conversion" warnings that I had punted on while doing that patch.)

(I note that this field is almost write-only.)

Comment 1

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

Comment on attachment 341696 [details] [diff] [review]
(rev 1) patch
[Checkin: Comment 4]

>-    if (higherContext->GetStyleFont()->mFlags & aGenericFontID) {
>+    if (higherContext->GetStyleFont()->mGenericID == aGenericFontID) {

I'd leave this as & unless you're sure that there's never more than one bit set in the generic font ID.

r+sr=dbaron

Comment 2

[Zack Weinberg (:zwol)]

(In reply to comment #1)
> I'd leave this as & unless you're sure that there's never more than one bit set
> in the generic font ID.

Yes, I'm sure.  Every place that manipulates this field is visible in this patch; every place that sets a bit cleared the entire field first (before the patch) / writes the field with '=' (after the patch).  The constants are all single-bit, but they needn't be.

Comment 3

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

(In reply to comment #2)
> Yes, I'm sure.  Every place that manipulates this field is visible in this
> patch; every place that sets a bit cleared the entire field first (before the
> patch) / writes the field with '=' (after the patch).  The constants are all
> single-bit, but they needn't be.

Neither nsFont::GetGenericID nor its caller in nsRuleNode is visible in this patch, so I don't think you're answering the question I was asking.  However,  nsFont::GetGenericID only ever sets one bit, so it looks like things are ok.

(That said, the CSS spec allows multiple generics...)

Comment 4

[Serge Gautherie (:sgautherie)]

Comment on attachment 341696 [details] [diff] [review]
(rev 1) patch
[Checkin: Comment 4]

http://hg.mozilla.org/mozilla-central/rev/bf3f9fbfb543