595888 - Show file size when attempting to download a file

Status: RESOLVED FIXED

(Firefox :: File Handling, enhancement)

Description

[Oskar Ivanić (:icecold)]

User-Agent:       Mozilla/5.0 (Windows NT 6.1; rv:2.0b6pre) Gecko/20100913 Firefox/4.0b6pre
Build Identifier: Mozilla/5.0 (Windows NT 6.1; rv:2.0b6pre) Gecko/20100913 Firefox/4.0b6pre

I read this post on DownloadSquad, and I think this should be added to Firefox. Doesn't seem like a big job. 

Reproducible: Always

Comment 1

[HRipper]

Attachment: Example with "Show File Size" addon

Comment 2

[bthaxor]

I agree, this should be part of basic functionality for a download manager (knowing how much you're downloading before you start).

Comment 3

[antonnio94]

This must be a blocker. Seems very easy to implement if a 3KB addon can do it.

Comment 4

[Csaba Kozák [:WonderCsabo]]

Kyle, Jim: is this planned to add to Fx4?

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Probably not, even though it'd probably be simple to add we're past string freeze.

Comment 6

[Csaba Kozák [:WonderCsabo]]

Thanks!

Mike: what's your thoughts on this?

Comment 7

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

We're past string freeze at this point, so unless we can do this using existing strings, I won't be able to approve it.

I would also want the design to be:

which is a: 5.21 MB Binary File
which is a: 24 KB PDF

etc.

Comment 8

[Csaba Kozák [:WonderCsabo]]

But why we need new strings for this?

The current download popup shows this:

wich is a: Binary file

Put in the size before the type doesn't need to change the strings, does it?

Comment 9

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

The proper order may or may not be locale specific.

Comment 10

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

In other words, while "24 KB PDF" might be best in English, in another language it might be proper to say something like "PDF that is 24 KB".

Comment 11

[Csaba Kozák [:WonderCsabo]]

True.

I think 

wich is a: Binary file, 5.21 MB

would be an acceptable compromise for all languages. Is there a forum for Fx localizers to discuss this?

Comment 12

[Kailas]

Attachment: showFileSize Patch

Patch to display file size.

Comment 13

[Philipp von Weitershausen [:philikon]]

Comment on attachment 505825 [details] [diff] [review]
showFileSize Patch

Numbers, units and punctuation needs to be localized (=translated). DownloadUtils.jsm already does the numbers and units bits for you:

  Cu.import("resource://gre/modules/DownloadUtils.jsm");
  let [number, unit] = DownloadUtils.convertByteUnits(bytes)
  return number + " " + unit;

For the punctuation you really want to a string saying "$1, $2". Unfortunately we're string frozen for Firefox 4 already...

Comment 14

[Justin Dolske [:Dolske]]

Yeah, alas we won't be able to take this for Firefox 4, but we can as soon as we ship 4 (And we're moving to faster release cycles, which means this could then be shipping in a release in as soon as a few months instead of a whole year).

Please also update to reflect to requested format in comment 7, and I think you'll also need to support the case of the file size being unknown (since not all servers provide that info).

Comment 15

[Kailas]

Attachment: Updated Patch

