Closed
Bug 395092
Opened 17 years ago
Closed 17 years ago
Don't send dl-start event for download resume, (Add dl-pause and dl-resume events?)
Categories
(Toolkit :: Downloads API, defect)
Toolkit
Downloads API
Tracking
()
VERIFIED
FIXED
mozilla1.9beta2
People
(Reporter: velcrospud, Assigned: sdwilsh)
References
Details
(Keywords: regression)
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review |
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
Assignee | ||
Updated•17 years ago
|
Depends on: 103487
Keywords: regression
Hardware: PC → All
Target Milestone: --- → Firefox 3 M9
Version: unspecified → Trunk
Assignee | ||
Updated•17 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment 1•17 years ago
|
||
This might explain why when we resume, we start off with 0-byte files...
Reporter | ||
Comment 2•17 years ago
|
||
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.
Assignee | ||
Comment 3•17 years ago
|
||
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...
Reporter | ||
Comment 4•17 years ago
|
||
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?
Assignee | ||
Comment 5•17 years ago
|
||
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.
Updated•17 years ago
|
Assignee | ||
Comment 6•17 years ago
|
||
So, am I correct in understanding that what you opened this bug for is no longer true?
Reporter | ||
Comment 7•17 years ago
|
||
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.
Assignee | ||
Comment 8•17 years ago
|
||
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.
Assignee: nobody → comrade693+bmo
Flags: blocking-firefox3?
Target Milestone: Firefox 3 M9 → Firefox 3 M10
Updated•17 years ago
|
Flags: blocking-firefox3? → blocking-firefox3+
Assignee | ||
Comment 9•17 years ago
|
||
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.
Assignee | ||
Comment 10•17 years ago
|
||
OK, so we get to the resume, but the NS_BINDING_ABORT doesn't take effect until after that...
Assignee | ||
Comment 11•17 years ago
|
||
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
Attachment #287230 -
Attachment is obsolete: true
Attachment #287263 -
Flags: review?(gavin.sharp)
Attachment #287263 -
Flags: review?(edilee)
Assignee | ||
Updated•17 years ago
|
Whiteboard: [has patch][needs r Mardak][needs r gavin]
Updated•17 years ago
|
Target Milestone: Firefox 3 M10 → Firefox 3 M11
Assignee | ||
Updated•17 years ago
|
Attachment #287263 -
Flags: review?(gavin.sharp)
Assignee | ||
Updated•17 years ago
|
Whiteboard: [has patch][needs r Mardak][needs r gavin] → [has patch][needs r Mardak]
Comment 12•17 years ago
|
||
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.
Attachment #287263 -
Flags: review?(edilee) → review+
Assignee | ||
Comment 13•17 years ago
|
||
Without the patch the test will loop forever, which is fine. That will fail.
Whiteboard: [has patch][needs r Mardak] → [needs new patch]
Comment 14•17 years ago
|
||
Any idea how long the timeout is for xpcshell testcases that loop forever? (Not to mention generating huge logs.)
Assignee | ||
Comment 15•17 years ago
|
||
Tinderbox times out after 15 minutes I think. For everyone else, there is no time out.
Assignee | ||
Updated•17 years ago
|
Keywords: checkin-needed
Whiteboard: [needs new patch] → [has patch][has reviews]
Assignee | ||
Updated•17 years ago
|
Whiteboard: [has patch][has reviews] → [has patch][has reviews][can land]
Assignee | ||
Comment 17•17 years ago
|
||
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
Status: NEW → RESOLVED
Closed: 17 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [has patch][has reviews][can land]
Target Milestone: Firefox 3 M11 → Firefox 3 M10
Assignee | ||
Updated•17 years ago
|
Flags: in-testsuite+
Flags: in-litmus-
Devon, if you get a chance, would you mind verifying this as fixed? Thanks!
Reporter | ||
Comment 19•17 years ago
|
||
Yes it is fixed
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b2pre) Gecko/2007120505 Minefield/3.0b2pre
Status: RESOLVED → VERIFIED
Updated•16 years ago
|
Product: Firefox → Toolkit
You need to log in
before you can comment on or make changes to this bug.
Description
•