Closed Bug 7032 Opened 25 years ago Closed 25 years ago

incorrect nsRect usages

Categories

(Core :: Layout: Form Controls, defect, P3)

x86
Linux
defect

Tracking

()

VERIFIED FIXED

People

(Reporter: stuartp, Assigned: kmcclusk)

Details

(Whiteboard: Have fix, will checkin as soon as tree as green.)

The use of nsRect in the majority of the codebase, from what I have seen, is invalid. It stores widths and heights as signed ints, whereas all uses of these are unsigned ints. This conversion is causing many calls to various things, such as nsWidget::Resize() to have extremely large sizes at times. The last time I looked at this, it seemed to be happening a great deal in nsView* where it was subtracting out the scrollbar dimensions. this was causing negative widths (instead of 0), which were then being passed to the widget code as unsigned ints, converting them to rather huge numbers. I don't see any purpose in nsRect's widths and heights being signed. Is there a reason for this?
Target Milestone: M7
Assignee: rickg → peterl
Yo Peter -- please respond.
Assignee: peterl → karnaze
Component: Layout → Widget Set
The error here that I see is in the widget code. nsRect uses nscoord for all of its values, not int, not unsigned int. Ignore the fact that nscoord is *currently* typedefed to an int, nscoord is a *signed cordinate value* nothing more, nothing less. nscoord and ints are NOT interchangeable. For the record, it's also perfectly legal to have a negative width or height in a rect. It's an interesting mathematical state, not an invalid one. The widget APIs and code should be using nscoord for its coordinate values, not ints. Nowhere in the codebase should we be interchanging nscoords and ints except when converting for native APIs or from *other* int values, like number of pixels. And those cases should be using sanctioned conversion functions to go from nscoord space into native or pixel space. (see nsUnitConversion for many examples...)
Assignee: karnaze → kmcclusk
Kevin, please make sure that we are using nscoords as Peter says.
ok. that makes sense. let me know if i can help fix it.
Target Milestone: M7 → M8
Status: NEW → ASSIGNED
Target Milestone: M8 → M9
M9?! This is an important bug and needs to be fixed.
Moving all Widget Set bugs, past and present, to new HTML Form Controls component per request from karnaze. Widget Set component will be retired shortly.
The widget library works in device units, not app units as implied by nscoord. Changing widget interface to accept nscoord values would require the widget library to understand how to convert app units into device units. The solution is to leave nsWidget accepting device units, but change any xpcode which passes bad values to nsWidget. We should also change all of the nsIWidget's usage of PRUint32's to PRInt32. This will not fix the bad usage it will simply allow us to put assertions in the platform specific widget code so we can detect when a bad call is made to a nsIWidget (i.e a negative value is passed for width or height). Restricting the device units to unsigned values is not desirable anyway. We don't need the extra positive extent of unsigned int's since the underlying window environments usually can not accept these large values anyway. I will change nsIWidget to use PRInt32's instead of PRUint32's when refering to device coordinates. I will also add assertions on WIN32 to flag places where negative values are passed for width's and heights.
Assignee: kmcclusk → beard
Status: ASSIGNED → NEW
Changed nsIWidget::Resize(...), nsIWidget::Move(...) to use PRInt32 instead of PRUInt32. Added assertions to nsWindow::Resize and nsWindow::Move under WIN32 to halt when a negative width or height is passed. Modified Mac and Linux to match. Patrick, I'm re-assigning this bug to you, Can you keep and eye on occurences when a negative width or height is passed to nsIWidget::Resize or nsIWIdget::Remove from the view manager? If this happens under WIN32 an assert will be triggered. I ran the viewer on a number of pages (test0..15) 20 of the top 100 and never saw an assert.
Status: NEW → ASSIGNED
I found and corrected a number of places in views, and viewer test app where negative values where being passed to nsIWidget::Resize. I have corrected them, and I will check them in when the tree opens.
Assignee: beard → kmcclusk
Status: ASSIGNED → NEW
Whiteboard: Have fix, will checkin as soon as tree as green.
Status: NEW → RESOLVED
Closed: 25 years ago
Resolution: --- → FIXED
Fixed in Aug, 10, 199 3:42 build.
Status: RESOLVED → VERIFIED
Based on Kevin's comments, marking as verified fixed in the Aug 11th build.
You need to log in before you can comment on or make changes to this bug.