454292 - prbool misuse in layout/

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[(dormant account)]

Attachment: fixes

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?

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 2

[(dormant account)]

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

Comment 3

[(dormant account)]

Attachment: 2 remaining fixes

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 337561 [details] [diff] [review]
2 remaining fixes

!! r+sr=dbaron

Comment 5

[(dormant account)]

pushed 4a1ad24a36bc