Open Bug 517553 Opened 10 years ago Updated 9 years ago

Remove '=='/'!=' 'PR_FALSE'/'PR_TRUE'

Categories

(Core :: General, defect, trivial)

defect
Not set
trivial

Tracking

()

REOPENED

People

(Reporter: lusian, Assigned: lusian)

References

()

Details

Attachments

(6 files, 26 obsolete files)

58.14 KB, patch
lusian
: review?
wtc
Details | Diff | Splinter Review
65.08 KB, patch
lusian
: review?
wtc
Details | Diff | Splinter Review
4.99 KB, patch
joe
: review-
Details | Diff | Splinter Review
58.35 KB, patch
Details | Diff | Splinter Review
8.99 KB, patch
Details | Diff | Splinter Review
9.07 KB, patch
Details | Diff | Splinter Review
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a1pre) Gecko/20090917 Minefield/3.7a1pre (.NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a1pre) Gecko/20090917 Minefield/3.7a1pre (.NET CLR 3.5.30729)

Enforce Mozilla Coding Style Guide

Do not compare == PR_TRUE or PR_FALSE. Use (x) or (!x) instead. == PR_TRUE, in fact, is *different* from if (x)!

Related to Bug 228448

Reproducible: Didn't try
Blocks: 228448
Thanks for the patch. You should find someone who is willed to review your patch.
Assignee: nobody → lusian
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
OS: Windows XP → All
Hardware: x86 → All
Attachment #401513 - Flags: review?
Attachment #401513 - Flags: review?
As talked on IRC there are tryserver builds available here:
https://build.mozilla.org/tryserver-builds/hskupin@mozilla.com-1253305907/

Please check if everything works. Afterward you have to ask for review.
Attachment #401513 - Flags: review?(shaver)
Attachment #401513 - Flags: review?(Olli.Pettay)
Attachment #401513 - Flags: review?(Jan.Varga)
Comment on attachment 401513 [details] [diff] [review]
Remove '=='/'!=' 'PR_FALSE'/'PR_TRUE' in mozilla-central

I will review the NSPR and NSS changes in this patch.
Attachment #401513 - Flags: review?(wtc)
Attached patch nspr partSplinter Review
I will break the patch into smaller parts.
Attachment #401513 - Attachment is obsolete: true
Attachment #409851 - Flags: review?(wtc)
Attachment #401513 - Flags: review?(wtc)
Attachment #401513 - Flags: review?(shaver)
Attachment #401513 - Flags: review?(Olli.Pettay)
Attachment #401513 - Flags: review?(Jan.Varga)
Attached patch nss partSplinter Review
Attachment #409852 - Flags: review?(wtc)
Attached patch widget beos (obsolete) — Splinter Review
Attachment #409853 - Flags: review?(cbiesinger)
Attached patch widget gtk2 (obsolete) — Splinter Review
Attachment #409854 - Flags: review?(mozbugz)
Attached patch xpcom (obsolete) — Splinter Review
Attachment #409855 - Flags: review?(shaver)
Attached patch netwerk (obsolete) — Splinter Review
Attachment #409856 - Flags: review?(darin.moz)
Attached patch toolkit (obsolete) — Splinter Review
Attachment #409857 - Flags: review?
Attachment #409857 - Flags: review? → review?(robert.bugzilla)
Attached patch rdf (obsolete) — Splinter Review
Attachment #409858 - Flags: review?(axel)
Attached patch xpconnect (obsolete) — Splinter Review
Attachment #409862 - Flags: review?(jband_mozilla)
Attached patch xptoolkit (obsolete) — Splinter Review
Attachment #409863 - Flags: review?(Jan.Varga)
Attached patch widget windows (obsolete) — Splinter Review
Attachment #409864 - Flags: review?(jmathies)
Attached patch svg (obsolete) — Splinter Review
Attachment #409865 - Flags: review?(jwatt)
Attached patch docshell (obsolete) — Splinter Review
Attachment #409867 - Flags: review?(benjamin)
Attached patch graphicsSplinter Review
Attachment #409868 - Flags: review?(pavlov)
Attachment #409865 - Flags: review?(jwatt) → review+
Attached patch style (obsolete) — Splinter Review
Attachment #409874 - Flags: review?(hyatt)
Attached patch intl (obsolete) — Splinter Review
Attachment #409875 - Flags: review?(smontagu)
Attachment #409875 - Flags: review?(smontagu) → review+
Attachment #409858 - Flags: review?(axel) → review+
Attachment #409864 - Flags: review?(jmathies) → review+
Attachment #409863 - Flags: review?(Jan.Varga) → review+
Attachment #409867 - Flags: review?(benjamin) → review+
Attachment #409855 - Flags: review?(shaver) → review+
Attachment #409856 - Flags: review?(darin.moz) → review?(cbiesinger)
Attachment #409862 - Flags: review?(jband_mozilla) → review+
Attachment #409874 - Flags: review?(hyatt) → review?(bzbarsky)
Attachment #409874 - Flags: review?(bzbarsky) → review+
Comment on attachment 409856 [details] [diff] [review]
netwerk

