Closed Bug 1127867 Opened 8 years ago Closed 8 years ago

Use the new back-end property to get the size of downloads asynchronously

Categories

(Firefox :: Downloads Panel, defect)

defect
Not set
normal
Points:
3

Tracking

()

RESOLVED FIXED
Firefox 38
Iteration:
38.3 - 23 Feb
Tracking Status
firefox38 --- fixed

People

(Reporter: Paolo, Assigned: Paolo)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Attached patch The patchSplinter Review
We can now use the DownloadTarget.size property and the Download.refresh methods to get information about the size of the download on disk asynchronously.
Attachment #8557121 - Flags: feedback?(mak77)
Points: --- → 3
Comment on attachment 8557121 [details] [diff] [review]
The patch

Review of attachment 8557121 [details] [diff] [review]:
-----------------------------------------------------------------

it looks reasonable, and I hated maxBytes from the beginning.

::: browser/components/downloads/DownloadsCommon.jsm
@@ +366,2 @@
>  
> +      if (!download.stopped) {

looks like you removed usage of numScanning, should also remove its definition.

::: browser/components/downloads/content/downloadsViewCommon.js
@@ +172,5 @@
>      let text = "";
>      let tip = "";
>  
>      if (!this.download.stopped) {
> +      let total = this.download.hasProgress ? this.download.totalBytes : -1;

totalBytes (let's keep the unit). is at least slightly clearer than maxBytes.

@@ +206,5 @@
>        if (this.download.succeeded) {
> +        // For completed downloads, show the file size (e.g. "1.5 MB").
> +        if (this.download.target.size !== undefined) {
> +          let [size, unit] = DownloadUtils.convertByteUnits(
> +                                                  this.download.target.size);

nit: I prefer this form:
let [size, unit] =
  DownloadUtils.convertByteUnits(this.download.target.size);
Attachment #8557121 - Flags: feedback?(mak77) → feedback+
Blocks: 1129896
Attachment #8557121 - Flags: feedback?(smacleod)
Attachment #8557121 - Flags: feedback?(jaws)
Comment on attachment 8557121 [details] [diff] [review]
The patch

Review comments addressed in bug 1129896.
Attachment #8557121 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/acb012724bf0
Assignee: nobody → paolo.mozmail
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 38
Iteration: --- → 38.3 - 23 Feb
Flags: qe-verify-
Flags: firefox-backlog+
Attachment #8557121 - Flags: feedback?(jaws) → feedback+
Attachment #8557121 - Flags: feedback?(smacleod) → feedback+
Comment on attachment 8557121 [details] [diff] [review]
The patch

>+++ b/browser/components/downloads/content/allDownloadsViewOverlay.js

>+      // These properties may be updated if the user interface is refreshed.
>+      this.exists = false;
Should this be:
this.target.exists = false;
instead?
>+      this.target.size = undefined;
>     }
Flags: needinfo?(paolo.mozmail)
(In reply to Ian Neal from comment #5)
> Should this be:
> this.target.exists = false;
> instead?

Good catch! Mind to file a new bug to fix the typo? :-)
Flags: needinfo?(paolo.mozmail)
Blocks: 1195279
You need to log in before you can comment on or make changes to this bug.