941063 - Add a property to get the size of the downloads data written to disk

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect)

Description

[:Paolo Amadini]

Add a property to the Download object to get the final file size on disk, that may work for completed downloads or canceled downloads that have partial data.

The size on disk might be different from the size of the data transferred from the server, if the data is decoded while downloading.

Comment 1

[:Paolo Amadini]

We could have a property like "download.target.size" that is updated from disk when the download stops, and also when the "refresh" method is called on a finished, failed, or canceled download. Any change would trigger the "onchange" callback.

Optionally, this property could consider the size of the ".part" file, if available, for failed or canceled downloads.

While here, it is quite simple to also add a boolean that tells if the target of a successful download can be launched, with a similar refresh logic. It could be called "download.canLaunch" (or alternatively a "download.target.exists" property can be used). This will allow the front-end to disable the launch action if the user removes the file.

Comment 2

[bhavana bajaj [:bajaj]]

Hi Paolo,

Given we are already done with a couple of beta's for Firefox 27 and since there has not been any progress on this bug yet, can you please recommend next steps for Firefox 27 ?

This was tracked in reference to https://bugzilla.mozilla.org/show_bug.cgi?id=943843#c3

Comment 3

[:Paolo Amadini]

Thanks for the reminder! I don't think I'll have the bandwidth to work on this until after the Firefox Desktop work week. Given that this affects reporting the download size only for data compressed at the HTTP encoding level, which is not a very common case in downloads, I lean towards WONTFIXing bug 943843 for Firefox 27, and aim to get a proper fix for Firefox 28.

Comment 4

[Lukas Blakk [:lsblakk] use ?needinfo]

Based on Paolo's comment 3 I don't see this as a release-blocking issue we need to track since it's not a common download case.  We can consider a low-risk nomination for uplift to branches when a fix is verified.

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Attachment: Initial work

This issue is more common on Thunderbird because most of attachment files are base64-encoded.
I confirmed this patch works fine on Thunderbird.
I think a new test case of decoding data is necessary but I have not idea how to write such case in mozilla-central now. Just using .gz for test file is sufficient?

One thing I noticed is that progress value which is calculated in refresh() is wrong in case of decoding data.  It is calculated from the file size on disk but it should be transferred bytes in case of the data is encoded.

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Attachment: Initial work v2

I found there are some test cases for .gz encoding data.

Comment 7

[:Paolo Amadini]

Comment on attachment 8518588 [details] [diff] [review]
Initial work v2

Thanks for the patch! The approach looks good, but I'd appreciate if you can update the implementation to be optimized, reusing existing OS.File.stat calls or information we already have about the download. This probably implies ad-hoc updates instead of a helper function.

We also need a test for the value of "target.size" for the case of interrupted downloads.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #5)
> One thing I noticed is that progress value which is calculated in refresh()
> is wrong in case of decoding data.  It is calculated from the file size on
> disk but it should be transferred bytes in case of the data is encoded.

This works because, unfortunately, encoded downloads cannot be resumed, so we never retain partial data for them. Feel free to add a comment to clarify!

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Attachment: Revised patch

I've forgotten that I pushed a new patch on try server.
This patch is revised as Paolo advised in comment #7.

Please correct me if I am misunderstanding the advices.
  
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=2253c3a6f7aa

Comment 9

[:Paolo Amadini]

Comment on attachment 8521866 [details] [diff] [review]
Revised patch

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

Sorry for the delay. The changes you made look good!

Since I haven't had time to investigate if all the cases we can encounter are covered, this patch might need another round of review. However, to speed things up, I'm already asking you about the possible improvements I found.

First, I've noticed "this.DownloadTarget.prototype" needs a definition for "size" with a JavaDoc-style comment. The value in the prototype, as well as the value set if the file does not exist, should be 0 (and the comment should of course mention this).

While not strictly required, it would be great if you could add a new "exists" property on the prototype, and update it in the code as well. In fact, the code path for it is the same and we need it for other uses in the interface.

::: toolkit/components/jsdownloads/src/DownloadCore.jsm
@@ +491,5 @@
>            this.stopped = true;
>            this.speed = 0;
> +          if (this.hasPartialData && this.target.partFilePath) {
> +            this.target.size = this.currentBytes;
> +          } else if (yield OS.File.exists(this.target.path)) {

In this code path, you can assume the target exists, as an optimization.

However, you should still catch and report exceptions with the "stat" call. Something like:

try {
  this.target.size = (yield OS.File.stat(this.target.path)).size;
} catch (ex) {
  this.target.size = 0;
  Cu.reportError(ex);
}

@@ +779,2 @@
>        }
> +      this._notifyChange();