+++ b/netwerk/cookie/src/nsCookie.h

I'd prefer you not to change this file... you're storing a PRBool (i.e. a 32-bit integer) in a PRPackedBool (an 8-bit integer). so, if someone passes a value that's not true or false to this function, this risks doing the wrong thing.
Attachment #409856 - Flags: review?(cbiesinger) → review+
Comment on attachment 409853 [details] [diff] [review]
widget beos

+++ b/widget/src/beos/nsDragService.cpp
-                                  aCanDrop == PR_TRUE?"TRUE":"FALSE"));
+                                   aCanDrop?"TRUE":"FALSE"));

while you're here, can you add spaces around ? and : ?

+++ b/widget/src/beos/nsPopupWindow.cpp
+	if (bState && mView && mView->Window() != NULL )

also, can you remove the space before the ) ?
Attachment #409853 - Flags: review?(cbiesinger) → review+
out of curiosity, why didn't you use bug 228448 for these patches? (at least the non-toolkit ones)
Serge Gautherie suggested to open my own bug blocking bug 228448.
Attached patch netwerk, comment 21 (obsolete) — Splinter Review
Attachment #409856 - Attachment is obsolete: true
Attached patch widget beos, comment 22 (obsolete) — Splinter Review
Attachment #409853 - Attachment is obsolete: true
Comment on attachment 409854 [details] [diff] [review]
widget gtk2

>-  PRBool allocate = (GlobalPrinters::GetInstance()->PrintersAreAllocated() == PR_FALSE);
>+  PRBool allocate = !(GlobalPrinters::GetInstance()->PrintersAreAllocated());

The parentheses are not necessary here.
Attachment #409854 - Flags: review?(mozbugz) → review+
Attached patch widget gtk2, comment 27 (obsolete) — Splinter Review
Attachment #409854 - Attachment is obsolete: true
Attached patch xpfe (obsolete) — Splinter Review
Attachment #410181 - Flags: review?(neil)
Attached patch content/smil (obsolete) — Splinter Review
Attachment #410183 - Flags: review?(birtles)
Attached patch embedding (obsolete) — Splinter Review
Attachment #410184 - Flags: review?(jst)
Attached patch dom (obsolete) — Splinter Review
Attachment #410189 - Flags: review?(hsivonen)
Attached patch layout (obsolete) — Splinter Review
Attachment #410193 - Flags: review?(roc)
Attachment #410183 - Flags: review?(birtles) → review+
Attached patch rdf part 2 (obsolete) — Splinter Review
Attachment #410195 - Flags: review?(axel)
Attached patch widget (obsolete) — Splinter Review
Attachment #410196 - Flags: review?(doug.turner)
Attached patch debugging (obsolete) — Splinter Review
Attachment #410198 - Flags: review?(dbaron)
Attachment #410195 - Flags: review?(axel) → review+
Attachment #409857 - Flags: review?(robert.bugzilla) → review?(wuno)
Comment on attachment 410189 [details] [diff] [review]
dom

