1135180 - Favicon storage for bookmarks

Status: RESOLVED FIXED

(Firefox for iOS :: General, defect)

Comment 1

[Wesley Johnston (:wesj)]

Attachment: Pull request

This adds a join-table for bookmarks <-> favicons. The table is smart enough to query history for a favicon if you don't provide one. It adds the entry to the bookmarks and favicons table, and then updates it own bookmarkId <-> faviconId mapping. I also added a test (basically a copy of the History test we have for the same thing).

I also (after a long rabbit hole) removed the files: argument from the Favicon constructor which isn't used anymore here. I left that in since its nice cleanup.

Comment 2

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8568092 [details] [review]
Pull request

I find this approach (with a separate table, rather than a SQlite view) fraught, but it looks like we do this in another place.  I have some questions but nothing major.  Perhaps bnicholson can take a look as well?  Brian certainly has a keener eye for formatting and Swift style than I do.

Comment 3

[Brian Nicholson (:bnicholson)]

Comment on attachment 8568092 [details] [review]
Pull request

Looks mostly fine. I'm curious why id is optional and title isn't.

Comment 4

[Wesley Johnston (:wesj)]

Comment on attachment 8568092 [details] [review]
Pull request

I updated this to just use the bookmarks table. Much simpler. You mind looking again nick?

Comment 5

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8568092 [details] [review]
Pull request

I have no major problems with this, but I don't understand how this works over time and am worried that our bookmark -> favIcon association is wrong.  I feel like it should be bookmark -> site -> favIcon.

What happens if a user creates a bookmark (setting the favIcon.id), and then visits the bookmark and sees a new favIcon.  Is the favIcon corresponding to favIcon.id updated already?  Does the new favIcon have the same id as the old favIcon?

Comment 6

[Wesley Johnston (:wesj)]

(In reply to Nick Alexander :nalexander from comment #5)
> What happens if a user creates a bookmark (setting the favIcon.id), and then
> visits the bookmark and sees a new favIcon.  Is the favIcon corresponding to
> favIcon.id updated already?  Does the new favIcon have the same id as the
> old favIcon?

These favicons won't update. i.e. favicon id's are associated with a url. If the image at the url changes, the favicon would update. We currently don't delete old favicon entries when a site changes what it provides (we should). i.e. if a site changes its favicon links, we'd have issues right now (that's a bug, I'll file it).

I'm hesitant to associate bookmark's with sites to avoid the complexity when it comes time to do something like "Clear history" i.e. we'd have to remove Sites that had visits but weren't bookmarked I'd guess, or had no visits or bookmarks.

Comment 7

[Wesley Johnston (:wesj)]

(In reply to Wesley Johnston (:wesj) from comment #6)
> These favicons won't update. i.e. favicon id's are associated with a url. If
> the image at the url changes, the favicon would update.

I should also note sdwebimage is doing the disk caching of these for us, and its pretty aggressive. If you visit a site a lot, it probably won't delete the old data for awhile. We should look into providing a maximum age for a cached image for it to use and occasionally forcing it to prune/reload them.

Comment 8

[Wesley Johnston (:wesj)]

Merged https://github.com/mozilla/firefox-ios/pull/173