Closed Bug 509956 Opened 12 years ago Closed 12 years ago

Bogus code in nsTextFrame::IsFloatingFirstLetterChild

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.3a1

People

(Reporter: bzbarsky, Assigned: masayuki)

References

Details

Attachments

(1 file)

This part of nsTextFrame:IsFloatingFirstLetterChild:

  if (!GetStateBits() & TEXT_FIRST_LETTER)
    return PR_FALSE;

does absolutely nothing, since ! has higher precedence than &.  Therefore we're doing either 0 & TEXT_FIRST_LETTER or 1 & TEXT_FIRST_LETTER, both of which are 0.  I assume the compiler just optimizes this whole block away.

If this block is needed for performance reasons, we need to fix the parentheses.  If it's not needed, it should be removed so as to not confuse people.

Note that this was pointed out almost a year ago in bug 426772 comment 55.
Status: NEW → ASSIGNED
Attached patch Patch v1.0Splinter Review
Sorry, I missed to check the neil's comment and thank you Boris, you reported this bug.
Attachment #394508 - Flags: superreview?(roc)
Attachment #394508 - Flags: review?(roc)
Comment on attachment 394508 [details] [diff] [review]
Patch v1.0

r=dbaron.  (sr is not needed)
Attachment #394508 - Flags: superreview?(roc)
Attachment #394508 - Flags: review?(roc)
Attachment #394508 - Flags: review+
http://hg.mozilla.org/mozilla-central/rev/606a5cf1b1e1
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
I'm not sure this patch should be also landed to 1.9.2 branch. The IsFloatingFirstLetterChild is called from nsTextFrame::Reflow. So, that is called many times. But I'm not sure the impact for the performance.
Keywords: checkin-needed
Target Milestone: --- → mozilla1.9.3a1
You need to log in before you can comment on or make changes to this bug.