Closed Bug 454292 Opened 11 years ago Closed 11 years ago

prbool misuse in layout/

Categories

(Core :: Layout, defect)

Other
Linux
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: taras.mozilla, Assigned: taras.mozilla)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch fixes (obsolete) — Splinter Review
It'd be cool to switch PRBools that are used within function bodies and do not escape to be C++ bool type. Any objection to changing one such case in the attached patch?
Attachment #337531 - Flags: review?(dbaron)
Comment on attachment 337531 [details] [diff] [review]
fixes

>diff --git a/layout/forms/nsListControlFrame.cpp b/layout/forms/nsListControlFrame.cpp

The changes in this file look wrong.  I don't think aStatus is related to what should be passed for the third argument of DidReflow.  This looks like a real bug, though, and should probably be put in a separate bug report.

>diff --git a/layout/xul/base/src/nsMenuPopupFrame.cpp b/layout/xul/base/src/nsMenuPopupFrame.cpp
>--- a/layout/xul/base/src/nsMenuPopupFrame.cpp
>+++ b/layout/xul/base/src/nsMenuPopupFrame.cpp
>@@ -576,7 +576,7 @@ LazyGeneratePopupDone(nsIContent* aPopup
>       if (!weakFrame.IsAlive())
>         return;
> 
>-      PRBool selectFirstItem = (PRBool)NS_PTR_TO_INT32(aArg);
>+      PRBool selectFirstItem = !!NS_PTR_TO_INT32(aArg);
>       if (selectFirstItem) {
>         nsMenuFrame* next = pm->GetNextMenuItem(popupFrame, nsnull, PR_TRUE);
>         popupFrame->SetCurrentMenuItem(next);

I don't see why this is needed.  This is a boolean that was cast to a pointer in order to be used as the data argument to a callback function; this is just casting it back to what it was originally.

The rest looks fine.

As for |bool|; starting to use bool seems ok if the compilers we care about support it, though if we're going to start switching we should probably have a discussion about where we should switch and where we shouldn't.
Attachment #337531 - Flags: review?(dbaron) → review-
(In reply to comment #1)
> (From update of attachment 337531 [details] [diff] [review])
> >diff --git a/layout/forms/nsListControlFrame.cpp b/layout/forms/nsListControlFrame.cpp
> 
> The changes in this file look wrong.  I don't think aStatus is related to what
> should be passed for the third argument of DidReflow.  This looks like a real
> bug, though, and should probably be put in a separate bug report.

Ok, I'll do that, who should I assign it to?

> 
> >diff --git a/layout/xul/base/src/nsMenuPopupFrame.cpp b/layout/xul/base/src/nsMenuPopupFrame.cpp
> >--- a/layout/xul/base/src/nsMenuPopupFrame.cpp
> >+++ b/layout/xul/base/src/nsMenuPopupFrame.cpp
> >@@ -576,7 +576,7 @@ LazyGeneratePopupDone(nsIContent* aPopup
> >       if (!weakFrame.IsAlive())
> >         return;
> > 
> >-      PRBool selectFirstItem = (PRBool)NS_PTR_TO_INT32(aArg);
> >+      PRBool selectFirstItem = !!NS_PTR_TO_INT32(aArg);
> >       if (selectFirstItem) {
> >         nsMenuFrame* next = pm->GetNextMenuItem(popupFrame, nsnull, PR_TRUE);
> >         popupFrame->SetCurrentMenuItem(next);
> 
> I don't see why this is needed.  This is a boolean that was cast to a pointer
> in order to be used as the data argument to a callback function; this is just
> casting it back to what it was originally.

This isn't obvious, I suppose we can let it be.
Attachment #337531 - Attachment is obsolete: true
Attachment #337561 - Flags: review?(dbaron)
Comment on attachment 337561 [details] [diff] [review]
2 remaining fixes

!! r+sr=dbaron
Attachment #337561 - Flags: superreview+
Attachment #337561 - Flags: review?(dbaron)
Attachment #337561 - Flags: review+
pushed 4a1ad24a36bc
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Assignee: nobody → tglek
You need to log in before you can comment on or make changes to this bug.