User selected Cache directory - imp in prefs needed.

VERIFIED FIXED

Status

()

P1
normal
VERIFIED FIXED
19 years ago
5 years ago

People

(Reporter: jce2, Assigned: skasinathan)

Tracking

(Blocks: 1 bug, {polish})

Trunk
polish
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [cache])

Attachments

(1 attachment, 19 obsolete attachments)

9.50 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

19 years ago
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

19 years ago
This bug is/was related to 45394
Hardware: PC → All
tever/gagan, should this go to Networking?
QA Contact: sairuh → tever

Comment 3

19 years ago
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.
(Reporter)

Comment 4

19 years ago
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. 

Comment 6

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

Comment 7

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

Comment 8

19 years ago
<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

19 years ago
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

19 years ago
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

Comment 12

19 years ago
Putting on [nsbeta2-] radar.  tever, please give verah a release note for PR2.
Whiteboard: [nsbeta2-]

Comment 13

19 years ago
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-]

Comment 14

19 years ago
Can you email me the old code which was removed and I'll meditate on it.
(Reporter)

Comment 15

19 years ago
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

Comment 17

19 years ago
Created attachment 12729 [details]
First stab at a patch to use cache subdirectory

Comment 18

19 years ago
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

19 years ago
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
(Reporter)

Comment 20

18 years ago
Adding keyword patch.

Gabriel, are you happy with the posted patch? Should I (or you) add the review
keyword?
Keywords: patch
*** Bug 50174 has been marked as a duplicate of this bug. ***

Comment 22

18 years ago
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

Comment 24

18 years ago
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.
(Reporter)

Comment 25

