Closed Bug 580449 Opened 10 years ago Closed 9 years ago

Address nsCookieService.cpp review

Categories

(Core :: Networking: Cookies, defect)

x86
Linux
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: jdm, Assigned: dwitte)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

>+++ b/netwerk/cookie/nsCookieService.cpp

We seem to be adding a lot of code duplicate to this file.  Why can't we push
the GetOriginatingURI stuff and whatnot into the *Internal methods?

>+    // Not passing an nsIChannel here means CanSetCookie will use the currently
>+    // active window to display the prompt. This isn't exactly ideal...

Please file a followup bug and cite the bug# here.
Attached patch patchSplinter Review
Assignee: nobody → dwitte
Status: NEW → ASSIGNED
Attachment #475934 - Flags: review?(josh)
Comment on attachment 475934 [details] [diff] [review]
patch

I'm assuming that switching from PRBool to a bool argument is OK, since we've been passing in a bool all this time.

We're also losing the null aHostURI check.  Isn't that bad?
(In reply to comment #2)
> I'm assuming that switching from PRBool to a bool argument is OK, since we've
> been passing in a bool all this time.

Yeah, and bool autopromotes to PRBool. (The reverse isn't true!)

> We're also losing the null aHostURI check.  Isn't that bad?

Nope, it's done by all the callers. (We could assert in the Internal method, I suppose!)
Comment on attachment 475934 [details] [diff] [review]
patch

r=me
Attachment #475934 - Flags: review?(josh) → review+
http://hg.mozilla.org/mozilla-central/rev/ae219ca8c0a1
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.