Closed Bug 941027 Opened 6 years ago Closed 6 years ago

Store metadata about completed downloads in the download history

Categories

(Toolkit :: Downloads API, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla28
Tracking Status
firefox25 --- unaffected
firefox26 + verified
firefox27 + verified
firefox28 --- verified
b2g-v1.2 --- fixed

People

(Reporter: Paolo, Assigned: Paolo)

References

Details

(Keywords: regression)

Attachments

(2 files)

Attached patch The patchSplinter Review
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.
Attachment #8335326 - Flags: review?(enndeakin)
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?
Attachment #8335326 - Flags: review?(enndeakin) → review?(mak77)
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 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 :)
Attachment #8335326 - Flags: review?(mak77) → review+
Updated comment and pushed to fx-team:

https://hg.mozilla.org/integration/fx-team/rev/15115556fbf4
(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 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
https://hg.mozilla.org/mozilla-central/rev/15115556fbf4
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
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
Attachment #8335326 - Flags: approval-mozilla-beta?
Attachment #8335326 - Flags: approval-mozilla-aurora?
Attachment #8335326 - Flags: approval-mozilla-beta?
Attachment #8335326 - Flags: approval-mozilla-beta+
Attachment #8335326 - Flags: approval-mozilla-aurora?
Attachment #8335326 - Flags: approval-mozilla-aurora+
Keywords: verifyme
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).
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.
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.