Closed
Bug 205938
Opened 22 years ago
Closed 22 years ago
Clean up download rate in nsDownloadProgressListener.js
Categories
(SeaMonkey :: Download & File Handling, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: helgedt, Assigned: bugzilla)
Details
Attachments
(1 file, 1 obsolete file)
1.54 KB,
patch
|
janv
:
review+
jag+mozilla
:
superreview+
|
Details | Diff | Splinter Review |
Calculation of download rate in kilobytes/sec in nsDownloadProgressListener.js
presently uses unnecessary parseInt()s, divitions and modulations. Patch below
replaces these with one .toFixed(1), and corrects indentation. toFixed() is as
far as I can see also a better way to handle it because it can be localized
(comments
appreciated).
(should the use of toFixed be more obvious? like, kRate = kRate.toFixed(1);?)
Attachment #123506 -
Flags: review?(jaggernaut)
Comment 2•22 years ago
|
||
Comment on attachment 123506 [details] [diff] [review]
Cleans up rate-calculation + fixes indentation
I think you may want to change this to:
var kRate = (rate / 1024).toFixed(1);
In the old code, e.g. rate = 1024 and rate = 1025, kRate for both would be 10
for the check (kRate != this.priorRate), and thus considered as no (visible)
change ("Don't update too often!").
With your current patch, kRate becomes 1 and 1.0009765625 respectively, meaning
it now thinks there's a visible change, even though later on in replaceInsert
it turns out there isn't.
That said, the code you're patching seems broken.
Currently there seems to be no point to check if there's no visual change,
other than to avoid some simple calculations. If however
speedcol.setAttribute("label", rateMsg) were to be made dependent on (kRate !=
this.priorRate), the code would make a little more sense since you'd avoid
doing some layout/reflow work if visibly nothing would change.
E.g.
var timeRemainingCol = elt.nextSibling.nextSibling.nextSibling;
var statusCol = timeRemainingCol.nextSibling;
var speedCol = statusCol.nextSibling;
var kRate = (rate / 1024).toFixed(1);
if (!kRate)
kRate = "??.?";
if (kRate != this.priorRate) {
rateMsg = replaceInsert(rateMsg, 1, kRate);
speedCol.setAttribute("label", kRate);
this.priorRate = kRate;
}
Leaving out rateChanges here since rateChangesLimit is always 0.
Of course, the above code is only needed if we feel there are performance
problems with the download manager window. Otherwise this code could be
simplified to just always set rateMsg like it's doing now (no check against
this.priorRate).
Perhaps another bug is in order to fix that code. For now, just move the
.toFixed(1) up.
Attachment #123506 -
Flags: review?(jaggernaut) → review-
My first version actually used toFixed() at the top. Then I noticed it would be
comparing strings instead of floats (kRate != this.priorRate). But I see your
point.
Attachment #123506 -
Attachment is obsolete: true
Attachment #123521 -
Flags: review?(jaggernaut)
Comment 4•22 years ago
|
||
Comment on attachment 123521 [details] [diff] [review]
Patch v0.2, moves .toFixed(1) up by jag's request.
r/sr=jag
Attachment #123521 -
Flags: review?(jaggernaut) → superreview+
Attachment #123521 -
Flags: review?(blaker)
Comment 5•22 years ago
|
||
4 months, and we're still waiting on review for this bug?
You've got to be kidding me.
Updated•22 years ago
|
Attachment #123521 -
Flags: review?(blake) → review+
Comment 6•22 years ago
|
||
Fix checked in.
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Updated•21 years ago
|
Product: Browser → Seamonkey
You need to log in
before you can comment on or make changes to this bug.
Description
•