User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.0.1) Gecko/20020823 Netscape/7.0 Build Identifier: Looks like the fix for this bug: http://bugzilla.mozilla.org/show_bug.cgi?id=23508, introduced a xul dialog implementation to prompt the user to accept a cookie. This is a problem for embeddors that do not use xul. Previously, the prompt was done via the nsIPrompt service. Now, clients that do not use xul can't get the cookie prompts at all. (Version is 1.2 beta.) Can the xul implementation be pushed into a service that can be overridden, or can nsPermissions.cpp/permission_CheckConfirmYN() be modified to fall back to using the nsIPrompt service if the xul dialog doesn't exist? Reproducible: Always Steps to Reproduce:

Ooops - the user agent reported above does not apply to this issue - I was using the bugzilla helper when I filed this bug from netscape 7. This problem can be seen in the 1.2 beta code.

Cookie manager should offer fallback behaviour to use nsIPrompt or bury the prompting code in a service where it can be overridden by the embedder if necessary.

Reasigning to mvl@shop-engine.nl who implemented the fix to bug 23508

I don't think offering fallback behavior would do. Just because an embedding app *can* make this XUL dialog (it might inadvertantly have the chrome to do it), doesn't mean it *wants to* use the XUL dialog. Since the dialog shown in bug 23508 is not a general purpose dialog, I'd say we need an nsICookiePromptService iface, which embeddors could implement. Also, how did bug 23508 get reviewed and checked in considering how it horks embeddors? I think it should be backed out until it's done using UI which can be overridden by embeddors.

sorry, I didn't realize that horked embeddors. If we can back it out cleanly, I say we go for it. Conrad, can you help the folks in that bug figure out a better Are embeddors screwed for 1.2? If so, sounds like we MUST back it out of the 1.2 branch.

Personally I think we need a "disable all XUL prompt dialogs" pref and enforce people via code reviews to recognize it and provide the means for them to be overridden. I don't know if I like idea of embedders implementing umpteen prompt services to catch all instances though, which is why I suggested falling back on nsIPrompt in simple cases like this.

Backing out the patch for bug 23508 would be time consuming and then would have to be redone once the developer (Michiel van Leeuwen) gets the correct patch. However I might be able to make a small change that will get back the old behavior while the developer looks at getting the correct fix.

time consuming or not, its the most straight forward way to unbust embeddors. If we can't get a fix soon (i.e. in the next 24-48 hours) then we need to back out of the 1.2 branch ASAP.

It's always better to make a small change to turn off the problem code than to spend all the time backing it out. I'm posting a one-line patch that turns it off.

Comment on attachment 106104 [details] [diff] [review] turn off xul dialog no, I'm sorry but that's just not how things work. This patch just obscures the problem and makes it even harder to back out. For other readers of this patch: cookie_s is a CookieStruct* that gets passed in much higher up the stack.. by setting it to null here, we're skipping over the problem code, but the problem code is still there.

I think that we should have an nsICookiePromptService. * The embeddor who does not want a XUL dialog isn't stuck with a less than ideal fallback UI. * The code in mozilla is less bulky if it doesn't have to check the "disable XUL" pref and have code for XUL and the fallback. * Pinkerton wanted to have more specific prompt ifaces so that the embedding app, knowing the context thru this specificity, would be able to customize more than with our generic prompt service. I'm happy to help make the iface and the default impl of the component. What else might Cookies want from it's own specific prompt service beside the prompt from bug 23508?

*** Bug 178179 has been marked as a duplicate of this bug. ***

Conrad, it would be great if you looked at this. I am nt experienced in writeing services.

This is my first shot at it. I need to look at where to get a parent window from (also bug 170172). I also dont know if I should use a service, or a component. And I don't know if this really fixes the bug. I don't know how to test that. So, comments on my first service are welcome.

> This is my first shot at it. Nice! Using a service here is appropriate rather than a component. The method cookieDialog() doesn't need to hold any state between calls - everything it needs is passed to it - therefore, only 1 instance (a service) is fine. > And I don't know if this really fixes the bug. It fixes the bug in that a XUL dialog is no longer being made without the embeddor being able to override it. If they do want to override it, they can register their own impl of the service, as they do for nsIPromptService. As far as finding the parent window, I'll have to look at the code which poses this dialog to see where you can get a parent window. Hopefully, there's an nsIInterfaceRequestor available somewhere. > + block->SetInt(nsICookieAcceptDialog::REMEMBER_DECISION, *checkValue); > + > + // We should joust pass nsICookie, but how to combine that with > + // the other strings above, and the return values? > + > + nsCAutoString cookieName; If you want to just pass the cookie, you'd have to make a specialized iface (nsICookieDialogParamBlock) instead of using the generic nsIDialogParamBlock.

The nsIInterfaceRequestor comes from the channel here: http://lxr.mozilla.org/seamonkey/source/extensions/cookie/nsCookieHTTPNotify.cpp#244 It gets an nsIPrompt interface from the requestor. It's possible that from the same nsIInterfaceRequestor impl may also be able to supply an nsIDOMWindow. Looks like the cookie code passes nsIPrompt around quite a bit. Maybe it should pass nsiInterfaceRequestor which would be more general?

