941027 - Store metadata about completed downloads in the download history

Status: VERIFIED FIXED

(Toolkit :: Downloads API, defect)

Description

[:Paolo Amadini]

Attachment: The patch

The new Downloads code currently doesn't update the metadata of Places history entries after a download is added to it for the first time. As a result, completed downloads in the Library window don't appear as completed anymore after the browser is restarted, unless the target file is still present.

The requirement of removing the file from disk, and the fact that download history is used less often than the Downloads Panel, is probably the reason why this regression was generally unnoticed.

Steps to reproduce:
- Start a new download and wait for it to finish.
- Open the Downloads view in the Library to verify that it has completed.
- Close the browser and delete the file from disk.
- Restart the browser and open the Library window.

Expected result:
- The download should appear as finished, with the final file size.

Actual result:
- The download appears as failed, without information about the file size.

Comment 1

[:Paolo Amadini]

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

I'm told Neil is away, do you think you can review this patch potentially
tracking Beta on short notice, Marco?

Comment 2

[Lukas Blakk [:lsblakk] use ?needinfo]

Tracking for regression that is user facing, but will be interested to see the risk/reward assessment on nomination since we're so close to the end of FF26 beta it's more risky to take unnecessary churn - please be clear about what assurances we have with this code change that there won't be further regression.

Comment 3

[Marco Bonardo [:mak]]

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

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

::: browser/components/downloads/src/DownloadsCommon.jsm
@@ +673,5 @@
> +
> +      // This state transition code should actually be located in a Downloads
> +      // API module dedicated to Places integration (bug 941009).  Moreover, the
> +      // fact that state is stored as annotations should be ideally hidden
> +      // behind methods of nsIDownloadHistory (bug 830415).

I think that if we do the latter and here we just call the nsIDownloadHistory method we will be fine, without the need for a separate module. If there's no service implementing nsIDownloadHistory you skip the call, otherwise you call the method, regardless it being implemented by Places or by third party history implementation. Sure it's evil xpcom :)

Comment 4

[:Paolo Amadini]

Updated comment and pushed to fx-team:

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

Comment 5

[:Paolo Amadini]

(In reply to Marco Bonardo [:mak] from comment #3)
> I think that if we do the latter and here we just call the
> nsIDownloadHistory method we will be fine, without the need for a separate
> module. If there's no service implementing nsIDownloadHistory you skip the
> call, otherwise you call the method, regardless it being implemented by
> Places or by third party history implementation. Sure it's evil xpcom :)

Yeah, what I meant is that these state update calls should be made in the back-end code (like the current nsIDownloadHistory call) instead of this module, that is located in the front-end "browser" directory.

Comment 6

[:Paolo Amadini]

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

Note: I'm nominating this before it has landed on mozilla-central since I'll be away next week, the patch should be safe to land on Aurora and Beta after Nightly testing.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 825588
User impact if declined: Per comment 0, missing file size in Library downloads, and incorrect state after removing the target file from disk. While the Library window is seldom used and thus the user impact is low, the incorrect state in history cannot be easily fixed afterwards.
Testing completed (on m-c, etc.): Tested locally, should be tested in Nightly when patch lands there
Risk to taking this patch (and alternatives if risky): This code was already present in the old implementation and was excluded by mistake when switching the front-end code to use the new API, we are just restoring it. Most of the code is enclosed in a try-catch block thus it is unlikely it will cause other regressions. The patch is self-contained and can be easily backed out in case any issue occurs.
String or IDL/UUID changes made by this patch: None

Comment 7

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

https://hg.mozilla.org/mozilla-central/rev/15115556fbf4

Comment 8

[:Paolo Amadini]

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

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 825588
User impact if declined: Per comment 0, missing file size in Library downloads, and incorrect state after removing the target file from disk. While the Library window is seldom used and thus the user impact is low, the incorrect state in history cannot be easily fixed afterwards.
Testing completed (on m-c, etc.): Tested locally, should be tested in Nightly when patch lands there
Risk to taking this patch (and alternatives if risky): This code was already present in the old implementation and was excluded by mistake when switching the front-end code to use the new API, we are just restoring it. Most of the code is enclosed in a try-catch block thus it is unlikely it will cause other regressions. The patch is self-contained and can be easily backed out in case any issue occurs.
String or IDL/UUID changes made by this patch: None

Comment 9

[:Paolo Amadini]

Attachment: Applied on mozilla-beta

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9f605581dc87
https://hg.mozilla.org/releases/mozilla-beta/rev/9f93c6dc702c

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/9f93c6dc702c

Comment 12

[Bogdan Maris, Desktop QA]

Mozilla/5.0 (X11; Linux i686; rv:26.0) Gecko/20100101 Firefox/26.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:26.0) Gecko/20100101 Firefox/26.0
Mozilla/5.0 (Windows NT 5.2; rv:26.0) Gecko/20100101 Firefox/26.0

Verified as fixed on Windows XP x32, Ubuntu 13.04 x32, Mac OS X 10.9 using Firefox 26 beta 8 (buildID: 20131125215016).

Comment 13

[Bogdan Maris, Desktop QA]

Mozilla/5.0 (Windows NT 6.3; WOW64; rv:27.0) Gecko/20100101 Firefox/27.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:27.0) Gecko/20100101 Firefox/27.0
Mozilla/5.0 (X11; Linux i686; rv:27.0) Gecko/20100101 Firefox/27.0

Also verified as fixed using latest Aurora 27.0a2 (buildID: 20131202004002) on Windows 8.1 x64, Ubuntu 12.04 x32 and Mac OS X 10.9.

Comment 14

[Ioana (away)]

Verified as fixed using the 12/11 Firefox 28.0a2 on Windows 7 64-bit, Ubuntu 13.04 32-bit and Mac OS X 10.9.