391864 - Don't keep a disk cache of file: favicons

Status: RESOLVED FIXED

(Camino Graveyard :: General, defect)

Description

[Stuart Morgan]

Attachment: fix

While having an in-memory cache of file: and about: favicons makes sense, persisting them to disk doesn't.

Comment 1

[Mark Mentovai]

Comment on attachment 276299 [details] [diff] [review]
fix

Looks like IconCache.plist is written with entries that ought to be memory-only.  This is a problem in the existing SiteIconCache.mm.

This code can use [inURIAsNSURL scheme], or even [inURIAsNSURL isFileURL].  Gecko's case-insensitive about schemes, can we be too?

Comment 2

[Stuart Morgan]

Attachment: v2

Uses isFileURL. I removed the about: part, since looking at my machines here I appear to have been mistaken that about: has favicons show up.

I'm not sure what you mean about memory-only things being written to disk; this patch prevented file: urls from being added in my testing, and I don't see a code path where memory-only objects can make it to disk.

Existing file: entries will certainly still be there after this patch, but since cleaning old stuff out is a general problem that we still really need to solve I don't want to make a hacky solution just for file: URLs.

Comment 3

[Mark Mentovai]

Comment on attachment 276492 [details] [diff] [review]
v2

([]) unnecessary here.  This method now calls [inURIAsNSURL isFileURL] twice - please reduce to a single call.  sr=me with those changes.

Comment 4

[Stuart Morgan]

Landed on trunk and MOZILLA_1_8_BRANCH with requested changes.

Comment 5

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Nominating for 1.5.2.

Comment 6

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Any reason not to land this on 1_5 for 1.5.2?

Comment 7

[Stuart Morgan]

No real reason not to, so I'd be fine with it, but I don't see any compelling reason to do so either, and we usually don't toss random changes into minor update releases.

Comment 8

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Is this not enough of a help against the ever-expanding site icon cache?

Comment 9

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

I think it's a well-contained fix for part of a very bad larger problem.  It'll help some, though maybe not tons, but since it's simple/contained, I think we should land it.

Comment 10

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: 1_5 branch patch as checked in

Since there wasn't a final diff of the trunk/18branch patch, I'll attach the diff I made for landing this on the 1_5 branch.

Comment 11

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Landed on the CAMINO_1_5_BRANCH for 1.5.2.