Closed Bug 812090 Opened 9 years ago Closed 9 years ago

Match imported favicons to multiple pages

Categories

(Firefox for Android Graveyard :: General, defect)

ARM
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 19

People

(Reporter: bnicholson, Assigned: bnicholson)

References

Details

Attachments

(1 file)

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).
Attachment #681877 - Flags: review?(lucasr.at.mozilla)
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.
Attachment #681877 - Flags: review?(lucasr.at.mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/fdad0a320e7d
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 19
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.