Move the front-end code that stores download metadata in history to the DownloadHistory module

RESOLVED FIXED in Firefox 56

Status

()

defect
P1
normal
RESOLVED FIXED
6 years ago
2 years ago

People

(Reporter: mano, Assigned: Paolo)

Tracking

(Blocks 1 bug)

unspecified
mozilla56
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

Assignee

Comment 1

6 years ago
While working on bug 899110, I've noticed that we disabled a code path that was related to setting metadata about history entries, where this bug number was mentioned. This hasn't come up as a noticeable regression, but I think it may be related to setting the state of history entries in the Library window. Is this correct? What could be the impact of this code not being executed? If we need to reimplement this, where is the most correct place to handle the annotations?
Blocks: 847863
(In reply to :Paolo Amadini from comment #1)
> While working on bug 899110, I've noticed that we disabled a code path that
> was related to setting metadata about history entries, where this bug number
> was mentioned. This hasn't come up as a noticeable regression, but I think
> it may be related to setting the state of history entries in the Library
> window.

yes, without that code history entries won't have the right data (regarding their download date and the final status of the download). That's a noticeable regression imo.

> If we need to reimplement this, where is the most correct place to
> handle the annotations?

As stated in the comment there should be a dedicated API in nsIDownloadHistory to store metadata about a download history entry and should be called from whatever code has replaced this one. basically the comment doesn't mean "we should not do this here" but "we should not set annotations directly here"... So basically the fact these are annotations should be a backend detail, an API should abstract that.
needinfo just to be sure you notice the answer to your question.
Flags: needinfo?(paolo.mozmail)
Assignee

Comment 4

6 years ago
I figured out why the regression was mostly unnoticed. Since that is a separate issue from this bug, I filed bug 941027 to address it.
Flags: needinfo?(paolo.mozmail)
Priority: -- → P3
Assignee

Updated

2 years ago
Blocks: 941009
Assignee

Comment 5

2 years ago
Reusing this bug, since it's mentioned directly in the code I'm about to move.

The full story of the migration of methods towards DownloadHistory.jsm in the long term is covered by bug 941009, while a glimpse of how the DownloadHistory module will look in the immediate future is in bug 1381411.
Assignee: nobody → paolo.mozmail
Blocks: 1381411
Status: NEW → ASSIGNED
Priority: P3 → P1
Summary: Places' nsIDownloadHistory implementation should store (and provide access) meta data about downloads (state, endTime) → Move the front-end code that stores download metadata in history to the DownloadHistory module
Comment hidden (mozreview-request)
Assignee

Comment 7

2 years ago
I did some clean up first, so we don't need to choose between using the recommended destructuring assignment in the new module or keep the style of the existing modules :-)
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Attachment #8889377 - Attachment is obsolete: true
Attachment #8889377 - Flags: review?(mak77)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 11

2 years ago
I've tested this manually with all the five download states. There are no regression tests for now because I will use this method as part of bug 1381411. In fact I realized I had to move this code to the back-end when designing the tests for DownloadHistoryList, and it made sense to do it first as a separate step.

Comment 12

2 years ago
mozreview-review
Comment on attachment 8889412 [details]
Bug 830415 - Part 1 - Update the header styles of the Downloads API modules.

https://reviewboard.mozilla.org/r/160446/#review166224
Attachment #8889412 - Flags: review?(mak77) → review+

Comment 13

2 years ago
mozreview-review
Comment on attachment 8889410 [details]
Bug 830415 - Part 2 - Move the front-end code that stores download metadata in history to the DownloadHistory module.

https://reviewboard.mozilla.org/r/160444/#review166248

::: toolkit/components/jsdownloads/src/DownloadHistory.jsm:1
(Diff revision 2)
> +/* This Source Code Form is subject to the terms of the Mozilla Public

put behind an if CONFIG['MOZ_PLACES']: since Android uses jsdownloads but not Places.

::: toolkit/components/jsdownloads/src/DownloadHistory.jsm:35
(Diff revision 2)
> +
> +const METADATA_STATE_FINISHED = 1;
> +const METADATA_STATE_FAILED = 2;
> +const METADATA_STATE_CANCELED = 3;
> +const METADATA_STATE_BLOCKED_PARENTAL = 6;
> +const METADATA_STATE_DIRTY = 8;

I wonder if we may need these exported from the DownloadHistory object.
I think at least tests, when we have them, may want to access these consts.

Same for METADATA_ANNO.

::: toolkit/components/jsdownloads/src/DownloadHistory.jsm:84
(Diff revision 2)
> +        download.error.reputationCheckVerdict;
> +    }
> +
> +    try {
> +      PlacesUtils.annotations.setPageAnnotation(
> +                                 NetUtil.newURI(download.source.url),

Can we use Services.io.newURI instead and don't import NetUtil at all? We're trying to move away from NetUtil when it's not strictly needed (that is basically only when the input may be an nsIFile).
Attachment #8889410 - Flags: review?(mak77) → review+
Assignee

Comment 14

2 years ago
(In reply to Marco Bonardo [::mak] from comment #13)
> I wonder if we may need these exported from the DownloadHistory object.
> I think at least tests, when we have them, may want to access these consts.
> 
> Same for METADATA_ANNO.

I think they should only be used internally, and ideally consumers and tests would only deal with Download objects.
Thus tests will be a little bit more abstract, I suppose.
OK, let's keep constants hidden for now, we can always change our mind later.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 19

2 years ago
Pushed by paolo.mozmail@amadzone.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/58dd26b26962
Part 1 - Update the header styles of the Downloads API modules. r=mak
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1826fe01838
Part 2 - Move the front-end code that stores download metadata in history to the DownloadHistory module. r=mak

Comment 20

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/58dd26b26962
https://hg.mozilla.org/mozilla-central/rev/b1826fe01838
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.