Closed Bug 830415 Opened 8 years ago Closed 3 years ago

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

Categories

(Toolkit :: Places, defect, P1)

x86
macOS
defect

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: mano, Assigned: Paolo)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

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)
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
Blocks: 941009
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
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 :-)
Attachment #8889377 - Attachment is obsolete: true
Attachment #8889377 - Flags: review?(mak77)
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 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 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+
(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.
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
https://hg.mozilla.org/mozilla-central/rev/58dd26b26962
https://hg.mozilla.org/mozilla-central/rev/b1826fe01838
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.