Closed Bug 830291 Opened 9 years ago Closed 4 years ago

remove "downloads/destinationFileName" annotation


(Toolkit :: Places, defect)

Not set



Tracking Status
firefox55 --- fixed


(Reporter: mak, Assigned: mak)



(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"
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]

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]

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)
Comment on attachment 703175 [details] [diff] [review]

Review of attachment 703175 [details] [diff] [review]:

Must update

::: 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.
Attachment #8867164 - Flags: review?(mak77) → review+
Pushed by
remove "downloads/destinationFileName" annotation. r=mak
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.