Closed Bug 870606 Opened 7 years ago Closed 6 years ago

nsBlockFrame.cpp has an ineffective GetSkipSides() check

Categories

(Core :: Layout, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: dholbert, Assigned: mats)

References

()

Details

nsBlockFrame.cpp has this GetSkipSides check right now:
> 907     if (GetSkipSides() & NS_SIDE_TOP) {
> 908       heightExtras.top = 0;

However -- NS_SIDE_TOP is the value *zero*, as asserted here:
> 1458 MOZ_STATIC_ASSERT(NS_SIDE_TOP == 0 && NS_SIDE_RIGHT == 1
http://mxr.mozilla.org/mozilla-central/source/layout/style/nsCSSValue.cpp#1458

So that check in nsBlockFrame is currently checking "if (GetSkipSides() & 0)", which is always false, since anything & 0 is false.

It **really** wants to be checking GetSkipSides() & (1 << NS_SIDE_TOP)) -- GetSkipSides returns its value as a bitfield, with the bits shifted by NS_SIDE_* values.
The lines in question has now changed to:
>1043     if (GetLogicalSkipSides() & (LOGICAL_SIDE_B_START)) {
>1044       blockDirExtras.BStart(wm) = 0;

http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsBlockFrame.cpp?rev=8bc5711c589e#1043

and the LOGICAL_SIDE_* values are side *bits* confusingly enough...
so this is now fixed.

(I'll address this in general in bug 1028460 so that any misuse
will fail to compile.)
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
(In reply to Mats Palmgren (:mats) from comment #2)
 
> and the LOGICAL_SIDE_* values are side *bits* confusingly enough...

Why is that confusing? I did it that way because I thought bits were  *less* confusing than shift values, but if I'm missing something it can always change back.
Assignee: nobody → mats
Target Milestone: --- → mozilla33
(In reply to Simon Montagu :smontagu from comment #3)
> Why is that confusing? I did it that way because I thought bits were  *less*
> confusing than shift values, but if I'm missing something it can always
> change back.

I prefer bits as well, but the *naming* makes it error prone because:
NS_SIDE_TOP -- a bit position, usage for skip-sides: (1 << NS_SIDE_TOP)
LOGICAL_SIDE_B_START -- an error if used for shifting
yet, they both have *_SIDE_* in the name so it's easy to make the
mistake that former is physical, the latter is logical, but otherwise
the same thing, i.e. to be used for shifting.  See bug 1028460 for
an example of that.

In that bug, I'm changing the 'int' type to a proper SkipSides type
so that making such a mistake will cause a compile error.
And perhaps soon (as writing-mode changes makes margins etc logical)
we won't need the physical GetSkipSides(), so maybe renaming the
values isn't needed at this point.
You need to log in before you can comment on or make changes to this bug.