18 years ago
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).
Status: NEW → ASSIGNED
Keywords: review
over to jce2, and removing helpwanted (and other stale kw's). thanks for taking
this!
Assignee: matt → jce2
Status: ASSIGNED → NEW
Keywords: helpwanted, nsbeta2, relnote2
Whiteboard: [nsbeta2-][nsbeta3-] → [nsbeta3-]

Comment 27

18 years ago
Sorry guys, I've been working away from home, had no time to look @ this.
(Reporter)

Comment 28

18 years ago
Accepting bug and trying to get SOMETHING in there for rtm, otherwise a lot of
power users are going to HATE us.
Status: NEW → ASSIGNED
*** Bug 50174 has been marked as a duplicate of this bug. ***

Comment 30

18 years ago
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. ***

Comment 32

18 years ago
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.
(Reporter)

Comment 33

18 years ago
Nominating for mozilla 0.9, if we can spend about 3-4 undistracted hours on
this, we can easily fix it.
Keywords: mozilla0.9
(Reporter)

Updated

18 years ago
Target Milestone: --- → mozilla0.9

Comment 34

18 years ago
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.
Keywords: nsmac2
(Reporter)

Comment 35

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

Removed nsbeta3- from Status whiteboard.
Whiteboard: [nsbeta3-]

Updated

18 years ago
Whiteboard: [cache]
(Reporter)

Comment 36

18 years ago
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

Comment 37

18 years ago
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

18 years ago
looks like this will have to wait for 0.9.2
Target Milestone: mozilla0.9.1 → mozilla0.9.2

Comment 39

18 years ago
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

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

Comment 41

18 years ago
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. ***

Comment 43

18 years ago
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

Updated

17 years ago
Target Milestone: mozilla0.9.4 → ---

Comment 44

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

Comment 45

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

Comment 46

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

Comment 47

17 years ago
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

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

Comment 49

17 years ago
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

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

;-p

Comment 51

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

Comment 52

17 years ago
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

Updated

17 years ago
Keywords: patch → 4xp, mozilla1.0, polish

Comment 53

17 years ago
Created attachment 57318 [details] [diff] [review]
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

17 years ago
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?

Comment 56

17 years ago
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

17 years ago
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

17 years ago
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

17 years ago
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

17 years ago
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

17 years ago
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

17 years ago
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

17 years ago
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.
Attachment #57318 - Flags: needs-work+

Comment 64

17 years ago
Created attachment 57935 [details] [diff] [review]
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?
Attachment #57318 - Attachment is obsolete: true
Diego: Well, just ask yourself "Do more nerds use Mozilla, or do more fools use
Mozilla?"

*ducks*

Comment 66

17 years ago
Created attachment 57938 [details] [diff] [review]
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?
Attachment #57935 - Attachment is obsolete: true

Comment 67

17 years ago
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
Status: ASSIGNED → NEW

Comment 68

17 years ago
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

17 years ago
Created attachment 57944 [details] [diff] [review]
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

17 years ago
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

17 years ago
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

17 years ago
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.
Status: NEW → ASSIGNED

Comment 73

17 years ago
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

17 years ago
Created attachment 59027 [details] [diff] [review]
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.
Attachment #57938 - Attachment is obsolete: true
Attachment #57944 - Attachment is obsolete: true

Comment 75

17 years ago
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.
Priority: P3 → P1
Target Milestone: --- → mozilla0.9.7

Comment 76

17 years ago
Just tried it on 2001112603 under Windows, works.

Desperately seeking Mac testers...

Comment 77

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

Comment 78

17 years ago
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

17 years ago
Created attachment 59633 [details] [diff] [review]
patch maker chromediff version of the same patch

This one might be easier to install with patch maker.

Comment 80

17 years ago
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

17 years ago
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

17 years ago
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

17 years ago
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

17 years ago
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

17 years ago
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

17 years ago
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

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

Updated

17 years ago
Blocks: 96876

Comment 88

17 years ago
Created attachment 60963 [details] [diff] [review]
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.
Attachment #59027 - Attachment is obsolete: true

Comment 89

17 years ago
Created attachment 60964 [details] [diff] [review]
chromediff for convenience

The chromediff version as produced by patch maker for your greater convenience.
Attachment #59633 - Attachment is obsolete: true

Comment 90

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

Comment 91

17 years ago
Works fine on Windows ME, build 2001120808.

Comment 92

17 years ago
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

17 years ago
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

17 years ago
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

17 years ago
Created attachment 61038 [details] [diff] [review]
cleaner version

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

Comment 96

17 years ago
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

17 years ago
+    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

17 years ago
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

17 years ago
PatchMaker patches on Mac are pretty much broken.

Comment 100

17 years ago
Created attachment 61177 [details] [diff] [review]
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.
Attachment #61038 - Attachment is obsolete: true

Comment 101

17 years ago
Created attachment 61178 [details] [diff] [review]
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

17 years ago
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

17 years ago
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.
Attachment #61178 - Flags: review+

Comment 104

17 years ago
Created attachment 61236 [details] [diff] [review]
includes Samirs suggestions

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

Comment 105

17 years ago
Created attachment 61237 [details] [diff] [review]
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.
Attachment #61178 - Attachment is obsolete: true

Comment 106

17 years ago
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

17 years ago
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

17 years ago
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

Updated

17 years ago
Keywords: relnote

Comment 109

17 years ago
Created attachment 64664 [details] [diff] [review]
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.
Attachment #61236 - Attachment is obsolete: true

Updated

17 years ago
Attachment #61237 - Attachment is obsolete: true

Comment 110

17 years ago
Created attachment 64722 [details] [diff] [review]
update the docs

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

Comment 111

17 years ago
Comment on attachment 64664 [details] [diff] [review]
version 1.0

sr=sfraser
Attachment #64664 - Flags: superreview+

Comment 112

17 years ago
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??
Keywords: mozilla0.9.8-

Comment 113

17 years ago
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
Status: ASSIGNED → NEW

Updated

17 years ago
Target Milestone: mozilla0.9.8 → mozilla0.9.9
*** Bug 120930 has been marked as a duplicate of this bug. ***

Updated

17 years ago
Keywords: nsbeta3 → nsenterprise
*** Bug 123080 has been marked as a duplicate of this bug. ***

Comment 116

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

Comment 117

17 years ago
I'll try and get to it today.

Comment 118

17 years ago
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

17 years ago
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

17 years ago
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

17 years ago
> Can we make the window bigger?

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

Comment 122

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

Comment 123

17 years ago
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

17 years ago
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]

Comment 126

17 years ago
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

17 years ago
> 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

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

Comment 129

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

