We need the ability to "safely" specify which directory mozilla uses for disk cache. This preference was disabled because of Macintosh related problems. We need to fix this before re-enabling the pref.

This bug is/was related to 45394

tever/gagan, should this go to Networking?

The problems resulting from mis-specifying the cache directory and then clearing it, are not Mac-specific. The work to minimize the resulting damage is cross- platform.

So basically, what we need is a dialog box to pop up, warning the user that the cache directory MUST be clear of any personal files, because if it isn't, then personal files will be deleted? Oh, and also something that expressly FORBIDS "/" ever being the cache directory.

Bah. Dialog boxes are lame (and who reads them, especially if the details in them are complex?) One idea might be to create a cache folder in the directory selected, such that mozilla always creates its own subfolder which it can clear at will.

Yes, that's a good suggestion Ben. I much prefer that to a dialog box.

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

<rant> Jeez you guys, put the dialogue box back in ! Where the hell is Mozilla writing my cache now ? It better not be writing it to my /home directory or to / otherwise it's gonna blow away the whole partition !!! I want my cache to be /usr/local/netscape/cache, like I always set it !!!! Put the bloody pref back or I will be forced to use CAPS LOCK on you...."£$$£%£$!"£!@:~#~33£ </rant>

No, no, I didn't mean 'put the dialogue box back', I meant 'put the option back in prefs'. See - I am so annoyed I can't even think straight...

I agree we need the preference. I'm just not sure we can make it safe in time for nsbeta2.

well, if this doesn't make the beta2 cut, plop it in the relnotes.

Putting on [nsbeta2-] radar. tever, please give verah a release note for PR2.

nav triage team: though we like Ben's solution, it is not that important to let a user change where the cache is being stored - not for a 1.0 release. Therefore, nsbeta3- Sorry gabriel - we want to ship on time. If you want to fix it yourself, send a patch. Adding helpwanted

Can you email me the old code which was removed and I'll meditate on it.

The only code that was removed was front-end code. The back end code is still there. (at least, that's what I thought)

Adding nsbeta3 keyword to bugs which already have nsbeta3 status markings so the queries don't get all screwed up.

Probably will need to check to make sure mDiskCacheFolder doesn't already end in /cache/, in case the function is called more than once.

just a note on why having a user defined cache location is important for mac users. Since the file system on Mac is so slow, pulling cached files from a disk cache helps, but what really makes a huge difference is putting the cache on a RamDisk. If we can't select the location, then a RamDisk is not an option. just food for thought

Adding keyword patch. Gabriel, are you happy with the posted patch? Should I (or you) add the review keyword?

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

Yes, please review it. It will need some more work before it can go in. (e.g. dialog box). I don't have time (unfortunately) to download the source and compile it myself, but I think it might be a start.

adding review to get a review. :)

Ignore the bit about dialogue box, I was getting confused with another bug. I think there are at least a couple of things that need to happen with this patch: check if the directory name already ends in '/cache' or '/cache/' in case we get called more than once; and also to make sure that the directory actually gets created on the first run - it looked like there was some code there to do that, but when I tried it out quickly (a while ago) it didn't seem like that was happening.

I'm taking this one as part of my "make nsbeta3 suck as little as possible" crusade, since no one else seems to be working on it. Removing "Review" keyword until we actually have something to review (possibly by tomorrow).