Show file Size Patch Updated according to [https://bugzilla.mozilla.org/show_bug.cgi?id=595888#c7  comment 7].  Pls ignore previous patch (attachment 505825 [details] [diff] [review]).

Comment 16

[Ed Lee :Mardak]

Comment on attachment 506667 [details] [diff] [review]
Updated Patch

sdwilsh: see last line! :p

>+++ b/toolkit/mozapps/downloads/src/nsHelperAppDlg.js.in	2011-01-25 14:39:52.820469641 +0800
Seems like nsHelperAppDlg.js.in no longer exists? (Seems to have gone ".in"-less")
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/downloads/nsHelperAppDlg.js

>+    showFileSize: function (bytes) {
>+      let [number, unit] = DownloadUtils.convertByteUnits(bytes) 
>+      return number + " " + unit + " ";
String concatenation is a big no-no for localization. The existing strings that this dialog uses are here:
http://mxr.mozilla.org/mozilla-central/source/toolkit/locales/en-US/chrome/mozapps/downloads/unknownContentType.properties#40

Using the string format that you've got in the patch, it would look something like..
%2$S %3$S %1$S

Where the localization note indicates 1 is the filetype string, 2 is the size, 3 is units

But it should probably be something different as in the screenshot, it shows:

"which is a: Binary File, 5.21MB"

We could change the prefix string "which is a:" or just make sure to fit the existing string. So maybe

%1$S, %2$S %3$S

or %1$S (%2$S %3$S)

?

>@@ -624,7 +630,7 @@
>+        type.value = this.mLauncher.contentLength ?  (this.showFileSize(this.mLauncher.contentLength) + typeString) : typeString;
Just pointing out if you didn't notice, just a few lines above this, there's a branch:
603         typeString = this.dialogElement("strings").getFormattedString("fileType", [primaryExtension.toUpperCase()]);

That'll convert something like "pdf" to "PDF file" according to the "fileType" string for the locale:
http://mxr.mozilla.org/mozilla-central/source/toolkit/locales/en-US/chrome/mozapps/downloads/unknownContentType.properties#51

The showFileSize might not be necessary and you could just do things inline while avoiding really long lines:

if (contentLength) {
  let size = ..;
  type.value = format(type, size, unit);
}
else
  type.value = type;

sdwilsh: We don't have a good way for testing this? I suppose we could go ahead with the function so that it could be callable.. somehow.... Or maybe push it into DownloadUtils.jsm where DownloadUtils.getSizeAndType(numberOfBytes, MimeInfo) returns a string? That would be much easier to test.

Comment 17

[Shawn Wilsher :sdwilsh]

(In reply to comment #16)
> sdwilsh: We don't have a good way for testing this? I suppose we could go ahead
> with the function so that it could be callable.. somehow.... Or maybe push it
> into DownloadUtils.jsm where DownloadUtils.getSizeAndType(numberOfBytes,
> MimeInfo) returns a string? That would be much easier to test.
We could test it by throwing up the download window and checking its contents like this test does:
https://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/downloads/tests/chrome/test_unknownContentType_dialog_layout.xul

Comment 18

[Ed Lee :Mardak]

(In reply to comment #16)
> Using the string format that you've got in the patch, it would look something
> like..
> %2$S %3$S %1$S
> 
> But it should probably be something different as in the screenshot, it shows:
> "which is a: Binary File, 5.21MB"
Oh nevermind this comment. I see comment #7 and reading actual text instead of numbers, it makes more sense. ;)

Comment 19

[Kailas]

Attachment: Updated Patch

Updated Patch

Comment 20

[Shawn Wilsher :sdwilsh]

Comment on attachment 520426 [details] [diff] [review]
Updated Patch

>+++ b/mozilla-central/toolkit/locales/en-US/chrome/mozapps/downloads/unknownContentType.properties	2011-03-19 18:44:09.681722553 +0800
>+fileSizeWithType=%1S %2S %3S
Need a localization note here otherwise localizes will have no idea what each of these is for.

>+++ b/mozilla-central/toolkit/mozapps/downloads/nsHelperAppDlg.js	2011-03-19 18:42:43.951658472 +0800
>+    if(this.mLauncher.contentLength){
>+      let [number, unit] = DownloadUtils.convertByteUnits(this.mLauncher.contentLength);
>+      type.value = this.dialogElement("strings").getFormattedString("fileSizeWithType", [number,unit,typeString]);
nit: space after commas in the array please

Should be r+ once you fix the l10n issue (and should happen faster; sorry for the delay in getting this review).

Comment 21

[Kailas]

swdlish: The UI for review shows:
 Binary File, 5.21 MB
Whereas the patch I developed will show:
 5.21 MB Binary File

I created patch according to comment 7.

Comment 22

[Kailas]

Attachment: Updated Patch: changeset: 64633:df19f4679db1

Updated patch according to comment 20

Prepare patch on changeset:64633:df19f4679db1.

Comment 23

[Shawn Wilsher :sdwilsh]

(In reply to comment #21)
> swdlish: The UI for review shows:
>  Binary File, 5.21 MB
> Whereas the patch I developed will show:
>  5.21 MB Binary File
> 
> I created patch according to comment 7.
Can you upload an image with the correct output then please?

Comment 24

[Kailas]

Attachment: Screenshot of Show File Size Patch

Screenshot of the output of the patch

Comment 25

[Shawn Wilsher :sdwilsh]

Comment on attachment 523751 [details] [diff] [review]
Updated Patch: changeset: 64633:df19f4679db1

>+++ b/toolkit/mozapps/downloads/nsHelperAppDlg.js
>+    if(this.mLauncher.contentLength){
nit: space after if, before (.  Likewise, space after ), before {

>+      let [size, unit] = DownloadUtils.convertByteUnits(this.mLauncher.contentLength);
>+      type.value = this.dialogElement("strings").getFormattedString("fileSizeWithType", [size, unit, typeString]);
nit: we have line wrapping requirements at 80 characters.

>+    }
>+    else
>+      type.value = typeString;
nit: brace all parts of an if statement

r=sdwilsh with these changes; thanks for the patch!

Comment 26

[Kailas]

Attachment: Updated patch - changeset: 67893:f9c427576ca9

Patch updated according to suggestion in comment 25. 
Patch is generated on  changeset:67893:f9c427576ca9

Comment 27

[Oskar Ivanić (:icecold)]

Will this make it in Firefox 5? Since merge to Aurora is today. (2011-04-12)

Comment 28

[Philipp von Weitershausen [:philikon]]

(In reply to comment #27)
> Will this make it in Firefox 5? Since merge to Aurora is today. (2011-04-12)

Nope, the merge already happened this morning. But the next train isn't far from now (6 weeks)!

Comment 29

[Shawn Wilsher :sdwilsh]

We are also still waiting for UI review.

Comment 30

[Shawn Wilsher :sdwilsh]

Comment on attachment 525303 [details] [diff] [review]
Updated patch - changeset:   67893:f9c427576ca9

r=sdwilsh

Comment 31

[[Baboo]]

Note that theoretically a web site might report an exaggerated large value but then send a FIN after N bytes and Firefox (and other browsers) would still report the download as a success.

Comment 32

[Siddhartha Dugar [:sdrocking]]

would love to see some work on this.

Comment 33

[Kailas]

sdwilsh: Are we still waiting for UI review?

Comment 34

[Shawn Wilsher :sdwilsh]

(In reply to Kailas from comment #33)
> sdwilsh: Are we still waiting for UI review?
Yeah, the flag is still set to ? here sadly.

Comment 35

[Justin Dolske [:Dolske]]

Comment on attachment 523847 [details]
Screenshot of Show File Size Patch

Limi's currently on vacation, bouncing over to faaborg.

I suggest making it read slightly differently, though. Instead of:

    which is a: 632 KB PDF document

change to

    which is a: PDF document (634 KB)

Comment 36

[Kailas]

Attachment: Patch 2: on changest:72611:8753de11b181

Patch 2 according to comment 35

Comment 37

[Kailas]

Attachment: Screenshot for Patch 2

Screenshot for patch 2 created according to comment 35

Comment 38

[Justin Dolske [:Dolske]]

Comment on attachment 553173 [details] [diff] [review]
Patch 2: on changest:72611:8753de11b181

(Stealing trivial review... *yoink*!)

Comment 39

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

Comment on attachment 523847 [details]
Screenshot of Show File Size Patch

sure

Comment 40

[Justin Dolske [:Dolske]]

Thanks for the patch!

I've landed this on the fx-team integration branch, it should be merged to mozilla-central in the next day or two.

http://hg.mozilla.org/integration/fx-team/rev/5ff103433c3b

Comment 41

[Rob Campbell [:rc] (:robcee)]

http://hg.mozilla.org/mozilla-central/rev/5ff103433c3b

Comment 42

[Stefan]

I try to download a file which is 4,194,304 bytes big. This is exactly 4 MiB. Fx shows instead:

   BIN file (4,0  MB)

IMHO Fx should use one of the two space characters between the number and the unit to indicate that "M" actually means 1024². Cf. http://en.wikipedia.org/wiki/Mebi#Adoption_by_IEC_and_NIST