Comment 130

17 years ago
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

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

Comment 132

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

Comment 133

17 years ago
> 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

17 years ago
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

17 years ago
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.

Updated

17 years ago
Keywords: nsbeta1+

Updated

17 years ago
Blocks: 127025
Comment on attachment 64722 [details] [diff] [review]
update the docs

r=bzbarsky
Attachment #64722 - Flags: review+

Comment 137

17 years ago
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?

Updated

17 years ago
Blocks: 123569

Comment 138

17 years ago
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.

Updated

17 years ago
Target Milestone: mozilla0.9.9 → mozilla1.0

Comment 139

17 years ago
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

17 years ago
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

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

Comment 142

17 years ago
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

17 years ago
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

17 years ago
Created attachment 75833 [details] [diff] [review]
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

17 years ago
Created attachment 75834 [details] [diff] [review]
reset button - chromediff version

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

Comment 146

17 years ago
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

Updated

17 years ago
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.

Comment 148

17 years ago
Created attachment 78128 [details] [diff] [review]
remove the dots

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

Comment 149

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

Comment 150

17 years ago
Changing nsbeta1+ [adt3] bugs to nsbeta1- on behalf of the adt.  If you have any
questions about this, please email adt@netscape.com.  You can search for
"changing adt3 bugs" to quickly find and delete these bug mails.
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 :)

Comment 152

16 years ago
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

16 years ago
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

16 years ago
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

Comment 155

16 years ago
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

16 years ago
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

16 years ago
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

16 years ago
Darin:  Thanks, I thought Gordon was just back from sabbatical.  Christopher
Aillon will try to review this now, mpt sounds like a good second choice, too,
since he is involved with bug 125567.
Comment on attachment 78128 [details] [diff] [review]
remove the dots

So (timeless, diego, et.al) does this patch make us *worse* than we are wrt how
much can fit in the panels?  If not, I see no problem in letting this go in and
attempt to fix that in the other bug.  Otherwise, please make sure that you
don't "regress" that bug even further.

Aside from that issue, I do have a few comments on this patch.

>--- xpfe/components/prefwindow/resources/content/pref-cache.xul.bak	Sun Mar 24 13:32:17 2002
>+++ xpfe/components/prefwindow/resources/content/pref-cache.xul	Sun Mar 24 14:10:54 2002
...
>+      <hbox align="center">
>+        <label value="&diskCacheFolder.label;"/>
>+        <textbox id="browserCacheDiskCacheFolder" readonly="true" flex="1"/>
>+      </hbox>
>+      <hbox align="center" pack="end">
>+        <button label="&resetDiskCacheFolder.label;" accesskey="&resetDiskCacheFolder.accesskey;"
>+                oncommand="prefCacheResetFolder();" id="resetDiskCacheFolder"/>
>+        <button label="&chooseDiskCacheFolder.label;" accesskey="&chooseDiskCacheFolder.accesskey;"
>+                oncommand="prefCacheSelectFolder();" id="chooseDiskCacheFolder"/>

Prefer the id earlier in the attribute list, as you've done above.  It makes it
easier to quickly scan for that id.

>+      </hbox>
>+    </vbox>
> 
>     <description>&diskCacheFolderExplanation;</description>
> 
>-      </rows>
>-    </grid>
>+

This extra line is unneeded.


>--- xpfe/components/prefwindow/resources/locale/en-US/pref-cache.dtd.bak	Sun Mar 24 13:37:11 2002
>+++ xpfe/components/prefwindow/resources/locale/en-US/pref-cache.dtd	Sun Apr  7 23:15:38 2002


It would be nice if this file could have the entity values line up...


