Bug 99087

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

(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.

(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.

(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. :)

Created attachment 168551 [details] Patch

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?

(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.

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.

> 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.

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?

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

Created attachment 168654 [details] Patch for landing Take Kenneth's feedback into consideration. Using RefPtr to Evas_Object looks much better indeed, thanks.

(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.

Comment on attachment 168654 [details] Patch for landing Clearing flags on attachment: 168654 Committed r131337: <http://trac.webkit.org/changeset/131337>

All reviewed patches have been landed. Closing bug.