User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a8pre) Gecko/2007090504 Minefield/3.0a8pre Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a8pre) Gecko/2007090504 Minefield/3.0a8pre I'm listening to "dl-start" events from my extension. The problem is that "dl-start" events are being sent whenever a paused download is resumed. This didn't happen in firefox 2. I think that dl-start should only be sent once, when the download is initiated. dl-start is being sent whenever the state changes to DOWNLOAD_DOWNLOADING See: http://mxr.mozilla.org/seamonkey/source/toolkit/components/downloads/src/nsDownloadManager.cpp#1969 and http://mxr.mozilla.org/seamonkey/source/toolkit/components/downloads/src/nsDownloadManager.cpp#1560 Perhaps dl-pause and dl-resume events could be added - otherwise guess I'll have to register as a onDownloadStateChange listener... Reproducible: Always
This might explain why when we resume, we start off with 0-byte files...
Apparently the resuming is fixed, but I thought that as long as I'm requesting events... Ideally, an event would be sent for *every* download state. This would mean that in addition to a dl-pause and dl-resume events, one would like to see dl-notstarted and dl-queue events at the very beginning of the download.
Absolutely not. If you want something for every single state change, you should register a download progress listener. This is why we changed the interface to allow for multiple listeners in the first place...
Seems a waste to receive every progress update for every download when all I want are the state changes, but ok - it prob doesn't slow things down too much when cycling through that listener array all the time. Maybe it should be considered to depreciate the "dl-*" events if they aren't going to be comprehensive?
The point of the observers is to give a high level overview of downloads - when they start and when the stop. The point of the listeners is to give you a low level overview, which includes state change and a whole bunch of other stuff.
So, am I correct in understanding that what you opened this bug for is no longer true?
Well I'm no longer going to be using dl-start events in my extension, but that doesn't change the fact that doesn't make sense to issue a dl-start event for a resumed download I don't know what other extensions might currently be relying on dl-start events. Prob not many if any.
We just need to add a check here (http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/toolkit/components/downloads/src/nsDownloadManager.cpp&rev=1.144#1865) if the oldState was DOWNLOAD_PAUSED and *not* send out the event.
Created attachment 287230 [details] [diff] [review] v1.0 This test doesn't pass on my build, but I think it's just build hokeyness. I'm rebuidling and will request review if it works.
OK, so we get to the resume, but the NS_BINDING_ABORT doesn't take effect until after that...
Created attachment 287263 [details] [diff] [review] v1.1 Finally got this. Only took my four hours... The timer hack is sadly necessary for reasons that Edward can explain much better than I... For the record, concurrency sucks
Comment on attachment 287263 [details] [diff] [review] v1.1 >+ if (nsIDownloadManager::DOWNLOAD_QUEUED == oldState) Any reason for not doing it "variable == value"? Everywhere else we have it that way. I know it helps avoid accidentally using a single = instead of ==.. I had to follow it as a coding guideline for a previous place, but I could never get used to it. :p The code flow thought process is the other way ;) "is this variable equal to <something>?" About the test, it runs forever without the patch for me for some strange reason.. I look at the log and I see a bunch of CHECK FAILED: 1 == 2, 1 == 3, 1 == 4, etc and it keeps going on forever. I might have something strange going on in my test harness though.
Without the patch the test will loop forever, which is fine. That will fail.
Any idea how long the timeout is for xpcshell testcases that loop forever? (Not to mention generating huge logs.)
Tinderbox times out after 15 minutes I think. For everyone else, there is no time out.
Created attachment 288242 [details] [diff] [review] v1.2 for checkin
Checking in toolkit/components/downloads/src/nsDownloadManager.cpp; new revision: 1.146; previous revision: 1.145 Checking in toolkit/components/downloads/test/unit/test_bug_395092.js; initial revision: 1.1
Devon, if you get a chance, would you mind verifying this as fixed? Thanks!
Yes it is fixed Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b2pre) Gecko/2007120505 Minefield/3.0b2pre