Closed Bug 452797 Opened 11 years ago Closed 11 years ago

Clean up unused variables in layout/

Categories

(Core :: Layout, defect, minor)

x86
Linux
defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla1.9.1b1

People

(Reporter: Swatinem, Assigned: Swatinem)

References

Details

Attachments

(2 files, 7 obsolete files)

Attached patch cleanup (obsolete) — Splinter Review
Cleans up all the unused variable warnings I encounter during non-debug builds.
Some unused variables are obvious but the most are only used during debug.
I wrapped those with #ifdefs, I hope that doesn't hurt readability too much.
Attachment #336056 - Flags: superreview?(roc)
Attachment #336056 - Flags: review?(roc)
+#ifdef DEBUG
   nsIFrame* prevInFlow = aNextInFlow->GetPrevInFlow();
+#endif
   NS_PRECONDITION(prevInFlow, "bad next-in-flow");

Can you just replace prevInFlow with aNextInFlow->GetPrevInFlow()?

+#ifdef DEBUG
         float min = position->mMinWidth.GetPercentValue();
+#endif
         NS_ASSERTION(min == 0.0f, "Non-zero percentage values not currently supported");

similar here?
Attached patch v2 [Backed out: Comment 5] (obsolete) — Splinter Review
Ok, done.
Attachment #336056 - Attachment is obsolete: true
Attachment #336326 - Flags: superreview?(roc)
Attachment #336326 - Flags: review?(roc)
Attachment #336056 - Flags: superreview?(roc)
Attachment #336056 - Flags: review?(roc)
Attachment #336326 - Flags: superreview?(roc)
Attachment #336326 - Flags: superreview+
Attachment #336326 - Flags: review?(roc)
Attachment #336326 - Flags: review+
Keywords: checkin-needed
Comment on attachment 336326 [details] [diff] [review]
v2
[Backed out: Comment 5]

http://hg.mozilla.org/mozilla-central/rev/54215f2cbc66
Attachment #336326 - Attachment description: v2 → v2 [Checkin: Comment 3]
Severity: normal → minor
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite-
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1b1
IIRC:
[20:52]	<Jesse>	sgautherie: i think your checkin for bug 452797 broke my build
	layout/generic/nsContainerFrame.cpp: In member function 'virtual void nsContainerFrame::DeleteNextInFlowChild(nsPresContext*, nsIFrame*)':
	layout/generic/nsContainerFrame.cpp:1136: error: 'prevInFlow' was not declared in this scope
[20:54]	prevInFlow is also used at the bottom of that function (in debug builds)

***

I reverted to the fix from the first patch ;->
http://hg.mozilla.org/mozilla-central/rev/18b48ea2b188

