We need to be able to read in cookies from an arbitrary file to support import/export in Firefox. This requires changes to nsICookieManager2.

taking, setting dependencies

+ * Reads cookies from a file, augmenting the current in-memory set. what kind of file is that? same format as cookies.txt? would be nice if the documentation mentioned that...

Can you explain more about what you're trying to do here? I ask this because: a) in exposing those methods, you don't provide a means for the caller to specify what file format (or more subtly, what file format version) the specified cookie file adheres to; b) we are considering implementing file format versioning for cookies, i.e. specifying a filename alone is insufficient information to read/write cookies - version information would also be required. (this would allow for file format changes while still providing full forward compatibility, and partial backward compatibility.)

Basically these files read/write cookies.txt formatted files of compatible versions. Perhaps my comment was a little over-zealous when mentioning export as a use. I don't know that the cookie manager is the right place to insert a lot of export/conversion code, so the real use of the writeCookies method is to give a little more control over when cookies are written out. I can change the documentation to mention this. As far as the cookie manager is concerned, assuming it reads one format, of various versions, all that's required is an additional parameter containing the version number. (as an aside... Opera stores its file versions as a number in the file header, making parameter passing unnecessary)

>the real use of the writeCookies method is to give a little more control over >when cookies are written out. Okay, but can you explain why you need this? You're trying to change nsCookieService::Write from dealing with one file in a fixed location, into dealing with multiple files in different locations - what's the benefit?

