Closed Bug 1161630 Opened 7 years ago Closed 7 years ago

Don't blindly delete all favicons


(Firefox for iOS :: Data Storage, defect)

iOS 8
Not set



Tracking Status
fxios + ---


(Reporter: rnewman, Assigned: wesj)




(1 file)

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.
tracking-fennec: --- → ?
Assignee: nobody → wjohnston
tracking-fennec: ? → +
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...)
Attachment #8620569 - Flags: review?(rnewman)
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.
(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 on attachment 8620569 [details] [review]

A few questions on the PR.
Attachment #8620569 - Flags: review?(rnewman) → feedback+
Comment on attachment 8620569 [details] [review]

Updated with a run([(String: Args?)]) method.
Attachment #8620569 - Flags: feedback+ → review?(rnewman)
Sorry, Wes  -- could you rebase this?
Flags: needinfo?(wjohnston)
Blocks: 1174227
Rebased. I know there's another bug being worked on for this somewhere.... I'll find and try to close it (gently).
tracking-fennec: + → ---
Flags: needinfo?(wjohnston) → needinfo?(rnewman)
Duplicate of this bug: 1174227
Comment on attachment 8620569 [details] [review]

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.
Flags: needinfo?(rnewman)
Attachment #8620569 - Flags: review?(rnewman) → review+
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.