Thanks for your comments. > If you want to just pass the cookie, you'd have to make a specialized iface > (nsICookieDialogParamBlock) instead of using the generic nsIDialogParamBlock. While looking at this I found that the security manager does something like this, and has a nsIPKIParamBlock which has SetISupportAtIndex(). I could use something like that. That would mean I have to copy that interface. Seems redundant. Would I have a chance when asking to extent nsIDialogParamBlock? It is not frozen yet. An other option would be to create an interface just like a c-struct, with just the data I need (cookie, questionstring etc). Would that work?

I tried using a specialized paramblock. That part works. But with this patch, I get crashes. It turned out to have something to to with freeing strings. It seems to me that somewhere in nsCookies.cpp the strings are allocated. I copy the pointers to an nsICookie. But nsCookie.cpp want to free the strings in it's desctructor. I commented out that part as a test, and no more crashes. Anyone got an idea?

This seems not to crash. ~nsCookie() will free the string buffers for the path etc, so I allocate my own buffers. The parent issue still has to be solved. I tried Conrad's suggestion, but that didn't work. The nsIPrompt comes from more places, which don't have an nsIInterfaceRequestor. Also see bug 170172.

Comment on attachment 109244 [details] [diff] [review] Updated again, copy the strings >Index: extensions/cookie/nsCookie.cpp >=================================================================== > nsCookie::~nsCookie(void) { >+printf("~nsCookie name: %d %s\n",cookieName, cookieName); Make sure this is inside #ifdef DEBUG >Index: extensions/cookie/nsCookieDialogParamBlock.cpp >=================================================================== >+NS_IMETHODIMP >+nsCookieDialogParamBlock::GetCookie(nsICookie * *aCookie) >+{ >+ NS_ENSURE_ARG_POINTER(aCookie); >+ if (mCookie) { >+ *aCookie = mCookie; >+ NS_ADDREF(*aCookie); >+ } >+NS_IMETHODIMP >+nsCookieDialogParamBlock::GetMessageText(PRUnichar * *aMessageText) >+{ >+ NS_ENSURE_ARG_POINTER(aMessageText); >+ *aMessageText = ToNewUnicode(mMessageText); See below about using string types instead of pointers. >+NS_IMETHODIMP >+nsCookieDialogParamBlock::GetRememberDecision(PRBool *aRememberDecision) >+{ NS_ENSURE_ARG_POINTER(aRememberDecision); Ditto for GetAccept() >Index: extensions/cookie/nsCookiePromptService.cpp >=================================================================== >+nsresult >+nsCookiePromptService::Init() >+{ >+ nsresult rv; >+ mWatcher = do_GetService(NS_WINDOWWATCHER_CONTRACTID, &rv); I don't think it's worth caching this in a member var. It can be gotten quickly enough when needed compared to the job that's about to be done. >+ return rv; >+} >Index: extensions/cookie/nsICookieDialogParamBlock.idl >=================================================================== >RCS file: extensions/cookie/nsICookieDialogParamBlock.idl >diff -N extensions/cookie/nsICookieDialogParamBlock.idl >--- /dev/null 1 Jan 1970 00:00:00 -0000 >+++ extensions/cookie/nsICookieDialogParamBlock.idl 13 Dec 2002 18:42:34 -0000 >@@ -0,0 +1,11 @@ >+#include "nsISupports.idl" >+ >+interface nsICookie; >+ >+[scriptable, uuid(8b7084d9-9706-4026-a459-80f1ce011dc2)] >+interface nsICookieDialogParamBlock : nsISupports { >+ attribute nsICookie cookie; >+ attribute wstring messageText; Use AStrings in all these ifaces instead of wstring. More efficient, no pointer ownership troubles. >+ attribute boolean rememberDecision; >+ attribute boolean accept; >+}; >Index: extensions/cookie/nsICookiePromptService.idl >=================================================================== >+[scriptable, uuid(CE002B28-92B7-4701-8621-CC925866FB87)] >+interface nsICookiePromptService : nsISupports >+{ >+ [noscript] boolean cookieDialog(in nsIDOMWindow parent, >+ in wstring szMessage, >+ in wstring szCheckMessage, >+ in nsICookie cookie, >+ inout PRBool checkValue); AStrings all around. >Index: extensions/cookie/nsPermissions.cpp >=================================================================== >+ char *name = nsnull; >+ char *value = nsnull; >+ char *host = nsnull; >+ char *path = nsnull; >+ >+ // Copy the strings to a new buffer. >+ // nsCookie::~nsCookie will free them, so don't use the original strings. Better to define a copy constructor for nsCookie and hide this detail in the nsCookie object. >+ >+ CKutil_StrAllocCopy(name, cookie_s->name); >+ CKutil_StrAllocCopy(value, cookie_s->cookie); >+ CKutil_StrAllocCopy(host, cookie_s->host); >+ CKutil_StrAllocCopy(path, cookie_s->path); >+ >+ nsICookie *cookie = >+ new nsCookie(name,value,cookie_s->isDomain, >+ host,path,cookie_s->isSecure, >+ cookie_s->expires,cookie_s->status,cookie_s->policy); >

> See below about using string types instead of pointers. > Use AStrings in all these ifaces instead of wstring. More efficient, no > pointer ownership troubles. I used wstrings, because most of the cookie modules uses them (and PRUnichar*). I can change this interface, but how fack back into the cookie module should it be changed? The text for the dialog is generated by CKutil_Localize, which returns an |PRUnichar*|. Speaking of the text, wouldn't it be better to let the text come from the new promptservice? Morse, what do you think? > Better to define a copy constructor for nsCookie and hide this detail in the > nsCookie object. Well, I don't understand why nsCookie isn't already doing that. It is freeing the strings at the destructor.

Mike - I'm working on rewriting nsCookies and friends, including the API's. I'd be happy to lend a hand to this effort, especially since it looks like nsCookie could use some refactoring to handle the new prompter... If you'd like, send me a mail and we can chat about this stuff (or let me know if you hang out on #mozilla).

whoo hoo! It would be so nice to have this stuff rewritten.. or at least the interfaces.. is there a seperate bug on that? as for using nsAString, you should always use it in new interfaces, but I'm not a huge fan of this cookie-specific dialog param block. Why isn't the existing nsIDialogParamBlock sufficient? (granted, the interface to it kind of sucks, but we should be improving a shared implementation rather than introducing new param blocks!)

I used an own dialogparamblock, because when useing the general paramblock, the code got messy. I need to pass a lot of parameters. If it were possible to update nsIDialogParamBlock to support passing nsISupports, that would solve the messy code.

oh! Then we should fix nsIDialogParamBlock, rather than introducing all sorts of cookie-proprietary code. How about adding attribute nsIMutableArray objects; to the nsIDialogParamBlock interface? that would make life much easier for you...

(I should again voice my general opposition to the structure of nsIDialogParamBlock, but we're not making the situation any worse by adding this new attribute)

I liked Alec's suggestion, so here is the patch. One problem with it, why do we have two files named nsIDialogParamBlock.idl? http://lxr.mozilla.org/seamonkey/source/xpfe/appshell/public/nsIDialogParamBlock.idl seems to be unused. I also updated nsCookie.cpp and .h to use nsStrings instead of char *, to prevent the string ownership trouble.

Comment on attachment 110529 [details] [diff] [review] Start using string classes, use general paramblock ok, a few comments on the patch: - I don't think this actually applies to your patch, but I remember that for some strings, we had switched to raw character strings for certain member variables because there were a lot of that particular object being created, and the 16 byte overhead of nsString was too much.. - lets remove the obsolete nsIDialogParamBlock - yuck! Lets do it as part of this landing. - You don't need to create nsCString's to wrap raw character strings to make nsACString&'s - use const nsDependentCString instead. - use ToNewCString() instead of PL_strdup - the advantage of nsDependentCString is that the length of the string is already calculated, and ToNewCString() takes this into account. - your patch still has nsICookieDialogParamBlock.. doh! indenting is a little odd in nsICookiePromptService - don't use hungarian notation (szMessage, etc), this aint microsoft - instead use mozilla style ("a" for argument, etc, so you'd have aMessage) I'd like someone (conrad?) to take a look at the pattern here as well - I'm not psyched about having a completely seperate service just to bring up a dialog.

Since this dialog is not generic, we need to have a separate interface. We can't just tack more methods onto nsIPromptService. And, if we just make the dialog generic enough so that it can be done with nsIPromptService, it's a loss of UE. If the code was written so that it attempted to get a specialized prompt service and, if that failed, made a generic prompt, it might lead to bloat. In general, I like having prompt interfaces which are specific to a certain context. It allows embedding apps a better chance to customize or even do their own localization. See bug 95649, comment 21.

Sorry, including nsICookieDialogParamBlock was a mistake. I also adressed the other points. PL_strdup -> I removed the construction of the struct, it is replaced by nsICookie szMessage -> came from old code nsDependentCString -> done. But I hope that one day, nsCookies will be rewritten to use nsStrings. That would clean up.

What is the target milestone for this fix? Will it make 1.3?

A small update to the previous patch. NS_INIT_ISUPPORTS can be removed, and it now apllies to the current trunk. Asking for review.

Comment on attachment 111632 [details] [diff] [review] small update dwitte is rewriting some of these code. He might be the right person to review this code. :)

suresh, I can look at the code, and give an r= for what it's worth, but I think this should be in addition to another r/sr. I'm new to the project, and though I'm rewriting cookies, I don't know the codebase *that* well to feel good about giving reviews. I'm not sure that a rewrite qualifies me for reviewer status yet, I still have *tons* of general questions before I feel happy about what I've written. Besides, I don't have editbugs yet :) So... 1) I'll look at the code and give comments etc, or r= if I can get editbugs. 2) If you have time, please give a proper r=, or if you can't, let us know who can... the reason I'm being pedantic is because I think this should go in before the 1.3b freeze, so ASAP would be really good :) also, note that this code doesn't really touch nsCookies, it's more in nsPermissions. so working on nsCookies doesn't explicitly qualify me to look at this...

