Closed Bug 830291 Opened 12 years ago Closed 8 years ago

remove "downloads/destinationFileName" annotation

Categories

(Toolkit :: Places, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: mak, Assigned: mak)

Details

Attachments

(1 file, 2 obsolete files)

Not a blocker for the feature, though would bring a small perf gain on opening the view. We actually don't seem to need this annotation since we can extract the filename from "downloads/destinationFileURI"
Assignee: nobody → mano
Now that bug 826991 is fixed we no longer use this annotation. Remaining work: * Don't set it in the back-end. * DB migration This is defiantly trunk material, as it won't provide any performance gain.
Attached patch wip (obsolete) — Splinter Review
This builds, but I haven't tested it at all. And we need a test.
Attachment #703045 - Flags: feedback?(mak77)
Component: Downloads Panel → Places
Product: Firefox → Toolkit
Comment on attachment 703045 [details] [diff] [review] wip As I said on IRC, I think a schema migration is excessive, we can just reuse PlacesDBUtils here
Attachment #703045 - Flags: feedback?(mak77)
Comment on attachment 703045 [details] [diff] [review] wip Review of attachment 703045 [details] [diff] [review]: ----------------------------------------------------------------- the history.cpp changes look good
Attached patch patch (obsolete) — Splinter Review
Attachment #703045 - Attachment is obsolete: true
Attachment #703175 - Flags: review?(mak77)
Attachment #703175 - Attachment is patch: true
Comment on attachment 703175 [details] [diff] [review] patch Review of attachment 703175 [details] [diff] [review]: ----------------------------------------------------------------- Must update http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/tests/unit/test_download_history.js and http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/downloads/tests/chrome/test_destinationURI_annotation.xul ::: toolkit/components/places/PlacesDBUtils.jsm @@ +293,1 @@ > ")"); This is broken due to missing string concatenation "+" (test_preventive_maintenance.js would have noticed this and fail)
Attachment #703175 - Flags: review?(mak77)
The original purpose of the "downloads/destinationFileName" annotation was to support native database text search on historical downloads, like we do for history page titles when a query is specified in the Library text box. The "downloads/destinationFileURI" annotation cannot be used for that purpose because it is URI-encoded and would fail badly with high Unicode code points. If we don't plan to do that optimization, but always filter after querying all the downloads (I don't actually know how complicated it would be to implement that optimized search on the current view code), then I agree that we don't need that annotation anymore, it just increases database size.
annotation values are not searchable, we can evaluate better solution when we figure how to make it searchable. Since we can rebuild this data at any time from fileuri it's not a big deal.
Assignee: asaf → nobody
Flags: needinfo?(mak77)
I will just address my comments and land Mano's patch.
Assignee: nobody → mak77
Flags: needinfo?(mak77)
Attachment #703175 - Attachment is obsolete: true
Comment on attachment 8867164 [details] Bug 830291 - remove "downloads/destinationFileName" annotation. https://reviewboard.mozilla.org/r/138764/#review142050
Attachment #8867164 - Flags: review?(mak77) → review+
Pushed by mak77@bonardo.net: https://hg.mozilla.org/integration/autoland/rev/4ca6d53664ac remove "downloads/destinationFileName" annotation. r=mak
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: