Closed Bug 382388 Opened 15 years ago Closed 14 years ago

nsDownload::OnStateChange doesn't check aStatus for failure


(Toolkit :: Downloads API, defect)

Not set





(Reporter: Mardak, Assigned: Mardak)




(1 file, 3 obsolete files)

Attached patch FailDownloads if aStatus FAILED (obsolete) — Splinter Review
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 [1], 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).

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 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?
Attached patch v2 (obsolete) — Splinter Review
Assignee: nobody → edilee
Attachment #266557 - Attachment is obsolete: true
Attachment #282968 - Flags: review?(comrade693+bmo)
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]

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-
Attached patch v2.1 (obsolete) — Splinter Review
NS_ENSURE_SUCCESS for everything and return the last thing (Alert).
Attachment #282968 - Attachment is obsolete: true
Attachment #283046 - Flags: review?(comrade693+bmo)
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]

nit: s/NULL/nsnull/

Attachment #283046 - Flags: review?(comrade693+bmo)
Attachment #283046 - Flags: review+
Attachment #283046 - Flags: approval1.9?
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+
Attached patch for checkinSplinter Review
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
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
Checking in toolkit/locales/en-US/chrome/mozapps/downloads/;
/cvsroot/mozilla/toolkit/locales/en-US/chrome/mozapps/downloads/,v  <--
new revision: 1.10; previous revision: 1.9
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3 M9
Depends on: 408300
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.