Closed
Bug 27609
Opened 25 years ago
Closed 23 years ago
Need to have alerts for all file save failures
Categories
(Core Graveyard :: File Handling, defect, P1)
Core Graveyard
File Handling
Tracking
(Not tracked)
VERIFIED
FIXED
mozilla0.9.9
People
(Reporter: rzach, Assigned: law)
References
Details
(Keywords: dataloss)
Attachments
(6 files, 12 obsolete files)
22.63 KB,
text/html
|
Details | |
11.20 KB,
patch
|
Details | Diff | Splinter Review | |
17.64 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
5.86 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
1.51 KB,
patch
|
hwaara
:
review+
|
Details | Diff | Splinter Review |
3.83 KB,
text/plain
|
Details |
Left over from bug 23821.
Need more informative alerts when file save fails due to permission violation.
Right now it just says "Unknown error". Same thing for similar errors (eg, no
space left on device).
Updated•25 years ago
|
QA Contact: paulmac → sairuh
Updated•25 years ago
|
OS: Linux → All
Comment 5•25 years ago
|
||
see it on windows too.
Updated•25 years ago
|
Target Milestone: M16 → M18
Updated•24 years ago
|
Component: XP Apps → XP Apps: GUI Features
Comment 8•24 years ago
|
||
The file `KingFred.html' could not be saved to the folder `Documents', because
{reason}.
{suggestion}
( Save As ... ) ( Cancel ) (( Retry )) [Mac, X]
(( Retry )) ( Save As ...) ( Cancel ) [Windows]
For lack of write permission:
* reason = `you do not have permission to change the contents of that folder'
* suggestion = `Change the folder properties and try again, or try saving in a
different location.'
(Do you have an XP way of telling whether it's a floppy which is
write-protected?)
Lack of space:
* reason = `there is not enough room on the disk'
(Ideally this would be followed by `(42 kb needed, 15 kb available)', but
that's getting into RFE territory.)
* suggestion = `Make more space on the disk and try again, or try saving in a
different location.'
Unknown error:
* reason = `an unknown error occurred'
* suggestion = nothing
Any other causes?
Comment 10•24 years ago
|
||
i'm bumping up the sev on this a bit, due to lorca's observations in bug 60266.
bill/mscott, shouldn't we throw a dialog after the hits OK in the filepicker
dialog, saying something to the effect of, "cannot save since you don't have the
proper permissions for this directory"? since the download progress dialog does
now appear, this gives the perception of dataloss.
Severity: minor → major
Keywords: correctness
Assignee | ||
Comment 11•24 years ago
|
||
Yes, we should inform the user about what happens.
I had a fix for at least one aspect of this general problem but it got nixed for
rtm. We need to merge the file download logic and make sure that all paths
properly handle errors. There's lots of bugs relating to that; if there weren't
so many, maybe I could sort them all out :-(.
Comment 12•24 years ago
|
||
Raising Severity due to data loss status. Adding myself to cc: list.
From DUPLICATE 60266 (that I wrote up):
Happens in: Linux 11-08-09
Does not happen in: Mac/Win->N/A
Reason for Severity: Data loss/silent data loss/perceived data loss
Reproducible: Always.
Steps to Reproduce:
1. Open up a download box
2. Select directory to save in-make sure its not normally accessible to your
particular account
3. Watch download/save progress
4. Confirm "Download/Save Complete" dialog box.
5. Check directory for contents
Actual Results: Nothing there.
Expected Results: File downloaded to be there.
Severity: major → critical
Assignee | ||
Comment 13•24 years ago
|
||
Nominating for nsbeta1. This overlaps with other bug(s) but I want to flag it
anyway.
Keywords: nsbeta1
Comment 14•24 years ago
|
||
Netscape Nav triage team: this is a Netscape beta stopper. Changed priority to
P2.
Priority: P3 → P2
Updated•24 years ago
|
Target Milestone: --- → mozilla0.8
Comment 15•24 years ago
|
||
Did a fix for this get checked in last night?
Assignee | ||
Comment 16•24 years ago
|
||
Almost. I added error handling code in the xfer code. That isn't used all the
time, yet. I'm using bug 67216 to track that issue.
So, I'm marking this one as dependent on that bug.
Depends on: 67216
Target Milestone: mozilla0.8 → mozilla0.9
Assignee | ||
Comment 17•24 years ago
|
||
Adjusting keywords; dependent bug 67216 should be the one marked nsbeta1.
Keywords: nsbeta1
Comment 19•24 years ago
|
||
nav triage team:
Yes, this sucks, but push has come to shove. Bumping from mozilla0.9 train,
marking nsbeta1-, resetting target milestone
Comment 20•24 years ago
|
||
Marking nsbeta1+, mozilla0.9
Assignee | ||
Comment 21•24 years ago
|
||
I'm not getting to this dialog till next round...
Target Milestone: mozilla0.9 → mozilla0.9.1
Comment 22•24 years ago
|
||
nav triage team:
Marking mozilla0.9.2
Target Milestone: mozilla0.9.1 → mozilla0.9.2
Comment 23•24 years ago
|
||
*** Bug 80242 has been marked as a duplicate of this bug. ***
Comment 24•24 years ago
|
||
<q src="mpt" date="2000-07-26 20:38"> Any other causes?</q>
3. Enter a pathname which doesn't exist (I had a c:\download directory, but
accidently typed in the plural c:\downloads instead)
Actual Results: "There was an error writing to the target location"
Expected Results: "This directory does not exist. Should it be created?"
Comment 25•24 years ago
|
||
bill/bryner, are there even hooks for creating directories from the file
pickers?
Comment 26•24 years ago
|
||
nav triage: frequency of this problem and severity are low enough to defer to
m0.9.3.
Target Milestone: mozilla0.9.2 → mozilla0.9.3
Comment 27•23 years ago
|
||
nav triage team:
Unfortunately, the work involved to fix this problem the right way is too much to
take for mozilla0.9.3. Pushing out to mozilla1.0
Target Milestone: mozilla0.9.3 → mozilla1.0
Updated•23 years ago
|
Keywords: helpwanted
Keywords: nsenterprise
Comment 29•23 years ago
|
||
spam: over to File Handling.
Component: XP Apps: GUI Features → File Handling
Comment 30•23 years ago
|
||
->098, actual text used is no longer the issue in this bug, we need to ensure
that users are alerted to failed saves. updating summary to reflect bug morphage.
Summary: Need more informative alert for file save failure → Need to have alerts for all file save failures
Target Milestone: mozilla1.0 → mozilla0.9.8
Comment 31•23 years ago
|
||
related: bug 65770 - not getting an alert when disk full for file downloads.
Assignee | ||
Comment 32•23 years ago
|
||
*** Bug 87438 has been marked as a duplicate of this bug. ***
Comment 33•23 years ago
|
||
*** Bug 108669 has been marked as a duplicate of this bug. ***
Comment 34•23 years ago
|
||
*** Bug 109069 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 35•23 years ago
|
||
*** Bug 105718 has been marked as a duplicate of this bug. ***
Comment 36•23 years ago
|
||
*** Bug 112175 has been marked as a duplicate of this bug. ***
Comment 37•23 years ago
|
||
*** Bug 113062 has been marked as a duplicate of this bug. ***
Comment 38•23 years ago
|
||
*** Bug 116753 has been marked as a duplicate of this bug. ***
Comment 39•23 years ago
|
||
I think my bug (116753) should be blocked by this, rather then duped, as it is a
specific issue, on only one type of OS, which warrants seperate verification
after this fix is in. Unless there is contention, I'll undupe it tomarrow.
Comment 40•23 years ago
|
||
*** Bug 115197 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 41•23 years ago
|
||
This document describes how I plan to fix this problem (once and for all).
Anybody who cares about how we do this (or has an interest in the components
that are modified) should have a look and offer feedback, ASAP.
Assignee | ||
Comment 42•23 years ago
|
||
All these download related items are moving out; delayed due to overhaul of
downloading and many regressions in this area.
Target Milestone: mozilla0.9.8 → mozilla0.9.9
Assignee | ||
Comment 43•23 years ago
|
||
Attachment #64387 -
Attachment is obsolete: true
Keywords: helpwanted → nsbeta1
Assignee | ||
Comment 44•23 years ago
|
||
Assignee | ||
Comment 45•23 years ago
|
||
Assignee | ||
Comment 46•23 years ago
|
||
This patch makes the uriloader use the new nsIProgressDialog.
It does not try to reuse nsIWebBrowserPersist as its stream listener. The main
drawback is that there are still two places where we have network I/O that has
to check for errors, etc. I will try to see if I can re-use the
nsWebBrowserPersist as the uriloader's stream listener, but I'm worried that
that might be hard.
So, this is an interim fix to add error handling and use the new and improved
progress dialog.
Assignee | ||
Comment 47•23 years ago
|
||
OK, I need some reviews, approvals, and super-reviews now.
There are 4 separate items:
1. The new nsIProgressDialog interface and implementation in
mozilla/embedding/components/ui/progressDlg. The files of interest are all
named nsProgressDialog.* (the ones named nsProgressDlg.* are obsolescent). I
need a volunteer to review it; I'd like either or both of ben and mscott to
super-review.
2. The nsWebBrowserPersist changes, attached to this bug. Adam Lock needs to
review/approve these changes. I need a volunteer to super-review (ben or
mscott?).
3. The contentAreaUtils.js changes, attached to this bug. I need a volunteer
to review this (Boris? I know you've been hacking code real near this); Ben
should super-review/approve.
4. The uriloader/exthandler changes, attached to this bug. I need a volunteer
to review (Boris?); mscott should super-review/approve.
The only open issues are:
a. Some problems related to handling errors returned on OnStopRequest. E.g.,
if I abort a download from my ftp server console, I get an alert saying "The
operation was abortted by the system administrator" (or something like that)
but the progress dialog then jumps to 100% and indicates the download is
completed. Neither nsWebBrowserPersist nor the uriloader seem to look for
errors and pass them to the dialog. I've marked where they should do that with
comments but some code still needs to be written.
b. There is some flakiness in the networking code (I think) when saving to a
diskette. I've opened bug 124035 and bug 124036 for those problems. The one
may be due to an error occuring when we create the file transport, which we're
not handling. The timing one requires some investigation.
Assignee | ||
Comment 48•23 years ago
|
||
I forgot this file in the nsWebBrowserPersist patch. This needs to be added in
mozilla/embedding/components/webbrowserpersist/locale/en-US
as nsWebBrowserPersist.properties.
Assignee | ||
Comment 49•23 years ago
|
||
Attachment #65500 -
Attachment is obsolete: true
Comment 50•23 years ago
|
||
This patch doesn't build as-is on Linux because
embedding/components/ui/progressDlg isn't hooked up to the build.
Comment 51•23 years ago
|
||
Comment 52•23 years ago
|
||
brian or bill, have you had a chance to test these patches on Mac [either 9.x or
10.1.x]? just curious. :)
Assignee | ||
Comment 53•23 years ago
|
||
Thanks, Brian; I had those unix makefile changes on my to-do list.
Sarah, no Mac testing, yet. But it's all xp code so it should work fine :-).
There does need to be comparable tweaks to the Mac build scripts, though. I'll
work on that.
Comment 54•23 years ago
|
||
Comment on attachment 68248 [details] [diff] [review]
Patch to nsWebBrowserPersist to work with new nsIProgressDialog
r=adamlock
A couple of nitpicks. First (and this is a holdover from the old code) we
should use PRUint32 instead of unsigned int for bytesRead/written/remaining
variables.
And can you unindent the switch statements so the case line up underneath the
switch?
Attachment #68248 -
Flags: review+
Comment 55•23 years ago
|
||
Comment on attachment 68268 [details] [diff] [review]
Patch to uriloader to make it work with the new nsIProgressDialog
sr=mscott
can we pull the string bundle #define properties line out and up at the top of
the file instead of nested in the if clause just for clarity?
Just for kicks can you make sure you can still open a large mail attachment
after these changes?
Attachment #68268 -
Flags: superreview+
Comment 56•23 years ago
|
||
Comment on attachment 68248 [details] [diff] [review]
Patch to nsWebBrowserPersist to work with new nsIProgressDialog
sr=mscott modulo Adam's comments about using PRUint32s instead of "unsinged
int"
Attachment #68248 -
Flags: superreview+
Assignee | ||
Comment 57•23 years ago
|
||
re: comment 54
I've made the changes suggested; thanks.
Attachment #68248 -
Attachment is obsolete: true
Comment 58•23 years ago
|
||
Minor grouch: Progress should never be shown in a dialog (not minimizable, not
closable). Normally it will be in a progress window (minimizable, not closable);
the exception is the download manager, where it will be in a document window
(minimizable, closable). So calling anything `nsIProgressDialog' is just going
to confuse people who use the API ... s/ProgressDialog/ProgressWindow/g would be
nice. :-)
Comment 59•23 years ago
|
||
Comment on attachment 68251 [details] [diff] [review]
Patch to contentAreaUtils.js to make it work with new nsIProgressDialog
>+ var filesFolderLeafName = getStringBundle().GetStringFromName("filesFolder");
>+ filesFolderLeafName = filesFolderLeafName.replace(/\^BASE\^/, nameWithoutExtension);
Any reason not to use FormatStringFromName here?
>+ if (!filesFolder.exists())
>+ filesFolder.create(lfIID.DIRECTORY_TYPE, 0755);
>+ }
I'd suggest not doing this. This is done in any case by the WebBrowserPersist
code (see nsWebBrowserPersist::SaveDocumentInternal) and that code actually
has the smarts to not create this dir if there are no linked resources to
persist. Let's not ruin that over here. :)
>+dump("onStateChange exception=" + e + "\n" );
This is going away before checkin, right? :)
Comment 60•23 years ago
|
||
Comment on attachment 68268 [details] [diff] [review]
Patch to uriloader to make it work with the new nsIProgressDialog
> helperAppDlg \
>+ progressDlg \
> plugin \
Fix the indentation (makefile.win).
>+NS_IMETHODIMP nsExternalAppHandler::Observe(nsISupports *aSubject, const char *aTopic, const PRUnichar *aData )
>+{
>+ // User pressed cancel button on dialog.
>+ return this->Cancel();
>+}
>+
Should we check aTopic? If it's always going to be "oncancel" (as the
interface comments for nsIProgressDialog suggest), then the "observer"
attr of nsIProgressDialog should perhaps be renamed "cancelObserver" for
clarity.... As it stands people may be tempted to make things other
than cancelling observable and then this code could fail.
> mWebProgressListener = aWebProgressListener;
>- webProgress->AddProgressListener(mWebProgressListener);
>+ mTimeDownloadStarted = PR_Now();
>+
Why move this? It seems more correct to call that right at the end of
OnStartRequest as before....
> while (count > 0) // while we still have bytes to copy...
Shouldn't this be | while (NS_SUCCEEDED(rv) && count > 0) | ?
Otherwise we'll go into an infinite loop on a read error....
>+ rv = mOutStream->Write(mDataBuffer, numBytesRead, &numBytesWritten);
That should be bufPtr instead of mDataBuffer, no?
>+ if (action!=nsIMIMEInfo::saveToDisk)
add spaces around the "!="
>+ if(openWith.IsEmpty())
Space after the "if" as elsewhere in this file? Same for the "if"s inside
this one....
>+ // Tell progress dialog what we're opening with.
>+ progressDlg->SetOpeningWith(openWith);
I'd say that we should either have a default value for "openWith" in case it's
_still_ empty after the nsILocalFile dance or assert that it's not empty.
I'm not sure how it can be empty at this point without something being
deeply wrong... Also, you want openWith.get() to get a wstring, no?
>+ PRInt32 mProgress;
Could we add a brief comment explaining what this integer is?
Comment 61•23 years ago
|
||
Comment on attachment 68270 [details] [diff] [review]
Additional nsWebBrowserPersist file
>diskFull=There is not enough free space to store the file. Free up space and try again, or, choose a different target location.
There should be no comma after the "or".
I'm not sure we want to have the "try again" text in there.
Also, I think we should have two different error messages:
1) Disk full when writing to our temp file in the uriloader. Since the user
has no way to easily configure where we put this file (short of shutting
down mozilla, changing some environment variables, and restarting) we
should
just say that the user should free up some space. Bonus points for adding
the name of the directory or file we were saving to in this alert so the
user knows _where_ to free up space (this would replace "the file" in the
message).
2) Disk full when writing to the file/dir the user chose to save to when doing
"Save As" (error in nsWebBrowserPersist). In this case the error should
be similar to what we have now, again with "the file" replaced by the
actual
path.
Attachment #68270 -
Flags: needs-work+
Comment 62•23 years ago
|
||
Comment 63•23 years ago
|
||
Apparently the %1, %2, etc in that attachment of mine should be something else,
but no-one in #mozilla seems to know precisely what. :-)
Comment 64•23 years ago
|
||
I didn't say that. bz said to use %s, he's right. I merely noted that if the
order didn't match (eg if you localized to spanish or something) and needed to
use the second param first, you could.
Assignee | ||
Comment 65•23 years ago
|
||
Re: Scott's comments (in comment 55)
I've removed the #define for |properties| and just put the chrome url in the
call as a literal. Likewise modified in nsWebBrowserPersist.
Re: Boris's comments (in comment 60)
I've made all those changes, except for:
* the moving of the setting of mTimeDownloadStarted; this value is needed in
ShowProgressDialog, which gets called before the previous setting of this field
occurred. That could be worked around somehow, but it was easier to just move
this up a few lines. The data is coming in, so it's not like we're lying.
* SetOpenWith(openWith) not using |.get()|. It doesn't seem necessary and I
prefer to let the compiler figure out the best way of passing the argument. Is
that politically incorrect for some reason?
Updated patches coming soon.
Assignee | ||
Comment 66•23 years ago
|
||
re: comment 59
>Any reason not to use FormatStringFromName here?
No, but I just copied this code from nsProgressDlg.js as is. I suppose I can
fix it now.
>>+ if (!filesFolder.exists())
>>+ filesFolder.create(lfIID.DIRECTORY_TYPE, 0755);
>>+ }
>
>I'd suggest not doing this
Ditto for this. I'll remove these lines now, based on your say so. Ben/Adam,
you guys seemed to want this before; please holler if removing this is a mistake.
>+dump("onStateChange exception=" + e + "\n" );
Deleted (was temporary debug code).
Assignee | ||
Comment 67•23 years ago
|
||
re: comment 61...comment 64
I'm not sure what to make of the "suggestion" entries in mpt's new
nsWebBrowserPersist.properties.
We are a bit constrained here in that the code can only return a single string
(from nsWebBrowserPersist.cpp and nsExternalHelperAppService.cpp) to the
progress dialog code.
mpt described his proposal as "ideal," but I'm stuck here in the real world.
I can add code to nsProgressDialog.js to look for some specific return values
(the NS_ERROR_FILE_* ones), at which point I could fetch additional "suggestion"
strings for use in the alert. But, given that there seems to be only one
suggestion for each error, I'd rather just combine the two sentences in the same
property string (I'm ignoring the implied different suggestion in the case of
the user running out of space on the same volume as the trash/recycle-bin
because that's too hard to check for anyway and users will just have to figure
that out, I say).
I think the suggestions for putting the file names and amount of space still
needed are useful. I'm going to work on mashing together mpt's error text and
suggestion strings and getting the file name and size inserted, as he and Boris
suggest.
I may not get to that real soon, though. Can I get OK to check in what I have
and deal with improving the messages via another bug?
Comment 68•23 years ago
|
||
> SetOpenWith(openWith) not using |.get()|.
There was a push at some point to remove operator char* from all the string
classes (bug 53057). I guess nsXPIDLC?String is safe from that for now, so do
whichever you prefer here. :)
Assignee | ||
Comment 69•23 years ago
|
||
Blake commented (in another bug) that the title bar should read something like:
37% - Saving foobar.zip
i.e., move the percentage to the beginning so it appears in the task bar.
I like the idea of getting as much info as possible into the task bar, but I'm
not sure this doesn't make the full title look a little awkward.
Comments?
Comment 70•23 years ago
|
||
I think we should only show the percentage in the titlebar when the window is
minimized, a la IE. This is very easy now that minimized/maximized/restored
state flags have been implemented on nsIDOMChromeWindow.
Someone commented that they want it in the titlebar all the time in case they
have something obscuring the rest of that window, but that hardly seems like the
common case to me. I think always displaying it in the titlebar as well as the
window is overkill, and awkward as you point out.
Comment 71•23 years ago
|
||
I'd like to ditto Blake's comments.
Comment 72•23 years ago
|
||
A couple comments on the nsWebBrowserPersist changes:
* could the block of new code (switch) in OnDataAvailable be moved to a
separate method? (I worry this block may need to grow to accommodate new errors.)
* I'm concerned about the string bundle dependency. Wouldn't it be better if
the persist code returned a specific set of error codes which the client/caller
could interpret/describe as it saw fit?
Hardware: PC → All
Assignee | ||
Comment 73•23 years ago
|
||
re: moving the switch statement
Sounds reasonable. It wouldn't need to be a method (member function), though.
Is a static utility function OK? That would make it easier for me to cut/paste
it into the uriloader code.
re: property file dependency
This was suggested by Jud Valeski so that the code could be shared by clients
that didn't care to embellish the alert text at all. Also, it kind of matches
the strategy deployed by the networking code.
A more pragmatic reason is that it is difficult, in general, to pass nsresult
values to JavaScript code because they're only defined by C++ macros (although
some, including all that we'd want to examine now, are also available to JS via
Components.results).
I wasn't thrilled with this scheme, either, but it's what we decided to do when
the design was being debated.
Comment 74•23 years ago
|
||
As a note, the patches in this bug do not solve the problem of MoveTo() failing
in the uriloader due to lack of space or some such. This is covered by bug
115201.
Assignee | ||
Comment 75•23 years ago
|
||
Your note is correct. Nor does it handle some cases where the error
notifications come in via OnStopRequest.
If I get to those things soon, then I'll incorporate them; otherwise, that will
have to wait a bit longer.
Assignee | ||
Comment 76•23 years ago
|
||
I've updated this patch to refactor the error notification code into a separate
routine as suggested by Kathy Brade.
I've also slightly tweaked the code she added recently to call StartUpload.
That should only happen if no error has occurred.
Attachment #68684 -
Attachment is obsolete: true
Assignee | ||
Comment 77•23 years ago
|
||
Updated patch to uriloader/exthandler stuff.
This refactors the code that sends the error notifications as was done in
nsWebBrowserPersist.
I also added some error checking to handle errors when moving the temp file.
This is less that ideal still, because the return value is always
NS_ERROR_FAILURE so the message ends up as the generic "There was an error
writing to the target location." nsILocalFile implementationsof MoveFile are
going to have to be fixed to give better NS_ERROR_FILE_* messages.
Likewise, I added error checking after calling LauchAppWithTempFile. That
doesn't quite work though, because it seems the implementors of that function
don't return error codes.
Attachment #68268 -
Attachment is obsolete: true
Assignee | ||
Comment 78•23 years ago
|
||
Updated patch; no material changes.
Attachment #68251 -
Attachment is obsolete: true
Assignee | ||
Comment 79•23 years ago
|
||
re: progress window title
OK, I'm trying to wrap this part up and I see where IE (v5.5) uses
"37% of big.zip Completed"
and that's the title all the time.
It would be easier if I didn't have two distinct titles so I wonder if I can
just use the same technique? That would also permit me to ignore differences
when saving versus opening-with.
Thoughts?
Also, noboby has come up with a solid proposal for the error alert text. I'm
going to proceed with my best shot at merging the comments that have been
received. I don't think we need to embellish the "out of space" message beyond
inserting the target file path (the about downloaded so far is in the progress
window and if users are counting their bytes that closely, they can do the
math).
Comment 80•23 years ago
|
||
Comment on attachment 69774 [details] [diff] [review]
Revised patch for uriloader/exthandler code
r=bzbarsky. Make sure to make the mac build changes corresponding to those
REQUIRES changes.
Attachment #69774 -
Flags: review+
Comment 81•23 years ago
|
||
Comment on attachment 69776 [details] [diff] [review]
Updated patch to contentAreaUtils
>+ const flags = nsIWBP.PERSIST_FLAGS_NO_CONVERSION | nsIWBP.PERSIST_FLAGS_REPLACE_EXISTING_FILES;
...
>+ var encodingFlags = flags;
>+ if (persistArgs.contentType == "text/plain") {
>+ encodingFlags |= nsIWBP.ENCODE_FLAGS_FORMATTED;
>+ encodingFlags |= nsIWBP.ENCODE_FLAGS_ABSOLUTE_LINKS;
>+ encodingFlags |= nsIWBP.ENCODE_FLAGS_NOFRAMES_CONTENT;
>+ }
Do we really want to have "PERSIST_FLAGS_NO_CONVERSION" and
"PERSIST_FLAGS_REPLACE_EXISTING_FILES"
in the encoding flags? That seems odd.... For one thing as encoding flags
that would
be ENCODE_FLAGS_PREFORMATTED and ENCODE_FLAGS_WRAP.... Is that purposeful? If
so,
just make it explicit.
Also, could we always pass nsIWBP.ENCODE_FLAGS_ENCODE_ENTITIES? that will fix
some
bugs we have on entities being saved as characters in some charset instead...
The rest looks good.
Assignee | ||
Comment 82•23 years ago
|
||
I suspect that encodingFlags stuff is wrong. I just cribbed that code from
elsewhere and may have garbled it in the translation. The code I copied it
from was messed up, too, as I recall.
I'll straighten that out.
Assignee | ||
Comment 83•23 years ago
|
||
I've rolled into this patch the new .properties file, in which I've
incorporated mpt's wording (for the most part). Each message incorporates the
target file path as an insert.
Attachment #68270 -
Attachment is obsolete: true
Attachment #68301 -
Attachment is obsolete: true
Attachment #68790 -
Attachment is obsolete: true
Attachment #69769 -
Attachment is obsolete: true
Assignee | ||
Comment 84•23 years ago
|
||
I've enhanced the error handling to insert the file name (temporary vs. final)
into the messages.
Attachment #69774 -
Attachment is obsolete: true
Assignee | ||
Comment 85•23 years ago
|
||
Fixes the encodingFlags snafu and changes the title text to put the percentage
first.
Attachment #69776 -
Attachment is obsolete: true
Comment 86•23 years ago
|
||
Comment on attachment 70175 [details] [diff] [review]
Final(?) contentAReaUtils.js patch
r=bzbarsky
Attachment #70175 -
Flags: review+
Comment 87•23 years ago
|
||
Comment on attachment 70173 [details] [diff] [review]
Final(?) uriloader/exthandler patch
r=bzbarsky
Attachment #70173 -
Flags: review+
Assignee | ||
Comment 89•23 years ago
|
||
fix checked in
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Comment 90•23 years ago
|
||
The nsWebBrowserPersist changes created a new compiler warning:
embedding/components/webbrowserpersist/src/nsWebBrowserPersist.cpp:674
`PRBool readError' might be used uninitialized in this function
I am not absolutely sure if this can really happen, or it's just a compiler
being stuping, but it seems that if function is called with 0 aLenght, something
indeed might happen...
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 91•23 years ago
|
||
Another warning caused by this check-in:
uriloader/exthandler/nsExternalHelperAppService.cpp:1159
`PRBool readError' might be used uninitialized in this function
Comment 92•23 years ago
|
||
I looked at that when I was reviewing.. I don't think onDataAvailable is ever
called with a 0 data length (what would be the point?).
We can add an initializer just for kicks, I suppose....
Comment 93•23 years ago
|
||
FYI, I've had bugs in the past where the length in an ODA call was 0 and I had
code which wasn't coping with that properly. So it does happen.
Comment 94•23 years ago
|
||
OK, looking over the code again nsExternalHelperAppService will never hit the
code that uses readError with readError unset. WebBrowserPersist could do it
if mCancel is true....
Attaching simple patch to fix warnings.
Comment 95•23 years ago
|
||
Comment on attachment 70686 [details] [diff] [review]
Patch to fix warnings
r=hwaara
Attachment #70686 -
Flags: review+
Assignee | ||
Comment 96•23 years ago
|
||
Mozilla lets you open new bugs, too. I suggest we do that in this case.
And by the way, the reporting of errors while saving files has been totally
fucked up from the git go. If we find some residual problem in that area, it
would not surprise me. In fact, if we don't, that would shock me. But when it
happens. let's please open new bugs specific to the remaining problems rather
than keep re-opening this one.
Status: REOPENED → RESOLVED
Closed: 23 years ago → 23 years ago
Resolution: --- → FIXED
Comment 97•23 years ago
|
||
Am I right assuming that marking thius bug as "fixed" would not stop attachment
70686 [details] [diff] [review] from going in? Or should I open a new bug for the warnings?
Comment 98•23 years ago
|
||
I'd open a separate bug. Just assign it to me.
Comment 99•23 years ago
|
||
tested using 2002.03.06 commercial verif builds on linux rh7.2, mac OS X v10.1.3
and win2k. seems that there are four basic test cases used to verify this bug:
a. write permissions
b. out of space in target directory
c. check that target directory exists
d. out of space in temp directory
detailed results are below. since issues still remain here, i'm not sure if i
should verify this particular bug. however, i have spun off or reopened bugs as
needed, so maybe this should get closed...?
in any case, there still are related bugs which might be part of the dependency
tree. will verify those seperately (when fixed) since they haven't been dup'd
here. i wish to verify this particular bug in a finite manner. :)
a. write permissions. primarily Linux and prolly mac OS X. make sure to display
an alert when the user lacks the priv to write to targetted directory. many dups.
example: create a dir that's owned by root (or another user) and give it 700
mode. the try to save file to that dir.
RESULTS:
* linux: PASS. get an alert dlg saying "Cannot create file. Directory [path] is
not writable."
* mac: PASS. in the file picker, folders that don't have the proper privs are
simply inaccessible, eg, greyed out or have a lock icon.
* win2k: tested with a locked floppy. PASS, sort of. got the alert dlg "The disk
cannot be written to because it is write protected. Please remove the write
protection from the volume in drive A:." but the download progress remained and
appeared completed --even though no saving occurred.
b. out of space. All platforms. should get an alert where download cannot
complete due to lack of space. many dups. cleanup issues remain: see bug 129332.
linux example: try to save to full/limited harddisk --different partition than /tmp.
mac example: try to save to full/limited Zip disk.
win2k example: try to save to full/limited ramdisk.
RESULTS:
* linux: FAIL. see bug 129614. seemed to download successfully, but it was
incomplete. no error dlg came up --and the salted file persisted in /tmp even
after quitting. will try w/floppy later on...
* mac: PASS. got an alert dlg "There is not enough room on the disk to save to
Zip 100:[file]. Remove unnecessary files from that disk and try again, or try
saving in a different location."
* win2k: FAIL, also see bug 129614.
one-time occurrence, though, on win2k: got an alert dlg "A:\[file] could not be
saved, because an unknown error occurred. Sorry about that. Try saving to a
different location."
c. make sure target directory exists. All platforms. was bug 80242.
example: in file picker for saving, enter a non-existent directory name.
RESULTS:
* linux: PASS. get an alert dlg saying " Path [path] doesn't exist, can't save
[path/filename]"
* mac: PASS. in the file picker for saving, i'm prevented from entering a bogus
directory --unlike the file picker for opening, which has a "go to" field for
such a purpose.
* win2k: FAIL. reopened bug 80242. bogus dir is created anyway, and file is
saved there.
d. since we first download to a temp location, then move to user-determined
target, if there's not enough room in the temp location, we should display an
alert. All platforms. was bug 105718 and bug 112175.
linux example: need to fill up /tmp a lot, then test saving.
mac example: need to fill up Desktop folder, then test saving.
win2k example: need to fill up c:\WINNT\Temp, then test saving.
RESULTS:
* linux: FAIL. see bug 129604.
* mac: overall FAIL when the default temp location is the Desktop --same results
as linux. this occurred with a new profile. also see bug 129604. one-time
"success": got an alert dlg "There is not enough room on the disk to save to
[non-Desktop-remembered location]:[salted filename]. Remove unnecessary file
from the disk and try again, or try saving to a different location." this.
however, was for an existing profile with that pref set.
* win2k: FAIL, also see bug 129604.
Assignee | ||
Comment 100•23 years ago
|
||
Hmmm. Maybe 8 test cases, because clicking on a link and going through the
helper app dialog (open-with vs. save-to-disk) is completely distinct from
right clicking and choosing save-link-as.
I'm guessing all your tests were using the former, which complicates things in
a number of ways (especially prone to error when the downloaded copy in temp is
moved/copied to the final destination).
I think things work more reliably if you use the save-link-as technique. Not
that the problems you cite aren't problems, too. They're just more likely
problems in the nsExternalAppHandlerService code.
Comment 101•23 years ago
|
||
Sairuh--when you are testing "write permissions" be sure to test not only
directory permissions but also file permissions. On a Mac, you can lock a file
by selecting it and choosing "File | Get Info | General Information" and then
checking the box near the bottom. I'm sure you know how to change the
permissions on Linux files. I think you can lock Windows-files but I don't
recall how at the moment.
Comment 102•23 years ago
|
||
Bug 131108 was just opened, which tells us that we are not dealing well with
saves to files on which we do not have write-permissions.
Comment 103•23 years ago
|
||
that bug is a duplicate of a separate bug which was recently fixed
Comment 104•23 years ago
|
||
hokay, finally ran through the more common scenarios, but this time using "save
link target as" from the link context menu.
this will be a partial rubberstamp verification, since i didn't repeat the cases
of a full temp location or full linux partition. if anyone would like to test
those scenarios, pls do add your observations/methods here!
anyhow, the detail tests and results (based on 2002.05.02.0x-1.0.0 branch comm
bits) are at http://hopey.mcom.com/tests/bug27609-save-link.txt --will post as
an attachment soon, since it's on an internal server.
Status: RESOLVED → VERIFIED
Comment 105•23 years ago
|
||
Updated•8 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•