Closed Bug 443057 Opened 13 years ago Closed 12 years ago

replace some (coord,chars) nsStyleCoord with nscoord

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.1b1

People

(Reporter: dbaron, Assigned: dbaron)

References

Details

Attachments

(5 files, 2 obsolete files)

As I said in bug 363706 comment 40, in nsStyleStruct.h, there are a bunch of nsStyleCoord members (and maybe nsStyleSides too?) that are nsStyleCoord only to carry the (coord,chars) tagging, which after bug 363706 no longer exists.  These could just become nscoord members:

# Another followup we can do is replace a number of nsStyleCoord in the style
# structs (e.g., mOutlineOffset, nsTextShadowItem::m{X,Y}Offset, and probably
# some others) with nscoord, since they only reason they used the union type was
# to carry the coord/chars distinction.

(I'd also note that all nsStyleCoord and nsStyleSides members should really list which of the unioned types they allow in the comment next to the member; most of them do this, but we should probably finish it for the rest.  We should probably also clean up the ones that say "length (chars, coord)".)
I missed mBorderSpacingX, mBorderSpacingY.
Assignee: nobody → dbaron
Attached patch fix mBorderSpacing{X,Y} (obsolete) — Splinter Review
I didn't do the text-shadow pieces because I know ventnor has a box-shadow patch floating around that it would conflict with.
Comment on attachment 327747 [details] [diff] [review]
fix comments describing types

May as well land this part; I'd also be interested in your opinion on the other parts.
Attachment #327747 - Flags: superreview?(bzbarsky)
Attachment #327747 - Flags: review?(bzbarsky)
Comment on attachment 327747 [details] [diff] [review]
fix comments describing types

r+sr=bzbarsky, but how would SetCoord return false here?  Should we be asserting that it returns true?

The outline offset patch looks fine, except that "Note that these are" comment near the end needs to have GetOutlineOffset removed from it.

The border-spacing patch looks great.
Attachment #327747 - Flags: superreview?(bzbarsky)
Attachment #327747 - Flags: superreview+
Attachment #327747 - Flags: review?(bzbarsky)
Attachment #327747 - Flags: review+
Er, the comments on SetCoord apply to the other two patches, not to the one I marked r+sr on.  I just got lost in the tiny textfield.
Attachment #327748 - Attachment is obsolete: true
Attachment #338385 - Flags: superreview?(bzbarsky)
Attachment #338385 - Flags: review?(bzbarsky)
Attachment #327749 - Attachment is obsolete: true
Attachment #338386 - Flags: superreview?(bzbarsky)
Attachment #338386 - Flags: review?(bzbarsky)
This makes the terminology in the comments consistent (internally, and with nsStyleCoord itself).  Most places already used "coord" rather than "length".
Attachment #338388 - Flags: superreview?(bzbarsky)
Attachment #338388 - Flags: review?(bzbarsky)
Comment on attachment 338385 [details] [diff] [review]
fix mOutlineOffset

r+sr=bzbarsky
Attachment #338385 - Flags: superreview?(bzbarsky)
Attachment #338385 - Flags: superreview+
Attachment #338385 - Flags: review?(bzbarsky)
Attachment #338385 - Flags: review+
Comment on attachment 338386 [details] [diff] [review]
fix mBorderSpacing{X,Y}

r+sr=bzbarsky
Attachment #338386 - Flags: superreview?(bzbarsky)
Attachment #338386 - Flags: superreview+
Attachment #338386 - Flags: review?(bzbarsky)
Attachment #338386 - Flags: review+
Comment on attachment 338387 [details] [diff] [review]
fix shadow arrays (mBoxShadow and mTextShadow)

r+sr=bzbarsky
Attachment #338387 - Flags: superreview?(bzbarsky)
Attachment #338387 - Flags: superreview+
Attachment #338387 - Flags: review?(bzbarsky)
Attachment #338387 - Flags: review+
Comment on attachment 338388 [details] [diff] [review]
a few last comment fixes

Looks good.
Attachment #338388 - Flags: superreview?(bzbarsky)
Attachment #338388 - Flags: superreview+
Attachment #338388 - Flags: review?(bzbarsky)
Attachment #338388 - Flags: review+
You need to log in before you can comment on or make changes to this bug.