steve, can you have a look? we need this for embedding. thanks
These changes look fine to me (they are safer than the previous code, but appear to be equivalent in all other respects). Looking at the code, I suspect that we've turned over a rock where a bunch of I18n issues are crawling. Can cookies contain non-ASCII characters? Also, it sort of sucks that we have to copy the string twice to return it. scc, any suggestions for the string-fu?
No, cookies cannot contain non-ascii characters. There is no i18n problem here.
r=morse on the patch
Waterson, since you already looked this over, would you like to super-review it?
I'll let scc do the honors, as he may have some ideas for improving the string usage.
+ str.AssignWithConversion(cookie); + nsCRT::free(cookie); + aCookie.Assign(str); Should be: CopyASCIItoUCS2(nsDependentCString(cookie), aCookie); nsMemory::free(cookie); to eliminate one string copy.
> No, cookies cannot contain non-ascii characters. There is no i18n > problem here. Well, with the APIs that we have, people can certainly _set_ cookies with non-ASCII characters: nsHTMLDocument::SetCookie(const nsAReadableString& aCookie) So, is the right behavior here to just completely corrupt the data (at line nsHTMLDocument.cpp:1992)? (Oy, is the right behavior to allocate a new PRUnichar buffer with the nsString() ctor, copy the data into the buffer, and then corrupt it?) Gratuitous copies aside, I'd have thought UTF-8 encoding it might be more polite. Anyway, if any of the intl guys want to take issue with that, let's file another bug.
How about changing: char *cookie = nsString(aCookie).ToNewCString(); to char *cookie = ToNewCString(aCookie); in ::SetCookie() to eliminate one extra malloc and string copy while we're at it? Or we could just as easily make that ToNewUTF8String(aCookie) and make ::GetCookie() code use NS_ConvertUTF8toUCS2() (which does one extra unavoidable string copy) to fix the unicode dataloss problems?
> Gratuitous copies aside, I'd have thought UTF-8 > encoding it might be more polite. Anyway, if any of > the intl guys want to take issue with that, let's file > another bug. I think the relevant RFC is the one above. The 'value' according to this RFC could be anything the server wants to set. But if we do any conversion on our side, how do we make sure that the stored value matches that expected by the server at the next visit? (Let's move this to another bug...)
nav triage team: Marking nsbeta1+ and p1.
This ia 0.9.2 bug. What's the deal here?
Awaiting for super-review.
Created attachment 39959 [details] [diff] [review] similar patch, fewer copies, still no UTF-8 support
r=morse for waterson's patch still awaiting sr from scc.
I like the patch, the only thing I might change is that I would try to manage the life of the |char*| at the end automatically, e.g., nsXPIDLCString cookie; cookie.Adopt(ToNewCString(aCookie)); if ( cookie.get() ) ... I don't feel strongly enough about this to deny you an sr, but if this feels right to you, go ahead and add it. We're just punting on UTF8? sr=scc
a=chofmann branch and trunk
Fix checked in to both trunck and branch
verified: win2k 2001082303