Closed Bug 346586 Opened 18 years ago Closed 18 years ago

Non-PRBool values passed as PRBool in bug 241972

Categories

(Core :: DOM: Navigation, defect)

Product:
Core
Component:
DOM: Navigation
Platform:
x86
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: bzbarsky, Assigned: marria)

References

Details

(Keywords: fixed1.8.1)

Attachments

(1 file, 1 obsolete file)

pass PRBool into DoURILoad
(deleted), patch
bzbarsky
: review+
bzbarsky
: superreview+
dbaron
: approval1.8.1+
Details | Diff | Splinter Review
The patch for bug 241972 passes |aFlags & INTERNAL_LOAD_FLAGS_NEW_WINDOW| as a PRBool. It's not a boolean value; it shouldn't be being passed to a PRBool arg; doing such things has led to hard-to-track-down bugs in the past. Please fix accordingly.
Attached patch make sure to pass a PRBool into DoURILoad (obsolete) (deleted) — — Splinter Review
Sorry about that, thanks for catching it
Attachment #231552 - Flags: review?(bzbarsky)
Comment on attachment 231552 [details] [diff] [review] make sure to pass a PRBool into DoURILoad Why not just pass in (aFlags & INTERNAL_LOAD_FLAGS_NEW_WINDOW) != 0 ? If you do want to have the explicit boolean variable, I'm not sure I buy this name. Something like isFirstLoad would make more sense; see bug 346851.
Attached patch pass PRBool into DoURILoad (deleted) — — Splinter Review
Attachment #231552 - Attachment is obsolete: true
Attachment #231649 - Flags: review?(bzbarsky)
Attachment #231552 - Flags: review?(bzbarsky)
(In reply to comment #2) > (From update of attachment 231552 [details] [diff] [review] [edit]) > Why not just pass in (aFlags & INTERNAL_LOAD_FLAGS_NEW_WINDOW) != 0 ? > > If you do want to have the explicit boolean variable, I'm not sure I buy this > name. Something like isFirstLoad would make more sense; see bug 346851. > I don't care about having an explicit variable, so I just took it out. Thanks, Marria
Comment on attachment 231649 [details] [diff] [review] pass PRBool into DoURILoad Looks good. I assume you need someone to check this in, right?
Attachment #231649 - Flags: superreview+
Attachment #231649 - Flags: review?(bzbarsky)
Attachment #231649 - Flags: review+
(In reply to comment #5) > (From update of attachment 231649 [details] [diff] [review] [edit]) > Looks good. I assume you need someone to check this in, right? > I actually have a cvs account, thanks though.
Attachment #231649 - Flags: approval1.8.1?
Ah, cool. Thanks for fixing this!
Comment on attachment 231649 [details] [diff] [review] pass PRBool into DoURILoad a=dbaron on behalf of drivers. Please check in to MOZILLA_1_8_BRANCH and mark fixed1.8.1 once you have done so.
Attachment #231649 - Flags: approval1.8.1? → approval1.8.1+
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Keywords: fixed1.8.1
checked in on trunk and branch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: