945099 - [Download API] Enhance basic download API mochitest with attribute checks

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Jason Smith [:jsmith]]

Enhance the existing basic download API mochitest with more attribute checks.

Comment 1

[Jason Smith [:jsmith]]

Attachment: Patch v1

Comment 2

[Jason Smith [:jsmith]]

Comment on attachment 8340873 [details] [diff] [review]
Patch v1

Here's a first pass at enhancing the basic download API mochitest with more attribute checks.

Some questions for feedback before I clean this up for review:

1. When I checked the value of download.path, I noticed that it matched serve_file.sjs, rather than the download attribute name of test.bin. Any ideas why?

2. Accessing startTime right at downloadStart apparently is firing NS_ERROR_UNEXPECTED. Any ideas why?

3. Accessing error right at downloadStart sometimes will also fire NS_ERROR_UNEXPECTED. Any ideas why?

4. The total bytes amount at download start is equal to zero, not 1024 bytes. Any ideas why?

Comment 3

[[:fabrice] Fabrice Desré]

(In reply to Jason Smith [:jsmith] from comment #2)
> Comment on attachment 8340873 [details] [diff] [review]
> Patch v1
> 
> Here's a first pass at enhancing the basic download API mochitest with more
> attribute checks.
> 
> Some questions for feedback before I clean this up for review:
> 
> 1. When I checked the value of download.path, I noticed that it matched
> serve_file.sjs, rather than the download attribute name of test.bin. Any
> ideas why?

Hm no, that should really do the right thing.

> 2. Accessing startTime right at downloadStart apparently is firing
> NS_ERROR_UNEXPECTED. Any ideas why?
>
> 3. Accessing error right at downloadStart sometimes will also fire
> NS_ERROR_UNEXPECTED. Any ideas why?

We probably fail to initialize these when creating the DOM download objects. Should be an easy fix.

> 4. The total bytes amount at download start is equal to zero, not 1024
> bytes. Any ideas why?

It seems that the backend sends us a downloadStarted event as soon as possible, including even before all the http headers are available. Note that in some cases we will never now the total bytes anyway, if the server doesn't set it.

Comment 4

[Jason Smith [:jsmith]]

(In reply to Fabrice Desré [:fabrice] from comment #3)
> (In reply to Jason Smith [:jsmith] from comment #2)
> > Comment on attachment 8340873 [details] [diff] [review]
> > Patch v1
> > 
> > Here's a first pass at enhancing the basic download API mochitest with more
> > attribute checks.
> > 
> > Some questions for feedback before I clean this up for review:
> > 
> > 1. When I checked the value of download.path, I noticed that it matched
> > serve_file.sjs, rather than the download attribute name of test.bin. Any
> > ideas why?
> 
> Hm no, that should really do the right thing.

Okay. I filed bug 945323 to address that.

> 
> > 2. Accessing startTime right at downloadStart apparently is firing
> > NS_ERROR_UNEXPECTED. Any ideas why?
> >
> > 3. Accessing error right at downloadStart sometimes will also fire
> > NS_ERROR_UNEXPECTED. Any ideas why?
> 
> We probably fail to initialize these when creating the DOM download objects.
> Should be an easy fix.

Okay - bug 944682 & bug 945366 are filed for fixing that.

> 
> > 4. The total bytes amount at download start is equal to zero, not 1024
> > bytes. Any ideas why?
> 
> It seems that the backend sends us a downloadStarted event as soon as
> possible, including even before all the http headers are available. Note
> that in some cases we will never now the total bytes anyway, if the server
> doesn't set it.

Would we know the totalBytes by the time onstatechange fires (assuming the server sets it in the headers)?

Comment 5

[Jason Smith [:jsmith]]

What's existing here passed on try so far:

https://tbpl.mozilla.org/?tree=Try&rev=59c1bd61e390

Comment 6

[Jason Smith [:jsmith]]

Attachment: Patch v2

Comment 7

[Jason Smith [:jsmith]]

Comment on attachment 8345152 [details] [diff] [review]
Patch v2

Ready for review. Will kick off a try run shortly.

Comment 8

[Jason Smith [:jsmith]]

https://tbpl.mozilla.org/?tree=Try&rev=545f1ac4772e

Comment 9

[Jason Smith [:jsmith]]

(In reply to Jason Smith [:jsmith] from comment #8)
> https://tbpl.mozilla.org/?tree=Try&rev=545f1ac4772e

Try run is green.

Comment 10

[Aus Lacroix [:aus]]

Comment on attachment 8345152 [details] [diff] [review]
Patch v2

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

Looks like a good addition to the current tests. We can land this on it's own or as part of the first set of fixes for accessing attributes. Either/or is fine with me. :)

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/3bd167b0f89a

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3bd167b0f89a

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/33fe99145553