Bug 99087 – [EFL][WK2] Implement Favicons API

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Chris Dumez]

We should implement the Favicons API so that clients can retrieve icons for specific URLs.

The GTK port added it recently via Bug 96476.

[Jinwoo Song]

Chris, did you already work for a patch? AFAIK, Jaehun has been refactoring and prepared upstream the internal code.

[Chris Dumez]

(In reply to comment #1)
> Chris, did you already work for a patch? AFAIK, Jaehun has been refactoring and prepared upstream the internal code.

Yes, sorry. I have it implemented and I'm in the middle of unit tests.

[Chris Dumez]

(In reply to comment #1)
> Chris, did you already work for a patch? AFAIK, Jaehun has been refactoring and prepared upstream the internal code.

In the future, please file a bug upstream to indicate that you are working on it. This would minimize the risk of overlapping work.

[Jinwoo Song]

(In reply to comment #3)
> (In reply to comment #1)
> > Chris, did you already work for a patch? AFAIK, Jaehun has been refactoring and prepared upstream the internal code.
> 
> In the future, please file a bug upstream to indicate that you are working on it. This would minimize the risk of overlapping work.

I see and I hope the overlapping work should not happen anymore. :)

[Chris Dumez]

Created attachment 168551 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 168551 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=168551&action=review

Just a general question; favicons can be animated and include multiple images (or be generated). Do we support this? and do we have a limit (there can be infinite new images which can fill up memory) I know this could be a security issue and other ports have limits.

> Source/WebCore/platform/graphics/efl/EvasCairoUtilities.cpp:28
> +#include "config.h"
> +#include "EvasCairoUtilities.h"
> +
> +Evas_Object* cairoImageSurfaceToEvasObject(Evas* canvas, cairo_surface_t* surface)
> +{
> +    EINA_SAFETY_ON_NULL_RETURN_VAL(canvas, 0);
> +    EINA_SAFETY_ON_NULL_RETURN_VAL(surface, 0);
> +

We have similar methods (create cairo surface for edje object etc) in RenderThemeEfl, maybe they should be moved.

> Source/WebKit/efl/ewk/ewk_history.cpp:358
> +    return surface ? cairoImageSurfaceToEvasObject(canvas, surface) : 0;

wouldn't evasObjectFromCairoImageSurface not be a better name?

[Chris Dumez]

(In reply to comment #6)
> (From update of attachment 168551 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=168551&action=review
> 
> Just a general question; favicons can be animated and include multiple images (or be generated). Do we support this? and do we have a limit (there can be infinite new images which can fill up memory) I know this could be a security issue and other ports have limits.

We don't support animated favicons at this point (neither does GTK port). We always return the first frame in the Image.

I cannot find the limit you are talking about in GTK or Qt port. Could you point me in the right direction? They do have functions to clear the icon database though (I'm planning to add this as well but in a later patch for size reasons).


> > Source/WebCore/platform/graphics/efl/EvasCairoUtilities.cpp:28
> > +#include "config.h"
> > +#include "EvasCairoUtilities.h"
> > +
> > +Evas_Object* cairoImageSurfaceToEvasObject(Evas* canvas, cairo_surface_t* surface)
> > +{
> > +    EINA_SAFETY_ON_NULL_RETURN_VAL(canvas, 0);
> > +    EINA_SAFETY_ON_NULL_RETURN_VAL(surface, 0);
> > +
> 
> We have similar methods (create cairo surface for edje object etc) in RenderThemeEfl, maybe they should be moved.

Ok, I'll take a look, thanks. I'll probably rename the file to CairoUtilitiesEfl.cpp then.

> 
> > Source/WebKit/efl/ewk/ewk_history.cpp:358
> > +    return surface ? cairoImageSurfaceToEvasObject(canvas, surface) : 0;
> 
> wouldn't evasObjectFromCairoImageSurface not be a better name?

Why not. I will change it.

[Chris Dumez]

Created attachment 168643 [details]
Patch

Take Kenneth's feedback into consideration except for limiting the number of icons.

I cannot find similar icon limiting API in other ports and there is already a mechanism in WebCore's IconDatabase to expire icons after 4 days.

[Kenneth Rohde Christiansen]

> We don't support animated favicons at this point (neither does GTK port). We always return the first frame in the Image.
> 
> I cannot find the limit you are talking about in GTK or Qt port. Could you point me in the right direction? They do have functions to clear the icon database though (I'm planning to add this as well but in a later patch for size reasons).

Maybe it was never added, but I remember the bugzilla discussions. I think Jesus Sanchez-Palencia worked on it, but I might be wrong.

[Kenneth Rohde Christiansen]

Comment on attachment 168643 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=168643&action=review

> Source/WebKit2/UIProcess/API/efl/ewk_favicon_database.cpp:205
> +    Evas_Object* icon = response->surface ? WebCore::evasObjectFromCairoImageSurface(response->callbackData.evas, response->surface.get()) : 0;

RefPtr? Evas_Object should support it

> Source/WebKit2/UIProcess/API/efl/ewk_favicon_database.cpp:229
> +    // If there's not a valid icon, but there's an iconURL registered,

If there is no valid icon?

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_favicon_database.cpp:100
> +    while (!iconChanged)
> +        ecore_main_loop_iterate();

newline after this

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_favicon_database.cpp:156
> +    while (!data.icon)
> +        ecore_main_loop_iterate();

newline

> Source/WebKit2/UIProcess/efl/WebContextEfl.cpp:57
> +    return String::fromUTF8(efreet_data_home_get()) + "/WebKitEfl/IconDatabase";

so I suppose this is a hidden dir?

[Chris Dumez]

Comment on attachment 168643 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=168643&action=review

>> Source/WebKit2/UIProcess/efl/WebContextEfl.cpp:57
>> +    return String::fromUTF8(efreet_data_home_get()) + "/WebKitEfl/IconDatabase";
> 
> so I suppose this is a hidden dir?

Yes. By default, efreet_data_home_get() (or XDG_DATA_HOME) is ~/.local/share

[Chris Dumez]

Created attachment 168654 [details]
Patch for landing

Take Kenneth's feedback into consideration.

Using RefPtr to Evas_Object looks much better indeed, thanks.

[Jesus Sanchez-Palencia]

(In reply to comment #9)
> > We don't support animated favicons at this point (neither does GTK port). We always return the first frame in the Image.
> > 
> > I cannot find the limit you are talking about in GTK or Qt port. Could you point me in the right direction? They do have functions to clear the icon database though (I'm planning to add this as well but in a later patch for size reasons).
> 
> Maybe it was never added, but I remember the bugzilla discussions. I think Jesus Sanchez-Palencia worked on it, but I might be wrong.

It was Rafael who worked on it, Kenneth. He is now CC'ed here.

[WebKit Review Bot]

Comment on attachment 168654 [details]
Patch for landing

Clearing flags on attachment: 168654

Committed r131337: <http://trac.webkit.org/changeset/131337>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.