Last Comment Bug 595888 - Show file size when attempting to download a file
: Show file size when attempting to download a file
Status: RESOLVED FIXED
[fixed-in-fx-team]
:
Product: Firefox
Classification: Client Software
Component: File Handling (show other bugs)
: Trunk
: All All
: -- enhancement with 10 votes (vote)
: Firefox 8
Assigned To: Kailas
:
Mentors:
http://www.downloadsquad.com/2010/09/...
: 328954 (view as bug list)
Depends on: 680812 683258
Blocks:
  Show dependency treegraph
 
Reported: 2010-09-13 09:29 PDT by Oskar Ivanić (:icecold)
Modified: 2012-06-09 02:29 PDT (History)
25 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix


Attachments
Example with "Show File Size" addon (19.79 KB, image/jpeg)
2010-09-13 10:15 PDT, HRipper
no flags Details
showFileSize Patch (1.20 KB, patch)
2011-01-21 09:09 PST, Kailas
no flags Details | Diff | Splinter Review
Updated Patch (1.16 KB, patch)
2011-01-24 22:49 PST, Kailas
edilee: review-
Details | Diff | Splinter Review
Updated Patch (2.42 KB, patch)
2011-03-19 03:53 PDT, Kailas
sdwilsh: review-
Details | Diff | Splinter Review
Updated Patch: changeset: 64633:df19f4679db1 (4.90 KB, patch)
2011-04-01 21:31 PDT, Kailas
sdwilsh: review+
Details | Diff | Splinter Review
Screenshot of Show File Size Patch (24.20 KB, image/jpeg)
2011-04-02 22:12 PDT, Kailas
no flags Details
Updated patch - changeset: 67893:f9c427576ca9 (5.00 KB, patch)
2011-04-11 22:17 PDT, Kailas
sdwilsh: review+
Details | Diff | Splinter Review
Patch 2: on changest:72611:8753de11b181 (5.01 KB, patch)
2011-08-15 07:34 PDT, Kailas
dolske: review+
Details | Diff | Splinter Review
Screenshot for Patch 2 (41.81 KB, image/png)
2011-08-15 07:38 PDT, Kailas
faaborg: ui‑review+
Details

Description Oskar Ivanić (:icecold) 2010-09-13 09:29:18 PDT
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 2010-09-13 10:15:25 PDT
Created attachment 474730 [details]
Example with "Show File Size" addon
Comment 2 bthaxor 2010-09-13 13:52:35 PDT
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 2010-10-16 13:58:50 PDT
This must be a blocker. Seems very easy to implement if a 3KB addon can do it.
Comment 4 Csaba Kozák [:WonderCsabo] 2010-10-16 14:20:42 PDT
Kyle, Jim: is this planned to add to Fx4?
Comment 5 Kyle Huey [:khuey] (khuey@mozilla.com) 2010-10-16 14:22:20 PDT
Probably not, even though it'd probably be simple to add we're past string freeze.
Comment 6 Csaba Kozák [:WonderCsabo] 2010-10-18 09:09:31 PDT
Thanks!

Mike: what's your thoughts on this?
Comment 7 Mike Beltzner [:beltzner, not reading bugmail] 2010-10-21 10:37:37 PDT
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] 2010-10-22 03:14:49 PDT
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 [:khuey] (khuey@mozilla.com) 2010-10-22 03:36:10 PDT
The proper order may or may not be locale specific.
Comment 10 Kyle Huey [:khuey] (khuey@mozilla.com) 2010-10-22 03:49:04 PDT
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] 2010-10-22 03:56:49 PDT
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 2011-01-21 09:09:44 PST
Created attachment 505825 [details] [diff] [review]
showFileSize Patch

Patch to display file size.
Comment 13 Philipp von Weitershausen [:philikon] 2011-01-24 20:43:48 PST
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] 2011-01-24 20:56:32 PST
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 2011-01-24 22:49:24 PST
Created attachment 506667 [details] [diff] [review]
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 2011-02-14 14:19:46 PST
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 2011-02-14 14:23:57 PST
(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 2011-02-14 14:31:26 PST
(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 2011-03-19 03:53:14 PDT
Created attachment 520426 [details] [diff] [review]
Updated Patch

Updated Patch
Comment 20 Shawn Wilsher :sdwilsh 2011-04-01 12:12:32 PDT
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 2011-04-01 21:22:11 PDT
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 2011-04-01 21:31:38 PDT
Created attachment 523751 [details] [diff] [review]
Updated Patch: changeset: 64633:df19f4679db1

Updated patch according to comment 20

Prepare patch on changeset:64633:df19f4679db1.
Comment 23 Shawn Wilsher :sdwilsh 2011-04-02 20:51:13 PDT
(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 2011-04-02 22:12:42 PDT
Created attachment 523847 [details]
Screenshot of Show File Size Patch

Screenshot of the output of the patch
Comment 25 Shawn Wilsher :sdwilsh 2011-04-11 11:39:04 PDT
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 2011-04-11 22:17:24 PDT
Created attachment 525303 [details] [diff] [review]
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) 2011-04-12 08:54:16 PDT
Will this make it in Firefox 5? Since merge to Aurora is today. (2011-04-12)
Comment 28 Philipp von Weitershausen [:philikon] 2011-04-12 08:56:57 PDT
(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 2011-04-12 09:02:04 PDT
We are also still waiting for UI review.
Comment 30 Shawn Wilsher :sdwilsh 2011-04-12 13:42:29 PDT
Comment on attachment 525303 [details] [diff] [review]
Updated patch - changeset:   67893:f9c427576ca9

r=sdwilsh
Comment 31 [Baboo] 2011-04-19 07:54:27 PDT
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] 2011-07-03 00:51:40 PDT
would love to see some work on this.
Comment 33 Kailas 2011-08-03 20:43:27 PDT
sdwilsh: Are we still waiting for UI review?
Comment 34 Shawn Wilsher :sdwilsh 2011-08-07 15:30:11 PDT
(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] 2011-08-14 16:23:21 PDT
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 2011-08-15 07:34:38 PDT
Created attachment 553173 [details] [diff] [review]
Patch 2: on changest:72611:8753de11b181

Patch 2 according to comment 35
Comment 37 Kailas 2011-08-15 07:38:05 PDT
Created attachment 553174 [details]
Screenshot for Patch 2

Screenshot for patch 2 created according to comment 35
Comment 38 Justin Dolske [:Dolske] 2011-08-15 10:05:45 PDT
Comment on attachment 553173 [details] [diff] [review]
Patch 2: on changest:72611:8753de11b181

(Stealing trivial review... *yoink*!)
Comment 39 Alex Faaborg [:faaborg] (Firefox UX) 2011-08-15 12:42:35 PDT
Comment on attachment 523847 [details]
Screenshot of Show File Size Patch

sure
Comment 40 Justin Dolske [:Dolske] 2011-08-15 12:55:25 PDT
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) 2011-08-16 08:37:53 PDT
http://hg.mozilla.org/mozilla-central/rev/5ff103433c3b
Comment 42 Stefan 2011-08-16 10:47:25 PDT
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
Comment 43 Edgaras 2011-08-23 01:33:43 PDT
*** Bug 328954 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.