273971 - nsIDownload should use bytes rather than kbytes

Status: RESOLVED FIXED

(SeaMonkey :: Download & File Handling, defect)

Description

[Zibi Braniecki [:zbraniecki][:gandalf]]

At the moment nsIDownload after fixing bug 239006 will be using kbytes.

Biesi wants to have it in bytes.

Comment 1

[Son Le]

Attachment: proposed patch

Changes pretty much localized to just nsDownloadManager.

The only place I could see that used amountTransferred and size was in downloads.js, but only as a ratio so no changes was necessary.

Comment 2

[[e-mail]]

(In reply to comment #1)
>Changes pretty much localized to just nsDownloadManager.
What about the original nsDownloadManager? :-P

Comment 3

[Son Le]

Attachment: proposed patch w/ xpfe changes

umm.. yes, and the original nsDownloadManager, too, even though it's not actually used.

Comment 4

[Christian :Biesinger (don't email me, ping me on IRC)]

eh, that download manager is very much used.

Comment 5

[Son Le]

yes, I meant to say that the variables amountTransferred/size (mCurrBytes/mMaxBytes) are not used by xpfe.

Comment 6

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 201629 [details] [diff] [review]
proposed patch w/ xpfe changes

xpfe does use it:
http://lxr.mozilla.org/seamonkey/source/xpfe/components/download-manager/src/nsDownloadManager.cpp#396

so does toolkit actually:
http://lxr.mozilla.org/seamonkey/source/toolkit/components/downloads/src/nsDownloadManager.cpp#464

you need to change those two, don't you?

Comment 7

[[e-mail]]

Comment on attachment 201629 [details] [diff] [review]
proposed patch w/ xpfe changes

This change will make the download manager display bytes (although still using the KB of KB string!). Is it outside the scope of this bug to make it display scientifically i.e. K/M/GB as appropriate? Also is it outside the scope of this bug to set mMaxBytes to mCurrBytes at the end of an unknown amount of tranfer?

Comment 8

[Son Le]

Attachment: patch v1

Hmmm... changed both references as per biesi's comment, although firefox works regardless.

> This change will make the download manager display bytes (although still using
> the KB of KB string!).

Yep, noticed this after building seamonkey. Seems xpfe uses the above code, but toolkit doesn't.

> Is it outside the scope of this bug to make it display scientifically 
> i.e. K/M/GB as appropriate?

Hmmm.. I'm be happy to do this is a separate bug, although there is existing toolkit code that does at least K/MB.

> Also is it outside the scope of this bug to set mMaxBytes to mCurrBytes at the 
> end of an unknown amount of tranfer?

Done.

Comment 9

[Son Le]

Comment on attachment 202490 [details] [diff] [review]
patch v1

updated superreview request to use neil's new email address.

Comment 10

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 202490 [details] [diff] [review]
patch v1

-  TransferInformation GetTransferInformation() {
+  void GetTransferInformation(PRInt64* aCurr, PRInt64* aMax) {

_why_?

Comment 11

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 202490 [details] [diff] [review]
patch v1

+  // update progress - for some reason in the last progress change
+  // for transfers of unknown size, aCurTotalProgress = -1.

is that a question? if so, the answer is:
http://lxr.mozilla.org/seamonkey/source/uriloader/exthandler/nsExternalHelperAppService.cpp#2067

I kinda think that this should be fixed in exthandler instead, and I actually believe that I once reviewed a patch that does that, no idea what happened to it...

anyway, this comment is stupid. remove the "for some reason" part. the reason is clear, and a bug (imo).

Comment 12

[Son Le]

(In reply to comment #10)
> (From update of attachment 202490 [details] [diff] [review] [edit])
> -  TransferInformation GetTransferInformation() {
> +  void GetTransferInformation(PRInt64* aCurr, PRInt64* aMax) {
> 
> _why_?
> 

To make xpfe code the same as toolkit. Couldn't see any value in making it pass a struct instead of variables and it wasn't being used anywhere else.

> I kinda think that this should be fixed in exthandler instead, and I actually
> believe that I once reviewed a patch that does that, no idea what happened to
> it...

that would be bug 149237.

> anyway, this comment is stupid. remove the "for some reason" part. the reason
> is clear, and a bug (imo).

I put the comment there as I didn't understand why xpfe did this while toolkit didn't. I agree it's a bug and will attach a new patch.

Comment 13

[Christian :Biesinger (don't email me, ping me on IRC)]

I explicitly changed the xpfe code to use that style (per a sr comment), and my patch to port that and a lot of other stuff to toolkit did never get review... the reason is to group variables together that belong together.

Comment 14

[Son Le]

hmmm... should I change toolkit code instead or just leave as it?

Comment 15

[Christian :Biesinger (don't email me, ping me on IRC)]

whatever you prefer.

Comment 16

[Son Le]

Attachment: patch v2

Fixed last progress change for transfers of unknown size and changed toolkit to use TransferInformation struct (making it the same as xpfe code).

carrying forward sr+

Comment 17

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 204110 [details] [diff] [review]
patch v2

r=biesi, sorry for the delay

Comment 18

[Christian :Biesinger (don't email me, ping me on IRC)]

fixed on trunk
Checking in toolkit/components/downloads/src/nsDownloadManager.cpp;
/cvsroot/mozilla/toolkit/components/downloads/src/nsDownloadManager.cpp,v  <--  nsDownloadManager.cpp
new revision: 1.56; previous revision: 1.55
done
Checking in toolkit/components/downloads/src/nsDownloadManager.h;
/cvsroot/mozilla/toolkit/components/downloads/src/nsDownloadManager.h,v  <--  nsDownloadManager.h
new revision: 1.19; previous revision: 1.18
done
Checking in uriloader/base/nsIDownload.idl;
/cvsroot/mozilla/uriloader/base/nsIDownload.idl,v  <--  nsIDownload.idl
new revision: 1.16; previous revision: 1.15
done
Checking in uriloader/exthandler/nsExternalHelperAppService.cpp;
/cvsroot/mozilla/uriloader/exthandler/nsExternalHelperAppService.cpp,v  <--  nsExternalHelperAppService.cpp
new revision: 1.295; previous revision: 1.294
done
Checking in xpfe/components/download-manager/src/nsDownloadManager.cpp;
/cvsroot/mozilla/xpfe/components/download-manager/src/nsDownloadManager.cpp,v  <--  nsDownloadManager.cpp
new revision: 1.113; previous revision: 1.112
done
Checking in xpfe/components/download-manager/src/nsDownloadManager.h;
/cvsroot/mozilla/xpfe/components/download-manager/src/nsDownloadManager.h,v  <--  nsDownloadManager.h
new revision: 1.43; previous revision: 1.42
done