>--- xpfe/components/prefwindow/resources/content/pref-cache.js.bak	Sun Mar 24 14:08:02 2002
>+++ xpfe/components/prefwindow/resources/content/pref-cache.js	Sun Mar 24 17:12:00 2002
>@@ -1,11 +1,14 @@
> /* -*- Mode: Java; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*-
>  *
>+ * Contributor(s): 
>+ *      Diego Biurrun   <diego@biurrun.de>
>  */
> 
> const nsIFilePicker       = Components.interfaces.nsIFilePicker;
> const nsILocalFile        = Components.interfaces.nsILocalFile;
> const nsIFile             = Components.interfaces.nsIFile;
> const nsIProperties       = Components.interfaces.nsIProperties;
>+const nsIPrefBranch       = Components.interfaces.nsIPrefBranch;
> const kCacheParentDirPref = "browser.cache.disk.parent_directory";
> 
> function Startup()
>@@ -15,7 +18,7 @@
>   try 
>   {
>     pref = Components.classes["@mozilla.org/preferences-service;1"]
>-      .getService(Components.interfaces.nsIPrefBranch);
>+      .getService(nsIPrefBranch);

Please fix the alignment here.	.getService should align with .classes


>     path = pref.getComplexValue(kCacheParentDirPref, nsILocalFile);
>   }
>   catch (ex)
>@@ -37,10 +40,10 @@
>       // now remember the new assumption
>       if (pref)
>         pref.setComplexValue(kCacheParentDirPref, nsILocalFile, path);
>-    }
>-    catch (ex)
>-    {
>-    }
>+     }
>+     catch (ex)
>+     {
>+     }
>   }
> 
>   // if both pref and dir svc fail leave this field blank else show path
>@@ -50,12 +53,25 @@
> }
> 
> 
>+function prefCacheResetFolder()
>+{
>+  var pref = null;
>+  pref = Components.classes["@mozilla.org/preferences-service;1"]
>+    .getService(nsIPrefBranch);

Please make .getService align with .classes.

>+
>+  pref.clearUserPref(kCacheParentDirPref);
>+
>+  // Our changes need to get displayed
>+  Startup();

Startup is a special function in prefs.  It is called by the pref window itself
on load and we should not add a dependency on it by other panel specific
functions such as this.  If you need the same stuff to happen here, then please
split out the stuff in Startup() to a separate function and please have both
Startup() and this call into that.


>+}
>+
>+
> function prefCacheSelectFolder()
> {
>   var fp = Components.classes["@mozilla.org/filepicker;1"]
>                      .createInstance(nsIFilePicker);
>   var pref = Components.classes["@mozilla.org/preferences-service;1"]
>-                     .getService(Components.interfaces.nsIPrefBranch);
>+                     .getService(nsIPrefBranch);

More .getService alignment fun :-)

>   var prefutilitiesBundle = document.getElementById("bundle_prefutilities");
>   var title = prefutilitiesBundle.getString("cachefolder");
> 
>@@ -78,7 +94,7 @@
>     var viewable = fp.file.unicodePath;
>     var folderField = document.getElementById("browserCacheDiskCacheFolder");
>     folderField.value = viewable;
>-    pref.setComplexValue(kCacheParentDirPref, nsILocalFile, localFile)
>+    pref.setComplexValue(kCacheParentDirPref, nsILocalFile, localFile);
>   }
> }
>
Attachment #78128 - Flags: needs-work+

Comment 160

16 years ago
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

16 years ago
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

16 years ago
Created attachment 95326 [details] [diff] [review]
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
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.
Keywords: mozilla0.9.8-, mozilla1.0, nsbeta1- → nsbeta1
Target Milestone: mozilla1.0 → ---

Comment 164

16 years ago
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

16 years ago
Suresh, can you take a look at this?  Thanks.
Assignee: gordon → suresh

Comment 166

16 years ago
adt: nsbeta1-
Keywords: nsbeta1 → nsbeta1-

Comment 167

16 years ago
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
Keywords: relnote

Comment 168

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

Comment 169

15 years ago
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
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED

Comment 170

15 years ago
V: Mac OS X, Mozilla 1.6f.
Whiteboard: [cache][adt3] → [cache] checklinux, checkwin

Comment 171

14 years ago
V/fixed: mozilla 1.7.2 / win xp
Whiteboard: [cache] checklinux, checkwin → [cache] checklinux

Comment 172

14 years ago
Verified on Linux, Mozilla 1.8a3, marking VERIFIED.
Status: RESOLVED → VERIFIED

Updated

14 years ago
Whiteboard: [cache] checklinux → [cache]
You need to log in before you can comment on or make changes to this bug.