Rename nsStyleTableBorder::mBorderSpacing{X,Y} to logical style

RESOLVED FIXED in Firefox 39

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: xidorn, Assigned: xidorn)

Tracking

Trunk
mozilla39
Points:
---

Firefox Tracking Flags

(firefox39 fixed)

Details

(URL)

Attachments

(2 attachments)

(Assignee)

Description

4 years ago
We currently use mBorderSpacing{X,Y}, but the writing mode spec says:

The first and second values of the border-spacing property represent spacing between columns and rows respectively, not necessarily the horizontal and vertical spacing respectively. 

Hence, intend to rename them to mBorderSpacing{Col,Row}

I guess it makes sense to also rename nsTableFrame::GetCellSpacing{X,Y}, but I'm not sure how they should be renamed. Probably GetCell{I,B}Spacing, or Get{Col,Row}Spacing?
(Assignee)

Comment 1

4 years ago
Created attachment 8579941 [details] [diff] [review]
patch 1 - rename mBorderSpacingX/Y
Assignee: nobody → quanxunzhen
Attachment #8579941 - Flags: review?(cam)
(Assignee)

Comment 2

4 years ago
Created attachment 8579943 [details] [diff] [review]
patch 2 - rename GetCellSpacingX/Y
Attachment #8579943 - Flags: review?(roc)
Comment on attachment 8579941 [details] [diff] [review]
patch 1 - rename mBorderSpacingX/Y

Review of attachment 8579941 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/nsRuleNode.cpp
@@ +7906,5 @@
>                NS_STYLE_BORDER_SEPARATE, 0, 0, 0, 0);
>  
>    const nsCSSValue* borderSpacingValue = aRuleData->ValueForBorderSpacing();
>    if (borderSpacingValue->GetUnit() != eCSSUnit_Null) {
> +    // border-spacing-column/row: length, inherit

This comment shouldn't change, since the property names aren't changing.
Attachment #8579941 - Flags: review?(cam) → review+
(Assignee)

Comment 4

4 years ago
(In reply to Cameron McCormack (:heycam) from comment #3)
> Comment on attachment 8579941 [details] [diff] [review]
> patch 1 - rename mBorderSpacingX/Y
> 
> Review of attachment 8579941 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/style/nsRuleNode.cpp
> @@ +7906,5 @@
> >                NS_STYLE_BORDER_SEPARATE, 0, 0, 0, 0);
> >  
> >    const nsCSSValue* borderSpacingValue = aRuleData->ValueForBorderSpacing();
> >    if (borderSpacingValue->GetUnit() != eCSSUnit_Null) {
> > +    // border-spacing-column/row: length, inherit
> 
> This comment shouldn't change, since the property names aren't changing.

I don't think we ever have properties called border-spacing-x/y.
Ah, thought that we had split out properties, but both values just get set from border-spacing.  The comment should say "border-spacing: pair(length), inherit" then, to match other comments in the file.
https://hg.mozilla.org/mozilla-central/rev/bd5e71db6003
https://hg.mozilla.org/mozilla-central/rev/778616c8f74e
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.