452797 - Clean up unused variables in layout/

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Arpad Borsos [:Swatinem]]

Attachment: cleanup

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.

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+#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?

Comment 2

[Arpad Borsos [:Swatinem]]

Attachment: v2 [Backed out: Comment 5]

Ok, done.

Comment 3

[Serge Gautherie (:sgautherie)]

Comment on attachment 336326 [details] [diff] [review]
v2
[Backed out: Comment 5]

http://hg.mozilla.org/mozilla-central/rev/54215f2cbc66

Comment 4

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1) (debug) bustage fix [Backed out: Comment 5]

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 :-(

Comment 5

[timeless]

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.

Comment 6

[Arpad Borsos [:Swatinem]]

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.

Comment 7

[Arpad Borsos [:Swatinem]]

Attachment: v3

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.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+        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.

Comment 9

[Arpad Borsos [:Swatinem]]

Attachment: crashtest for bug454186 [Checkin: Comment 17]

Ok, so here is a crashtest that hits nsTreeBodyFrame::PaintProgressMeter.
But the latest patch is still crashing with it, I will investigate that further.

Comment 10

[Arpad Borsos [:Swatinem]]

Attachment: v4, passes crashtest

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.

Comment 11

[Arpad Borsos [:Swatinem]]

Attachment: v4, right patch

Sorry, some changed from another patch have sneaked in.

Comment 12

[Benjamin Smedberg]

Comment on attachment 338647 [details] [diff] [review]
v4, right patch

I do not think we want to paper over errors in ToInteger.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 338624 [details] [diff] [review]
crashtest for bug454186
[Checkin: Comment 17]

This can land right away.

Comment 14

[Arpad Borsos [:Swatinem]]

Attachment: v5, without changes to ToInteger

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.

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 16

[Arpad Borsos [:Swatinem]]

Attachment: v6 [Checkin: Comment 18]

(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

Comment 17

[Serge Gautherie (:sgautherie)]

Comment on attachment 338624 [details] [diff] [review]
crashtest for bug454186
[Checkin: Comment 17]

http://hg.mozilla.org/mozilla-central/rev/b4d91cd61404

Comment 18

[Serge Gautherie (:sgautherie)]

Comment on attachment 339428 [details] [diff] [review]
v6
[Checkin: Comment 18]

http://hg.mozilla.org/mozilla-central/rev/53ef3c05e895