Closed
Bug 46783
Opened 24 years ago
Closed 23 years ago
nsICookieService.h needs to be idl'ized
Categories
(Core :: Networking: Cookies, defect, P3)
Tracking
()
VERIFIED
FIXED
mozilla0.9
People
(Reporter: jud, Assigned: morse)
References
Details
(Keywords: arch)
Attachments
(29 files)
currently the cookie service interface is .h, it needs to be .idl. This will entail some api changes as the .h isn't using strings properly right now.
Reporter | ||
Updated•24 years ago
|
Target Milestone: --- → M19
Assignee | ||
Updated•24 years ago
|
Status: NEW → ASSIGNED
Whiteboard: [y]
Target Milestone: M19 → ---
Comment 2•24 years ago
|
||
Netscape nav triage team: based on Steve Morse's pre-triage recommendation, this is not a beta stopper.
Keywords: nsbeta1-
Assignee | ||
Updated•24 years ago
|
Whiteboard: [y]
Reporter | ||
Comment 3•24 years ago
|
||
requesting mozilla0.9
Reporter | ||
Comment 4•24 years ago
|
||
based on the 2/8/01 API review meeting which morse attended, we came up w/ the following interface for public consumption: nsICookieService - nsISimpleEnumerator enumerate(); remove(name, domain); reset();
Assignee | ||
Updated•23 years ago
|
Target Milestone: --- → mozilla0.9
Assignee | ||
Comment 5•23 years ago
|
||
There's much more to this bug report than Judson mentioned. Namely all the following which is in the embeddeed api review notes at http://www.mozilla.org/projects/embedding/apiReviewNotes.html#nsICookieService separate into permission iface, and a cookie iface, and an image iface. permissions (maybe image): Permission_Add image Image_Block Image_CheckForPermission nsICookieService remove(in string name, in string domain); nsISimpleEnumerator enumerate(); clearAll(); // deletes the cookie list, *not* persistingit. SetCookieString()'s nsIDocument (which is private) needs to become an nsIDOMDocument which can internally be QI'd to a nsIDocument. SetCookieString() and SetCookieStringFromHttp() should be combined. CookieEnabled() should be removed in favor of direct pref manipulation.
Mass moving most of mozilla0.9 bugs to mozilla0.8.1
Target Milestone: mozilla0.9 → mozilla0.8.1
Assignee | ||
Comment 9•23 years ago
|
||
Attaching first attempt at a patch. It doesn't yet have any of the mac-specific changes but otherwise should be complete. It's been tested on windows and I'm in the process of testing it on linux. It's a bit complicated so let me explain what I have. This is mainly a restructuring with very little code change in the individual methods. However from the diff's that's not at all apparent because some files were renamed (nsCookie.* became nsCookies.*) and others were subdivided into several files. 1. The changed files are included in a cvs diff patch. They are: mozilla\extensions\cookie\ Makefile.in, makefile.win nsCookie.cpp, nsCookie.h these are really new files -- the old nsCookie.cpp/h files have been renamed to nsCookies.cpp/h nsCookieService.cpp, nsCookieService.h, nsICookieService.h mozilla\extensions\wallet\cookieviewer\nsCookieViewer.idl, nsCookieViewer.cpp mozilla\extensions\wallet\cookieviewer\CookieViewer.js mozilla\modules\libimg\src\if.cpp mozilla\dom\src\base\nsGlobalWindow.cpp 2. The following are new files and are included in an attached zip file mozilla\extensions\cookie nsCookieImageFactory.cpp nsCookieManager.cpp, nsCookieManager.h, nsICookieManager.idl nsCookies.cpp, nsCookies.h (actually a renaming of nsCookie.cpp/h) nsICookie.idl, nsCookie.h, nsCookie.cpp nsIImageService.h, nsImageService.h, nsImageService.cpp nsImages.h, nsImages.cpp nsIPermissionService.h, nsPermissionService.h, nsPermissionService.cpp nsPermissions.h nsPermissions.cpp nsUtils.cpp, nsUtils.h
Assignee | ||
Comment 10•23 years ago
|
||
Assignee | ||
Comment 11•23 years ago
|
||
Assignee | ||
Comment 12•23 years ago
|
||
I forgot to mention that the zip file contains the changed files (which are also in the patch-of-changed-files) as well as the new files. I did this because it was very difficult to determine what was left in the changed files by just looking at the patch of changed files.
Reporter | ||
Comment 13•23 years ago
|
||
Assignee | ||
Comment 14•23 years ago
|
||
About to attach new diffs that address all of the issues raised in valeski's review above. Keep in mind that the files in extensions/cookies have been restructured and reorganized into many new files. So although most of the code existed before, the reorganization makes it meaningless to do a diff with the previous files of the same name. So, for readability, all files in that directory are being diff'ed to a blank file instead. Also, here are answers to the questions that valeski asked: Q: nsGlobapWindow.cpp - I don't see the diff to all.js to include the new pref. A: It's not a new pref, it existed all along and was previously being tested in the cookie module. Q: Shouldn't the image service [now renamed to image manager] be listening to profile changes and taking some action just like cookies and permissions? A: No, there is no data structure associated with the image manager. There is permssion data and that is stored in the permission manager module. Q: Does the image service [manager] write/persist data to disk? A: No
Assignee | ||
Comment 15•23 years ago
|
||
Assignee | ||
Comment 16•23 years ago
|
||
Assignee | ||
Comment 17•23 years ago
|
||
Caveat: there is still some additional work needed on the patches that I just posted. Namely: 1. mac project files need to be updated 2. nsCookieService should probably be made a subclass of nsCookieManager 3. SetCookieString()'s nsIDocument (which is private) needs to become an nsIDOMDocument which can internally be QI'd to a nsIDocument. 4. SetCookieString and SetCookieStringFromHTTP have not been combined. This will probably not be done as part of this checkin.
Assignee | ||
Updated•23 years ago
|
Target Milestone: mozilla0.8.1 → mozilla0.9
Reporter | ||
Comment 18•23 years ago
|
||
nsICookie.idl (I overlooked a few things last time, sorry) * domainOrHost() attribute should be a boolean and renamed to domain(). returns true if domain, otherwise false. * secure() attribute should be a boolean and return true if secure, otherwise false. * the expires string needs its format defined in the comment. nsCookie.cpp|.h * member variables should reflect the boolean changes to the iface nsICookieManager.idl - * contains the following comment as the iface description +/* + + A sample of XPConnect. This file contains a CookieManager interface. + + */ needs to be cleaned up. nsModuleFactory.cpp - * rename description ---- +NS_IMPL_NSGETMODULE("nsCookieImageModule", components) -> +NS_IMPL_NSGETMODULE("nsCookieModule", components) After the above are taken care of, r=valeski on the c/cpp/h changes. I'm probably not the best reviewer for the js/xul changes.
Assignee | ||
Comment 19•23 years ago
|
||
Attaching new patches to address valeski's latest comments. Note that the reason I previously used strings for domainOrHost and secure was to avoid having to make changes to a .properties file in violation of the i18n freeze. But I agree that it is cleaner to use booleans there so we'll just bite the bullet and request an exception to the freeze.
Assignee | ||
Comment 20•23 years ago
|
||
Assignee | ||
Comment 21•23 years ago
|
||
Reporter | ||
Comment 22•23 years ago
|
||
nsICookie.idl - * the correct idl type for a boolean is "boolean" rather than PRBool. * referencing private headers in a public iface file (as nsICookie.idl is), isn't valid. We *should* return some int based standard time instead, otherwise rather than referencing the private comments, let's copy the comments into the idl.
Assignee | ||
Comment 23•23 years ago
|
||
Attaching another set of diffs to address the two points that valeski raised in his most recent comment. Also fixed a problem whereby I was using createInstance instead of getService to access the cookie functions.
Assignee | ||
Comment 24•23 years ago
|
||
Assignee | ||
Comment 25•23 years ago
|
||
Reporter | ||
Comment 26•23 years ago
|
||
I missed if.cpp. Rather than addressing the perf issues in this bug, let's revert if.cpp to just contain the service iface changes and *not* use the global stuff that you just added per my suggestions. I've opened 71752 for this.
Assignee | ||
Comment 27•23 years ago
|
||
Reporter | ||
Comment 28•23 years ago
|
||
r=valeski on the 03/12/01 17:04 patch. extensions/cookie/makefile.win - * the CSRCS and C_OBJS macros can go away as they're empty * you can pull the !ifdef NECKO out as we only producs netwerk.lib now. nsICookie.idl - * ick, seeing that your method can return any of those strings, we're doomed. We can't possibly expect someone to handle all those cases. expires should be a PRUint32 (yes, that's the idl type in this case), and it should map to PRTime. Then in JS you should be able to do a "new Date(theTimeReturnedFromExpires);" We should get this right the first time around, creating nsICookie2 just for the expires header would be bad.
Assignee | ||
Comment 29•23 years ago
|
||
I stand corrected. Those are all the formats that the cookie module is able to handle from the set-cookie header returned from the server. But there is only one format that it actually puts into the cookie object to be returned by the enumerator. Specifically, it is the one returned by ctime(), namely: Fri Dec 31 16:01:45 2010 I'll update the comment in nsICookie.idl to reflect this.
Assignee | ||
Comment 30•23 years ago
|
||
Posting my latest set of patches. This covers just about all that I planned on implenting for this bug report. The changes since the last set of patches are: 1. pass expiration time in the interface as a PRUint64 2. remove nsStrings from all interfaces 3. idl-ize nsCookieService (this is the last non-idl-ized interface) 4. nsIDocument argument removed from SetCookieString interface The only work still remaining is the mac project files. Jud please do a final review. Thanks.
Assignee | ||
Comment 31•23 years ago
|
||
Assignee | ||
Comment 32•23 years ago
|
||
Assignee | ||
Comment 33•23 years ago
|
||
Here is a list of the new files (in extensions/cookie) that will need to be added to the mac project files. nsICookie.idl nsICookieManager.idl nsIImgManager.idl nsIPermission.idl nsIPermissionManager.idl nsICookieService.idl nsCookieManager.cpp nsCookies.cpp nsCookieService.cpp nsImages.cpp nsImgManager.cpp nsModuleFactory.cpp nsPermission.cpp nsPermissionManager.cpp nsPermissions.cpp nsUtils.cpp nsCookieManager.h nsCookies.h nsCookieService.h nsImages.h nsImgManager.h nsPermission.h nsPermissionManager.h nsPermissions.h nsUtils.h
Reporter | ||
Comment 34•23 years ago
|
||
nsICookie.idl - * expires as PRUint64, excellent! one question, can we add a statement to the comment about the timezone it represents. Is it GMT? Is its timezone localized to whatever machine the code is running on? nsCookies.cpp - + time_t expiresTime; + if (cookie->expires) { + /* + * Cookie expiration times on mac will not be decoded correctly because + * they were based on get_current_time() instead of time(NULL) -- see comments in + * get_current_time. So we need to adjust for that now in order for the + * display of the expiration time to be correct + */ + expiresTime = cookie->expires + (time(NULL) - get_current_time()); + } + *expires = expiresTime; if cookie->expires == 0, expiresTime isn't defined. we should default it to 0 when we declare expiresTime. Have you verified that the new expires PRUint64 is working? Are the dates showing up in the cookie viewer correct?
Assignee | ||
Comment 35•23 years ago
|
||
Amazing coincidence. I just caught that bug (uninitialized cookie->expires) about 15 minutes ago while doing my testing. I have already put the fix in my tree. I'm continuing testing and will probably be making other trivial changes as I find and fix bugs. Yes, my testing has verified the correct expiration times for the non-zero cases.
Assignee | ||
Comment 36•23 years ago
|
||
The timezone will be localized to whatever timezone the client machine is running in (recall that we take the difference of the current local time and the server time and add that to the expire time received from the server. That has the effect of making the expire time be in local time. Will add comment to that effect.
Assignee | ||
Comment 37•23 years ago
|
||
Posting new patches. Fixes the cookie->expire initialization problem discussed above. But, more significantly, it accounts for all the changes that were made to the files involved since the last time I pulled a tree.
Assignee | ||
Comment 38•23 years ago
|
||
Assignee | ||
Comment 39•23 years ago
|
||
Assignee | ||
Comment 40•23 years ago
|
||
cc'ing mcafee for a review of the .xul and .js changes.
Assignee | ||
Comment 41•23 years ago
|
||
Attaching slightly modified patch for cookieviewer.xul and cookieviewer.js to fix an unrelated problem. Turns out hyatt made a change to the style sheets and as a result the image tab in the cookie-viewer dialog was not being displayed. Simply changing style="display:none" to hidden="true" fixes that problem. Since I'm already changing cookieviewer.xul and cookieviewer.js for this bug report, I'll piggyback this additional fix along with it.
Assignee | ||
Comment 42•23 years ago
|
||
Comment 43•23 years ago
|
||
Morse, did you also fix that problem with the title? If you change between tabs, then you will notice that the title is not synchronized with the current selection. There's some code in the xul file but it doesn't seem to work, XUL bug?
Assignee | ||
Comment 44•23 years ago
|
||
The implementation never intended to synchronize the title with the tab selected -- rather it is synchronized with the way you entered the dialog (from the image-manager menu item or from the cookie-manager menu item). That is the code you probably saw in CookieViewer.js and it works as intended. The correct thing is to separate the cookie manager and image manager into two separate dialogs. There will still be one .xul and .js file but it will hide the parts not used. This is actually the topic of bug 40026 but I can easily accomodate that here since I'm already making changes to CookieManger.xul and CookieManager.js. Will attach a revised set of patches that accomplishes this.
Assignee | ||
Comment 45•23 years ago
|
||
Assignee | ||
Comment 46•23 years ago
|
||
And while I'm at it making changes to CookieViewer.js, I might as well fix up the javascript warning message that bug 64888 is complaining about. Simply requires the addition of a "var" declaration. Attaching yet another revised patch for this.
Assignee | ||
Comment 47•23 years ago
|
||
Assignee | ||
Comment 48•23 years ago
|
||
Correction, the bug that is complaining about javascript warning message is bug 64588, not 64888. Sorry about that.
Assignee | ||
Comment 49•23 years ago
|
||
And I should also mention that the patches already included here also take care of the javascript warning in bug 70437.
Comment 50•23 years ago
|
||
Assignee | ||
Comment 51•23 years ago
|
||
Chris, the file nsIImgManager.idl is in the extensions/cookie directory and is included in the patches that I've attached above for that directory. It builds just fine for me on linux. One thing I forgot to mention. In order for everything to compile you need to remove the file extensions/cookie/nsICookieService.h. That file is now being generated by the idl compiler from the new nsICookieService.idl file and is put into the _xpidlgen subdirectory. If you have the old nsICookieService.h file in the extensions/cookie directory, it will get used instead and the compile will fail. So when the attached patches are checked in, I will also do a "cvs remove" to remove the existing nsICookieService.h file.
Comment 52•23 years ago
|
||
QueryInterface() usage in cookieTasksOverlay.xul: Is this right? Maybe needs a try/catch clause in case we can't get the service. xul/js changes look ok. .cpp patch is really big, if all that's going in I recommend we test this pretty well before checking in. r=mcafee
Assignee | ||
Comment 53•23 years ago
|
||
It's not as big a change as it looks. Nearly all the code existed before in the original nsCookie.cpp file. All I did now was reorganize it into many files. Unfortunately I couldn't do a diff to demonstrate that because diff-ing just wouldn't work in this case. But, yes, I have been doing a lot of testing. cc'ing alecf for a superreview.
Assignee | ||
Comment 54•23 years ago
|
||
Adding one more patch that reduces the number of cookie writes so that they occur at the end of the document load rather than each time a cookie is changed. Since the other patches were already reviewed, this patch is being added as an incremental patch over the previous patches. This will make the review of this one change simpler.
Assignee | ||
Comment 55•23 years ago
|
||
Assignee | ||
Comment 56•23 years ago
|
||
By the way, the writing of the cookies file was the topic of bug 71071. So once th patches in this bug report get checked in, I'll be closing out that bug report as well. To recap, the checkin of the patches here will fix all of the following bugs as well: bug 40026: cookiemanager and imagemanager should be separate dialogs bug 64588: javascript warning in CookieViewer.js bug 71071: writing cookies file on every cookie bug 70437: javascript warning in cookieTasksOverlay.xul
Assignee | ||
Comment 57•23 years ago
|
||
One more I forgot to mention: bug 70989: clean up lots of shadows That bug report addresses many different files that need to be cleaned up and nsCookie.cpp is amongst them. So the cookie part of bug 70989 is taken care of by the patches presented here.
Reporter | ||
Comment 58•23 years ago
|
||
r=valeski on all the c/c++ changes, including the write of cookies at end of doc load. I'm glad that write optimization was straightforward.
Assignee | ||
Comment 59•23 years ago
|
||
There's yet another bug which involves the files that I'm modifying here. Namely bug 703391 which involves another change to cookieTasksOverlay.xul. The bug has to do with the image-manager menu showing up in the netscape build (it should appear in the mozilla build only) and that is causing a crash. Again, to make matters simpler for the reviewers, I'll attach an incremental patch here that covers the change to avoid image-manager from showing up in the netscape build.
Assignee | ||
Comment 60•23 years ago
|
||
Comment 61•23 years ago
|
||
Steve, what's the reason for Netscape to take the image blocker out of there builds? Security or add earnings issues ??
Comment 62•23 years ago
|
||
r=hwaara on the very last xul patch. filed bug 72389 on a separate thing I noticed while viewing cookieTasksOverlay.xul.
Assignee | ||
Comment 63•23 years ago
|
||
OK, once again all patches to this point have been reviewed. Three separate reviewers: r=valeski for all c++ changes r=mcaffee for all xul/js changes except for change for bug 70391 r=hwaara for change for bug 70391 Now for simplicity of the super-reviewer, I am attaching the combined patches (last few patches have been incremental ones) to this report. So the only thing the super-reviewer will need to look at are the next two patches which are the combined patches for all preceding changes. The reason there are two of them is that one contains the code in extensions/cookies (which is a complete reorg) and the other contains the changes outside of extensions/cookies.
Assignee | ||
Comment 64•23 years ago
|
||
Assignee | ||
Comment 65•23 years ago
|
||
Assignee | ||
Comment 66•23 years ago
|
||
alecf, this is all ready for you to super-review.
Reporter | ||
Comment 67•23 years ago
|
||
I missed something in my review of nsCookieService.* and therefore need to revoke my r= until we resolve it. " NS_IMPL_ISUPPORTS3(nsCookieService, nsICookieService, nsIObserver, nsISupportsWeakReference); +// adding NSIDocumentLoaderObserver here would cause a crash at shutdown +//NS_IMPL_ISUPPORTS4(nsCookieService, nsICookieService, +// nsIObser " The comments threw me off there as I was perusing the code. I thought the supports4() macro was replacing the supports3() macro. nsCookieService implemnents the docloaderobserver, and therefore must reflect that fact in it's QI impl (which gets spun out of the NS_IMPL_ macro).
Assignee | ||
Comment 68•23 years ago
|
||
My fault. I was getting a crash with the NS_IMPL_ISUPPORTS4 statement (that includes the NSIDocumentLoaderObserver) but not with the NS_IMPL_ISUPPORTS3 statement (without that observer). That's why I had the ISUPORTS4 commented out. But Judson reminded me that a clobber was necessary and when I did that, no more crash with the ISUPPORTS4. So consider the attached patches to contain the ISUPPORTS4 and the code for the ISUPPORTS3 is removed. With that change, the r=valeski should again stand.
Comment 69•23 years ago
|
||
One thing - from nsICookieManager: + nsISimpleEnumerator enumerate(); I think "enumerate" is the wrong name for this accessor. "Enumerate" (verb) is what you do with the enumerator which this returns, but this function does not itself enumerate. I'd call it getEnumerator() or getCookies().
Reporter | ||
Comment 70•23 years ago
|
||
I had originally suggested "enumerate," but you're right Conrad, "getEnumerator" is best. Steve, can you just make it a readonly attribute in the idl file? readonly attribute enumerator; Glad the clobber worked. r=valeski after the enumerator attribute change.
Comment 71•23 years ago
|
||
Steve, if you accidently wipe your cookies, after pressing the wrong button (: your lost. My list was soooo beautifully build up, started that a long time ago, but heck now i need to build it again!! So please add a dialog asking for some confirmation. This will most certainly make it more foolproof <== and Steve, that fool is me!
Assignee | ||
Comment 72•23 years ago
|
||
You can't lose your cookies with a single wrong buttonpress. You have to first press either REMOVE COOKIE or REMOVE ALL COOKIES in the cookie manager dialog. At this point the cookies are not yet gone. You then have to press OK in order for the deletion to take effect. So you have to make two wrong button presses.
Comment 73•23 years ago
|
||
can you attach a CVS diff? all your files are coming up new, so I can't actually see what is/isn't changing, let alone try the patch... I mean, according to your diff you're adding a file nsCookie.cpp, but I already have one of these files in my tree.
Comment 74•23 years ago
|
||
also, your .idl files belong in a public/ directory, not grouped in with all your source.
Comment 75•23 years ago
|
||
Oh, and (now that I've read the additional comments here) I have to agree that you need a confirmation dialog before doing something as destructive as removing all your cookies - I realize that it doesn't REALLY happen till you hit OK, but we can't expect users to just know that.
Assignee | ||
Comment 76•23 years ago
|
||
First attaching a new set of patches for valeski. This consists of the namechange that he requested for emumerator. With this change, the r=valeski is implied because of his last comment: > r=valeski after the enumerator attribute change.
Assignee | ||
Comment 77•23 years ago
|
||
Assignee | ||
Comment 78•23 years ago
|
||
Assignee | ||
Comment 79•23 years ago
|
||
> Oh, and (now that I've read the additional comments here) I have to agree
> that you need a confirmation dialog before doing something as destructive
> as removing all your cookies - I realize that it doesn't REALLY happen
> till you hit OK, but we can't expect users to just know that.
Aw, come on, give me a break.
1. This has nothing at all to do with the current bug report.
2. This bug report is already too big, attempting to fix all the worlds ills.
3. Such behavior occurs all over the place. For example, manage bookmarks,
select all, press delete. Oops, all bookmarks gone and no warning dialog (you
didn't even get a chance to press cancel which at least you can do with
cookies). I'm sure I could give many more such examples (password manager, form
manager, prefs, probably address book, etc).
4. Losing cookies in not a terrible thing. The browser does it all the time
behind your back when you are not even aware of it. For example, if you already
have more than the max number allowed, the browser starts throwing out the
least-recently used.
Assignee | ||
Comment 80•23 years ago
|
||
> can you attach a CVS diff? all your files are coming up new, so I can't
> actually see what is/isn't changing, let alone try the patch... I mean,
> according to your diff you're adding a file nsCookie.cpp, but I already
> have one of these files in my tree.
The nsCookie.cpp that you have in your tree has absolutely nothing to do with
the new one. First, the old one became the new nsCookies.cpp (note the
"s"). And, second, many of the routines that were in the old nsCookie.cpp
became redistrubuted among the new files such as nsPermissions.cpp and
nsImages.cpp. So a diff in this case would be completely meaningless and not
give you any hint at all as to what has changed. The only files in the
extensions/cookie directory for which a diff would be useful are the makefiles
(makefile.win and Makefile.in).
If you want to try it out, you should:
1. Remove the following file from extensions/cookie
nsICookieService.h
nsCookie.h,
2. Replace the following files with new files of the same name
makefile.win
Makefile.in
nsCookie.h
nsCookie.cpp
nsCookieService.h
nsCookieService.cpp
nsCookieHTTPNotify.cpp
3. Add the following new files to nsCookie.cpp
nsICookie.idl
nsICookieManager.idl
nsIImgManager.idl
nsIPermission.idl
nsIPermissionManager.idl
nsICookieService.idl
nsCookieManager.cpp
nsCookies.cpp
nsCookieService.cpp
nsImages.cpp
nsImgManager.cpp
nsModuleFactory.cpp
nsPermission.cpp
nsPermissionManager.cpp
nsPermissions.cpp
nsUtils.cpp
nsCookieManager.h
nsCookies.h
nsCookieService.h
nsImages.h
nsImgManager.h
nsPermission.h
nsPermissionManager.h
nsPermissions.h
nsUtils.h
So, per your request, I am attaching a diff for the files in section 2 above.
Assignee | ||
Comment 81•23 years ago
|
||
Comment 82•23 years ago
|
||
ok, thanks for the explanation..the diff was useful, if only for the makefile.win/Makefile.in updates :) I'm reviewing the rest now, from the original, non-cvs diff.
Assignee | ||
Comment 83•23 years ago
|
||
> also, your .idl files belong in a public/ directory, not grouped in with
> all your source.
Agreed, but can we leave this for a follow-up bug report. This bug report
already has too much in it and if I start adding new directories as well, the
chances of getting everything right and not breaking the tree get reduced
even more.
Assignee | ||
Comment 84•23 years ago
|
||
Correction: Above I said 1. Remove the following file from extensions/cookie nsICookieService.h nsCookie.h, The file nsCookie.h did not belong in that section. This file is covered in section 2. The only file that you need to explicitly remove is nsICookieService.h and it is very important that you do so -- otherwise things won't compile.
Assignee | ||
Comment 85•23 years ago
|
||
To the list of bugs that are fixed by the patches contained here (see my comment of 2001-03-17 10:00), we need to add bug 72785. This is the problem I alluded to in my comment of 2001-03-14 17:07 but no separate bug report had been filed on that problem at that time. To recap, the total list of bugs fixed by this patch are: bug 40026: cookiemanager and imagemanager should be separate dialogs bug 64588: javascript warning in CookieViewer.js bug 71071: writing cookies file on every cookie bug 70437: javascript warning in cookieTasksOverlay.xul bug 72785: image manager shows nothing and part of bug 70989: clean up lots of shadows (cookie part of that bug)
Assignee | ||
Comment 86•23 years ago
|
||
Oops, there's one more for the above list. See my comment of 2001-03-17 22:51 which erroneously mentions 703391. That should have been bug 70391. bug 70391: commercial browser crashes when mousing over to image manager
Comment 87•23 years ago
|
||
ok, my comments on this patch are:
- COOKIE_IS_SPACE isspace/isprint thing - use nsString::IsSpace() and maybe
nsString::IsPrint() - if you can't use IsPrint(), explain in a comment why you
need to compare against 0x7f
- get_current_time() - comments say not to use PR_Now, but the function
actually uses it, just fix the comment
- use NS_PREF_CONTRACTID and CATEGORYMANAGER_CONTRACTID instead of their raw
text equivalents
- use ns[C]AutoString, not ns[C]String for local variables
- this doesn't block your check in, but in the future, try to use bundle-
>formatStringFromName instead of retrieving the string, and then formatting it.
This reduces the number of allocations done.
- fix CKutil_Localize to take a const PRUnichar*, and fix callers to use
NS_LITERAL_STRING("foo").get(), then fix the function to return the character
pointer that is returned from GetStringFromName(), instead of making excess
copies of it. This will signifigantly reduce allocations
- remove that excess parsing stuff from cookie_ParseDate()
And yes, I realize that much of this code was already in the tree, but if we're
going to fix this, we're going to fix it right... now is the time.
Assignee | ||
Comment 88•23 years ago
|
||
Attaching new patch for files in extensions/cookie which takes care of all of
alecf's comments except for the following:
> get_current_time() - comments say not to use PR_Now, but the function
> actually uses it, just fix the comment
No, the comment is absolutely correct. It says we can't call PR_Now *directly*
but instead must use this get_current_time routine which will call PR_Now for us
and do the appropriate arithmetic around it.
Assignee | ||
Comment 89•23 years ago
|
||
Comment 90•23 years ago
|
||
ack! one thing I didn't notice in my last review was that in the idlized nsCookie and nsPermission structures, you're storing direct pointers to the strings that you're consuming in the constructor, AND the accessors, which are supposed to return copies of this data, are returning the actual pointers. If the constructor is actually taking ownership of these strings, then you should explicily say so in a comment. however, the accessors MUST make copies of these strings for the caller because otherwise they are not XPCOM compliant and consumers will try to free the strings, leaving you with a dangling pointer in your class instance. I understand what you mean about the comment - I misread the comment, and assumed you had updated the function without updating the comment.
Assignee | ||
Comment 91•23 years ago
|
||
Here's my next set of patches in which the accessor makes a copy of the string before returning it to the caller. Changes are in nsCookie.cpp and nsPermission.cpp, both of which are in the extensions/cookie module. Also making a minor change to CookieViewer.js (not in extensions/cookie module) in which an overzelous change of "value" to "label" occurred as a result of the recent xul syntax change. However that "value" attribute is on a textfield (textbox in the new syntax) and as such it should remain "value." See bug 73288 which describes this error in the currently checked-in cookie viewer and I blindly copied that error into my patches attached here. So now bug 73288 is added to the list of bugs that will be fixed if the patches presented here ever gets to see the tree. To recap, the attached patches will correct all of the following: bug 40026: cookiemanager and imagemanager should be separate dialogs bug 64588: javascript warning in CookieViewer.js bug 71071: writing cookies file on every cookie bug 70437: javascript warning in cookieTasksOverlay.xul bug 72785: image manager shows nothing bug 70391: commercial browser crashes when mousing over to image manager bug 73288: host/domain indicator in cookie manager dialog is incorrect and part of bug 70989: clean up lots of shadows (cookie part of that bug)
Assignee | ||
Comment 92•23 years ago
|
||
Assignee | ||
Comment 93•23 years ago
|
||
Comment 94•23 years ago
|
||
ok, this looks good and I'm glad to see the warning about how the nsCookie takes ownership of it's members, but now I think you're leaking all the members of nsCookie - the destructor looks empty..we're so close! :)
Assignee | ||
Comment 95•23 years ago
|
||
Attaching patch for files in extensions/cookie updated to prevent leak of the members of nsCookie and also nsPermission
Assignee | ||
Comment 96•23 years ago
|
||
Comment 97•23 years ago
|
||
ok, sr=alecf... finally!
Assignee | ||
Comment 98•23 years ago
|
||
Fix checked in. All the above-mentioned bugs are now being marked fixed as well.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•