The mozilla will suspend when open the directory,it is not allow read. Download a file,and open a directory that not allow read, and save the file under this directory.mozilla will suspend.

build NETSCAPE_6_2_1_RELEASE

This patch can fix this bug and bug #85309,

Adding keywords....This needs a review etc...Antonio you might want to contact the module owners for a review.

Please review this patch and let me check in.

adding a couple of people that might be able to review this patch.

Will this throw up the permission alert from the filepicker _and_ close the filepicker? Or will the user get the alert and then be able to select a different file? Also, > + if (!parnt.isWritable() || (!file.isWritable())){ use "parentDir" or something, not "parnt". The only time the dir needs to be writable is if the file does not exist. So maybe something more like: if ((file.exists() && !file.isWritable()) || !parentDir.isWritable()) {

if ((file.exists() && !file.isWritable()) || !parentDir.isWritable()) { I think "file.exists()" should not add,because this only allow replease a exist file,not allow save file.

> because this only allow replease a exist file, not allow save file. Um... no. If you look at my suggested "if" statement, it can be stated in words as "if we need to create a file and can't or if the file exists but can't be written then throw an error". Your statement says "if we can't create a file or can't write the file then throw an error". Your version will throw an error in the case when we have a non-writable directory but a writable file in the directory and try to overwrite the file. There is no reason to throw an error in those conditions, since we have the permissions needed to save the data.

OK.... I'm on crack. The whole thing is in a "if (isFile)" statement, so the file does exist. And we need to make sure the parent is writable because the "save web page, complete" mode will need to create a directory, not just write our file. The patch looks pretty good overall, with the following nits: 1) Looks like it uses tabs; spacing is a little odd.... 2) +errorOpenDirDoesntExistMessage=Permission denied! Could not open the directory %S. I would do: +errorDirNotReadableMessage=Directory %S is not readable or does not exist. (note the different name for the string -- one can have an existing non-readable dir) 3) +saveWithoutPermissionMessage_file=Permission denied! Could not overwrite the file %S.Please use a different filename! I would do: +saveWithoutPermissionMessage_file=File %S is not writable. 4) +saveWithoutPermissionMessage_dir=Permission denied! Could not save the file %S under this directory. I would do: +saveWithoutPermissionMessage_dir=Cannot create file. Directory %S is not writable. mpt? Thoughts on phrasing?

I'm adding Brian Rynder to cc: list. He is the person who should approve this patch since he has done much of the recent Unix file picker work.

Thanks for bzbarsky's advice,i had changed my patch for this bug,the patch is diff with trunc,i had test the patch on trunc,that is ok.So please review my patch,and let me check in.

Comment on attachment 63718 [details] [diff] [review] This is final patch,please review my patch! >@@ -243,9 +258,13 @@ > break; > case nsIFilePicker.modeSave: > if (isFile) { // can only be true if file.exists() >+ if (!file.isWritable()){ >+ throwFileSavingErrorDialog(file); >+ ret = nsIFilePicker.returnCancel; >+ }else{ > // we need to pop up a dialog asking if you want to save >- var message = gFilePickerBundle.getFormattedString("confirmFileReplacing", >- [file.unicodePath]); >+ var message = gFilePickerBundle.getFormattedString("confirmFileReplacing",[file.unicodePath]); >+ > var rv = window.confirm(message); > if (rv) { > ret = nsIFilePicker.returnReplace; Please try not to mess up the spacing. Other than that, looks fine. r=bryner.

Comment on attachment 63718 [details] [diff] [review] This is final patch,please review my patch! Patch looks good, just some nits: > errorSavingFileTitle=Error saving %S > saveParentIsFileMessage=%S is a file, can't save %S > saveParentDoesntExistMessage=Path %S doesn't exist, can't save %S >- >+saveWithoutPermissionMessage_file=File %S is not writable. >+saveWithoutPermissionMessage_dir=Cannot create file. Directory %S is not writable. Could you insert a white line here? This'll keep the grouping intact. > noPermissionTitle=Error opening directory > noPermissionError=You do not have the permissions necessary to view this directory. >Index: filepicker.js >=================================================================== >RCS file: /cvsroot/mozilla/xpfe/components/filepicker/res/content/filepicker.js,v >retrieving revision 1.62 >diff -u -w -r1.62 filepicker.js >--- filepicker.js 2001/12/12 05:05:09 1.62 >+++ filepicker.js 2002/01/06 08:31:22 >@@ -44,7 +44,6 @@ > var okButton; > > var gFilePickerBundle; >- > function filepickerLoad() { > gFilePickerBundle = document.getElementById("bundle_filepicker"); > Please keep the white line there, it makes the var easier to find. >@@ -160,10 +159,26 @@ > outlinerView.setFilter(filterTypes); > window.setCursor("auto"); > } Please insert a white line here. >+function throwFileSavingErrorDialog(file){ >+ var errorTitle, errorMessage, promptService; >+ errorTitle = gFilePickerBundle.getFormattedString("errorSavingFileTitle", [file.unicodePath]); >+ errorMessage = gFilePickerBundle.getFormattedString("saveWithoutPermissionMessage_file", [file.unicodePath]); >+ promptService = Components.classes["@mozilla.org/embedcomp/prompt-service;1"].getService(Components.interfaces.nsIPromptService); >+ promptService.alert(window, errorTitle, errorMessage) >+} Could you call this |showFileSavingErrorDialog|? Naming style in Mozilla generally uses |show| here, not |throw|. > > function openOnOK() > { > var dir = outlinerView.getSelectedFile(); >+ if (!dir.isReadable()){ >+ errorTitle = gFilePickerBundle.getFormattedString("errorOpenFileDoesntExistTitle",[dir.unicodePath]); >+ errorMessage = gFilePickerBundle.getFormattedString("errorDirNotReadableMessage",[dir.unicodePath]); >+ promptService = Components.classes["@mozilla.org/embedcomp/prompt-service;1"] >+ .getService(Components.interfaces.nsIPromptService); >+ promptService.alert(window, errorTitle, errorMessage); >+ return false; >+ } The rest of this file uses 2 space indent, please use that style. Same for space before '{'. You haven't explicitely declared the vars errorTitle, errorMessage and promptService here, though I suspect those names are declared lower so you'll have to remove the declaration there. >+ > if (dir) > gotoDirectory(dir); > retvals.file = dir; >@@ -243,9 +258,13 @@ > break; > case nsIFilePicker.modeSave: > if (isFile) { // can only be true if file.exists() >+ if (!file.isWritable()){ >+ throwFileSavingErrorDialog(file); >+ ret = nsIFilePicker.returnCancel; >+ }else{ Same here, space after '}' and before '{' (twice), and use 2 space indentation. > // we need to pop up a dialog asking if you want to save >- var message = gFilePickerBundle.getFormattedString("confirmFileReplacing", >- [file.unicodePath]); >+ var message = gFilePickerBundle.getFormattedString("confirmFileReplacing",[file.unicodePath]); >+ Two nits here: space after comma and indentation. However, I'd rather you undo this change, which seems purely a formatting change. The old formatting (distributing over two lines) keeps the line length within 80 characters. You should (try to) apply this to the other lines you're adding (I forgot to mention this above). > var rv = window.confirm(message); > if (rv) { > ret = nsIFilePicker.returnReplace; >@@ -253,6 +272,7 @@ > } else { > ret = nsIFilePicker.returnCancel; > } >+ } This looks like it's only indented one, not two spaces. Could you attach a version of this patch (after you've addressed the nits I've picked) without -w? > } else if (isDir) { > if (!sfile.equals(file)) { > gotoDirectory(file); >@@ -262,7 +282,7 @@ > ret = nsIFilePicker.returnCancel; > } else { > var parent = file.parent; >- if (parent.exists() && parent.isDirectory()) { >+ if (parent.exists() && parent.isDirectory() && parent.isWritable()) { > ret = nsIFilePicker.returnOK; > retvals.directory = parent.unicodePath; > } else { >@@ -280,6 +300,7 @@ > errorMessage = gFilePickerBundle.getFormattedString("saveParentDoesntExistMessage", > [oldParent.unicodePath, file.unicodePath]); > } >+ if (!parent.isWritable()) errorMessage = gFilePickerBundle.getFormattedString("saveWithoutPermissionMessage_dir", [file.unicodePath]); Could you spread this over two or three lines to make the code more readable and keep in style with the rest of the file? > promptService = Components.classes["@mozilla.org/embedcomp/prompt-service;1"] > .getService(Components.interfaces.nsIPromptService); > promptService.alert(window, errorTitle, errorMessage);

This just updates attachment 63718 [details] [diff] [review] to jag's comments.

Comment on attachment 64982 [details] [diff] [review] patch addressing jag's whitespace nits carrying over bryner's r=

Comment on attachment 64982 [details] [diff] [review] patch addressing jag's whitespace nits sr=jag

Thanks for Brian Ryner's advice,I change my patch according to Brian Ryner's advice,so i attach a new attachment,please r and sr my patch

Antonio, jag _has_ sr'ed your patch. You seem to have completely ignored his comments.... I can check in the patch that has r and sr sometime today if you'd like, or we could go through the r/sr cycle on a new patch that you attach (not using -w) that addresses jag's comments. Please let me know.

Sorry for being late to the party, but Boris says in comment 9: >And we need to make sure the parent is writable because the >"save web page, complete" mode will need to create a directory, not just write >our file. We're not *always* saving as "web page, complete" so maybe this check is overly restrictive. It would kick in when you open the file picker from Composer, too, correct? Maybe we need to pass in another file picker flag or let the calling code deal with the problem of creating the associated sub-directory downstream someplace (we do get an I/O error at some point; we just don't pay attention to it; that problem is being remedied).

Hi Boris Zbarsky Thanks for your help.Because i didn't pay attention to the comment,I created a new attachment,so this is my fault.Please check in the patch that has r and sr.I feel so happy,and thank you,thanks for your help.

Bill, sorry about my confusing comments... now that I have a tree and have applied the patch, the code is something like: if (isFile) { // do stuff } else if (isDir) { // do more stuff } else { // creating file // check in question happens } So we do need parent.isWritable(). Attachment 64982 [details] [diff] checked in, marking fixed.

rs vrfy --but will test more for bug 115197 when it's resolved.