1448143 - Factor out a size getter from DownloadsViewUI.jsm so other consumers can access it

Status: RESOLVED FIXED

(Firefox :: New Tab Page, enhancement, P1)

Description

[Ursula Sarracini (:ursula)]

This will make it easier for Activity stream's downloaded items in highlights to get the "description" of the download. Or else I'd have to re-implement this function to get the size

Comment 1

[Ursula Sarracini (:ursula)]

Attachment: Bug 1448143 - Factor out a size getter from DownloadsViewUI.jsm so other consumers can access it

Review commit: https://reviewboard.mozilla.org/r/230318/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/230318/

Comment 2

[Ed Lee :Mardak]

Comment on attachment 8961528 [details]
Bug 1448143 - Factor out a size getter from DownloadsViewUI.jsm so other consumers can access it

https://reviewboard.mozilla.org/r/230318/#review235898

::: browser/components/downloads/DownloadsViewUI.jsm:272
(Diff revision 1)
>          stateLabel = s.fileMovedOrMissing;
>          hoverStatus = stateLabel;
>        } else 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] =
> +        // For completed downloads, show the file size
> +        stateLabel = this.size.stateLabel;
> +        status = this.size.status;

This is going to cause the `size` getter to run twice. You could avoid it with:

({stateLabel, status} = this.size);

Or if you want to avoid the ()s, have .size return a pair/array similar to how `convertByteUnits` returns an array, then `[stateLabel, status] = this.size`;

Comment 3

[Ed Lee :Mardak]

Or just save it locally first
let size = this.size;


Although looking closer, perhaps the getter should be named sizeStrings? Or something else? Odd that there's status for a "size"

Comment 4

[Ursula Sarracini (:ursula)]

Comment on attachment 8961528 [details]
Bug 1448143 - Factor out a size getter from DownloadsViewUI.jsm so other consumers can access it

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/230318/diff/1-2/

Comment 5

[Ursula Sarracini (:ursula)]

Yeah `sizeStrings` is a better name. Changed the name, and saved it to a variables locally first.

Comment 6

[Ed Lee :Mardak]

Comment on attachment 8961528 [details]
Bug 1448143 - Factor out a size getter from DownloadsViewUI.jsm so other consumers can access it

https://reviewboard.mozilla.org/r/230318/#review236190

::: browser/components/downloads/DownloadsViewUI.jsm:122
(Diff revision 2)
> +   */
> +  get sizeStrings() {
> +    let s = DownloadsCommon.strings;
> +    let sizeStrings = {};
> +
> +    if (this.download.target.size) {

I believe the !== undefined is to guard against a 0 size, which is a valid size

Comment 7

[Ursula Sarracini (:ursula)]

Comment on attachment 8961528 [details]
Bug 1448143 - Factor out a size getter from DownloadsViewUI.jsm so other consumers can access it

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/230318/diff/2-3/

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/15b7991887bc
Factor out a size getter from DownloadsViewUI.jsm so other consumers can access it r=Mardak

Comment 9

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/15b7991887bc