46490 - User selected Cache directory - imp in prefs needed.

Status: VERIFIED FIXED

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

Description

[Jason Eager]

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.

Comment 1

[Henrik Gemal]

This bug is/was related to 45394

Comment 2

[sairuh (rarely reading bugmail)]

tever/gagan, should this go to Networking?

Comment 3

[gordon]

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.

Comment 4

[Jason Eager]

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.

Comment 5

[Ben Goodger (use ben at mozilla dot org for email)]

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. 

Comment 6

[gordon]

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

Comment 7

[Peter "jag" Annema]

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

Comment 8

[gabriel]

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

Comment 9

[gabriel]

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

Comment 10

[gordon]

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

Comment 11

[sairuh (rarely reading bugmail)]

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

Comment 12

[leger]

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

Comment 13

[johng]

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

Comment 14

[gabriel]

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

Comment 15

[Jason Eager]

The only code that was removed was front-end code. The back end code is still there.

(at least, that's what I thought)

Comment 16

[Daniel Veditz [:dveditz]]

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

Comment 17

[gabriel]

Attachment: First stab at a patch to use cache subdirectory

Comment 18

[gabriel]

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

Comment 19

[meiering]

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

Comment 20

[Jason Eager]

Adding keyword patch.

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

Comment 21

[sairuh (rarely reading bugmail)]

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

Comment 22

[gabriel]

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.
 

Comment 23

[sairuh (rarely reading bugmail)]

adding review to get a review. :)

Comment 24

[gabriel]

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.

Comment 25

[Jason Eager]

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

Comment 26

[sairuh (rarely reading bugmail)]

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

Comment 27

[gabriel]

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

Comment 28

[Jason Eager]

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

Comment 29

[sairuh (rarely reading bugmail)]

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

Comment 30

[Matthew T (active 1999-2002)]

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?

Comment 31

[sairuh (rarely reading bugmail)]

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

Comment 32

[gordon]

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.

Comment 33

[Jason Eager]

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

Comment 34

[Kathleen :Brade]

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.

Comment 35

[Jason Eager]

I should be able to do something with this over Christmas.

Removed nsbeta3- from Status whiteboard.

Comment 36

[Jason Eager]

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

Comment 37

[gordon]

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.

Comment 38

[chris hofmann]

looks like this will have to wait for 0.9.2

Comment 39

[ian]

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

Comment 40

[Gilles Durys]

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

Comment 41

[Asa Dotzler [:asa]]

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)

Comment 42

[Andreas M. "Clarence" Schneider]

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

Comment 43

[Blake Ross]

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

Comment 44

[chris hofmann]

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

Comment 45

[Diego Biurrun]

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

Comment 46

[gabriel]

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

Comment 47

[Julian Hsiao]

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.

Comment 48

[R.K.Aa.]

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

Comment 49

[Christopher Beland]

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.

Comment 50

[brian]

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

;-p

Comment 51

[timeless]

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

Comment 52

[Diego Biurrun]

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.

Comment 53

[Diego Biurrun]

Attachment: first stab at a fix

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.

Comment 54

[Diego Biurrun]

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!

Comment 55

[Bradley Baetz (:bbaetz)]

+<!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?

Comment 56

[Diego Biurrun]

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?

Comment 57

[lohphat]

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.

Comment 58

[Diego Biurrun]

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.

Comment 59

[lohphat]

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

Comment 60

[Diego Biurrun]

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.

Comment 61

[timeless]

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

Comment 62

[Diego Biurrun]

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 63

[Samir Gehani]

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.

Comment 64

[Diego Biurrun]

Attachment: updated description, field now readonly

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?

Comment 65

[Ashley Bischoff (blog at handcoding.com)]

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

*ducks*

Comment 66

[Diego Biurrun]

Attachment: fixed bugs pointed out by Samir Gehani

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?

Comment 67

[Diego Biurrun]

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.

Comment 68

[gordon]

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.

Comment 69

[Diego Biurrun]

Attachment: the chromediff from patch maker, probably easier to apply with patch

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.

Comment 70

[Samir Gehani]

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

Comment 71

[Simon Fraser [no longer active]]

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.

Comment 72

[Diego Biurrun]

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.

Comment 73

[Diego Biurrun]

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.

Comment 74

[Diego Biurrun]

Attachment: version that might work on the mac

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.

Comment 75

[Diego Biurrun]

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.

Comment 76

[Diego Biurrun]

Just tried it on 2001112603 under Windows, works.

Desperately seeking Mac testers...

Comment 77

[Julian Hsiao]

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

Comment 78

[nnooiissee]

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"/>

Comment 79

[Diego Biurrun]

Attachment: patch maker chromediff version of the same patch

This one might be easier to install with patch maker.

Comment 80

[Diego Biurrun]

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.

Comment 81

[Diego Biurrun]

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?

Comment 82

[Julian Hsiao]

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.

Comment 83

[nnooiissee]

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.

Comment 84

[Diego Biurrun]

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?

Comment 85

[gordon]

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.

Comment 86

[timeless]

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

Comment 87

[tao]

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

Comment 88

[Diego Biurrun]

Attachment: path should be visible on the mac now

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.

Comment 89

[Diego Biurrun]

Attachment: chromediff for convenience

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

Comment 90

