1111103 - Use core data for storing favicons locally

Status: RESOLVED FIXED

(Firefox for iOS :: Favicons, defect)

Description

[Wesley Johnston (:wesj)]

As a first step using core data, I want to convert the favicons store. The favicons store doesn't actually do anything anymore, so this won't actually affect UI yet.

Comment 1

[Wesley Johnston (:wesj)]

Attachment: Pull request

Shows how to use core-data for these api's a bit. I'd like to abstract this a bit more in the long term. i.e. My old favicon code had a cache protocol

protocol Cache<Key, Value> {
  subscript(Key) -> Value
  clear()
}

that could be backed by a memory or a CoreData store (or sqlite if we eventually want it instead...). Its still unclear to me how much of a memory store CoreData has, but I think putting the abstraction in place would still make it easier to opt in a new cache if we want.

For now, this is a simple flip. I'll pull apart the old favicon in some new bugs to use this, and then hook it up to the new browser code.

Comment 2

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

This PR also includes 250 files for MagicalRecord, which was also part of another PR. Is that correct? And it includes changes to the Reader part. Maybe those should be separate (bug & PR) too?

I like the usage of Core Data, it seems to simplify things a lot.

I added a few comments inline in the PR.

Comment 3

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

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

I'm +'ing this but I think the println()s should be removed and ideally the PR should be split up. (Magical Record inclusion in a separate commit and the Reader changes in a separate bug / pr)

Comment 4

[Wesley Johnston (:wesj)]

I removed the println's. MagicalRecord is a separate commit. I haven't found a good way in git to do these types of PR's where one depends on another. Do you know of a way?

Comment 5

[Richard Newman [:rnewman]]

(In reply to Wesley Johnston (:wesj) from comment #4)
> I removed the println's. MagicalRecord is a separate commit. I haven't found
> a good way in git to do these types of PR's where one depends on another. Do
> you know of a way?

GitHub only really allows two ways:

* You issue the second pull request against the first -- i.e., merging into wesj/foobar instead of master. This obviously makes the merge button do insane things, so it isn't recommended.

* Only issue the second pull request when the first one has been merged.

Both of these suck.

This is one reason why we have been very thorough about including bug numbers in every commit in android-sync, even if you end up with "Part 9" etc., and I apply that same style to the work I've been doing in firefox-ios -- e.g., https://github.com/mozilla/firefox-ios/pull/37/commits

When you look at the second pull request, you can spot where the bug number in the commit stream changes to match the one in the pull request description, and just start reading there.