NB: Arpad, I guess you had not recompiled your v2 patch :-(
Depends on: 454186
i've backed this out tip changeset: a5491c33992e because it caused bug 454186 which i guess isn't covered by tests.

bz suggests/requests/requires that you audit this code carefully and include crash tests for the crashes you introduced.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Flags: in-testsuite-
Target Milestone: mozilla1.9.1b1 → ---
Attachment #337216 - Attachment description: (Bv1) (debug) bustage fix [Checkin: Comment 4] → (Bv1) (debug) bustage fix [Backed out: Comment 5]
Attachment #336326 - Attachment description: v2 [Checkin: Comment 3] → v2 [Backed out: Comment 5]
Ok, so it looks like ns*String::ToInteger writes an error Code to the outparameter, even if I pass it a null pointer. I have a patch ready that fixes that behavior, just need to kick off some builds to test it.
Attached patch v3 (obsolete) — Splinter Review
This patch fixes another warning I missed the first time.
It also removes the #ifdef DEBUGs from nsLayoutUtils::sDisableGetUsedXAssertions and nsAutoDisableGetUsedXAssertions to fix the last two warnings as discussed with roc over irc.
I also fixed up the ns*String::ToInteger methods as mentioned in comment 6, that should theoretically fix bug 454186. I'm afraid however, that I couldn't test this with seamonkey as I'm having some other issues with that.
I would be glad if someone could check if this patch works correctly with seamonkey.
Attachment #336326 - Attachment is obsolete: true
Attachment #337216 - Attachment is obsolete: true
Attachment #337734 - Flags: superreview?(roc)
Attachment #337734 - Flags: review?(roc)
Attachment #337734 - Flags: review?(benjamin)
+        NS_ASSERTION(position->mMinWidth.GetPercentValue() == 0.0f, "Non-zero percentage values not currently supported");
+        NS_ASSERTION(position->mMinHeight.GetPercentValue() == 0.0f, "Non-zero percentage values not currently supported");

Add line breaks so these lines don't get too long.

+    if (aErrorCode != nsnull)

if (aErrorCode) (several places)

Can you add a crashtest or mochitest for bug 454186?

Otherwise fine.
Ok, so here is a crashtest that hits nsTreeBodyFrame::PaintProgressMeter.
But the latest patch is still crashing with it, I will investigate that further.
Attachment #337734 - Attachment is obsolete: true
Attachment #338624 - Flags: review?(roc)
Attachment #337734 - Flags: superreview?(roc)
Attachment #337734 - Flags: review?(roc)
Attachment #337734 - Flags: review?(benjamin)
Attached patch v4, passes crashtest (obsolete) — Splinter Review
gdb revieled that the nsTString_CharT::ToInteger method inside nsTStringObsolete.cpp was hit and not the methods inside nsStringAPI.cpp.
So I fixed that method too and the patch now passes the crashtest.
Attachment #338645 - Flags: superreview?(roc)
Attachment #338645 - Flags: review?(roc)
Attachment #338645 - Flags: review?(benjamin)
Attached patch v4, right patch (obsolete) — Splinter Review
Sorry, some changed from another patch have sneaked in.
Attachment #338645 - Attachment is obsolete: true
Attachment #338647 - Flags: superreview?(roc)
Attachment #338647 - Flags: review?(roc)
Attachment #338647 - Flags: review?(benjamin)
Attachment #338645 - Flags: superreview?(roc)
Attachment #338645 - Flags: review?(roc)
Attachment #338645 - Flags: review?(benjamin)
Attachment #338647 - Flags: review?(benjamin) → review-
Comment on attachment 338647 [details] [diff] [review]
v4, right patch

I do not think we want to paper over errors in ToInteger.
Comment on attachment 338624 [details] [diff] [review]
crashtest for bug454186
[Checkin: Comment 17]

This can land right away.
Attachment #338624 - Flags: review?(roc) → review+
Keywords: checkin-needed
Whiteboard: crashtest needs checkin
Attached patch v5, without changes to ToInteger (obsolete) — Splinter Review
Bug #435293 introduced 4 new warnings on my build. 2 of the vars are only used in NS_ASSERTIONs so I wrapped them in #ifdef. The other 2 are completely unused but look like they are meant to be useful, so I just commented them out.

The patch doesn't include the fix for the unused outparam in PaintProgressMeter and thus does not include the changes to ToInteger functions.
Attachment #338647 - Attachment is obsolete: true
Attachment #339274 - Flags: superreview?(roc)
Attachment #339274 - Flags: review?(roc)
Attachment #338647 - Flags: superreview?(roc)
Attachment #338647 - Flags: review?(roc)
Attachment #339274 - Flags: superreview?(roc)
Attachment #339274 - Flags: superreview+
Attachment #339274 - Flags: review?(roc)
Attachment #339274 - Flags: review+
Comment on attachment 339274 [details] [diff] [review]
v5, without changes to ToInteger

-  PRInt32 cssPxWidth = 0, cssPxHeight = 0;
+  //PRInt32 cssPxWidth = 0, cssPxHeight = 0; // unused so far
 
Just remove it.
(In reply to comment #15)
> Just remove it.

Done. Also removed the comment above that says those vars would be used.
r/sr+ as per comment 15
Attachment #339274 - Attachment is obsolete: true
Attachment #339428 - Flags: superreview+
Attachment #339428 - Flags: review+
Whiteboard: crashtest needs checkin
Blocks: 456150
Comment on attachment 338624 [details] [diff] [review]
crashtest for bug454186
[Checkin: Comment 17]

http://hg.mozilla.org/mozilla-central/rev/b4d91cd61404
Attachment #338624 - Attachment description: crashtest for bug454186 → crashtest for bug454186 [Checkin: Comment 17]
Comment on attachment 339428 [details] [diff] [review]
v6
[Checkin: Comment 18]

http://hg.mozilla.org/mozilla-central/rev/53ef3c05e895
Attachment #339428 - Attachment description: v6 → v6 [Checkin: Comment 18]
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1b1
You need to log in before you can comment on or make changes to this bug.