443057 - replace some (coord,chars) nsStyleCoord with nscoord

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

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

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)".)

Comment 1

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

I missed mBorderSpacingX, mBorderSpacingY.

Comment 2

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

Attachment: fix comments describing types

Comment 3

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

Attachment: fix mOutlineOffset

Comment 4

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

Attachment: fix mBorderSpacing{X,Y}

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 5

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

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.

Comment 6

[Boris Zbarsky [: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.

Comment 7

[Boris Zbarsky [:bzbarsky]]

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.

Comment 8

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

Landed the first patch: http://hg.mozilla.org/mozilla-central/index.cgi/rev/8666db48bea6

Comment 9

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

Attachment: fix mOutlineOffset

Comment 10

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

Attachment: fix mBorderSpacing{X,Y}

Comment 11

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

Attachment: fix shadow arrays (mBoxShadow and mTextShadow)

Comment 12

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

Attachment: a few last comment fixes

This makes the terminology in the comments consistent (internally, and with nsStyleCoord itself).  Most places already used "coord" rather than "length".

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 338385 [details] [diff] [review]
fix mOutlineOffset

r+sr=bzbarsky

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 338386 [details] [diff] [review]
fix mBorderSpacing{X,Y}

r+sr=bzbarsky

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 338387 [details] [diff] [review]
fix shadow arrays (mBoxShadow and mTextShadow)

r+sr=bzbarsky

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 338388 [details] [diff] [review]
a few last comment fixes

Looks good.

Comment 17

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

http://hg.mozilla.org/mozilla-central/rev/47e7de9ac585
http://hg.mozilla.org/mozilla-central/rev/ade776b76598
http://hg.mozilla.org/mozilla-central/rev/e26688231f7b
http://hg.mozilla.org/mozilla-central/rev/4d29ef2bee5e
->fixed