(In reply to comment #6) > Okay, but can you explain why you need this? You're trying to change > nsCookieService::Write from dealing with one file in a fixed location, into > dealing with multiple files in different locations - what's the benefit? I need to write the cookies file manually because after migrating there are in-memory changes to certain sets of data that aren't persisted, so I need to force a write. I don't really need the parameter however, I just added that for symmetry with ReadCookies. I can remove it if you want.

(In reply to comment #7) > I need to write the cookies file manually because after migrating there are > in-memory changes to certain sets of data that aren't persisted, so I need to > force a write. I still don't follow. If you are adding cookies from some other datasource via nsICookieManager2::Add, the cookies will always be flushed to disk at app shutdown, so you don't need to force a write in that situation. If you're wanting to read cookies from a different location (in the mozilla cookie file format, so it's not an "import" per se), can you explain why and how, specifically?

I discovered I don't actually need writeCookies for anything after all. Here's the same patch but without writeCookies, and with a note in the documentation saying that the function reads Netscape Cookies files.

(In reply to comment #9) > with a note in the documentation > saying that the function reads Netscape Cookies files. I don't see such a comment in the patch?

(In reply to comment #10) > (In reply to comment #9) > > with a note in the documentation > > saying that the function reads Netscape Cookies files. > > I don't see such a comment in the patch? Huh. I wonder what happened, maybe didn't save the file before diffing. Here it is: /** * Reads cookies from a Netscape Cookies file, augmenting the current * in-memory set. * * @param aCookiesFile The Netscape Cookies file to read cookies from. */

I'm not reviewing this patch until you explain in gory detail why you need this.

(In reply to comment #12) > I'm not reviewing this patch until you explain in gory detail why you need this. I need this because I need to be able to read in Netscape Cookies files from Seamonkey or Netscape 4.x after the app has started, when the user invokes the migration system from the File menu. Before the app has started in automigration, cookies can be migrated by simply copying the file over, but once the app is running the file from the other app can't just copy the file over since the app has the file open.

Hmm... the thing that bothers me here is that you're exposing to the world a fairly internal detail of the cookie backend. For instance, if we did implement versioning (so there are possibly multiple cookie files in the profile, and a given seamonkey version uses the one it understands), firefox would need to know which version to use. If you'll always be migrating from netscape/seamonkey cookie files, neater ways to do this might be: a) use the profile service to fire profile-before-change and profile-do-change notifications, and copy the appropriate cookie file(s) into the new user profile. this avoids touching the cookie (or pwdmanager, or any other) backends, but still requires firefox know which files to copy. (i prefer this, because there's no impact on seamonkey or the GRE, for something which really should be an app-specific detail) b) instead of passing a filename into readCookies, just pass the profile directory. this lets readCookies handle any and all details of versioning, but still requires readCookies be publicly exposed. (aside: for IE cookie migration, see extensions/tridentprofile/src/nsTridentPreferencesWin.cpp) darin, any thoughts?

a) is out of the question since we also need to be able to do this after profile startup. (In reply to comment #14) > b) instead of passing a filename into readCookies, just pass the profile > directory. -- How does this work for Netscape 4.x profiles? Do you really want to add ifdefs to Cookie code to know to look for whatever the file was called on MacOS9 4.x? > (aside: for IE cookie migration, see > extensions/tridentprofile/src/nsTridentPreferencesWin.cpp) -- I'm aware of this code, I am using it and similar systems to it (for Opera cookies etc) in my migration engine.

(In reply to comment #14) > fairly internal detail of the cookie backend. For instance, if we did implement > versioning (so there are possibly multiple cookie files in the profile, and a > given seamonkey version uses the one it understands), firefox would need to know > which version to use. --- ... Why would you store separate files? Why not store the version number as metadata in a header in the file? Assume files that have no header are old-style Netscape Cookies files and those that do are versioned appropriately. This is what most other browsers do for data files that may change - they use version metadata in the file rather than playing musical file names version-version.

(In reply to comment #15) > a) is out of the question since we also need to be able to do this after profile > startup. No - shut down the profile using profile-before-change, copy the files, re-open the profile using profile-do-change. That's effectively like shutting the app down, from the profile's point of view. I think that should work, no? > How does this work for Netscape 4.x profiles? Do you really want to add ifdefs > to Cookie code to know to look for whatever the file was called on MacOS9 4.x? If looking for a Netscape 4.x cookie file is beyond what nsCookieService::Read() does now, then we can't do that... so you'd either have to write your own NS4 cookie importer code (a la nsTridentPreferencesWin.cpp), or pick a different option. > I'm aware of this code, I am using it and similar systems to it (for Opera > cookies etc) in my migration engine. Cool, just wanted to make sure you were aware of it, since it might've saved some work :) >... Why would you store separate files? Why not store the version number as >metadata in a header in the file? I agree it's neater to have one file, but we also need back/fwd compatibility... e.g. a Moz 1.8 user (new cookie format) goes back to NS7, which knows nothing of the new file format and will read in the file with unpredictable results. Having separate files keeps both fwd and back migration happy.

(In reply to comment #17) > No - shut down the profile using profile-before-change, copy the files, re-open > the profile using profile-do-change. That's effectively like shutting the app > down, from the profile's point of view. I think that should work, no? --- Perhaps, but it seems like a hack and has far more implications as far as code that gets executed (lots of things get shut down and restarted, not just cookies). > If looking for a Netscape 4.x cookie file is beyond what nsCookieService::Read() > does now, then we can't do that... so you'd either have to write your own NS4 > cookie importer code (a la nsTridentPreferencesWin.cpp), or pick a different option. --- Why, when the system now reads those files just fine? Brendan tells me he doesn't expect there to be any major changes to this format at least in the context of Seamonkey. We are curious as to what is being planned. Are there docs on mozilla.org? > I agree it's neater to have one file, but we also need back/fwd compatibility... > e.g. a Moz 1.8 user (new cookie format) goes back to NS7, which knows nothing of > the new file format and will read in the file with unpredictable results. Having > separate files keeps both fwd and back migration happy. --- You're saying the cookie service as it existed in 1.4 won't handle header lines in the file? Won't handle additional fields on each line? What if the header was a comment... cookies.txt has comments at the top: # HTTP Cookie File # http://www.netscape.com/newsref/std/cookie_spec.html # This is a generated file! Do not edit. # To delete cookies, use the Cookie Manager. I assume those are stripped by the Cookie file parser, both now and in the past. # FileVersion 2.0 or some such.

(In reply to comment #18) > Why, when the system now reads those files just fine? If the filename really is different, as you suggest, then reading a 4.x cookie file _is_ beyond what nsCookieService::Read does now, and so it does _not_ read those files fine. If the name is the same, then it's not a problem. > Brendan tells me he doesn't expect there to be any major changes to this format > at least in the context of Seamonkey. We are curious as to what is being > planned. Are there docs on mozilla.org? Nope, it's just a future possibility at this stage... I mention it because it's relevant to your patch here. > You're saying the cookie service as it existed in 1.4 won't handle header lines > in the file? Won't handle additional fields on each line? What if the header was > a comment... cookies.txt has comments at the top: It cannot handle additional fields on each line. Any changes involving adding additional fields will give unpredictable results when reading the files. (And of course, the older version would clobber the cookie file when it writes it out, losing any information the newer version contained.) We could store additional fields on a comment line below each cookie, but that's getting moderately ugly...

(In reply to comment #19) > (In reply to comment #18) > > Why, when the system now reads those files just fine? > > If the filename really is different, as you suggest, then reading a 4.x cookie > file _is_ beyond what nsCookieService::Read does now, and so it does _not_ read > those files fine. Sure it does. The file under Netscape 4.x on MacOS is called "MagicCookie" and yet the file format is identical to "cookies.txt" on the other platforms. Older versions of the application under MacOS use different file names for many files that do use recognized application formats. Assuming there ever was a file name change, it would be the migration engine's responsibility to identify the most applicable, recent file existing in the profile directory and get the cookie manager to load that, whether that be MagicCookie, cookies.txt, or cookiesBlah.dat.

There's too much future-proofing talk here, not enough action. If we can't extend the cookie file format for new versions that degrade gracefully, no bad effects, when read by old builds, then I suppose we need a new filename. But we should not anticipate renaming yet again -- we should be sure the new name's format can be extended backward compatibly. SeaMonkey is on a sustaining engineering path, and we shouldn't be taking code change risks without a big win. Dan, what is the big win? And: since it's in the future, can you help Ben make progress sooner on Firefox automigration, without constraining him to solve whatever problems you hope to solve in that further future? /be

Let's put the discussion of filenames aside for a moment and just consider the implications of the ReadCookies API method. Here's some questions I have: (1) How does this patch resolve conflicts? Does an imported cookie always stompt over an existing cookie of the same name? Does this issue matter? (2) Should this API provide a way for the caller to say "merge these cookies, but if the existing ones are 'better' use them instead"? I'm not sure what 'better' means exactly. Some cookies have expiration times... maybe we define 'better' based on that?!? (3) What happens if the imported cookies cause us to go over the limit of cookies per host or per profile? My blind assumption is that our cookie reading code does not check any of these limits b/c it assumes it wrote the file. Merging two files together breaks this assumption, and it could cause the cookie database to not adhere to its size limits. Does that matter? Maybe the next time a cookie is set, we'll prune the list of stored cookies appropriately? (4) Would it be better to provide a general API for reading cookies from a file, resulting in a list of nsICookie objects that don't live in any database yet? That way we separate the reading of cookies from the adding of new cookies to the database. I think all the naming issues and forwards/backwards compatiblity issues can be deferred until we need to cross that bridge. This new API would be for internal use only and could be changed in the future if necessary. As for specifying a nsIFile to the cookies file or specifying a nsIFile of the parent directory, I think for now we should just go with a nsIFile to the cookies file. It's simple, and it gets the job done. Again, we can change this later if we really need to.

(In reply to comment #22) > (1) How does this patch resolve conflicts? Does an imported cookie always > stompt over an existing cookie of the same name? Does this issue matter? --- Existing cookies are stomped. This does not matter from the migration point of view, you are requesting cookies from another app, those cookies should win. The case where you have two similar high use profiles is less likely to occur. > (2) Should this API provide a way for the caller to say "merge these cookies, > but if the existing ones are 'better' use them instead"? I'm not sure > what 'better' means exactly. Some cookies have expiration times... maybe > we define 'better' based on that?!? --- This would only be useful for synchronization between two browsers, which is not a use case for Migration. > (3) What happens if the imported cookies cause us to go over the limit of > cookies per host or per profile? My blind assumption is that our cookie --- There's such a limit? Why? To prevent infinite growth of the cookie file due to bad scripts? I assume the cookie manager already has a policy for normalizing or evicting items from this list as new ones are added. > (4) Would it be better to provide a general API for reading cookies from a > file, resulting in a list of nsICookie objects that don't live in any > database yet? That way we separate the reading of cookies from the adding > of new cookies to the database. --- this would involve: 1 refactoring the cookie parser to do this and adjusting the calling code in cookie service already to take that list and add to the database as the app is launched or writing an additional parser 2 adding an additional method to one of the cookie apis that takes an nsICookie parameter 3 adjusting my code to call 1 and then callling 2 with the list returned by 1. I'm happy to do 3 if that's what you think is best because 3 by itself is fairly easy to do. Can 1 & 2 be done in the next few weeks though? I'm not really keen on doing that though since I'm not still convinced of the value.

> (3) What happens if the imported cookies cause us to go over the limit of > cookies per host or per profile? this is my biggest concern. we need to make sure we don't foul up the accounting process here. i checked the code, and it looks to me like we would need to add a step to ReadCookies to prune the final cookie list (or prune on insert). dwitte? your thoughts on this?

Actually, nevermind.