1094360 - nsRuleNode::ComputeTextData sets up mControlCharacterVisibility in the wrong order

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Daniel Holbert [:dholbert]]

Generally, I think the nsRuleNode::ComputeXYZData functions tend to initialize things in the same order that they're listed in the style-struct.

Noticed one exception to this, when reviewing a patch & looking at contextual code: mControlCharacterVisibility.

In nsStyleText, "mControlCharacterVisibility" is listed as the last enum value (after mTextOrientation and mTextCombineHorizontal).  But in nsRuleNode::ComputeTextData, it's initialized just before mTextOrientation and mTextCombineHorizontal.

It's been this way since we added mControlCharacterVisibility:
  http://hg.mozilla.org/mozilla-central/rev/cb4a327786ae#l4.12

We should just move that chunk of ComputeTextData down, to make it consistent.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: fix v1: just reorder the SetDiscrete calls.

Comment 2

[Daniel Holbert [:dholbert]]

Comment on attachment 8517643 [details] [diff] [review]
fix v1: just reorder the SetDiscrete calls.

(not sure why the patch chose to represent this as moving two calls up, instead of moving one call down, but whatever; same result.)

Here's the nsStyleStruct member-var list, for reference:
> 1598   uint8_t mTextSizeAdjust;              // [inherited] see nsStyleConsts.h
> 1599   uint8_t mTextOrientation;             // [inherited] see nsStyleConsts.h
> 1600   uint8_t mTextCombineUpright;          // [inherited] see nsStyleConsts.h
> 1601   uint8_t mControlCharacterVisibility;  // [inherited] see nsStyleConsts.h
http://mxr.mozilla.org/mozilla-central/source/layout/style/nsStyleStruct.h#1598

(This nsRuleNode code will match that ordering now.)

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8517643 [details] [diff] [review]
fix v1: just reorder the SetDiscrete calls.

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

I'm not really the right reviewer for this, but it's trivial enough...

Comment 4

[Daniel Holbert [:dholbert]]

(I picked you because you added this chunk of code. :) In the unlikely scenario that there was reason for the ordering, you'd be the most likely to know about that.)

Thanks for the review!

(I'm marking this as depending on bug 1055665, since he's touching this chunk of code, and I don't want to unnecessarily cause bitrot for him. I'll rebase & land after his patch lands.)

Comment 5

[Daniel Holbert [:dholbert]]

(comment-editing fail; "he" = xidorn, in bug 1055665)

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

I think it's better to land this patch first, since I'm not going to land my patch until I finish the rest of that bug which is currently blocked by bug 1052924.

Comment 7

[Daniel Holbert [:dholbert]]

OK, I'll go ahead & land then; sounds like you're OK rebasing on top of this.  (Shouldn't be too bad, just didn't want to surprise you with extra rebasing work. :))

Thanks!

Comment 8

[Daniel Holbert [:dholbert]]

Landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/2efb632cedb8

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2efb632cedb8