Closed Bug 835803 Opened 7 years ago Closed 7 years ago

Add tests for downloads whose size is zero bytes

Categories

(Toolkit :: Downloads API, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: Paolo, Assigned: Paolo)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

The progress properties should have a consistent behavior and allow to
distinguish the case where the size of the file being downloaded is zero
bytes from the case where it is unknown.

Add tests so that this behavior is not regressed in the jsdownloads API.
Attached patch The patch (obsolete) — Splinter Review
This adds regression tests for the zero-byte cases and more general progress
tests, but also refactors the existing tests which use interruptible requests,
that previously had issues with the handler being unregistered and an error
page being returned.
Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED
Attachment #717527 - Flags: review?(enndeakin)
Attachment #717527 - Flags: feedback?(mak77)
Comment on attachment 717527 [details] [diff] [review]
The patch

Review of attachment 717527 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/jsdownloads/src/DownloadCore.jsm
@@ +604,5 @@
> +
> +          // Ensure we report the value of "Content-Length", if available, even
> +          // if the download doesn't generate any progress events later.
> +          if (aRequest instanceof Ci.nsIChannel &&
> +              aRequest.contentLength >= 0) {

may not aRequest need an explicit QueryInterface to nsIChannel before accessing contentLength?

::: toolkit/components/jsdownloads/test/unit/head.js
@@ +258,5 @@
>  ////////////////////////////////////////////////////////////////////////////////
>  //// Initialization functions common to all tests
>  
>  let gHttpServer;
> +let gEmptyRequestReceived;

this thing looks hackish, could you make it more like deferNextResponse?
Attachment #717527 - Flags: feedback?(mak77) → feedback+
(In reply to Marco Bonardo [:mak] (Away Mar 1) from comment #2)
> > +          // Ensure we report the value of "Content-Length", if available, even
> > +          // if the download doesn't generate any progress events later.
> > +          if (aRequest instanceof Ci.nsIChannel &&
> > +              aRequest.contentLength >= 0) {
> 
> may not aRequest need an explicit QueryInterface to nsIChannel before
> accessing contentLength?

"instanceof" takes care of that.

> > +let gEmptyRequestReceived;
> 
> this thing looks hackish, could you make it more like deferNextResponse?

I'll try!
(In reply to Paolo Amadini [:paolo] from comment #3)
> "instanceof" takes care of that.

oops, you're right
Attached patch Updated patchSplinter Review
Attachment #717527 - Attachment is obsolete: true
Attachment #717527 - Flags: review?(enndeakin)
Attachment #720421 - Flags: review?(enndeakin)
Attachment #720421 - Flags: review?(enndeakin) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e34d50ae941
Target Milestone: --- → mozilla22
https://hg.mozilla.org/mozilla-central/rev/3e34d50ae941
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.