812090 - Match imported favicons to multiple pages

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Brian Nicholson (:bnicholson)]

Attachment: Match imported favicons to multiple pages

An IRC user reported missing favicons, and he may have been referring to this. Because of the GROUP BY clause in the query, we don't select existing favicons that have duplicate URLs. The problem with doing this is that any other pages that use the same favicon URL won't be associated with that favicon.

This patch uses a different approach by selecting all favicons from the images table. Before insertion, the favicons table is first queried to prevent duplicates. This allows all pages before the upgrade to be associated with the correct favicons.

This could have been simplified with insertWithOnConflict() and the CONFLICT_IGNORE flag, but unfortunately, it wasn't returning the id of the existing row (https://code.google.com/p/android/issues/detail?id=13045).

Comment 1

[Lucas Rocha (:lucasr)]

Comment on attachment 681877 [details] [diff] [review]
Match imported favicons to multiple pages

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

Good.

::: mobile/android/base/db/BrowserProvider.java.in
@@ +1401,5 @@
> +                        Cursor c2 = db.query(TABLE_FAVICONS,
> +                                new String[] { Favicons._ID },
> +                                Favicons.URL + " = ?",
> +                                new String[] { faviconUrl },
> +                                null, null, null);

It's usually better to close cursors in a finally block.

Comment 2

[Brian Nicholson (:bnicholson)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/fdad0a320e7d

Comment 3

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/fdad0a320e7d