We should notify a change only if we really changed the "size" (or "exists") properties.

::: toolkit/components/jsdownloads/test/unit/common_test_Download.js
@@ +148,5 @@
>    // Check additional properties on the finished download.
>    do_check_true(download.source.referrer === null);
>  
>    yield promiseVerifyContents(download.target.path, TEST_DATA_SHORT);
> +  do_check_eq(download.target.size, TEST_DATA_SHORT.length);

Since we're using this pattern many times, can you please define a test helper function at the top of the file, that gets called like this:

promiseVerifyTarget(download.target, TEST_DATA_SHORT);

It should call promiseVerifyContents using the "path" property, then check that the "size" property matches the length of the second argument, and finally check that "exists" is true.

(In cases where we're checking that the file does not exits, we won't use this helper function.)

Comment 10

[:Paolo Amadini]

(In reply to :Paolo Amadini from comment #9)
> @@ +779,2 @@
> >        }
> > +      this._notifyChange();
> 
> We should notify a change only if we really changed the "size" (or "exists")
> properties.

Clarification: this in the "refresh" code path (it didn't show up in the review context).

Comment 11

[:Paolo Amadini]

Hey hiro, if you're not working on this I might help in moving it forward, as the patch may be useful for reducing main-thread I/O in the front-end.

Comment 12

[Hiroyuki Ikezoe (:hiro)]

Yes, please!

Comment 13

[:Paolo Amadini]

Attachment: Continued

Updated hiro's patch. Steven, do you think you can review this?

Comment 14

[Steven MacLeod [:smacleod]]

Comment on attachment 8545965 [details] [diff] [review]
Continued

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

This looks good to me.

It seems that the new properties are not updated based on partial progress:

(In reply to :Paolo Amadini from comment #1)
> We could have a property like "download.target.size" that is updated from
> disk when the download stops, and also when the "refresh" method is called
> on a finished, failed, or canceled download. Any change would trigger the
> "onchange" callback.
> 
> Optionally, this property could consider the size of the ".part" file, if
> available, for failed or canceled downloads.

All of the in code comments are clear that this is only used for the target
file and not the partFilePath, so I'm just going to assume you decided against
using this with partial data as well.

::: toolkit/components/jsdownloads/src/DownloadCore.jsm
@@ +1364,5 @@
> +   * Size in bytes of the target file, or zero if the download has not finished.
> +   *
> +   * Even if the target file does not exist anymore, this property may still
> +   * have a value taken from the download metadata, but it will be zero if the
> +   * file has been deleted and the size information is not available.

"but it will be zero if the file has been deleted" is not true if the file has been deleted by an external program or user action. We should maybe clarify this comment.

Comment 15

[Marco Mucci [:MarcoM]]

Hi Paolo, can you provide a point value.

Comment 16

[:Paolo Amadini]

(In reply to Steven MacLeod [:smacleod] from comment #14)
> All of the in code comments are clear that this is only used for the target
> file and not the partFilePath, so I'm just going to assume you decided
> against using this with partial data as well.

Correct, this turned out to be unneeded in the front-end.

> ::: toolkit/components/jsdownloads/src/DownloadCore.jsm
> "but it will be zero if the file has been deleted" is not true if the file
> has been deleted by an external program or user action. We should maybe
> clarify this comment.

What about:

   * Even if the target file does not exist anymore, this property may still
   * have a value taken from the download metadata. If the metadata has never
   * been available in this session and the size cannot be obtained from the
   * file because it has been deleted, this property will be zero.

Comment 17

[:Paolo Amadini]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a6e598f0a41c

Comment 18

[:Paolo Amadini]

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

Comment 19

[:Paolo Amadini]

Documentation needed for the new DownloadTarget properties and the extended descriptions of some Download properties.

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 21

[Eric Shepherd [:sheppy]]

The following articles have been updated:

https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/Downloads.jsm/Download
https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/Downloads.jsm
https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/Downloads.jsm/DownloadTarget

Also updated:

https://developer.mozilla.org/en-US/Firefox/Releases/38