1094130 - DownloadAutoSaveView sometimes does not save 'download'.

Status: RESOLVED INVALID

(Toolkit :: Downloads API, defect)

Description

[Hiroyuki Ikezoe (:hiro)]

Attachment: download_shouldPersistDownload_fix.patch

DownloadAutoSaveView does not save if the download is finished before cheking of this.onsaveitem() in DownloadStore.save().

Pushed try (not finished yet):
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=ad3ff8be29bd

Comment 1

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8517345 [details] [diff] [review]
download_shouldPersistDownload_fix.patch

The patch seems to work fine on the try.

Comment 2

[:Paolo Amadini]

Comment on attachment 8517345 [details] [diff] [review]
download_shouldPersistDownload_fix.patch

In Firefox for Desktop, this is by design, because completed downloads are stored in the browser history. You can just add a conditional for Thunderbird and return "true" here to store all downloads forever, which is what the modified code effectively does, or even better follow the B2G conditional path that adds a time limit, so that performance isn't degraded over time by long lists of downloads.

While nice to have, a test for the current Firefox and B2G behavior is not required to land this patch. Testing DownloadAutoSaveView directly is tricky and relatively slow, as it is time-dependent, so I suggest you just call shouldPersistDownload on a sample of downloads and check the return value of the function. If you prefer, the tests for the Thunderbid-specific behavior of shouldPersistDownload may just live in comm-central.

The production code itself will move outside of mozilla-central once we have a way of registering application-specific DownloadIntegration modules (bug 899013).

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Thanks Paolo!  I did not know that is an intentional. In that case we can handle it as you suggested. 

I will write new unit tests to check that download list is surely stored on disk in comm-central. Do you have a plan to add an interface to modify kSaveDelayMs outside the module? It will eliminate the slowness of the tests.

Comment 4

[:Paolo Amadini]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #3)
> Do you have a plan to add an interface to modify
> kSaveDelayMs outside the module? It will eliminate the slowness of the tests.

That's a good idea. We may also use this to write more complete DownloadAutoSaveView tests in mozilla-central, that we need if we add shutdown safety to "downloads.json" using the AsyncShutdown module, that is something we had in mind.

Comment 5

[Hiroyuki Ikezoe (:hiro)]

(In reply to :Paolo Amadini from comment #4)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #3)
> > Do you have a plan to add an interface to modify
> > kSaveDelayMs outside the module? It will eliminate the slowness of the tests.
> 
> That's a good idea. We may also use this to write more complete
> DownloadAutoSaveView tests in mozilla-central, that we need if we add
> shutdown safety to "downloads.json" using the AsyncShutdown module, that is
> something we had in mind.

Thanks. Good to know.