Wesley Johnston (:wesj) Reporter
	Description • 9 years ago

Looking into favicon expiration, I need to know when a particular favicon was stored for a particular site. Right now we store that on the Favicon itself, but since a single favicon can be shared across many sites it really doesn't mean the right thing there. The date should be stored on the mapping to the site itself.

We could leave the date field in place for the favicon itself and have it represent a "We last checked that this url was valid on X" date, or perhaps, "the width/height recorded here were accurate last on X"?

	Wesley Johnston (:wesj) Reporter
	Comment 1 • 9 years ago

I also moved all the sql statements used here out of the create() method so that I could reuse them in upgrades wherever I needed. I also made downgrades do something. Looking into tests...

	Richard Newman [:rnewman]
	Comment 2 • 9 years ago

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

A couple of questions in the PR. Also we need to be careful when we land this and Bug 1184767!

But to take a step back: if favicons are shared between sites, then two things can happen.


* A site changes favicon URL (or a new site is recorded). We only know that if we visit the site, and we'll update our site mapping accordingly. This can leave orphan icons that we want to clean up, or it can 'refresh' an old icon.

* A favicon changes. We'll figure that out whenever we fetch the icon.


So it seems to me that there *is* (at least one) date associated with the favicon itself, and it's the date last fetched, which is updated whenever we re-fetch it.

Perhaps favicon expiration can be phrased in terms of two things:

* When was an icon last used, and by how many sites? That's recorded by history and faviconSites. This bug is currently going in this direction.

* When was an icon last fetched?


I haven't had enough coffee yet this morning to turn this into a concrete "I think we should do X".

But I do think it's worth being clear about which date we mean when we attach a date field to an icon object, and the above observations might eliminate the need to drop the date column from favicons, or even change the focus of this bug to "update favicons.date when we fetch", or something simpler than this.

	Wesley Johnston (:wesj) Reporter
	Comment 3 • 9 years ago

Hmm... I could buy that we should keep the date field on the favicon itself as well. We don't actually check on each page load if all the icons we found exist (we do the first time, but on next load we'll use the cache).

I would find that row a little confusing then. Does it mean "We checked and this image existed on date X" or does it mean "We last added a site that pointed to this icon on day X"... Maybe we're better to just dump icons if we ever can't load the url (that could happen because you visited the site or because you scrolled the history pane to the end).

	Wesley Johnston (:wesj) Reporter
	Comment 4 • 9 years ago

Not really needed for 1.0. Someone will probably need to make favicon-expiration a lot better sometime soon though. We (I) basically punted on it for 1.0.