Comment on attachment 111632 [details] [diff] [review] small update >Index: extensions/cookie/nsCookie.cpp <snip> > nsCookie::nsCookie >- (char * name, >- char * value, >+ (const nsACString &name, >+ const nsACString &value, > PRBool isDomain, >- char * host, >- char * path, >+ const nsACString &host, >+ const nsACString &path, > PRBool isSecure, > PRUint64 expires, > nsCookieStatus status, While you're in there, change that PRUint64 to a PRTime. Ditto for all other usages of PRUint64 in the patch. > nsCookie::~nsCookie(void) { >- if (cookieName) >- nsCRT::free(cookieName); >- if (cookieValue) >- nsCRT::free(cookieValue); >- if (cookieHost) >- nsCRT::free(cookieHost); >- if (cookiePath) >- nsCRT::free(cookiePath); > } Are all the nsCookies being freed in the right places now? > NS_IMETHODIMP nsCookie::GetName(nsACString& aName) { >- if (cookieName) { >- aName = cookieName; >- return NS_OK; >- } >- return NS_ERROR_NULL_POINTER; >+ aName.Assign(cookieName); >+ return NS_OK; > } What happened to our return value if cookieName is null? Ditto for the others. >Index: extensions/cookie/nsCookie.h <snip> > protected: >- char * cookieName; >- char * cookieValue; >+ nsSharableCString cookieName; >+ nsSharableCString cookieValue; > PRBool cookieIsDomain; >- char * cookieHost; >- char * cookiePath; >+ nsSharableCString cookieHost; >+ nsSharableCString cookiePath; > PRBool cookieIsSecure; > PRUint64 cookieExpires; > nsCookieStatus cookieStatus; >- nsCookiePolicy cookiePolicy; >+ nsCookiePolicy cookiePolicy; >+ > }; Nice use of Sharables! (don't forget that PRUint64...) >Index: extensions/cookie/nsCookiePromptService.cpp <snip> >+ * >+ * The Original Code is cookie manager code. >+ * Replace "cookie manager code" with "mozilla.org code". >+ * The Initial Developer of the Original Code is >+ * Michiel van Leeuwen. Include your email address, if it's permanent :) >+NS_IMETHODIMP >+nsCookiePromptService::CookieDialog(nsIDOMWindow *parent, >+ nsICookie *cookie, >+ const nsACString & hostname, Make "& hostname" "&hostname" please. >+ PRInt32 count, >+ PRBool modify, >+ PRBool *checkValue, >+ PRBool *accept) Painful though it is, we have a naming convention for arguments passed into a function. Rename these to aParent,aCookie,... please. For outparams, use e.g. aOutAccept. Also, change those *'s to &'s on the PRBools (and all appropriate instances, since the & will break things). It makes assignments etc more consistent. You should do this on functions you write from scratch, but not for the functions you modify in this patch (most of them will be rewritten, so there's no point). >+ nsCOMPtr<nsIDialogParamBlock> block = do_CreateInstance(NS_DIALOGPARAMBLOCK_CONTRACTID); Add an nsresult check (do_CreateInstance can fail). >+ nsCOMPtr<nsIMutableArray> objects; >+ NS_NewArray(getter_AddRefs(objects)); >+ objects->AppendElement(cookie, PR_FALSE); >+ block->SetObjects(objects); Do we need nsresult checking here? >+ nsCOMPtr<nsIWindowWatcher> wwatcher = do_GetService(NS_WINDOWWATCHER_CONTRACTID, &res); Ditto with the nsresult check here. >+ nsCOMPtr<nsIDOMWindow> activeParent; // retain ownership for method lifetime >+ if (!parent) { >+ wwatcher->GetActiveWindow(getter_AddRefs(activeParent)); >+ parent = activeParent; >+ } "parent" isn't an outparam, should you be assigning to it? Maybe create a local ptr and assign into that. >+ if (NS_FAILED(res)) { >+ *checkValue = 0; >+ *accept = 1; >+ } PR_TRUE and PR_FALSE here... >Index: extensions/cookie/nsCookiePromptService.h <snip> >+ * The Original Code is cookie manager code. >+ * >+ * The Initial Developer of the Original Code is >+ * Michiel van Leeuwen. Ditto with mozilla.org code here, and your email address... >+#ifndef __nsCookiePromptService_h Do we need this #ifndef? >Index: extensions/cookie/nsCookies.cpp <snip> >+ // put the cookie information into the cookie structure. >+ nsDependentCString name2(name_from_header); >+ nsDependentCString value2(cookie_from_header); >+ nsDependentCString host2(host_from_header); >+ nsDependentCString path2(path_from_header); >+ nsICookie *this_cookie = >+ new nsCookie(name2, >+ value2, >+ isDomain, >+ host2, >+ path2, >+ isSecure, >+ cookie_TrimLifetime(timeToExpire), >+ status, >+ cookie_GetPolicy(P3P_SitePolicy(curURL, aHttpChannel))); I'm not sure you can do this. this_cookie used to be a cookieStruct, which isn't the same as nsICookie (it includes lastAccessed, and uses time_t instead of a 64-bit int for Expires). Also, you've shifted this_cookie init inside the if() (it was outside before), so if PERMISSION_Read() fails, bad things will happen. Shift it back outside? >Index: extensions/cookie/nsICookiePromptService.idl <snip> >+ * The Original Code is cookie manager code. And again... >+ [noscript] boolean cookieDialog(in nsIDOMWindow parent, >+ in nsICookie cookie, >+ in ACString hostname, >+ in long count, >+ in boolean modify, >+ inout PRBool checkValue); Is the inconsistent use of "boolean" & "PRBool" required? >Index: extensions/cookie/nsPermissions.cpp <snip> >+ if (cookie) { >+ nsCOMPtr<nsICookiePromptService> cookiePromptService(do_GetService(kCookiePromptServiceCID)); Another nsresult check here. >+ nsCString host(hostname); Can this be nsDependentCString instead? > if (NS_FAILED(res)) { > *checkValue = 0; >- buttonPressed = 1; <snip> >+ PRInt32 buttonPressed = 1; /* in case user exits dialog by clickin X */ Ditto PR_FALSE for checkValue. Can buttonPressed be a PRBool too? Or can it be other than 0 or 1? > if (NS_FAILED(res)) { > *checkValue = 0; > } PR_FALSE... >+ acceptCookie = buttonPressed ? PR_FALSE : PR_TRUE; Use "(buttonPressed == 1)" here, if you stick with PRInt32. > if (*checkValue!=0 && *checkValue!=1) { > NS_ASSERTION(PR_FALSE, "Bad result from checkbox"); > *checkValue = 0; /* this should never happen but it is happening!!! */ > } Hmm. Does this mean checkValue isn't a PRBool? If it is, we don't need this. If not, other things need changing... >- cookie_CookieStruct *cookie_s, >- const char * message_string, >- int count_for_message) >+ nsICookie *cookie, See previous comment about cookieStruct and nsICookie, can you do this? Do what's best for this patch... if it gets messy, just leave it to me, it'll get taken care of as part of the rewrite. Good job, this patch looks really nice. Once you fix up this stuff, we can request SR from darin (or suresh?), and hopefully get this thing checked in before the freeze.

