Closed Bug 46490 Opened 24 years ago Closed 21 years ago

User selected Cache directory - imp in prefs needed.


(Core :: Networking: Cache, defect, P1)






(Reporter: jce2, Assigned: skasinathan)


(Blocks 1 open bug)


(Keywords: polish, Whiteboard: [cache])


(1 file, 19 obsolete files)

9.50 KB, patch
Details | Diff | Splinter Review
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
Hardware: PC → All
tever/gagan, should this go to Networking?
QA Contact: sairuh → tever
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-
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£
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.
Keywords: nsbeta2, relnote2
Putting on [nsbeta2-] radar.  tever, please give verah a release note for PR2.
Whiteboard: [nsbeta2-]
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
Keywords: helpwanted
Whiteboard: [nsbeta2-] → [nsbeta2-][nsbeta3-]
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.
Keywords: nsbeta3
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
Keywords: patch
*** 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. :)
Keywords: 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
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).
Keywords: review
over to jce2, and removing helpwanted (and other stale kw's). thanks for taking
Assignee: matt → jce2
Whiteboard: [nsbeta2-][nsbeta3-] → [nsbeta3-]
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.
Keywords: mozilla0.9
Target Milestone: --- → mozilla0.9
Here are comments related to this option being missing from NS6 builds (from

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.
Keywords: nsmac2
I should be able to do something with this over Christmas.

Removed nsbeta3- from Status whiteboard.
Whiteboard: [nsbeta3-]
Whiteboard: [cache]
Retargeting bug..Now that my professional product has gone into beta, I should
be able to bear down and fix this.
Target Milestone: mozilla0.9 → mozilla0.9.1
Is this really going in for 0.9.1?  If so, great!  Here's the issue from bug 

<quote from bug 45394>
The cache pref panel contains a textfield that contains the 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 

          <textfield id="browserCacheDirectory" flex="1" pref="true" 

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 

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
Target Milestone: mozilla0.9.1 → mozilla0.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.

*** 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)
Target Milestone: mozilla0.9.2 → mozilla0.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.
Target Milestone: mozilla0.9.3 → mozilla0.9.4
Target Milestone: mozilla0.9.4 → ---
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...

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.
Attachment #12729 - Attachment is obsolete: true
Keywords: patch4xp, mozilla1.0, polish
Attached patch first stab at a fix (obsolete) — Splinter Review
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!
Keywords: patch, review
+<!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("", "D:\\cache\\moz\\");
user_pref("browser.cache.disk.capacity", 10240);
user_pref("", "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 @@


>+            <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:
(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.


>+++ 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.
Attachment #57318 - Flags: needs-work+
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?
Attachment #57318 - Attachment is obsolete: true
Diego: Well, just ask yourself "Do more nerds use Mozilla, or do more fools use

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?
Attachment #57935 - Attachment is obsolete: true
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.
Assignee: jce2 → diego
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.
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 <>.

For more about the need for the QueryInterface() method read up on 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.
Attachment #57938 - Attachment is obsolete: true
Attachment #57944 - Attachment is obsolete: true
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 file,
but I cannot make much sense of it.
Priority: P3 → P1
Target Milestone: --- → mozilla0.9.7
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

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

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

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 = 
+    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 = 
localFile.persistentDescriptor =    
document.getElementById("browserCacheDiskCacheFolder").value = 

good luck
*** Bug 114095 has been marked as a duplicate of this bug. ***
Blocks: 96876
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.
Attachment #59027 - Attachment is obsolete: true
Attached patch chromediff for convenience (obsolete) — Splinter Review
The chromediff version as produced by patch maker for your greater convenience.
Attachment #59633 - Attachment is obsolete: true
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[";1"]

other than that behavior has not changed since the last patch. i will try and
retest under mac os 9 later.
Attached patch cleaner version (obsolete) — Splinter Review
Now I use setComplexValue to complement getComplexValue.  Some cleanup to
address Samir Gehanis suggestions.
Attachment #60963 - Attachment is obsolete: true
Attachment #60964 - Attachment is obsolete: true
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 

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.
Attached patch end of an odyssey? (obsolete) — Splinter Review
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.
Attachment #61038 - Attachment is obsolete: true
Attached patch chromediff version (obsolete) — Splinter Review
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[";1"]
>+        .getService(Components.interfaces.nsIPrefBranch);
>+    var path = Components.classes[";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.
Attachment #61178 - Flags: review+
Attached patch includes Samirs suggestions (obsolete) — Splinter Review
The function is now inlined into Startup() and uses const defined in the js
file, as suggested by Samir Gehani.
Attachment #61177 - Attachment is obsolete: true
In case somebody wants to apply this with patch maker, here is the chromediff
version of the latest patch revision.
Attachment #61178 - Attachment is obsolete: true
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.
Target Milestone: mozilla0.9.7 → mozilla0.9.8
Keywords: relnote
Attached patch version 1.0 (obsolete) — Splinter Review
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.
Attachment #61236 - Attachment is obsolete: true
Attachment #61237 - Attachment is obsolete: true
Attached patch update the docs (obsolete) — Splinter Review
This adds two lines to the help to explain the newly created fields.
Comment on attachment 64664 [details] [diff] [review]
version 1.0

Attachment #64664 - Flags: superreview+
Hallelujah, I have super-review!  I can tell you guys, it's a warm and fuzzy

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!
Assignee: diego → gordon
Target Milestone: mozilla0.9.8 → mozilla0.9.9
*** Bug 120930 has been marked as a duplicate of this bug. ***
Keywords: nsbeta3nsenterprise
*** 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.
Keywords: nsbeta1+
Blocks: 127025
Comment on attachment 64722 [details] [diff] [review]
update the docs

Attachment #64722 - Flags: review+
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?
Blocks: 123569
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.
Target Milestone: mozilla0.9.9 → mozilla1.0
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, 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.
Attached patch reset button (obsolete) — Splinter Review
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:-(

Whiteboard: [cache] → [cache][adt3]
>+<!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.
Attached patch remove the dots (obsolete) — Splinter Review
Included cbiesinger's suggestions.

Marked all other patches obsolete, as they are either checked in or obsolete.
Attachment #64664 - Attachment is obsolete: true
Attachment #64722 - Attachment is obsolete: true
Attachment #75833 - Attachment is obsolete: true
Attachment #75834 - Attachment is obsolete: true
Changing nsbeta1+ [adt3] bugs to nsbeta1- on behalf of the adt.  If you have any
questions about this, please email  You can search for
"changing adt3 bugs" to quickly find and delete these bug mails.
Keywords: nsbeta1-
Changing nsbeta1+ [adt3] bugs to nsbeta1- on behalf of the adt.  If you have any
questions about this, please email  You can search for
"changing adt3 bugs" to quickly find and delete these bug mails.
Keywords: nsbeta1+
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.
Depends on: 125567
timeless:  I think my patch will fix bug 125567 at no additional cost, just see
my post there and the screen shot I attached.
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, 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   <>
>  */
> 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[";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[";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[";1"]
>                      .createInstance(nsIFilePicker);
>   var pref = Components.classes[";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);
>   }
> }
Attachment #78128 - Flags: needs-work+
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
Attachment #78128 - Attachment is obsolete: true
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.
Target Milestone: mozilla1.0 → ---
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.
Assignee: gordon → suresh
adt: nsbeta1-
Keywords: nsbeta1nsbeta1-
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?
Component: Preferences → Networking: Cache
QA Contact: tever → benc
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
Closed: 21 years ago
Resolution: --- → FIXED
V: Mac OS X, Mozilla 1.6f.
Whiteboard: [cache][adt3] → [cache] checklinux, checkwin
V/fixed: mozilla 1.7.2 / win xp
Whiteboard: [cache] checklinux, checkwin → [cache] checklinux
Verified on Linux, Mozilla 1.8a3, marking VERIFIED.
Whiteboard: [cache] checklinux → [cache]
You need to log in before you can comment on or make changes to this bug.