[Alvaro Munoz-Aycuens]

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

Comment 91

[Diego Biurrun]

Works fine on Windows ME, build 2001120808.

Comment 92

[nnooiissee]

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

Comment 93

[Diego Biurrun]

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.

Comment 94

[nnooiissee]

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.

Comment 95

[Diego Biurrun]

Attachment: cleaner version

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

Comment 96

[Diego Biurrun]

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.

Comment 97

[Simon Fraser [no longer active]]

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

Comment 98

[Julian Hsiao]

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?

Comment 99

[Simon Fraser [no longer active]]

PatchMaker patches on Mac are pretty much broken.

Comment 100

[Diego Biurrun]

Attachment: end of an odyssey?

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.

Comment 101

[Diego Biurrun]

Attachment: chromediff version

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.

Comment 102

[Julian Hsiao]

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 103

[Samir Gehani]

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.

Comment 104

[Diego Biurrun]

Attachment: includes Samirs suggestions

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

Comment 105

[Diego Biurrun]

Attachment: chromediff version for use with patch maker

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

Comment 106

[nnooiissee]

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. 

Comment 107

[Diego Biurrun]

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?

Comment 108

[Diego Biurrun]

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.

Comment 109

[Diego Biurrun]

Attachment: version 1.0

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.

Comment 110

[Diego Biurrun]

Attachment: update the docs

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

Comment 111

[Simon Fraser [no longer active]]

Comment on attachment 64664 [details] [diff] [review]
version 1.0

sr=sfraser

Comment 112

[Diego Biurrun]

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

Comment 113

[gordon]

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!

Comment 114

[Boris Zbarsky [:bzbarsky]]

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

Comment 115

[Matthias Versen [:Matti]]

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

Comment 116

[Diego Biurrun]

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

Comment 117

[gordon]

I'll try and get to it today.

Comment 118

[gordon]

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.

Comment 119

[Diego Biurrun]

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?

Comment 120

[gordon]

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?

Comment 121

[Simon Fraser [no longer active]]

> Can we make the window bigger?

No; the prefs window ain't getting any bigger. You'll have to rejig your items.

Comment 122

[Diego Biurrun]

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

Comment 123

[Simon Fraser [no longer active]]

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

Comment 124

[gordon]

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

Comment 125

[[not reading bugmail]]

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]

Comment 126

[Henrik Gemal]

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

Comment 127

[Diego Biurrun]

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

Comment 128

[Diego Biurrun]

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

Comment 129

[Pierre Chanial]

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

Comment 130

[Jeremy M. Dolan]

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

Comment 131

[R.K.Aa.]

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

Comment 132

[Diego Biurrun]

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

Comment 133

[Blake Ross]

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

Comment 134

[Blake Ross]

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

Comment 135

[Diego Biurrun]

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 136

[Boris Zbarsky [:bzbarsky]]

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

r=bzbarsky

Comment 137

[Simon Fraser [no longer active]]

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?

Comment 138

[Diego Biurrun]

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.

Comment 139

[Simon Fraser [no longer active]]

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

Comment 140

[Boris 'pi' Piwinger]

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

Comment 141

[Diego Biurrun]

[e-mail], please provide some detailed steps to reproduce, I have no
problems with Linux 0.9.9 and my current CVS build..

Comment 142

[Andrew Hagen]

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?

Comment 143

[Diego Biurrun]

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.

Comment 144

[Diego Biurrun]

Attachment: reset button

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?

Comment 145

[Diego Biurrun]

Attachment: reset button - chromediff version

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

Comment 146

[Boris 'pi' Piwinger]

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

Comment 147

[Christian :Biesinger (don't email me, ping me on IRC)]

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

Comment 148

[Diego Biurrun]

Attachment: remove the dots

Included cbiesinger's suggestions.

Marked all other patches obsolete, as they are either checked in or obsolete.

Comment 149

[scottputterman]

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

Comment 150

[scottputterman]

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

Comment 151

[Vidar Haarr (not reading bugmail)]

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

Comment 152

[Diego Biurrun]

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

Comment 153

[Darin Fisher]

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.

Comment 154

[timeless]

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.

Comment 155

[Diego Biurrun]

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

Comment 156

[Diego Biurrun]

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

Comment 157

[Darin Fisher]

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?

Comment 158

[Diego Biurrun]

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 159

[Christopher Aillon (sabbatical, not receiving bugmail)]

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   <[e-mail]>
>  */
> 
> 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);
>   }
> }
>

Comment 160

[timeless]

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.

Comment 161

[Diego Biurrun]

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.

Comment 162

[Diego Biurrun]

Attachment: addresses caillon's comments

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

Comment 163

[Christopher Aillon (sabbatical, not receiving bugmail)]

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.

Comment 164

[Diego Biurrun]

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.

Comment 165

[gordon]

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

Comment 166

[Samir Gehani]

adt: nsbeta1-

Comment 167

[benc]

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?

Comment 168

[OstGote!]

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

Comment 169

[Diego Biurrun]

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

Comment 170

[benc]

V: Mac OS X, Mozilla 1.6f.

Comment 171

[benc]

V/fixed: mozilla 1.7.2 / win xp

Comment 172

[Diego Biurrun]

Verified on Linux, Mozilla 1.8a3, marking VERIFIED.