nominating as a 1.3 beta blocker. embedding products based on moz 1.3 will need this.

This patch has addressed dwitte's review comments, and what I discussed with him on IRC.

Comment on attachment 111697 [details] [diff] [review] Addressed review notes r=dwitte, all my comments were addressed... a lot of them turned out to be redundant :) darin, over to you. We'll mail alecf to take a look as well, since mvl wanted him to review the changes to dialogparamblock.

We can close bug 23508 when this gets checked in.

Without looking at the updated patch, I'm slightly concerned that we're biting off more than we can chew w.r.t. this bug - I mean, are we fixing this bug or trying to rewrite cookies? Don't get me wrong, I welcome cookie rewrites, but does that have to be done in this bug? I'll take a look at this later today.

I'm working on a complete rewrite of our cookie module at the moment, and IMHO this patch doesn't change *too* much more than it needs to. There's a little bit of seepage into calling functions (throwing that nsCookie struct into nsCookies.cpp, that kinda thing), but I think it's elegant and minimal. If mvl hadn't have reworked small parts of cookies in this patch, it would've made things worse, because he probably would've had to duplicate things for his own purpose, or make the cookie strings mess worse. From my perspective, this patch is elegant, because when I finish the rewrite I won't have to come back and rework this code. It's been done in a way that's (hopefully) compatible with current code, but in a way that I won't be ripping up and re-doing in a weeks' time. I hope that's a fair tradeoff...

