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

Status: RESOLVED FIXED

(Firefox :: Downloads Panel, defect)

Description

[:Paolo Amadini]

Attachment: The patch

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.

Comment 1

[Marco Bonardo [:mak]]

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);

Comment 2

[:Paolo Amadini]

Comment on attachment 8557121 [details] [diff] [review]
The patch

Review comments addressed in bug 1129896.

Comment 3

[:Paolo Amadini]

https://hg.mozilla.org/integration/fx-team/rev/acb012724bf0

Comment 4

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/acb012724bf0

Comment 5

[Ian Neal]

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;
>     }

Comment 6

[:Paolo Amadini]

(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? :-)