Looks good to me, but I'm not really a module peer for these files.
Attachment #410189 - Flags: review?(hsivonen) → review+
Product: Firefox → Core
QA Contact: general → general
Attachment #410198 - Flags: review?(dbaron) → review+
Attachment #410196 - Flags: review?(doug.turner) → review+
Attachment #410181 - Flags: review?(neil) → review+
Comment on attachment 409857 [details] [diff] [review]
toolkit

looks good, I don't know anything from BeOS but I've had a look on the BeOS widget patch that got already r+
Attachment #409857 - Flags: review?(wuno) → review+
Attached patch check-in needed (obsolete) — Splinter Review
I would like check-in this patch before it gets too old.
Attachment #409855 - Attachment is obsolete: true
Attachment #409857 - Attachment is obsolete: true
Attachment #409858 - Attachment is obsolete: true
Attachment #409862 - Attachment is obsolete: true
Attachment #409863 - Attachment is obsolete: true
Attachment #409864 - Attachment is obsolete: true
Attachment #409865 - Attachment is obsolete: true
Attachment #409867 - Attachment is obsolete: true
Attachment #409874 - Attachment is obsolete: true
Attachment #409875 - Attachment is obsolete: true
Attachment #410027 - Attachment is obsolete: true
Attachment #410030 - Attachment is obsolete: true
Attachment #410055 - Attachment is obsolete: true
Attachment #410181 - Attachment is obsolete: true
Attachment #410183 - Attachment is obsolete: true
Attachment #410189 - Attachment is obsolete: true
Attachment #410193 - Attachment is obsolete: true
Attachment #410195 - Attachment is obsolete: true
Attachment #410196 - Attachment is obsolete: true
Attachment #410198 - Attachment is obsolete: true
Keywords: checkin-needed
Sadly, the trunk didn't reopen in time, and it's badly bit-rotted.
Keywords: checkin-needed
Attachment #413552 - Attachment is obsolete: true
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/53308118abed
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Attachment #425658 - Attachment description: check-in please → check-in please [Checkin: Comment 42]
Reopening since there are more patches outstanding.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #410184 - Flags: review?(jst) → review+
Attachment #410184 - Attachment is obsolete: true
Keywords: checkin-needed
Question: When testing this, did you see any crashes on Windows?  We hit a new crash on last night's nightly with a heavy amount of hits and we're trying to find the regression range.
Comment on attachment 425924 [details] [diff] [review]
embedding updated [checkin: Comment 46]

http://hg.mozilla.org/mozilla-central/rev/e0f83c3ba635
Attachment #425924 - Attachment description: embedding updated, checkin please → embedding updated [checkin: Comment 46]
I am working on this bug. I have created a patch that makes these changes to a single file. If the changes are good, I will continue making them elsewhere.
(In reply to comment #48)
> Fixes in nsprpub/pr/src/md/windows/ntio.c

Many of these changes look similar to changes in attachment 409851 [details] [diff] [review], which hasn't received much attention.
NSS and NSPR are third party libraries from Gecko's perspective.
Patches might receive more attention in an upstream bug report (but they might not).
Attachment #409868 - Flags: review?(pavlov) → review?(joe)
Comment on attachment 409868 [details] [diff] [review]
graphics

This patch needs to be updated to latest mozilla-central. Most of these files have been moved, and some no longer exist. However, in general, it looks good, with one caveat that I list below.

>diff --git a/gfx/thebes/public/gfxFont.h b/gfx/thebes/public/gfxFont.h

>         PRUint32 SetCanBreakBefore(PRBool aCanBreakBefore) {
>-            NS_ASSERTION(aCanBreakBefore == PR_FALSE || aCanBreakBefore == PR_TRUE,
>+            NS_ASSERTION(!aCanBreakBefore || aCanBreakBefore,
>                          "Bogus break-before value!");

This is actually checking whether aCanBreakBefore is PR_FALSE or PR_TRUE, so we can't change this assertion.
Attachment #409868 - Flags: review?(joe) → review-
(In reply to comment #50)
> This is actually checking whether aCanBreakBefore is PR_FALSE or PR_TRUE, so we
> can't change this assertion.

Yet adding an explicit comment would be an improvement.
You need to log in before you can comment on or make changes to this bug.