over to jce2, and removing helpwanted (and other stale kw's). thanks for taking this!

Sorry guys, I've been working away from home, had no time to look @ this.

Accepting bug and trying to get SOMETHING in there for rtm, otherwise a lot of power users are going to HATE us.

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

Can someone explain when and why Mozilla deletes all the files in the cache directory (which is presumably why we need to create a subdirectory in the first place), instead of just deleting the cache files it created and leaving the other files alone? What's the bug number for that?

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

Explanation for question above: we delete all the files in the cache directory when the cache database has become corrupted, because then we can't reliably tell what files to discard. Another instance in which the cache directory may get cleared is when a major upgrade to the cache module occurs (such as a change in format of the database). The cache directory and its contents are owned by the cache; other files should not be stored there.

Nominating for mozilla 0.9, if we can spend about 3-4 undistracted hours on this, we can easily fix it.

Here are comments related to this option being missing from NS6 builds (from http://www.macintouch.com/netscape6.html): from Mark on 11/16/2000: You cannot choose the location for the disk cache. If seems to default to the boot volume in the documents folder (it created) from Charles J. Srstka on 11/15/2000: how do I put the cache on my RAM Disk? The setting for that seems to be gone.

I should be able to do something with this over Christmas. Removed nsbeta3- from Status whiteboard.

Retargeting bug..Now that my professional product has gone into beta, I should be able to bear down and fix this.

Is this really going in for 0.9.1? If so, great! Here's the issue from bug 45394: <quote from bug 45394> The cache pref panel contains a textfield that contains the browser.cache.directory preference. That preference is stored as an nsIFileSpec, which on Linux and Windows equates to a string, but on Mac it is actually an alias data structure. The TextField prefType attribute in pref-cache.xul, however, specifies the preference as "string": <textfield id="browserCacheDirectory" flex="1" pref="true" preftype="nsIFileSpec" prefstring="browser.cache.directory" prefattribute="value"/> This causes the it to display as garbage on the Mac ( Bugzilla bug 45658 ), and if the user tries to edit the field can lead to Bugzilla bug 45656 and Bugzilla bug 45394 (which can cause the installed application to self-destruct). </quote> I want to see Ben's suggestion of specifying the directory that the cache directory will be created *IN*, rather than specifying the cache directory itself. The cache will create the directory, and it won't have pre-existing user, application, or system files in it. Also, the text field that displays the directory chosen should *not* be editable. This will avoid the problems we had on the Mac. The user should click a browse button to navigate to the desired directory, and select it. This business about placing the cache directory on a RAM disk is now rather silly. It would be much more efficient if you could simply disable the disk cache and give lots of RAM to the memory cache. nsCacheService automatically stores HTTP's entries in the memory cache if the disk cache is disabled. On debug builds, there is a debug preference for enabling/disabling the disk cache. I will add the ability to also disable it by setting the disk cache size to 0, which will then work for optimized builds as well.

looks like this will have to wait for 0.9.2

A reason for user selectable cache: on our departmental network $HOME is not local, it is on a central server. It also has a disk-quota. Therefore, if the cache is within $HOME, it uses far too much space on the server and slower because it is not local. I am sure that many other networks are configured this way. i

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

pushing out. 0.9.2 is done. (querying for this string will get you the list of the 0.9.2 bugs I moved to 0.9.3)

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

Doesn't look like this is getting fixed before the freeze tomorrow night. Pushing out a milestone. Please correct if I'm mistaken.

looks like this is in slide mode. unsetting target milestone until someone can put a good one on.

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

Can this now be targetted, since bug 78480 has been fixed.

Forgive my naive question, but wouldn't it be much easier if Mozilla supports symlink? This way, I create a symlink "Cache" that I can point to anywhere else. I know this doesn't work with 2001092508 on MacOS 9.1; IE 5 seems to be happy with an alias instead of a real folder.

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

It would seem much better if the UI would support selection, rather than relying on users to perform file system tricks most don't know are even possible. In fact, I already uuse symlinks under Linux to work around this bug, which is still annoying.

huh??? but i don't even have symlinks to play around with in windows... ;-p

I do, but that's w2k for me :-)

Comment on attachment 12729 [details] First stab at a patch to use cache subdirectory The backend work for this is completed. All we need to do is hook it up in the prefs panel.

OK, so I grabbed patch maker and had a go at this bugger. Part of it was already done in the js sources, I finished it off. Try it out people, give it a good whipping. I am awfully new at this so please review with extra caution.

I only tested this on Linux, it works here. Thanks to bbaetz and mondo for helping me out on IRC. You should really try playing around with xul and patchmaker, it's very simple!

+<!ENTITY diskCacheFolderExplanation "A subfolder named Cache will be created in the folder you specify."> Are you sure that that is correct? My pref in prefs.js includes the Cache dir, which seems more sensible. This won't take effect until restart, though, as I mentioned to you on IRC. I don't know if theres a bug for that, or if its fixable easily. gordon: theres nothing in the cache code which will get confused at runtime if the pref value != the value read at profile startup, is there?

Yes, I am sure. That's why the pref is called parent_directory. Have a look at the discussion in bug 78480 which is about setting the cache directory via prefs. I agree I should mention that a restart is necessary for the change to take effect. BTW, do I assign such a bug over to me, since I have come up with a patch or should it be assigned to someone with CVS account?

2001111108 win2Kpro-sp2 With my prefs set to: user_pref("browser.cache.check_doc_frequency", 0); user_pref("browser.cache.directory", "D:\\cache\\moz\\"); user_pref("browser.cache.disk.capacity", 10240); user_pref("browser.cache.disk.directory", "D:\\cache\\moz\\"); user_pref("browser.cache.disk.enable", false); user_pref("browser.cache.disk.parent_directory", "D:\\cache\\moz\\"); user_pref("browser.cache.disk_cache_size", 10240); user_pref("browser.cache.memory.capacity", 10240); Now *nothing* is getting written to the disk cache. I realize that some of these settings are historic, what's interesting is that I have the UI set to check "once per session" but the prefs here don't reflect that.

Lohphat, what are you trying to say? You have set the pref user_pref("browser.cache.disk.enable", false); which disables the disk cache... user_pref("browser.cache.disk.parent_directory", "D:\\cache\\moz\\"); is what you need to change the location of the disk cache directory. It will then be stored in D:\cache\moz\Cache. Under Preferences -> Debug -> Networking is a checkbox to enable/disable the disk cache.

I never set/added user_pref("browser.cache.disk.enable", false); and there's no UI for it. I can change it, but why also doesn't explain why user_pref("browser.cache.check_doc_frequency", 0); is "0" when it is set for "Once per session".

The UI for user_pref("browser.cache.disk.enable", false); is under Preferences -> Debug -> Networking -> "Enable Disk Cache" Your prefs.js file appears to be hosed. Some of your settings are historic. Try creating a fresh profile and tweaking the preferences to your liking. I'm afraid this is a little bit offtopic. Feel free to get in touch with me by mail if you have further questions.

Don asked on irc about this bug. I pointed out mac issues from memory, but i just went to reread this bug and remembered the reason that i could point out the mac issues from memory: Comment #37. back to the patch at hand +<!ENTITY diskCacheFolderExplanation "A subfolder named Cache will be created in the folder you specify."> I don't like this string. It's wrong. Something like "Use the Cache folder in this folder" would be less wrong (although it's still awkward English). As for the mac issue, if (navigator.platform.indexOf("Mac")>=0){/*it's probably a mac, so stick some code here to make the field readonly*/}. More later

Timeless, I added you to the cc list, I hope you don't mind.. I am Don. For simplicity's sake I will make that textfield readonly on all platforms. Does anyone disagree? About the explanatory string: What's wrong with "A subfolder named Cache will be created in the folder you specify." That's what happens. The wording could be better, though. What about: "Cache files will be stored in a subfolder named "Cache" of the directory you specify. Restart Mozilla for changes to take effect."

Comment on attachment 57318 [details] [diff] [review] first stab at a fix >+++ xpfe/components/./prefwindow/resources/content/pref-cache.xul Sat Nov 10 02:25:42 2001 >@@ -29,7 +29,7 @@ [snip] >+ <textbox id="browserCacheDiskCacheFolder" preftype="char" >+ prefstring="browser.cache.disk.parent_directory" prefattribute="value"/> (a) Users should not be able to edit this box. (b) Maybe you meant preftype="string"? The unrecognized preftype="char" would default to behavior as though the preftype is a string: <http://lxr.mozilla.org/seamonkey/source/xpfe/components/prefwindow/resources/c﷒0﷓> (c) I believe this solution needs more work to store a persistent descriptor rather than a unicode path because this method storing a string path causes bugs on the mac. [snip] >+++ xpfe/components/./prefwindow/resources/locale/en-US/pref-cache.dtd Sat Nov 10 02:26:57 2001 >@@ -15,6 +15,9 @@ > <!ENTITY clearMemCache.accesskey "c"> > <!ENTITY clearDiskCache.label "Clear Disk Cache"> > <!ENTITY clearDiskCache.accesskey "l"> >+<!ENTITY chooseDiskCacheFolder.label "Choose Folder"> >+<!ENTITY chooseDiskCacheFolder.accesskey "c"> This accesskey is used 4 lines above. Choose another.

I am not sure about having a readonly field for all platforms. It sure prevents people from typing in nonexistent directories, but some of us sure would prefer to just type away at it. What do you think, is it better to have this nerd-friendly or fool-proof?

Diego: Well, just ask yourself "Do more nerds use Mozilla, or do more fools use Mozilla?" *ducks*

OK, then we settle for a readonly field, as pointed out by Samir Gehani. Corrected the preftype and fixed the duplicate accesskey. This should now work on Linux and Windows. I do not know the solution for MacOS 9. Can somebody on a Mac try this patch out?

Jason, I am taking over this bug, if you do not mind. Samir, I added you to the CC list, please have a look at the revised patch.

I think fool-proof is more important in this case. We'll have less problems in the long run. If we can make it nerd-friendly as well, then great. Diego, can you submit a patch with diff -u, I'm having trouble applying this on a Mac.

Gordon, please try this version, I could apply it with patch. I am using patch maker to create the patches, however my version 0.73 has some bugs, does anybody have a newer version? The case about the editable field appears settled. We should go with readonly.

Diego, You will need to query the nsILocalFile interface of the |file| object returned by nsIFilePicker. Then extract the persistentDescriptor which is a string. Store this descriptor in the pref but display the path. You will also need to make sure that the backend is now reading the pref in as a persistentDescriptor and initializing the disk cache folder nsIFile representation from this string, in particular for the Mac. Since this feature is unexposed I don't think the format change should introduce any backwards compatibility issues. I'll defer to Gordon on the latter. Something like: var localFile = fp.file.QueryInterface(Components.interfaces.nsILocalFile); var storable = localFile.persistentDescriptor; var viewable = fp.file.unicodePath; Now |storable| should have the value you can set into the pref. And |viewable| should be the string value you can show in the <textbox/>. See <http://lxr.mozilla.org/seamonkey/source/xpcom/io/nsILocalFile.idl#107>. For more about the need for the QueryInterface() method read up on XPCOM: <http://www.mozilla.org/projects/xpcom/>

We need a XUL widget that is like an <input type="file">, which shows a read-only path on Mac, but an editable path on other platforms, because we use this in several places in the UI (e.g. local mail folder settings). I'd encourage you guys to come up with a general solution to this problem, not just hack in read- onlyness in this case.

Samir, thanks for the explanation. I will read up on this and post an updated patch soon. Simon, it's been decided that this should be readonly on all platforms, so I do not consider it a hack to set the textbox readonly. If such a widget would be useful a separate bug should be filed on it and my patch could be modified to use something like that later on.

Just for the record: I went to have a look at Netscape 4.7x on a Mac and the cache directory field is not editable. You have to change it via a filepicker.

Now I do all the extra gymnastics that are necessary to make this work on the mac. The preferences are set through the backend as pointed out by Samir. It works here on my machine under Linux and I can try it out under Windows, but I have no means to test this on a Mac. So if you are on a Mac, please give this a whirl. Enjoy.

Let's see if I manage to get this into the next milestone.. I have a question about some of the code I "inherited" (part of it was already in the file): var prefutilitiesBundle = document.getElementById("bundle_prefutilities"); var title = prefutilitiesBundle.getString("cachefolder"); What are the prefutilities? They are defined in a prefutilities.properties file, but I cannot make much sense of it.

Just tried it on 2001112603 under Windows, works. Desperately seeking Mac testers...

I'm happy to try it, except I don't know how to apply the patch...

Mozilla/5.0 (Macintosh; U; PPC; en-US; rv:0.9.4+) Gecko/20010921 works with one caveat. it looks like you are expecting browser.cache.disk.parent_directory to be a string--it's not. you are going to have to do something trickier than: <textbox id="browserCacheDiskCacheFolder" preftype="string" readonly="true" prefstring="browser.cache.disk.parent_directory" prefattribute="value"/>

This one might be easier to install with patch maker.

Julian: You have two possibilites: - Check out the sources from CVS, apply my patch and build. - Let patch maker do it for you. There are detailed instructions at http://www.mozilla.org/hacking/patch-maker/ Thanks for your willingness to try this out. Contact me by email if you need furthere assistance.

nnooiissee: Thanks for testing my patch. You are correct, I do expect browser.cache.disk.parent_directory to be a string. That is why I have two different representations var storable = localFile.persistentDescriptor; var viewable = fp.file.unicodePath; for displaying and storing the pref and fp.file.unicodePath returns a string. Am I overlooking something? You say it works. What exactly is the problem you are experiencing?

Hmm...I finally got the patch to work correctly, after Patch Maker nuked Mozilla several times ;-( Anyway, it seems to work as advertized, except that the cache directory path string gets messed up after I close the preferences dialog box. It still saves cache to the correct directory, however.

okay, for example here is a line from my prefs.js (i don't have your patch installed right now, but browser.cache.disk.parent_directory would be a simmilar string). user_pref("browser.download.dir", "AAAAAADQAAIAAQEvAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAS0gAAf////8HRGVza3RvcAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAGUAAAAAAAAAAAAAAAAP////8AAAAgY3UAAAAAAAAAAAAAAAIAFi86VXNlcnM6Y2FzZXk6RGVza3RvcDoADgAQAAcARABlAHMAawB0AG8AcAAPAAQAAQAv//8AAA=="); i assume that is a nsIFileSpec as ascii. the xul expects that pref to be a path (which i think was the original problem), only the js gets it right. exact description of what happens: open prefs->advanced->cache. text box empty. set cache dir. text box shows path. close prefs. open prefs->advanced->cache. text box empty. set cache dir (even to the same directory it is already set to). text box shows path. also of note as a bug (but not pertaining to this bug) the "Additional Comments:" text box has been acting wierd since i pasted in that long line. =( i thought most of that was fixed.

OK, thanks for the details, now I know what the problem is. I display a humanreadable string at first, but store the filesystem object in the pref and the next time I read that pref it is garbage. How can I solve this problem? Right now I have two ideas: - I could save the string path in a second pref and read that in for display, like browser.cache.disk.parent_directory.humanreadable. Anyone have a better (shorter) name? - I could hook up a new javascript function into the prefs panel that converts the filesystem object into a string for display. No idea how to implement this at the moment, I probably have to weave some observes= magic into the textbox. Clever hints anyone?

Don't store an extra pref for human readable. It raises issues of keeping the two prefs in sync, and which is the "real" pref. On the mac we don't store pathnames in prefs, we store hexified aliases. We need a function that can convert those to human readable form. I would expect we already have something like that floating around somewhere in the source.

This is the code you used to set the pref. + var localFile = fp.file.QueryInterface(Components.interfaces.nsILocalFile); + var storable = localFile.persistentDescriptor; + var viewable = fp.file.unicodePath; + var folderField = document.getElementById("browserCacheDiskCacheFolder"); + folderField.value = viewable; + + pref.setCharPref("browser.cache.disk.parent_directory", storable); use the same code sequence in reverse to get things back. This means you'll need to retrieve the pref manually instead + <data id="browserDiskCacheFolder" + prefstring="browser.cache.disk.parent_directory" + preftype="string" prefattribute="value"/> i think this will work. so onLoad you'll set + <textbox id="browserCacheDiskCacheFolder" readonly="true"/> using something like: var localFile = fp.file.QueryInterface(Components.interfaces.nsILocalFile); localFile.persistentDescriptor = document.getElementById("browserDiskCacheFolder").value; document.getElementById("browserCacheDiskCacheFolder").value = fp.file.unicodePath; good luck

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

The path textfield should no longer be empty on the Mac. I now extract the nsIFile path from the pref via a javascript function, so that it should work even if the pref is not a string. Mac testers wanted! Implemented on Linux, works here. The diff is against 2001120806.

The chromediff version as produced by patch maker for your greater convenience.

Zach, probably you can take a look at it.Thanks

Works fine on Windows ME, build 2001120808.

i patched by hand over dirty files, so i am not 100% sure, but it looks to me like it still does not work. afaict, you never call startUp().

Thanks for testing, nnooiissee. Can you give a detailed explanation of what you did and what went wrong? Startup() gets called automatically, I copied that over from other parts of the prefs panel. It works on Linux and Windows, so there should be some truth to it. Do you see a path in the textfield the first time you open the prefs panel after patching? Did you change the cache directory? Before you do that, you will probably not have the pref set and the field may be empty.

just ran this under Mozilla/5.0 (Macintosh; U; PPC Mac OS X; en-US; rv:0.9.5) Gecko/20011015 and got: Error: uncaught exception: [Exception... "Component returned failure code: 0x80520001 (NS_ERROR_FILE_UNRECOGNIZED_PATH) [nsILocalFile.initWithPath]" nsresult: "0x80520001 (NS_ERROR_FILE_UNRECOGNIZED_PATH)" location: "JS frame :: chrome://communicator/content/pref/pref-cache.xul :: getDiskCachePath :: line 45" data: no] line 45 is the second line of: var pref = Components.classes["@mozilla.org/preferences-service;1"] .getService(Components.interfaces.nsIPrefBranch); other than that behavior has not changed since the last patch. i will try and retest under mac os 9 later.

Now I use setComplexValue to complement getComplexValue. Some cleanup to address Samir Gehanis suggestions.

OK, it runs on Linux and Windows ME. Mac testers, please have a go at this one. The diff is against the latest nightly. You might see a few warnings like patch -p0 < ../../pm/mac.chromediff patching file `content/communicator/pref/pref-cache.js' patching file `content/communicator/pref/pref-cache.xul' Hunk #1 succeeded at 19 with fuzz 1. Hunk #3 succeeded at 95 (offset -2 lines). patching file `locale/en-US/communicator/pref/pref-cache.dtd' don't worry, it should work anyway. Or just update your nightly/tree. Thanks.

+ var storedPath = pref.getCharPref("browser.cache.disk.parent_directory"); + file.initWithPath(storedPath); This is wrong. "browser.cache.disk.parent_directory" is stored as a persistent descriptor http://lxr.mozilla.org/seamonkey/source/netwerk/cache/src/nsCacheService.cpp#300 so you need to use getComplexValue() to get it.

I tried to apply the patch under MacOS 9, using Patch Maker. But for some reason it says there's nothing to patch. So I tried using the patch tool under MacOS X to apply the patch (onto the MacOS 9 version), but patch returns with this error: can't find file to patch at input line 3 Perhaps you used the wrong -p or --strip option? The text leading up to this was: -------------------------- |--- xpfe/components/./prefwindow/resources/content/pref-cache.js.bak Mon Dec 10 02:47:36 2001 |+++ xpfe/components/./prefwindow/resources/content/pref-cache.js Mon Dec 10 02:47:36 2001 -------------------------- Any hints?

PatchMaker patches on Mac are pretty much broken.

Installment 42 in my quest to produce a patch that will someday work on MacOS. While my ability to hide bugs in simple code fragments continues to amaze there is hope that I might have finally succeeded considering the fact that every line of code has been revised at least twice by now... sfraser: The problem you pointed out is no more, I treat the pref as complex value everywhere now. MAC TESTERS WANTED, as usual, it works on Linux and Windows.

Julian, you might want to try this version of the patch and see if you have more luck. If that does not help, I have no clue.

Tried attachment 61178 [details] [diff] [review] with Patch Maker in MacOS 9 with 2001121008, patched without a hitch. The garbage cache path name is now replaced by the correct path, and nothing else seems to be broken so far. P.S. Sorry about my comment #98 not wrapping correctly; it seems that if I post comments with iCab, that's what happens. I'll use Navigator 4.x next time...

Comment on attachment 61178 [details] [diff] [review] chromediff version >--- content/communicator/pref/pref-cache.js.bak Mon Dec 10 23:00:41 2001 >+++ content/communicator/pref/pref-cache.js Mon Dec 10 23:00:41 2001 >@@ -3,23 +3,27 @@ >+ function Startup() >+ { >+ getDiskCachePath(); >+ } Inline the getDiskCachePath() code into Startup() and eliminate the extra function call. >+ function getDiskCachePath() >+ { >+ var pref = Components.classes["@mozilla.org/preferences-service;1"] >+ .getService(Components.interfaces.nsIPrefBranch); >+ var path = Components.classes["@mozilla.org/file/local;1"] >+ .createInstance(Components.interfaces.nsILocalFile); >+ >+ path = pref.getComplexValue("browser.cache.disk.parent_directory", Components.interfaces.nsILocalFile); Use the const you declared above for nsILocalFile (just like you have in your setComplexValue() usage). With those two changes r=sgehani.

The function is now inlined into Startup() and uses const defined in the js file, as suggested by Samir Gehani.

In case somebody wants to apply this with patch maker, here is the chromediff version of the latest patch revision.

i tried attachment 61237 [details] [diff] [review] with Mozilla/5.0 (Macintosh; U; PPC Mac OS X; en-US; rv:0.9.6) Gecko/20011120 and it works. the only issue i have is with the small size of the text box... and a reset button would be very nice.

nnooiissee: Great to hear that it finally works, thanks for trying. Adding a reset button sounds like a neat idea. Maybe I can get this checked in for 0.9.7 and add the button later on.. Another point: What about the help files? Should I update them or is there a separate team for documentation? I had a look at them, they are just plain html. Is that all or do they get generated from somewhere?

Bumping one milestone ahead while waiting for sr=... What do you guys think, should we have a bigger textbox and a reset button? I could implement that in almost no time by now.

I moved the Startup function into the js file for clearness, I was not aware I could use the const defined in the js file in the xul file until Samir pointed it out. There is no functional change, this is ready for checkin.

This adds two lines to the help to explain the newly created fields.

Comment on attachment 64664 [details] [diff] [review] version 1.0 sr=sfraser

Hallelujah, I have super-review! I can tell you guys, it's a warm and fuzzy feeling. I have no CVS privileges, so could some of you guys please check this in for me? Any chance this might go into 0.9.8??

Not for 0.9.8, but I'll check it in as soon as the tree opens for 0.9.9. Reassigning. Thanks for the work!

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

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

Gordon, the tree has reopened.. How about checking this in?

I'll try and get to it today.

Okay, I've checked in the patch. However, I noticed on my Mac that the cache directory text field was not filled in when I initially opened the pref dialog. When I changed the cache parent directory, the text was displayed properly.

That's because the pref is not set initially. I fill the textfield from the pref in Startup(). If it is not set the textfield remains blank.. Do you think this should be initialized with a default value? Did you check in the docs, too?

Duh! Of course. The disk cache device could set the pref to the default location if the pref doesn't exist. What do people think? It seems strange to have that text field blank. Also, the window seems a bit too small for some of the buttons, at least on the mac. A new bug I suppose, but who should get it? Can we make the window bigger?

> Can we make the window bigger? No; the prefs window ain't getting any bigger. You'll have to rejig your items.

Netscape 4.x does not have the text field blank. Admittedly, it does look a bit strange to have it empty.

It shoould not appear blank. If there is no pref yet, then the code should display the path to the default cache directory location.

Okay, I'll have the disk cache set the default location when it's initialized if it the preference doesn't exist.

gordon, Here is JS console error in build 2-8-03 w2k: Error: uncaught exception: [Exception... "Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIPrefBranch.getComplexValue]" nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)" location: "JS frame :: chrome://communicator/content/pref/pref-cache.js :: Startup :: line 15" data: no]

two problems: 1) choose a folder and then press Cancel in the prefs panel. Now you would expect that the folder you select didn't get set. But it does....! I just set the folder to X: and had to edit the prefs.js file manually to get it back to the default.... 2) really really miss a reset to default button

> two problems: > > 1) choose a folder and then press Cancel in the prefs panel. Now you would > expect that the folder you select didn't get set. But it does....! > I just set the folder to X: and had to edit the prefs.js file manually to get > it back to the default.... Just go to the cookies panel, delete a cookie, say OK and press Cancel in the prefs panel. The cookie is gone.. Isn't that the same problem? It would be solved, of course, by a reset button. FWIW 4.x does not have a reset button. > 2) really really miss a reset to default button Didn't I ask about this in comment #108 two months ago? Shall I implement this? Let's hear some comments first..

Gordon, what about the documentation patch? Does it need separate review?

I've just filed bug 124635 that could solve the reset to default cache issue and the folder cropping.

Can we change the pref from parent_directory to directory? The UI would still choose the parent directory, and add Cache/ before writing to the pref, but this way an advanced use could edit prefs.js manually so it doesn't have to be named "Cache" (which is awful design, by the way).

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

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

> Just go to the cookies panel, delete a cookie, say OK and press Cancel in the > prefs panel. The cookie is gone.. Isn't that the same problem? It would be > solved, of course, by a reset button. FWIW 4.x does not have a reset button. Yes, same bug, but the correct solution is making cancel do what it does everywhere else, i.e. provide a safe way out without making changes. This preference is not different from all the other prefs in the prefs dialog; it should not take action immediately. (Doesn't seem like this should have passed super review with that flaw...?)

Also, I read through the bug with regards to making the field editable (per request in bug 124635) but I didn't see any solid reasoning, except ease of implementation. Since we allow users to edit paths elsewhere in the app, and sine Windows lets people do it, such a decision really doesn't make sense in the long run. I agree with Simon that we should be using xbl here to facilitate use of the common textbox/path button combination. The field could be smart about being readonly on mac, handle path validation (the issue that seems to be causing the most trouble here), and so forth, all in a consistent manner. But I'll reopen bug 124635 to handle that. By the way, Diego, I don't mean to be knocking your patch -- it's very cool ;)

I'll whip up a reset button in the next few days and see if I can improve the logic so that Cancel works as expected. I just checked and 4.x gets this right. > By the way, Diego, I don't mean to be knocking your patch -- it's very cool Thanks, no offense taken.

Comment on attachment 64722 [details] [diff] [review] update the docs r=bzbarsky

Comment on attachment 64722 [details] [diff] [review] update the docs Is it OK to say "Mozilla" in the documentation, since it might be reused for Netscape builds?

If you look around the file you will find dozens of instances of the word "Mozilla", so the answer has to be yes. This is a branding nightmare, of course, but that must be solved in another bug.

If Ian gives an explicit "ok" on the branding issue, then you have my sr. I think Ian should check in the docs changes.

WORKSFORME unter Windows 98 SE with 0.9.9 (2002031104). Does not work for me unter Linux (and there it is needed most for multi-user-systems with quotas). pi

bugzilla@piology.org, please provide some detailed steps to reproduce, I have no problems with Linux 0.9.9 and my current CVS build..

The status of this bug is unclear. It seems to have been fixed on trunk, but this bug is still marked "new." Was there a check-in?

The bug has been fixed. It is still open waiting for someone (me probably) to come up with a reset button to return the pref to its default state.

OK, I finally got around to coding the reset button. We also have a larger textfield now, so this is looking much nicer. Please test and review this. I made it on Linux, but this does not really have any OS specific pieces, so it should work everywhere. After resetting the pref I just call the function Startup() to get the textfield updated with the new value of the pref. This is a bit of a hack. I believe there should be a function updateTextfield() (or whatever) that gets called both from Startup() and from prefCacheResetFolder(). Otherwise I believe it should be OK. Gordon, what do you think of it? On a more general note: This has been far too difficult so far. We have been implementing a lot of fancy stuff that should be handled by the backend IMHO. It is very easy to create and control string or boolean preferences in the prefs panel because the backend implements putting prefstring="somepref" in a XUL element. This should be possible for complex prefs like paths, too. Do you agree? Shall I open a new backend bug for this?

Somebody might wish to apply this with patch maker...

I have to correct myself. It *does* work under Linux. For some reason I have not received posts here, so I missed some discussion:-( pi

>+<!ENTITY resetDiskCacheFolder.label "Restore Default..."> I don't think this should have the "..." at the end - only menuitems/buttons which require user input before completion should have them.

Included cbiesinger's suggestions. Marked all other patches obsolete, as they are either checked in or obsolete.

Changing nsbeta1+ [adt3] bugs to nsbeta1- on behalf of the adt. If you have any questions about this, please email adt@netscape.com. You can search for "changing adt3 bugs" to quickly find and delete these bug mails.

Changing nsbeta1+ [adt3] bugs to nsbeta1- on behalf of the adt. If you have any questions about this, please email adt@netscape.com. You can search for "changing adt3 bugs" to quickly find and delete these bug mails.

Comment on attachment 78128 [details] [diff] [review] remove the dots This patch looks fine to me.. Although I haven't tested it, and don't have the power to mark it as reviewed :)

The patch should be OK, but I am getting tired of lobbying for reviews. I just get ignored. This patch is now 4 months (!) old. For external developers the review process stinks. If you are not lunch buddies with some of the Netscape folks it is impossible to get reviews. I am mightily frustrated by this and have already left Mozilla behind for other (more responsive) projects...

diego: i'm sorry to hear that working with mozilla has been frustrating for you... in your case, i'm not exactly sure who you requested a code review from. if you tried contacting gordon, then chances are you haven't gotten a timely response because he has been on sabbatical / vacation for most of the summer. moreover, your code only effects UI, which is something networking engineers (including gordon and myself) generally stay away from. there are other people responsible for mozilla's UI. you might try asking folks on IRC to help you locate a suitable reviewer.

i'm sorry, but you can't commit this patch atm. see bug 125567. i think that committing this patch would result in the preference panel not fitting horizontally or vertically.

timeless: I think my patch will fix bug 125567 at no additional cost, just see my post there and the screen shot I attached. http://bugzilla.mozilla.org/show_bug.cgi?id=125567#c17

Darin: I tried Samir Gehani because he reviewed my first patch in this bug, but he said prefs now belong to Ben Goodger who never answered my mails. Then I tried Gordon, because he modified and checked in my first patch in this bug. We have exchanged a few mails after he was back from sabbatical and he seemed willing to do it, but this was a month ago. Besides this touches file I/O in addition to the UI and I had a few questions about his code, so Gordon is my man... Why don't you do me an enormous favor, walk over to his cubicle and talk him into giving this a quick review? Threats of deadly force are not forbidden either ;-) Honestly, I could need some help here, the worst part is that sr= is even harder to come by...

like i said, gordon will not be back until the end of the month (or perhaps early-sept). so, someone else will need to review this patch unless you're ok waiting for gordon to return. i'll sr= it once it's been reviewed :-) try to find someone who works on the UE team... have you considered asking mpt?

Darin: Thanks, I thought Gordon was just back from sabbatical. Christopher Aillon will try to review this now, mpt sounds like a good second choice, too, since he is involved with bug 125567.

Comment on attachment 78128 [details] [diff] [review] remove the dots So (timeless, diego, et.al) does this patch make us *worse* than we are wrt how much can fit in the panels? If not, I see no problem in letting this go in and attempt to fix that in the other bug. Otherwise, please make sure that you don't "regress" that bug even further. Aside from that issue, I do have a few comments on this patch. >--- xpfe/components/prefwindow/resources/content/pref-cache.xul.bak Sun Mar 24 13:32:17 2002 >+++ xpfe/components/prefwindow/resources/content/pref-cache.xul Sun Mar 24 14:10:54 2002 ... >+ <hbox align="center"> >+ <label value="&diskCacheFolder.label;"/> >+ <textbox id="browserCacheDiskCacheFolder" readonly="true" flex="1"/> >+ </hbox> >+ <hbox align="center" pack="end"> >+ <button label="&resetDiskCacheFolder.label;" accesskey="&resetDiskCacheFolder.accesskey;" >+ oncommand="prefCacheResetFolder();" id="resetDiskCacheFolder"/> >+ <button label="&chooseDiskCacheFolder.label;" accesskey="&chooseDiskCacheFolder.accesskey;" >+ oncommand="prefCacheSelectFolder();" id="chooseDiskCacheFolder"/> Prefer the id earlier in the attribute list, as you've done above. It makes it easier to quickly scan for that id. >+ </hbox> >+ </vbox> > > <description>&diskCacheFolderExplanation;</description> > >- </rows> >- </grid> >+ This extra line is unneeded. >--- xpfe/components/prefwindow/resources/locale/en-US/pref-cache.dtd.bak Sun Mar 24 13:37:11 2002 >+++ xpfe/components/prefwindow/resources/locale/en-US/pref-cache.dtd Sun Apr 7 23:15:38 2002 It would be nice if this file could have the entity values line up... >--- xpfe/components/prefwindow/resources/content/pref-cache.js.bak Sun Mar 24 14:08:02 2002 >+++ xpfe/components/prefwindow/resources/content/pref-cache.js Sun Mar 24 17:12:00 2002 >@@ -1,11 +1,14 @@ > /* -*- Mode: Java; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- > * >+ * Contributor(s): >+ * Diego Biurrun <diego@biurrun.de> > */ > > const nsIFilePicker = Components.interfaces.nsIFilePicker; > const nsILocalFile = Components.interfaces.nsILocalFile; > const nsIFile = Components.interfaces.nsIFile; > const nsIProperties = Components.interfaces.nsIProperties; >+const nsIPrefBranch = Components.interfaces.nsIPrefBranch; > const kCacheParentDirPref = "browser.cache.disk.parent_directory"; > > function Startup() >@@ -15,7 +18,7 @@ > try > { > pref = Components.classes["@mozilla.org/preferences-service;1"] >- .getService(Components.interfaces.nsIPrefBranch); >+ .getService(nsIPrefBranch); Please fix the alignment here. .getService should align with .classes > path = pref.getComplexValue(kCacheParentDirPref, nsILocalFile); > } > catch (ex) >@@ -37,10 +40,10 @@ > // now remember the new assumption > if (pref) > pref.setComplexValue(kCacheParentDirPref, nsILocalFile, path); >- } >- catch (ex) >- { >- } >+ } >+ catch (ex) >+ { >+ } > } > > // if both pref and dir svc fail leave this field blank else show path >@@ -50,12 +53,25 @@ > } > > >+function prefCacheResetFolder() >+{ >+ var pref = null; >+ pref = Components.classes["@mozilla.org/preferences-service;1"] >+ .getService(nsIPrefBranch); Please make .getService align with .classes. >+ >+ pref.clearUserPref(kCacheParentDirPref); >+ >+ // Our changes need to get displayed >+ Startup(); Startup is a special function in prefs. It is called by the pref window itself on load and we should not add a dependency on it by other panel specific functions such as this. If you need the same stuff to happen here, then please split out the stuff in Startup() to a separate function and please have both Startup() and this call into that. >+} >+ >+ > function prefCacheSelectFolder() > { > var fp = Components.classes["@mozilla.org/filepicker;1"] > .createInstance(nsIFilePicker); > var pref = Components.classes["@mozilla.org/preferences-service;1"] >- .getService(Components.interfaces.nsIPrefBranch); >+ .getService(nsIPrefBranch); More .getService alignment fun :-) > var prefutilitiesBundle = document.getElementById("bundle_prefutilities"); > var title = prefutilitiesBundle.getString("cachefolder"); > >@@ -78,7 +94,7 @@ > var viewable = fp.file.unicodePath; > var folderField = document.getElementById("browserCacheDiskCacheFolder"); > folderField.value = viewable; >- pref.setComplexValue(kCacheParentDirPref, nsILocalFile, localFile) >+ pref.setComplexValue(kCacheParentDirPref, nsILocalFile, localFile); > } > } >

it turns out that we have negative available space in both directions on mac in modern, see attachment 95103 [details]. So this is definitely blocked by the other bug.

I don't agree. This patch will add some height to the panel and take away some width. Let's get this in first and then fix the other one for good, as doing this after the other bug might regress it.

This addresses all the comments caillon made on my patch. Alignment is fixed everywhere, id is now the first attribute of all buttons and Startup was renamed prefDisplay which is called where necessary. Caillon please have a look at this and give r= or make further comments. Thanks

Diego, you say that fixing this after fixing the other bug may regress that. The logic I get out of that is that your patch causes the panel to take up more space. You say that this removes width, and I say yea to that. But then you confirm my first statement by saying it adds to height and I don't want that to happen if space is an issue, which it is. This is not a good trade-off IMO because you have only one direction which is overflowing before. To add a second direction which overflows the panel will just make this look uglier. So, while your patch is technically sound, I will withhold r= until we figure out a solution here. We may need to move some items out of this panel, or something. Patrice, this looks like a bug which we could use your help on.

Caillon, have a look at bug 125567 again. The prefs panel is overflowing in both directions. My patch should fix the horizontal aspect and make the vertical aspect worse, so the situation will not really change. The real problem is that my patch might regress bug 125567 if it is checked in later and I have no way to check this as the problem described in bug 125567 is Mac specific. bug 125567 is about changing the layout of the cache prefs panel and this patch will add another button to that panel. If the layout is going to be redesigned it should be done once taking into account all elements of the panel. In other words, if we fix bug 125567 first, we may have to fix it twice. I therefore suggest that you give r=, darin gives sr=, this patch goes in first and then I will help fixing bug 125567 for good. We are in the pre alpha stage at the moment so I guess we can live with some temporary breakage.

Suresh, can you take a look at this? Thanks.

adt: nsbeta1-

This bug has been open for too long. The pref has been added, it works. We've already had other bugs that modify what has already been checked in. Can't you make a bug for each additional change, and mark this fixed?

Is this still an issue? Pref and UI are implemented.

This is no longer an issue. A button that restores default values does not seem to be necessary any longer and the panel does not overflow anymore, so time to resolve this bug ---> RESOLVED FIXED

V: Mac OS X, Mozilla 1.6f.

V/fixed: mozilla 1.7.2 / win xp

Verified on Linux, Mozilla 1.8a3, marking VERIFIED.