remove "downloads/destinationFileName" annotation

RESOLVED FIXED in Firefox 55

Status

()

RESOLVED FIXED
6 years ago
2 years ago

People

(Reporter: mak, Assigned: mak)

Tracking

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

6 years ago
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.
Created attachment 703045 [details] [diff] [review]
wip

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
(Assignee)

Comment 3

6 years ago
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)
(Assignee)

Comment 4

6 years ago
Comment on attachment 703045 [details] [diff] [review]
wip

Review of attachment 703045 [details] [diff] [review]:
-----------------------------------------------------------------

the history.cpp changes look good
(Assignee)

Comment 6

6 years ago
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)

Comment 7

6 years ago
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.
(Assignee)

Comment 8

6 years ago
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)

Updated

3 years ago
Assignee: asaf → nobody
(Assignee)

Updated

2 years ago
Flags: needinfo?(mak77)
(Assignee)

Comment 9

2 years ago
I will just address my comments and land Mano's patch.
Assignee: nobody → mak77
Flags: needinfo?(mak77)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #703175 - Attachment is obsolete: true
(Assignee)

Comment 11

2 years ago
mozreview-review
Comment on attachment 8867164 [details]
Bug 830291 - remove "downloads/destinationFileName" annotation.

https://reviewboard.mozilla.org/r/138764/#review142050
Attachment #8867164 - Flags: review?(mak77) → review+
Comment hidden (mozreview-request)

Comment 13

2 years ago
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/4ca6d53664ac
remove "downloads/destinationFileName" annotation. r=mak
https://hg.mozilla.org/mozilla-central/rev/4ca6d53664ac
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.