Closed Bug 326033 Opened 19 years ago Closed 17 years ago

debug code in nsTreeBodyFrame::Paint alters class members

Categories

(Core :: XUL, defect)

Product:
Core
Component:
XUL
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: ajschult784, Assigned: ajschult784)

References

(
URL
)

Details

Attachments

(2 files, 1 obsolete file)

the right patch
(deleted), patch
Details | Diff | Splinter Review
Change the assertion to a warning to allow flooding
(deleted), patch
smaug
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review
The debug code in nsTreeBodyFrame::Paint from bug 208093 alters the mRowCount field when it really just wants to check it.
Attached patch patch (obsolete) (deleted) — — Splinter Review
Attachment #210823 - Flags: superreview?(neil)
Attachment #210823 - Flags: review?(neil)
See bug 208093 comment 19.
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → INVALID
Comment on attachment 210823 [details] [diff] [review] patch Wrong patch anyway - you need to manage your patches better :-P
Attachment #210823 - Flags: superreview?(neil)
Attachment #210823 - Flags: review?(neil)
As a matter of policy debug code should not change program behavior. It means that debug builds behave differently than release builds. (Real, non-assertion) bugs are reproducible in one and not the other.
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
Attached patch the right patch (deleted) — — Splinter Review
> Wrong patch anyway - you need to manage your patches better :-P That, or not attach them a 2am local time. Also, as in bug 309967, it asserts multiple times because of this, rather than in spite of it.
Attachment #210823 - Attachment is obsolete: true
Comment on attachment 210869 [details] [diff] [review] the right patch But if you do this then when someone get the row count wrong you'll get flooded by the paint-time assertions thus making it useless. (In reply to comment #5) >Also, as in bug 309967, it asserts multiple times because of this, rather than >in spite of it. Bug 309967 was still an abuse of the API, so the asserts were valid.
Flags: blocking1.9a1+
Flags: blocking1.9+
Flags: blocking1.9a1+
Neil - can you take a review of the patch - looks like a straightforward bug fix.. would be great to get it in or close the bug out..
(In reply to comment #6) >(From update of attachment 210869 [details] [diff] [review]) >But if you do this then when someone get the row count wrong you'll get flooded >by the paint-time assertions thus making it useless. You could of course switch to paint-time warnings which wouldn't be so annoying.
Attachment #277707 - Flags: superreview?(bzbarsky)
Attachment #277707 - Flags: review?(Olli.Pettay)
Comment on attachment 277707 [details] [diff] [review] Change the assertion to a warning to allow flooding sr=bzbarsky but I personally am OK with the "flooding" thing.... Though I can see how on Windows it would be bad.
Attachment #277707 - Flags: superreview?(bzbarsky) → superreview+
Attachment #277707 - Flags: review?(Olli.Pettay) → review+
Fix checked in.
Status: REOPENED → RESOLVED
Closed: 19 years ago → 17 years ago
Resolution: --- → FIXED
Component: XP Toolkit/Widgets: Trees → XUL
QA Contact: xptoolkit.trees → xptoolkit.widgets
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: