830291 - remove "downloads/destinationFileName" annotation

Status: RESOLVED FIXED

Description

[Marco Bonardo [::mak]]

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

[Mano (::mano, needinfo? for any questions; not reading general bugmail)]

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

[Mano (::mano, needinfo? for any questions; not reading general bugmail)]

Created attachment 703045 [details] [diff] [review]
wip

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

And we need a test.

Comment 3

[Marco Bonardo [::mak]]

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

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

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

the history.cpp changes look good

Comment 5

[Mano (::mano, needinfo? for any questions; not reading general bugmail)]

Created attachment 703175 [details] [diff] [review]
patch

Comment 6

[Marco Bonardo [::mak]]

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

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

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

Comment 10

[Marco Bonardo [::mak]]

Created attachment 8867164 [details]
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]]

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

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

Comment 12

[Marco Bonardo [::mak]]

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 on intermittent or backout)]

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