Patch looks good. r=ccarlen. One thing: =================================================================== RCS file: /cvsroot/mozilla/extensions/cookie/macbuild/cookie.xml,v + <PATH>nsCookiePrompService.cpp</PATH> You're missing a 't' there. Also, I think you missed adding this file in 1 section. Adding a file to a 2-target project requires 5 additions: 2 in each target and 1 in the group list. See the changes to the IDL file - they look right.

Conrad, thanks for spotting.

Comment on attachment 111747 [details] [diff] [review] prevent mac bustage Carrying over the review r=dwitte and the sr request.

Comment on attachment 111697 [details] [diff] [review] Addressed review notes >Index: embedding/components/windowwatcher/public/nsIDialogParamBlock.idl > interface nsIDialogParamBlock: nsISupports { ... >+ attribute nsIMutableArray objects; how about adding a bit of documentation to this interface while you're at it. >Index: embedding/components/windowwatcher/src/nsDialogParamBlock.cpp >+nsDialogParamBlock::SetObjects(nsIMutableArray * aObjects) >+{ >+ mObjects = aObjects; >+ return NS_OK; >+} the above code lacks a NS_ADDREF. use a nsCOMPtr<nsIMutableArray> instead, so you don't have to worry about getting the NS_ADDREF/NS_RELEASE calls correct. plus your destructor would leak mObjects. >+ nsIMutableArray *mObjects; > }; >Index: embedding/components/windowwatcher/src/nsPromptService.cpp >+ block->SetInt(eCheckboxState, *checkValue ? 1 : 0); nit: block->SetInt(eCheckboxState, *checkValue != 0); eliminates branch. >+ *checkValue = (tempValue == 1) ? PR_TRUE : PR_FALSE; nit: *checkValue = (tempValue == 1); is fine. no need for ?: expression. >Index: extensions/cookie/nsCookie.cpp >+ aValue.Assign(cookieValue); >+ return NS_OK; minor nit: operator=() is nice in this case. >Index: extensions/cookie/nsCookie.h >+ nsSharableCString cookieName; >+ nsSharableCString cookieValue; >+ nsSharableCString cookieHost; >+ nsSharableCString cookiePath; some comments about nsSharableCString: keep in mind that a sharable string allocates an extra object to manage the reference count to the real string buffer. so, unless you know that a sharable string will be assigned to one of these member variables "in most cases" then it may not be worth it to use sharable strings here. they should be used as an optimization only when you are sure that there is value in doing so. >Index: extensions/cookie/nsCookiePromptService.cpp >+nsresult >+nsCookiePromptService::Init() >+{ >+ return NS_OK; >+} nit: do you really need an Init function that does nothing? >+ block->SetInt(nsICookieAcceptDialog::REMEMBER_DECISION, (*aCheckValue) ? 1 : 0); nit: (*aCheckValue != 0) >+ rv = NS_NewArray(getter_AddRefs(objects)); >+ if (NS_FAILED(rv)) { >+ return rv; >+ } nit: common practice in mozilla codebase to do the following: if (NS_FAILED(rv)) return rv; it sometimes helps improve code readability ;-) >+ nsCOMPtr<nsISupports> arguments(do_QueryInterface(block)); nit: nsCOMPtr<nsISupports> arguments = do_QueryInterface(block); operator=() form is slightly preferred as it tends to optimize better. >+ /* get back output parameters */ nit: prefer C++ style comments please. >+ *aAccept = (tempValue == 1) ? PR_TRUE : PR_FALSE; >+ *aCheckValue = (tempValue == 1) ? PR_TRUE : PR_FALSE; same nit here. >Index: extensions/cookie/nsCookiePromptService.h >+ * Portions created by the Initial Developer are Copyright (C) 2002 2003 right? ;-) >+#ifndef __nsCookiePromptService_h >+#define __nsCookiePromptService_h nit: convention is: #ifndef nsCookiePromptService_h__ (not that it is all that well followed!) >+#include "nsCOMPtr.h" >+#include "nsIWindowWatcher.h" >+#include "nsIGenericFactory.h" >+#include "nsXPIDLString.h" hmm... these include files don't seem to be needed by this header file. why don't you just #include them in the source file instead. ...anything to help reduce compilation time ;-) >Index: extensions/cookie/nsCookies.cpp >+ nsDependentCString name2(name_from_header); ... >+ new nsCookie(name2, so, here's an example where non-shared strings are passed to nsCookie ctor. >Index: extensions/cookie/nsICookiePromptService.idl >+[scriptable, uuid(CE002B28-92B7-4701-8621-CC925866FB87)] >+interface nsICookiePromptService : nsISupports ... >+ [noscript] boolean cookieDialog(in nsIDOMWindow parent, >+ in nsICookie cookie, >+ in ACString hostname, >+ in long count, >+ in boolean modify, >+ inout boolean checkValue); hmm.. interface is scriptable, but the only method on this interface is not scriptable. why not let it be scriptable? >Index: extensions/cookie/nsModuleFactory.cpp >+NS_GENERIC_FACTORY_CONSTRUCTOR_INIT(nsCookiePromptService, Init) use non _INIT form of generic factory constructor macro to avoid having to have an empty Init function.

