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.
Created attachment 8517643 [details] [diff] [review] fix v1: just reorder the SetDiscrete calls.
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Attachment #8517643 - Flags: review?(roc)
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.)
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+
(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
(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.
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!
Status: ASSIGNED → RESOLVED
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.