Closed
Bug 830415
Opened 11 years ago
Closed 7 years ago
Move the front-end code that stores download metadata in history to the DownloadHistory module
Categories
(Toolkit :: Places, defect, P1)
Tracking
()
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: asaf, Assigned: Paolo)
References
(Blocks 1 open bug)
Details
Attachments
(2 files, 1 obsolete file)
No description provided.
Assignee | ||
Comment 1•11 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
Comment 2•11 years ago
|
||
(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.
Comment 3•11 years ago
|
||
needinfo just to be sure you notice the answer to your question.
Flags: needinfo?(paolo.mozmail)
Assignee | ||
Comment 4•11 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)
Updated•7 years ago
|
Priority: -- → P3
Assignee | ||
Comment 5•7 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•7 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•7 years ago
|
Attachment #8889377 -
Attachment is obsolete: true
Attachment #8889377 -
Flags: review?(mak77)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 11•7 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•7 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•7 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•7 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.
Comment 15•7 years ago
|
||
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) |
Assignee | ||
Comment 18•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8b92dd24842bbe9b5ddeac5c4e0361cf67a09a83
Comment 19•7 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•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/58dd26b26962 https://hg.mozilla.org/mozilla-central/rev/b1826fe01838
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in
before you can comment on or make changes to this bug.
Description
•