I'm getting the following linker errors after applying the latest patch. Something needs fixing in nsPKIParamBlock.cpp? nsPKIParamBlock.o(.gnu.linkonce.d._ZTV15nsPKIParamBlock+0x38):security/manager/pki/src/nsPKIParamBlock.cpp:40: undefined reference to `nsPKIParamBlock::GetObjects(nsIMutableArray**)' nsPKIParamBlock.o(.gnu.linkonce.d._ZTV15nsPKIParamBlock+0x3c):security/manager/pki/src/nsPKIParamBlock.cpp:40: undefined reference to `nsPKIParamBlock::SetObjects(nsIMutableArray*)' nsPKIParamBlock.o(.gnu.linkonce.d._ZTV15nsPKIParamBlock+0x68):security/manager/pki/src/nsPKIParamBlock.cpp:40: undefined reference to `non-virtual thunk [nv:-4] to nsPKIParamBlock::GetObjects(nsIMutableArray**)' nsPKIParamBlock.o(.gnu.linkonce.d._ZTV15nsPKIParamBlock+0x6c):security/manager/pki/src/nsPKIParamBlock.cpp:40: undefined reference to `non-virtual thunk [nv:-4] to nsPKIParamBlock::SetObjects(nsIMutableArray*)'

Index: extensions/cookie/nsPermissions.cpp + nsCOMPtr<nsICookiePromptService> cookiePromptService(do_GetService(kCookiePromptServiceCID,&rv)); Shouldn't this be NS_COOKIEPROMPTSERVICE_CONTRACTID instead?

Updated to the sr-comments, and make it build. >>+ block->SetInt(eCheckboxState, *checkValue ? 1 : 0); >nit: block->SetInt(eCheckboxState, *checkValue != 0); No, that won't work. We have a PRBool, and must set an Int.

Fixing the bool-issue. my previous comment was wrong. Also fixing GetObjects(). Both as discussed with dwitte.

Comment on attachment 111909 [details] [diff] [review] yet another update darin, can you make sure the nsCOMPtrs are done ok? r=dwitte.

