46783 - nsICookieService.h needs to be idl'ized

Status: VERIFIED FIXED

(Core :: Networking: Cookies, defect, P3)

Description

[Judson Valeski]

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.

Comment 1

[Judson Valeski]

-> morse

Comment 2

[Marcia Knous [:marcia]]

Netscape nav triage team: based on Steve Morse's pre-triage recommendation, this 
is not a beta stopper.

Comment 3

[Judson Valeski]

requesting mozilla0.9

Comment 4

[Judson Valeski]

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();

Comment 5

[Stephen P. Morse]

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.

Comment 6

[Stephen P. Morse]

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

Comment 7

[Paul Chen]

Mass moving most of mozilla0.9 bugs to mozilla0.8.1

Comment 8

[HJ]

just CC spam

Comment 9

[Stephen P. Morse]

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

Comment 10

[Stephen P. Morse]

Attachment: Patch of changed files

Comment 11

[Stephen P. Morse]

Attachment: zip of new files

Comment 12

[Stephen P. Morse]

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.

Comment 13

[Judson Valeski]

Attachment: my review of the first iteration of these patches. no r= yet.

Comment 14

[Stephen P. Morse]

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

Comment 15

[Stephen P. Morse]

Attachment: Files in extensions/cookie (diff'ed to a blank file)

Comment 16

[Stephen P. Morse]

Attachment: Files not in extensions\cookie

Comment 17

[Stephen P. Morse]

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.

Comment 18

[Judson Valeski]

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.

Comment 19

[Stephen P. Morse]

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.

Comment 20

[Stephen P. Morse]

Attachment: revised: Files in extensions/cookie (diff'ed to a blank file)

Comment 21

[Stephen P. Morse]

Attachment: Revised: Files not in extensions\cookie

Comment 22

[Judson Valeski]

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.

Comment 23

[Stephen P. Morse]

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.

Comment 24

[Stephen P. Morse]

Attachment: next revision of files in extensions/cookie (diff'ed to a blank file)

Comment 25

[Stephen P. Morse]

Attachment: next revision of files not in extensions\cookie

Comment 26

[Judson Valeski]

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.

Comment 27

[Stephen P. Morse]

Attachment: files not in extensions/cookies with part of change to if.cpp backed out

Comment 28

[Judson Valeski]

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.

Comment 29

[Stephen P. Morse]

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.

Comment 30

[Stephen P. Morse]

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.

Comment 31

[Stephen P. Morse]

Attachment: latest revision of files in extensions/cookie (diff'ed to a blank file)

Comment 32

[Stephen P. Morse]

Attachment: latest revision of files not in extensions\cookie

Comment 33

[Stephen P. Morse]

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

Comment 34

[Judson Valeski]

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?

Comment 35

[Stephen P. Morse]

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.

Comment 36

[Stephen P. Morse]

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.

Comment 37

[Stephen P. Morse]

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.

Comment 38

[Stephen P. Morse]

Attachment: still later revisions of files in extensions/cookie

Comment 39

[Stephen P. Morse]

Attachment: still later revisions of files not in extensions/cookies

Comment 40

[Stephen P. Morse]

cc'ing mcafee for a review of the .xul and .js changes.

Comment 41

[Stephen P. Morse]

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.

Comment 42

[Stephen P. Morse]

Attachment: files not in extensions/cookies with addtional changes to cookieviewer.xul/js

Comment 43

[HJ]

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?

Comment 44

[Stephen P. Morse]

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.

Comment 45

[Stephen P. Morse]

Attachment: files not in extensins/cookies revised to have separate cookie-manager and image-manager dialogs

Comment 46

[Stephen P. Morse]

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.

Comment 47

[Stephen P. Morse]

Attachment: files not in extensions/cookie == revised to fix bug 64588 as well)

Comment 48

[Stephen P. Morse]

Correction, the bug that is complaining about javascript warning message is bug 
64588, not 64888.  Sorry about that.

Comment 49

[Stephen P. Morse]

And I should also mention that the patches already included here also take care 
of the javascript warning in bug 70437.

Comment 50

[Chris McAfee]

Attachment: top-level version of last patch. missing nsIImgManager.idl, patch does not build on linux

Comment 51

[Stephen P. Morse]

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

[Chris McAfee]

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

Comment 53

[Stephen P. Morse]

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.

Comment 54

[Stephen P. Morse]

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.

Comment 55

[Stephen P. Morse]

Attachment: incremental patch to reduce number of cookie writes

Comment 56

[Stephen P. Morse]

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
   

Comment 57

[Stephen P. Morse]

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.

Comment 58

[Judson Valeski]

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.

Comment 59

[Stephen P. Morse]

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.

Comment 60

[Stephen P. Morse]

Attachment: incremental patch to prevent image manager from appearing in netscape build

Comment 61

[HJ]

Steve, what's the reason for Netscape to take the image blocker out of there
builds? Security or add earnings issues ??

Comment 62

[Håkan Waara]

r=hwaara on the very last xul patch. filed bug 72389 on a separate thing I 
noticed while viewing cookieTasksOverlay.xul.

Comment 63

[Stephen P. Morse]

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.

Comment 64

[Stephen P. Morse]

Attachment: All changes to date for files outside of extensions/cookie

Comment 65

[Stephen P. Morse]

Attachment: all changes to date for files in extensions/cookie

Comment 66

[Stephen P. Morse]

alecf, this is all ready for you to super-review.

Comment 67

[Judson Valeski]

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).

Comment 68

[Stephen P. Morse]

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

[Conrad Carlen (not reading bugmail)]

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().

Comment 70

[Judson Valeski]

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

[HJ]

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!

Comment 72

[Stephen P. Morse]

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

[Alec Flett]

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

[Alec Flett]

also, your .idl files belong in a public/ directory, not grouped in with all
your source.

Comment 75

[Alec Flett]

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.

Comment 76

[Stephen P. Morse]

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.

Comment 77

[Stephen P. Morse]

Attachment: files not in extensions/cookie: change for enumerator per valeski's request

Comment 78

[Stephen P. Morse]

Attachment: files in extensions/cookie: change to enumerator per valeski's request

Comment 79

[Stephen P. Morse]

> 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.

Comment 80

[Stephen P. Morse]

> 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.

Comment 81

[Stephen P. Morse]

Attachment: cvs diffs for files in extensions/cookie but I do not recommend using them.

Comment 82

[Alec Flett]

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.

Comment 83

[Stephen P. Morse]

> 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.

Comment 84

[Stephen P. Morse]

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.

Comment 85

[Stephen P. Morse]

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)

Comment 86

[Stephen P. Morse]

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

[Alec Flett]

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.

Comment 88

[Stephen P. Morse]

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.

Comment 89

[Stephen P. Morse]

Attachment: revised files in extension/cookie to address super-reviewer's comments

Comment 90

[Alec Flett]

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.

Comment 91

[Stephen P. Morse]

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)

Comment 92

[Stephen P. Morse]

Attachment: files in extensions/cookie with accessors making copies of strings

Comment 93

[Stephen P. Morse]

Attachment: files not in extensions/cookie with host/domain indicator fixed

Comment 94

[Alec Flett]

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! :)

Comment 95

[Stephen P. Morse]

Attaching patch for files in extensions/cookie updated to prevent leak of the 
members of nsCookie and also nsPermission

Comment 96

[Stephen P. Morse]

Attachment: fix leak of members of nsCookie and nsPermission

Comment 97

[Alec Flett]

ok, sr=alecf... finally!

Comment 98

[Stephen P. Morse]

Fix checked in.  All the above-mentioned bugs are now being marked fixed as 
well.

Comment 99

[Tom Everingham]

verified