1161630 - Don't blindly delete all favicons

Status: RESOLVED FIXED

(Firefox for iOS :: Data Storage, defect)

Description

[Richard Newman [:rnewman]]

FaviconsTable.delete just deletes all favicons. Presumably we don't want to do this all the time: we want to delete icons that *aren't being used* by faviconSites or bookmarks.

Comment 1

[Wesley Johnston (:wesj)]

Attachment: PR https://github.com/mozilla/firefox-ios/pull/585

This adds a method to the faviconTable for removing any unused favicons. It calls it when history or bookmarks are removed. Refactored bookmarks a little bit to make that happen (but not all the way it needs to go...)

Comment 2

[Wesley Johnston (:wesj)]

Hmm... This is actually going to be a bit messy when this is more sync-aware. All our beautiful Cascades go away? I wonder if we'd be better to move these deleted entries to a different table instead of marking them the way we do.

Comment 3

[Richard Newman [:rnewman]]

(In reply to Wesley Johnston (:wesj) from comment #2)
> Hmm... This is actually going to be a bit messy when this is more
> sync-aware. All our beautiful Cascades go away?

When a user doesn't have an account, we can directly delete, rather than marking. In that situation the cascades will apply.

And when they do have an account, and we're marking history with is_deleted, we're at least immediately discarding visits… which will cascade to visit_sites, which will orphan favicons, and this patch will work.

(And then a few minutes later we'll sync, and the history rows will be deleted.)

We don't sync favicons, so nothing to worry about on that front yet.

Comment 4

[Richard Newman [:rnewman]]

Comment on attachment 8620569 [details] [review]
PR https://github.com/mozilla/firefox-ios/pull/585

A few questions on the PR.

Comment 5

[Wesley Johnston (:wesj)]

Comment on attachment 8620569 [details] [review]
PR https://github.com/mozilla/firefox-ios/pull/585

Updated with a run([(String: Args?)]) method.

Comment 6

[Richard Newman [:rnewman]]

Sorry, Wes  -- could you rebase this?

Comment 7

[Wesley Johnston (:wesj)]

Rebased. I know there's another bug being worked on for this somewhere.... I'll find and try to close it (gently).

Comment 9

[Richard Newman [:rnewman]]

Comment on attachment 8620569 [details] [review]
PR https://github.com/mozilla/firefox-ios/pull/585

r+ with comments. Pretty much just wanting to make sure we're thread-happy and aren't doing two big subqueries and a table walk each time the user does a delete.

Comment 10

[Richard Newman [:rnewman]]

https://github.com/mozilla/firefox-ios/commit/7496ea7dd2a1abc9374d7329117b7adaf6ee44dc

Comment 11

[Wesley Johnston (:wesj)]

Test updates: https://github.com/mozilla/firefox-ios/pull/726