Comment on attachment 111909 [details] [diff] [review] yet another update >+/* >+ * An interface to pass strings, integers and nsISupports to a dialog >+ */ please use: /** * Comment... */ esp here: >+ /* Get or set an interger to pass. >+ * Index must be in the range 0..7 >+ */ and here: >+ /* Set the maximum number of strings to pass. Default is 16. >+ * Use before setting any string. >+ */ and here: >+ /* Get or set an string to pass. >+ * Index starts at 0 >+ */ even here: >+ /* A place where you can store an nsIMutableArray to pass nsISupports */ > NS_IMETHOD GetNext(nsISupports **result) > { I think these should be nsCAutoString's: >+ nsCString name; >+ nsCString value; >+ nsCString host; >+ nsCString path; ... > nsresult rv = COOKIE_Enumerate >+ (mCookieCount++, name, value, &isDomain, host, path, &isSecure, &expires, > &status, &policy); >Index: extensions/cookie/nsCookiePromptService.cpp >=================================================================== >+NS_IMETHODIMP >+nsCookiePromptService::CookieDialog(nsIDOMWindow *aParent, >+ nsICookie *aCookie, >+ const nsACString &aHostname, >+ PRInt32 aCount, >+ PRBool aModify, >+ PRBool *aCheckValue, >+ PRBool *aAccept) >+{ >+ block->SetInt(nsICookieAcceptDialog::MODIFY, aModify != PR_FALSE); you aren't really supposed to do comparisons against PR_, but this is ok... >Index: extensions/cookie/nsCookies.cpp > /* use common code to determine if we can set the cookie */ > PRBool permission = PR_TRUE; > if (NS_SUCCEEDED(PERMISSION_Read())) { >+ PRBool modify = prev_cookie ? PR_TRUE : PR_FALSE; I don't like this but ... >+ // put the cookie information into the cookie structure. >+ nsDependentCString name2(name_from_header); >+ nsDependentCString value2(cookie_from_header); >+ nsDependentCString host2(host_from_header); >+ nsDependentCString path2(path_from_header); see comment somewhere else about making the temporaries really temporary. >+ nsICookie *thisCookie = >+ new nsCookie(name2, >+ value2, >+ isDomain, >+ host2, >+ path2, >+ isSecure, >+ cookie_TrimLifetime(timeToExpire), >+ status, >+ cookie_GetPolicy(P3P_SitePolicy(curURL, aHttpChannel))); >@@ -2060,11 +2037,11 @@ > PUBLIC nsresult > COOKIE_Enumerate > (PRInt32 count, >+ nsACString &name, >+ nsACString &value, > PRBool *isDomain, see comment somewhere else about sticking a bool between complex types. >+ nsACString &host, >+ nsACString &path, > PRBool * isSecure, > PRUint64 * expires, > nsCookieStatus * status, >Index: extensions/cookie/nsCookies.h > extern nsresult COOKIE_Enumerate > (PRInt32 count, >+ nsACString & name, >+ nsACString & value, >+ PRBool * isDomain, ^ i find sticking a boolean between a bunch of complex types to be messy. If you need to clarify that isDomain applies to host you could name it hostIsDomain. >+ nsACString & host, >+ nsACString & path, > PRBool * isSecure, > PRUint64 * expires, > nsCookieStatus * status, >Index: extensions/cookie/nsICookiePromptService.idl >=================================================================== >RCS file: extensions/cookie/nsICookiePromptService.idl >diff -N extensions/cookie/nsICookiePromptService.idl >--- /dev/null 1 Jan 1970 00:00:00 -0000 >+++ extensions/cookie/nsICookiePromptService.idl 18 Jan 2003 12:37:36 -0000 /** * Interface to get permission to accept a cookie. */ >+/* >+ >+ An interface to open a dialog to ask to accept the cookie >+ >+ */ >+interface nsICookiePromptService : nsISupports >+{ >+ >+ /* Open a dialog that asks for permission to accept a cookie >+ * Returns true or false. ^ but what does true mean? In fact, boolean acceptCookie(in nsIDOMWindow parent, in nsICookie cookie, in ACString hostname, in long cookiesFromHost, in boolean changingCookie, inout boolean rememberDecision); would be nicer. To me false would mean don't accept cookie. And I can tell what each of the params mean. (yes, i know, if i really cared I should have spoken up sooner) >+ * return values are not modified when something fails. >+ * >+ * @param parent >+ * @param cookie >+ * @param hostname the host that wants to set the cookie, >+ * not the domain: part of the cookie >+ * @param count the number of cookies there are already for this host >+ * @param modify is this a cookie that is modified? >+ * @param checkValue is the decision remembered? >+ */ >+ >+ boolean cookieDialog(in nsIDOMWindow parent, >+ in nsICookie cookie, >+ in ACString hostname, >+ in long count, >+ in boolean modify, >+ inout boolean checkValue); >+}; >+ >+%{C++ >+// {CE002B28-92B7-4701-8621-CC925866FB87} >+#define NS_COOKIEPROMPTSERVICE_CID \ >+ {0xCE002B28, 0x92B7, 0x4701, {0x86, 0x21, 0xCC, 0x92, 0x58, 0x66, 0xFB, 0x87}} CIDs should not live in interfaces. >+#define NS_COOKIEPROMPTSERVICE_CONTRACTID "@mozilla.org/embedcomp/cookieprompt-service;1" >+%} >+permission_CheckConfirmYN(nsIPrompt *aPrompter, >+ nsICookie *cookie, >+ const char * hostname, >+ int count, >+ PRBool modify, >+ PRBool* checkValue) { >+ >+ nsresult rv; >+ PRBool acceptThis = PR_TRUE; >+ >+ if (cookie) { >+ nsCOMPtr<nsICookiePromptService> cookiePromptService = do_GetService(NS_COOKIEPROMPTSERVICE_CONTRACTID,&rv); >+ if (NS_FAILED(rv)) return rv; >+ you should be able to construct the temporary for the function call: !>+ nsDependentCString host(hostname); !>+ rv = cookiePromptService->CookieDialog(nsnull, cookie, host, !>+ count, modify, checkValue, !>+ &acceptThis); -- ! rv = cookiePromptService-> ! CookieDialog(nsnull, cookie, ! nsDependentCString(hostname), ! count, moidfy, checkValue, &acceptThis); That's a general comment and applies both to code before this block and probably code after this block. >+ PRInt32 buttonPressed = 1; /* in case user exits dialog by clickin X */ On MacOS the box is []. on MacOSX it's a red dot. Perhaps: /* In case the user closed the dialog using a window manager feature. */ >@@ -247,9 +220,9 @@ > const char * hostname, > PRInt32 type, > PRBool warningPref, >+ nsICookie *cookie, >+ int count_for_message, >+ PRBool modify) At some point someone should convert arguments from ran_dom to aArgInterCaps >Index: extensions/cookie/resources/content/cookieAcceptDialog.js > params = window.arguments[0].QueryInterface(nsIDialogParamBlock); >+ var cookie = params.objects.queryElementAt(0,nsICookie); >+ if (cookie) >+ else >+ messageText = cookieBundle.getFormattedString(messageFormat,["",count]); What does it mean not to have a cookie object here? (A comment in the file would be helpful) > PermissionToAcceptImage = The site %1$s wants to load an image. Do you want to allow it? use %s unless you need to use the other syntax. -- Drop these comments; localizers know what %S means. >+# LOCALIZATION NOTE (PermissionToSetACookie) : Be careful about %S. Do Not localize it. >+# LOCALIZATION NOTE (PermissionToSetSecondCookie): Be careful about %S. Do Not localize it. >+# LOCALIZATION NOTE (PermissionToSetAnotherCookie): Be careful about %S, %S. Do Not localize them. A comment explaining what %s and %s are would be helpful. >+# LOCALIZATION NOTE (PermissionToModifyCookie): Be careful about %S. Do Not localize it. >+# LOCALIZATION NOTE (PermissionToModifyCookie): Be careful about %S. Do Not localize it.

r=dwitte, provided you change this one last branch (nice spotting timeless): change >+ PRBool modify = prev_cookie ? PR_TRUE : PR_FALSE; to >+ PRBool modify = prev_cookie != nsnull;

Instead of the pattern aFoo != PR_FALSE where aFoo is a boolean, why not just get rid of the comparison altogether? aFoo == PR_TRUE => comparison returns PR_TRUE => same as aFoo aFoo == PR_FALSE => comparison returns PR_FALSE => same as aFoo

It's a minor nit about the types. I chatted with darin about it on IRC, and I got the impression that we shouldn't rely on PRBool being 0 or 1; it can be anything that guarantees |if (myBool)| evaluates to a certain result (effectively PR_TRUE can be any number >= 1). So when we need to convert the PRBool to an int, we should use the == or != operators, since they are guaranteed to evaluate to 0 or 1. Which leaves |myBool != PR_FALSE| as the only choice. Is this correct? If you think it's not, and it's okay to munge types from PRBool -> PRInt32, then we can remove the comparison.

I think the usage in one case at least was that the parameter to function was of type PRBool, was compared to PR_FALSE, and this was passed on to another function taking PRBool. So it was PRBool always.

Comment on attachment 112105 [details] [diff] [review] Update to timeless' comments >Index: embedding/components/windowwatcher/public/nsIDialogParamBlock.idl >+/* >+ * An interface to pass strings, integers and nsISupports to a dialog >+ */ nit: please use javadoc style comments throughout. thx! /** * comment goes here. */ >Index: embedding/components/windowwatcher/src/nsPromptService.cpp >+ block->SetInt(eCheckboxState, *checkValue != PR_FALSE); nit: perhaps all that's needed here is a comment explaining that you are sanitizing the input parameter. >+ block->GetInt(eCheckboxState, &tempValue); >+ *checkValue = (tempValue == 1); nit: should this comparison be necessary? aren't you the one who set the value in the first place? why not trust that it will be 1 or 0? sr=darin

Since mvl isn't around right now, and darin wants this in 1.3b, I've updated the patch to address darin's SR comments. >>+ block->GetInt(eCheckboxState, &tempValue); >>+ *checkValue = (tempValue == 1); > >nit: should this comparison be necessary? aren't you the one who set >the value in the first place? why not trust that it will be 1 or 0? I think it's necessary... block is modified in between the get and set calls, and eCheckboxState is updated with the user's selection... so it can change. r=dwitte, carrying over sr=darin... ready for checkin.

Thanks to timeless for checking in, and to darin as well, for managing to fit in the SR today...! Marking resolved/fixed, along with bug 23508.