The default bug view has changed. See this FAQ.

Show file size when attempting to download a file

RESOLVED FIXED in Firefox 8

Status

()

Firefox
File Handling
--
enhancement
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: icecold, Assigned: Kailas)

Tracking

Trunk
Firefox 8
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(status2.0 wontfix)

Details

(Whiteboard: [fixed-in-fx-team], URL)

Attachments

(5 attachments, 4 obsolete attachments)

(Reporter)

Description

7 years ago
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

7 years ago
Created attachment 474730 [details]
Example with "Show File Size" addon
Severity: normal → enhancement

Comment 2

7 years ago
I agree, this should be part of basic functionality for a download manager (knowing how much you're downloading before you start).
Status: UNCONFIRMED → NEW
Ever confirmed: true
Version: unspecified → Trunk

Comment 3

7 years ago
This must be a blocker. Seems very easy to implement if a 3KB addon can do it.
Kyle, Jim: is this planned to add to Fx4?
Probably not, even though it'd probably be simple to add we're past string freeze.
Keywords: uiwanted
Thanks!

Mike: what's your thoughts on this?
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.
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?
The proper order may or may not be locale specific.
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".
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

6 years ago
Created attachment 505825 [details] [diff] [review]
showFileSize Patch

Patch to display file size.
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...
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).
(Assignee)

Comment 15

6 years ago
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]).
(Assignee)

Updated

6 years ago
Attachment #506667 - Flags: review?(sdwilsh)

Updated

6 years ago
Assignee: nobody → patilkr24
Status: NEW → ASSIGNED
status2.0: --- → wontfix

Updated

6 years ago
Attachment #506667 - Flags: review?(sdwilsh) → review?(edilee)

Updated

6 years ago
Attachment #505825 - Attachment is obsolete: true

Comment 16

6 years ago
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.
Attachment #506667 - Flags: review?(edilee) → review-
(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

6 years ago
(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. ;)
(Assignee)

Comment 19

6 years ago
Created attachment 520426 [details] [diff] [review]
Updated Patch

Updated Patch
Attachment #506667 - Attachment is obsolete: true
Attachment #520426 - Flags: review?(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).
Attachment #520426 - Flags: review?(sdwilsh) → review-

Updated

6 years ago
Attachment #474730 - Flags: ui-review?(limi)
(Assignee)

Comment 21

6 years ago
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.
(Assignee)

Comment 22

6 years ago
Created attachment 523751 [details] [diff] [review]
Updated Patch: changeset: 64633:df19f4679db1

Updated patch according to comment 20

Prepare patch on changeset:64633:df19f4679db1.
Attachment #520426 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #523751 - Flags: review?(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?
(Assignee)

Comment 24

6 years ago
Created attachment 523847 [details]
Screenshot of Show File Size Patch

Screenshot of the output of the patch

Updated

6 years ago
Attachment #523847 - Flags: ui-review?(limi)

Updated

6 years ago
Attachment #474730 - Flags: ui-review?(limi)
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!
Attachment #523751 - Flags: review?(sdwilsh) → review+
(Assignee)

Comment 26

6 years ago
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
Attachment #523751 - Attachment is obsolete: true
Attachment #525303 - Flags: review?(sdwilsh)
(Reporter)

Comment 27

6 years ago
Will this make it in Firefox 5? Since merge to Aurora is today. (2011-04-12)
(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)!
We are also still waiting for UI review.
Comment on attachment 525303 [details] [diff] [review]
Updated patch - changeset:   67893:f9c427576ca9

r=sdwilsh
Attachment #525303 - Flags: review?(sdwilsh) → review+

Comment 31

6 years ago
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.
would love to see some work on this.
(Assignee)

Comment 33

6 years ago
sdwilsh: Are we still waiting for UI review?
(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 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)
Attachment #523847 - Flags: ui-review?(limi) → ui-review?(faaborg)
(Assignee)

Comment 36

6 years ago
Created attachment 553173 [details] [diff] [review]
Patch 2: on changest:72611:8753de11b181

Patch 2 according to comment 35
(Assignee)

Updated

6 years ago
Attachment #553173 - Flags: review?(sdwilsh)
(Assignee)

Comment 37

6 years ago
Created attachment 553174 [details]
Screenshot for Patch 2

Screenshot for patch 2 created according to comment 35
(Assignee)

Updated

6 years ago
Attachment #553174 - Flags: review?(faaborg)
Comment on attachment 553173 [details] [diff] [review]
Patch 2: on changest:72611:8753de11b181

(Stealing trivial review... *yoink*!)
Attachment #553173 - Flags: review?(sdwilsh) → review+
Comment on attachment 523847 [details]
Screenshot of Show File Size Patch

sure
Attachment #523847 - Flags: ui-review?(faaborg) → ui-review+
Attachment #523847 - Flags: ui-review+
Attachment #553174 - Flags: review?(faaborg) → ui-review+
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
Keywords: uiwanted
Whiteboard: [fixed-in-fx-team]
http://hg.mozilla.org/mozilla-central/rev/5ff103433c3b
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 8

Comment 42

6 years ago
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

Updated

6 years ago
Depends on: 680812

Updated

6 years ago
Duplicate of this bug: 328954
Depends on: 683258
You need to log in before you can comment on or make changes to this bug.