Don't send dl-start event for download resume, (Add dl-pause and dl-resume events?)

VERIFIED FIXED in mozilla1.9beta2

Status

()

Toolkit
Downloads API
VERIFIED FIXED
10 years ago
10 years ago

People

(Reporter: Devon Jensen, Assigned: sdwilsh)

Tracking

({regression})

Trunk
mozilla1.9beta2
regression
Points:
---
Bug Flags:
blocking1.9 +
in-testsuite +
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

10 years ago
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

10 years ago
Depends on: 103487
Keywords: regression
Hardware: PC → All
Target Milestone: --- → Firefox 3 M9
Version: unspecified → Trunk
(Assignee)

Updated

10 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
This might explain why when we resume, we start off with 0-byte files...
(Reporter)

Comment 2

10 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

10 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

10 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

10 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.
Blocks: 103487
No longer depends on: 103487
(Assignee)

Comment 6

10 years ago
So, am I correct in understanding that what you opened this bug for is no longer true?
(Reporter)

Comment 7

10 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

10 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
Flags: blocking-firefox3? → blocking-firefox3+
(Assignee)

Comment 9

10 years ago
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.
(Assignee)

Comment 10

10 years ago
OK, so we get to the resume, but the NS_BINDING_ABORT doesn't take effect until after that...
(Assignee)

Comment 11

10 years ago
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
Attachment #287230 - Attachment is obsolete: true
Attachment #287263 - Flags: review?(gavin.sharp)
Attachment #287263 - Flags: review?(edilee)
(Assignee)

Updated

10 years ago
Whiteboard: [has patch][needs r Mardak][needs r gavin]

Updated

10 years ago
Target Milestone: Firefox 3 M10 → Firefox 3 M11
(Assignee)

Updated

10 years ago
Attachment #287263 - Flags: review?(gavin.sharp)
(Assignee)

Updated

10 years ago
Whiteboard: [has patch][needs r Mardak][needs r gavin] → [has patch][needs r Mardak]

Comment 12

10 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

10 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

10 years ago
Any idea how long the timeout is for xpcshell testcases that loop forever? (Not to mention generating huge logs.)
(Assignee)

Comment 15

10 years ago
Tinderbox times out after 15 minutes I think.  For everyone else, there is no time out.
(Assignee)

Comment 16

10 years ago
Created attachment 288242 [details] [diff] [review]
v1.2

for checkin
Attachment #287263 - Attachment is obsolete: true
(Assignee)

Updated

10 years ago
Keywords: checkin-needed
Whiteboard: [needs new patch] → [has patch][has reviews]
(Assignee)

Updated

10 years ago
Whiteboard: [has patch][has reviews] → [has patch][has reviews][can land]
(Assignee)

Comment 17

10 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
Last Resolved: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [has patch][has reviews][can land]
Target Milestone: Firefox 3 M11 → Firefox 3 M10
(Assignee)

Updated

10 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

10 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
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.