Closed Bug 382388 Opened 15 years ago Closed 14 years ago
Download::On State Change doesn't check a Status for failure
Downloads can get a failure status when stopping, but the download manager reports "Done." Other observers like nsExternalAppHandler will honor the failure even though the transfer completed , and it potentially will delete the file if it was requested to be opened. If the file is being saved to disk, it'll remain as a partial download. In either case, the download manager probably shouldn't be reporting "Done" and perhaps "Failed" instead. The attachment extracts the "failing" code of OnStatusChange to be reused for failures in OnStateChange, but it doesn't use the prompter to show the modal dialog. This means the downloads will just appear in the download manager with "Failed" (and no indication of why).  http://mxr.mozilla.org/mozilla/source/uriloader/exthandler/nsExternalHelperAppService.cpp#2129
Comment on attachment 266557 [details] [diff] [review] FailDownloads if aStatus FAILED Looks good to me, but I'm not a reviewer.
Attachment #266557 - Flags: review?(cbiesinger)
If bug 383716 is fixed, there's less of a need to check for failure because nsDownload will get OnStatus (and fail downloading) and won't be getting OnState Stop anyway. This also avoids the issue of making more error strings because there isn't one handy for OnState failures.
Comment on attachment 266557 [details] [diff] [review] FailDownloads if aStatus FAILED From discussion with biesi, we should probably also display a prompter message on failure from OnState and not just OnStatus. This will need a new generic error failure message in downloads.properties. Specific error messages /should/ be arriving from exthandler or webbrowserpersist OnStatus (i.e., OnState won't get called). If Bug 383716 is fixed, both exthandler and webbrowserpersist *won't* call OnState = stop. Are there any other observers that have nsDownload as a ProgressListener? Is it part of the "contract" that OnStatus /will/ be called on failure and *not* OnState?
Attachment #266557 - Flags: review?(cbiesinger) → review-
Edward, are you still working on this bug?
Patch's diff needs bug 394548. Tested with a modified WBP that always gave a failure OnStop because this bug was originally needed for Link Fingerprints.
Depends on: 394548
Comment on attachment 282968 [details] [diff] [review] v2 I know that you were just moving code around for the most part, but I'd be happier if you checked the result or cast to void (probably checking for most of those).
Attachment #282968 - Flags: review?(comrade693+bmo) → review-
NS_ENSURE_SUCCESS for everything and return the last thing (Alert).
Oops. I've fixed the spacing locally (in 2 places for GetStringFromName(NS_LITERAL_STRING... 2 spaces instead of 4)
Comment on attachment 283046 [details] [diff] [review] v2.1 nit: s/NULL/nsnull/ r=sdwilsh
Supposed to use nsnull instead of NULL even for raw PRUnichar pointers?
yes, use nsnull for everything
That's my understanding - technically they are the same thing (0) as far as I know
Attachment #283046 - Flags: approval1.9? → approval1.9+
NULL -> nsnull
Attachment #283046 - Attachment is obsolete: true
Checking in toolkit/components/downloads/src/nsDownloadManager.cpp; /cvsroot/mozilla/toolkit/components/downloads/src/nsDownloadManager.cpp,v <-- nsDownloadManager.cpp new revision: 1.131; previous revision: 1.130 done Checking in toolkit/components/downloads/src/nsDownloadManager.h; /cvsroot/mozilla/toolkit/components/downloads/src/nsDownloadManager.h,v <-- nsDownloadManager.h new revision: 1.49; previous revision: 1.48 done Checking in toolkit/locales/en-US/chrome/mozapps/downloads/downloads.properties; /cvsroot/mozilla/toolkit/locales/en-US/chrome/mozapps/downloads/downloads.properties,v <-- downloads.properties new revision: 1.10; previous revision: 1.9 done
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3 M9
You need to log in before you can comment on or make changes to this bug.