Closed Bug 940194 Opened 6 years ago Closed 6 years ago

Build netwerk/cookie in unified mode

Categories

(Core :: Networking: Cookies, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: mz_mhs-ctb, Assigned: cpeterson)

References

Details

(Whiteboard: [qa-])

Attachments

(2 files, 2 obsolete files)

No description provided.
Attached patch Patch (obsolete) — Splinter Review
Anyone else need to review this?
Attachment #8334345 - Flags: review?(ehsan)
I think this code change will be brittle because the unified network/cookie .cpp file will have two global variables called `gCookieService`: CookieChildService.cpp declares a CookieServiceChild* called gCookieService in the mozilla::net namespace and nsCookieService.cpp declares a nsCookieService* called gCookieService in the global namespace. nsCookieService.cpp also pulls the first gCookieService variable into the global namespace with `using namespace mozilla::net`.
I had a WIP patch for netwerk/cookie and, to avoid the gCookieService name conflicts, I renamed CookieServiceChild.cpp's gCookieService to sCookieServiceChild and made sCookieServiceChild a private static member of class CookieServiceChild.

I also consolidated the duplicated behavior and pref constants into a shared definition in CookieServiceChild.h.
Comment on attachment 8334345 [details] [diff] [review]
Patch

Review of attachment 8334345 [details] [diff] [review]:
-----------------------------------------------------------------

I agree with what Chris said.  :-)
Attachment #8334345 - Flags: review?(ehsan) → review-
Attached patch Patch (v2) (obsolete) — Splinter Review
Attachment #8334345 - Attachment is obsolete: true
Attachment #8334967 - Flags: review?(ehsan)
Attachment #8334967 - Flags: review?(ehsan) → review+
Attachment #8334967 - Attachment is obsolete: true
Keywords: checkin-needed
Michael, are you planning to update your patch here?  Thanks!
Flags: needinfo?(mz_mhs-ctb)
Yeah, might be a day or two though. :(
Flags: needinfo?(mz_mhs-ctb)
(In reply to comment #10)
> Yeah, might be a day or two though. :(

No rush, just wanted to check wit you to see if I need to pick this up!  Thanks :-)
I guess you should take this. The linker is not being friendly to me right now...
Assignee: mz_mhs-ctb → ehsan
This patch was much simpler than I expected: nsCookieService.cpp can't be unified because it #defines FORCE_PR_LOG and, conveniently, all the symbol name conflicts were between nsCookieService.cpp and CookieChildService.cpp. No code changes are necessary.
Assignee: ehsan → cpeterson
Attachment #8341560 - Flags: review?(ehsan)
Comment on attachment 8341560 [details] [diff] [review]
unify-netwerk-cookie.patch

Review of attachment 8341560 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah, that's what I suspected.
Attachment #8341560 - Flags: review?(ehsan) → review+
https://hg.mozilla.org/mozilla-central/rev/96800b422285
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.