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)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla1.9beta2

People

(Reporter: velcrospud, Assigned: sdwilsh)

References

Details

(Keywords: regression)

Attachments

(1 file, 2 obsolete files)

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
Depends on: 103487
Keywords: regression
Hardware: PC → All
Target Milestone: --- → Firefox 3 M9
Version: unspecified → Trunk
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.
Blocks: 103487
No longer depends on: 103487
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.
Assignee: nobody → comrade693+bmo
Flags: blocking-firefox3?
Target Milestone: Firefox 3 M9 → Firefox 3 M10
Flags: blocking-firefox3? → blocking-firefox3+
Attached patch v1.0 (obsolete) — Splinter Review
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...
Attached patch v1.1 (obsolete) — Splinter Review
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)
Whiteboard: [has patch][needs r Mardak][needs r gavin]
Target Milestone: Firefox 3 M10 → Firefox 3 M11
Attachment #287263 - Flags: review?(gavin.sharp)
Whiteboard: [has patch][needs r Mardak][needs r gavin] → [has patch][needs r Mardak]
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+
Without the patch the test will loop forever, which is fine.  That will fail.
Whiteboard: [has patch][needs r Mardak] → [needs new patch]
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.
Attached patch v1.2Splinter Review
for checkin
Attachment #287263 - Attachment is obsolete: true
Keywords: checkin-needed
Whiteboard: [needs new patch] → [has patch][has reviews]
Whiteboard: [has patch][has reviews] → [has patch][has reviews][can land]
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
Flags: in-testsuite+
Flags: in-litmus-
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
Status: RESOLVED → VERIFIED
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: