223895 - Completed downloads should include size and TLD instead of "Done"

Status: VERIFIED FIXED

(Toolkit :: Downloads API, enhancement, P2)

Description

[Steve Ward]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.6a) Gecko/20031027 Firebird/0.7+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.6a) Gecko/20031027 Firebird/0.7+

After a download is completed, observe the Status message.  It should display
something like:
"Download complete (elapsed time was {elapsed-time})"

There is redundant information.  For example, we already know that the download
is complete because of:
the window title bar (displays 100%),
the progress bar (displays 100%),
and the Time Left message (displays 00:00).

The Status message should give different information like:
"{download-size} completed at {transfer-rate}"

This new message displays (important) information which is not currently present
in a completed download dialog box:
the total size of the download,
and the effective transfer rate of the download.

By displaying the download size and the download transfer rate, the download box
would be more informative.

Because the Status message is the only thing being changed, this should not
result in a large code modification. hopefully :)


Reproducible: Always

Steps to Reproduce:
1. Download a file (without the download side bar, so you get a download box)
2. Wait for the download to complete
3. Observe the information in the "Status" message

Actual Results:  
The Status message displays:
"Download complete (elapsed time was {elapsed-time})"

Example:
"Download complete (elapsed time was 00:24)"


Expected Results:  
The Status message should display instead:
"{download-size} completed at {transfer-rate}"

Example:
"6136KB completed at 256 KB/sec"


I'll attach some pics of a completed download dialog box (current and suggested).

Comment 1

[Steve Ward]

Attachment: completed download dialog box - current

This is a pic of how the download dialog box looks when the download is
completed.

Comment 2

[Steve Ward]

Attachment: completed download dialog box - suggested

This is a modified pic of how the download dialog box would look when the
download is completed.
The only difference is the Status message.

Comment 3

[Robert Mohr]

I don't mind the redundant information, but the window is resized because of the
addition of this text.  If your mouse is on the left side of either [Launch
File] or [Show File Location], you mouse will no longer be on that button after
the download is completed.

Comment 4

[Ali Ebrahim]

taking qa

Comment 5

[Jo Hermans]

*** Bug 288385 has been marked as a duplicate of this bug. ***

Comment 6

[Shawn Wilsher :sdwilsh]

I might be able to get back to this, but if someone else wants to take it, please do!

Comment 7

[Ed Lee :Mardak]

The message shown for completed downloads will probably be in the form of "<size> in <time>" per mockups in http://wiki.mozilla.org/User:Dmose:Fx-Docs:DownloadManager/User_Interface#Iteration_2

Comment 9

[Ed Lee :Mardak]

Attachment: v1

Instead of saying just "Done"..

New string doneStatus=#1 #2 in #3 #4
#1 size; #2 size unit; #3 time; #4 time unit
examples: 1.1 MB in 33 seconds; 111 KB in 3.3 minutes

We need to store the end time in the richlist download item, so new param for createDownload.

Comment 10

[Ed Lee :Mardak]

Attachment: screenshot of v1

Comment 11

[Ed Lee :Mardak]

Patch needs bug 394798 to track the start time, bug 394516 for the time calculations (days, hours, minutes, seconds) [which in turn depends on bug 394263 for refactoring up updating the status].

Comment 12

[Ed Lee :Mardak]

Attachment: screenshot without stack of patches

There wasn't an up-to-date screenshot of what the download manager looks like now, so here's a screenshot without the stack of patches applied.

sdwilsh: good to know that the schema downgrade code is working perfectly ;)

$ hg qseries
DownloadProgressListener.cleanup.patch
trackProgress.patch
fixPaused.patch
queued.isdone.patch
better.time.patch
setStart.patch
done.sizeTime.patch

Comment 13

[Shawn Wilsher :sdwilsh]

Comment on attachment 279513 [details] [diff] [review]
v1

r=sdwilsh

Comment 14

[Ed Lee :Mardak]

Comment on attachment 279514 [details]
screenshot of v1

patch v1 will need to be refreshed (gQuery -> gBaseQuery)

Comment 15

[Ed Lee :Mardak]

Attachment: v1.1

Updating the patch now so that I don't forget later. :(

mconnor can set the ui-r on the screenshot (and this as well if he wants..)

Comment 16

[Ed Lee :Mardak]

Attachment: v1.2

unbitrot spacing and code motion from bug 394263

Comment 17

[Ed Lee :Mardak]

Comment on attachment 279514 [details]
screenshot of v1

switching to faaborg ui-r? per bug 394516 comment #12

Comment 18

[Alex Faaborg [:faaborg] (Firefox UX)]

Comment on attachment 279514 [details]
screenshot of v1

Switching ui-r to Madhava, who is taking over download manager design work.

Comment 19

[Ed Lee :Mardak]

Attachment: screenshot of v2

Not sure if this should be split off into another bug or just update this patch and rerequest reviews/approvals..

Unknown size = imported download data from previous versions where we didn't track file sizes
quick downloads = "about a second" (anything less than 1.5 seconds).. I originally went with "less than a second", but that wasn't quite accurate with 1.4 seconds being less than a second....

Comment 20

[Adam Kowalczyk]

(In reply to comment #19)
> Created an attachment (id=281484) [details]
> screenshot of v2

Please note that in the mockups the text is gray which makes the window look considerably less heavy.

Comment 21

[Ed Lee :Mardak]

madhava: Is this okay as an incremental step towards bug 397655? This bug will put in the size and transfer time. Another bug can be opened for the ETLD+1 part.

Comment 22

[Mike Beltzner [:beltzner, not reading bugmail]]

The design calls for "size - TLD" but not elapsed time. That information should be stored, however, so that it can be revealed by a detailed inspection of the download information (by an extension, or in the Places Manager, or whatever).

Morphing and blocking in support of bug 397665.

Comment 23

[Ed Lee :Mardak]

Depends on eTLD service instead of better time. Design will be based on attachment 283251 [details] from bug 397655.

Comment 24

[Ed Lee :Mardak]

Attachment: screenshot of v2

Comment 25

[Ed Lee :Mardak]

Attachment: v2

Use TLD instead of time. The extra switch/case is for later (the other states).

Comment 26

[Ed Lee :Mardak]

Attachment: v3

The beltzner part got moved into bug 400530, so this patch focuses on this bug. Changed it to use URIs instead of regex text parsing.. hopefully avoiding security issues ;) Added explicit port adding which was automatically there before with the regex. But now it's able to support unusual URIs like about:blank.

Comment 27

[Ed Lee :Mardak]

Attachment: screenshot of v3

Comment 28

[Shawn Wilsher :sdwilsh]

Comment on attachment 285594 [details] [diff] [review]
v3

>@@ -810,6 +816,55 @@ function updateStatus(aItem, aDownload) 
>       }
> 
>       break;
>+    case Ci.nsIDownloadManager.DOWNLOAD_FINISHED:
>+      let (stateSize) {
>+        switch (state) {
this doesn't make much sense at all - we are already switching on the state - why are you doing it again?

>+        let uri = ioService.newURI(getReferrerOrSource(aItem), null, null);
let's use Components.Constructor here like we do for nsLocalFile (nsURI or something)

Comment 29

[Ed Lee :Mardak]

(In reply to comment #28)
> >+    case Ci.nsIDownloadManager.DOWNLOAD_FINISHED:
> >+      let (stateSize) {
> >+        switch (state) {
> this doesn't make much sense at all - we are already switching on the state -
> why are you doing it again?
It's so that when I do case FAILED: case BLOCKED: case CANCELED:, I can put the correct text there and for FINISHED use the download size.

All of them show the TLD. I can do something fancier if you want if you want to avoid the double switch.

> >+        let uri = ioService.newURI(getReferrerOrSource(aItem), null, null);
> let's use Components.Constructor here like we do for nsLocalFile (nsURI or
> something)
Hrm? How so?

Comment 30

[Ed Lee :Mardak]

> > >+        let uri = ioService.newURI(getReferrerOrSource(aItem), null, null);
> > let's use Components.Constructor here like we do for nsLocalFile (nsURI or
> > something)
> Hrm? How so?
You sure there's something like that for services? The IOService will figure out what type of URI it is.. and nsIURI says..

An object of this interface must be created in the following way:
ioService.newURI(uri, charset, baseuri);

Comment 31

[Shawn Wilsher :sdwilsh]

(In reply to comment #30)
> You sure there's something like that for services? The IOService will figure
> out what type of URI it is.. and nsIURI says..
Oh bother - it only works for createInstance, and I knew that...
Disregard that remakr!

Comment 32

[Ed Lee :Mardak]

Attachment: v3.1

How about this instead of the extra switch? See next bug for the other states..

Comment 33

[Shawn Wilsher :sdwilsh]

Comment on attachment 285671 [details] [diff] [review]
v3.1

OK, this makes a lot more sense when looking at the other bug too...

r=sdwilsh

Comment 34

[Shawn Wilsher :sdwilsh]

dwitte is changing the eTLD api, so you may want to modify your patch accordingly.

Comment 35

[[e-mail]]

eTLD api is revised, patch ahead.

Comment 36

[[e-mail]]

Comment on attachment 285671 [details] [diff] [review]
v3.1

>+        try {
>+          // This might fail if it's an IP address or doesn't have >1 parts
>+          eTLD = eTLDService.getBaseDomain(uri, 1);

with the revised interface, you'll need to remove the |, 1|. also, naming the var |eTLD| is a little confusing since it's not necessarily the eTLD - how about calling it displayHost or somesuch?

also, the string from getBaseDomain will be always utf8 at present, irrespective of the idn whitelist. this may or may not jibe well for display (esp since you're using asciiHost as the fallback... why not just the regular host, btw?). not sure if you want to do anything about that, just pointing it out... i'll hopefully fix that behavior in a followup bug.

>+        } catch (e) {
>+          // Default to the host name
>+          eTLD = uri.asciiHost;
>+        }

r=me.

Comment 37

[Ed Lee :Mardak]

Attachment: v3.2

(In reply to comment #36)
> (From update of attachment 285671 [details] [diff] [review])
> >+          eTLD = eTLDService.getBaseDomain(uri, 1);
> with the revised interface, you'll need to remove the |, 1|
Removed.
> |eTLD| is a little confusing since it's not necessarily the eTLD
Yeah... -> displayHost ;)

> also, the string from getBaseDomain will be always utf8 at present,
> >+          eTLD = uri.asciiHost;
Switched to .host to be consistent with eTLD service. Also, I suppose to note, the whole statlus line will crop right.

approval1.9? and not M9? for now per mconnor's comments in bug 400616 comment #4:
"defer for now, if we get to a decision point real soon on what UI changes to make for M9 we may reconsider this."

Comment 38

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 286144 [details] [diff] [review]
v3.2

a=drivers for after M9

Comment 39

[Ed Lee :Mardak]

stephend: Do we have a litmus for this? "check that a completed download says something other than Done... hopefully size and TLD ;)"

Checking in toolkit/locales/en-US/chrome/mozapps/downloads/downloads.dtd;
/cvsroot/mozilla/toolkit/locales/en-US/chrome/mozapps/downloads/downloads.dtd,v  <--  downloads.dtd
new revision: 1.14; previous revision: 1.13
done
Checking in toolkit/locales/en-US/chrome/mozapps/downloads/downloads.properties;
/cvsroot/mozilla/toolkit/locales/en-US/chrome/mozapps/downloads/downloads.properties,v  <--  downloads.properties
new revision: 1.11; previous revision: 1.10
done
Checking in toolkit/mozapps/downloads/content/download.xml;
/cvsroot/mozilla/toolkit/mozapps/downloads/content/download.xml,v  <--  download.xml
new revision: 1.38; previous revision: 1.37
done
Checking in toolkit/mozapps/downloads/content/downloads.js;
/cvsroot/mozilla/toolkit/mozapps/downloads/content/downloads.js,v  <--  downloads.js
new revision: 1.99; previous revision: 1.98
done

Comment 40

[Stephen Donner [:stephend] Not actively reading bugmail]

(In reply to comment #39)
> stephend: Do we have a litmus for this? "check that a completed download says
> something other than Done... hopefully size and TLD ;)"

Not yet, but we will, soon; in the meantime, verified FIXED using:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b2pre) Gecko/2007111204 Minefield/3.0b2pre

Comment 41

[Stephen Donner [:stephend] Not actively reading bugmail]

https://litmus.mozilla.org/show_test.cgi?id=4855

in-litmus+

(I didn't really specify the testcases for size-rounding behavior, but I think it's not that big of a deal.)