Bug 22374 – Need API to access favicons using the favicon URL.

• Status: RESOLVED
• Resolution: INVALID
• Component: WebCore Misc.

[Yael]

Applications need to fetch the favicon data without having the exact URL of the page that loaded the favicon originally. An example would be when displaying a list of local storage databases. The application has only the security origin. By appending "/favicon.ico" applications can get the favicon in most cases.

[Yael]

Created attachment 25300 [details]
Implementation of the new required API

[Eric Seidel (no email)]

Comment on attachment 25300 [details]
Implementation of the new required API

If the application needs these icons cached, it seems it should have its own cache with a lifetime tied to the lifetime of the storage databases.  But maybe I'm misunderstanding the problem?

Also, you should note that WebCore has no public interfaces.  If you application directly depends on WebCore it will break in the future.  I'm not sure if that's the case here or not.  A good example of this is when Qt exposed the "is this url in your history database" function call from WebCore to Qt embedders.  This changed when we moved to a better hash-based history lookup, and broke the Qt build.  Now they have an ugly hack in place to support this (hopefully soon deprecated Qt API).

Marking as r- to remove it from the review queue pending further comment.  Certainly please correct me if I've misunderstood.

[Simon Hausmann]

(In reply to comment #2)
> (From update of attachment 25300 [details] [review])
> If the application needs these icons cached, it seems it should have its own
> cache with a lifetime tied to the lifetime of the storage databases.  But maybe
> I'm misunderstanding the problem?

I think there is a misunderstanding indeed :). The storage database list is just an example. If I understand this correctly it is more about the ability to query a site-wide favicon.   

> Also, you should note that WebCore has no public interfaces.  If you
> application directly depends on WebCore it will break in the future.  I'm not
> sure if that's the case here or not.  A good example of this is when Qt exposed
> the "is this url in your history database" function call from WebCore to Qt
> embedders.  This changed when we moved to a better hash-based history lookup,
> and broke the Qt build.  Now they have an ugly hack in place to support this
> (hopefully soon deprecated Qt API).

I agree that in the history case it was a mistake to rely on this design :(, but I feel this case is different in the sense that it's just about exposing something that exists by design.

[Darin Adler]

Comment on attachment 25300 [details]
Implementation of the new required API

I'm not sure I understand the use of the term "API" here. This is the internal interface between WebCore and WebKit. I don't see a patch adding API to any platform's WebKit.

A patch that adds an unused function is not a good idea.

Further, there's too much copied and pasted code here -- this should not just be a copy of iconForPageURL; instead it should be factored so the code is shared appropriately.

Because of these two issues I'm going to say review- for this version of the patch.