nsRuleNode::ComputeTextData sets up mControlCharacterVisibility in the wrong order

RESOLVED FIXED in mozilla36



4 years ago
4 years ago


(Reporter: dholbert, Assigned: dholbert)


Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)



(1 attachment)



4 years ago
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:

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


4 years ago
Depends on: 947588

Comment 1

4 years ago
Created attachment 8517643 [details] [diff] [review]
fix v1: just reorder the SetDiscrete calls.
Assignee: nobody → dholbert
Attachment #8517643 - Flags: review?(roc)

Comment 2

4 years ago
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

(This nsRuleNode code will match that ordering now.)
Attachment #8517643 - Attachment description: fix v1 → fix v1: just reorder the SetDiscrete calls.
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...
Attachment #8517643 - Flags: review?(roc) → review+

Comment 4

4 years ago
(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.)
Depends on: 1055665
Flags: needinfo?(dholbert)

Comment 5

4 years ago
(comment-editing fail; "he" = xidorn, in bug 1055665)
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

4 years ago
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. :))

Flags: needinfo?(dholbert)


4 years ago
No longer depends on: 1055665
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.