830291 - remove "downloads/destinationFileName" annotation

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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"

Comment 1

[Asaf Romano (gone)]

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.

Comment 2

[Asaf Romano (gone)]

Attachment: wip

This builds, but I haven't tested it at all.

And we need a test.

Comment 3

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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

Comment 4

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 703045 [details] [diff] [review]
wip

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

the history.cpp changes look good

Comment 5

[Asaf Romano (gone)]

Attachment: patch

Comment 6

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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)

Comment 7

[:Paolo Amadini]

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.

Comment 8

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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.

Comment 9

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

I will just address my comments and land Mano's patch.

Comment 10

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: Bug 830291 - remove "downloads/destinationFileName" annotation.

Review commit: https://reviewboard.mozilla.org/r/138764/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/138764/

Comment 11

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8867164 [details]
Bug 830291 - remove "downloads/destinationFileName" annotation.

https://reviewboard.mozilla.org/r/138764/#review142050

Comment 12

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8867164 [details]
Bug 830291 - remove "downloads/destinationFileName" annotation.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/138764/diff/1-2/

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4ca6d53664ac
remove "downloads/destinationFileName" annotation. r=mak

Comment 14

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/4ca6d53664ac