Bug 108598

Created attachment 185989 [details] Patch

Comment on attachment 185989 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=185989&action=review LGTM, but changelog could be improved > Source/WebKit2/ChangeLog:9 > + Refactor the Ewk_Favicon code so that it no longer relies on internal > + C++ API and so that it is based solely on the C API. Here you say it is a refactor, but below you detail behavior changes. I would add that here as well, which leads me to the question, how is this tested? > Source/WebKit2/ChangeLog:21 > + * UIProcess/API/efl/ewk_favicon_database.cpp: Client are now notified > + of favicon changes only when the favicon data becomes available and > + make API to retrieve a favicon synchronous. NULL is returned if the > + favicon data is not available. Not a needed change, but I prefer some spacing here... that makes it easier to read using the review tool > Source/WebKit2/ChangeLog:36 > + * UIProcess/API/efl/tests/test_ewk2_favicon_database.cpp: Update API > + tests to use the new favicon API. You should detail the testing further up. Before all the details > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:896 > + smartCallback<FaviconChanged>().call(); > +} > + > +Evas_Object* EwkView::createFavicon() const > +{ why is the above informURLChange not const? > Source/WebKit2/UIProcess/API/efl/ewk_favicon_database.cpp:111 > +Evas_Object* ewk_favicon_database_icon_get(Ewk_Favicon_Database* ewkIconDatabase, const char* page_url, Evas* evas) why the inconsistency? page_url vs ewkIconDatabase > Source/WebKit2/UIProcess/API/efl/ewk_favicon_database.cpp:122 > - return true; > + return WebCore::evasObjectFromCairoImageSurface(evas, surface.get()).leakRef(); > } it is a get method but it needs to be freed. Is this documented? > Source/WebKit2/UIProcess/API/efl/ewk_favicon_database.h:57 > + * The returned Evas_Object needs to be freed after use. apparently :-)

Comment on attachment 185989 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=185989&action=review >> Source/WebKit2/ChangeLog:21 >> + favicon data is not available. > > Not a needed change, but I prefer some spacing here... that makes it easier to read using the review tool Ok. >> Source/WebKit2/ChangeLog:36 >> + tests to use the new favicon API. > > You should detail the testing further up. Before all the details Ok will do. >> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:896 >> +{ > > why is the above informURLChange not const? You are right, it can probably be const now. I'll update. >> Source/WebKit2/UIProcess/API/efl/ewk_favicon_database.cpp:111 >> +Evas_Object* ewk_favicon_database_icon_get(Ewk_Favicon_Database* ewkIconDatabase, const char* page_url, Evas* evas) > > why the inconsistency? page_url vs ewkIconDatabase Right, my bad. Should be pageURL in the implementation. >> Source/WebKit2/UIProcess/API/efl/ewk_favicon_database.cpp:122 >> } > > it is a get method but it needs to be freed. Is this documented? Yes, I updated the doc. Unfortunately, we don't seem to have a naming convention to distinguish those cases, we rely solely on the doc.

Comment on attachment 185989 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=185989&action=review > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:895 > +Evas_Object* EwkView::createFavicon() const mmm.. const method returning mutable pointer? > Source/WebKit2/UIProcess/API/efl/EwkView.h:131 > + Evas_Object* createFavicon() const; either const Evas_Object* or non const member

(In reply to comment #4) > (From update of attachment 185989 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=185989&action=review > > > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:895 > > +Evas_Object* EwkView::createFavicon() const > > mmm.. const method returning mutable pointer? > > > Source/WebKit2/UIProcess/API/efl/EwkView.h:131 > > + Evas_Object* createFavicon() const; > > either const Evas_Object* or non const member oops, it's not returning a member, so I think it's OK

Comment on attachment 185989 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=185989&action=review >>> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:896 >>> +{ >> >> why is the above informURLChange not const? > > You are right, it can probably be const now. I'll update. informURLChange() is not const because it updates m_url member which is not mutable. We could mark m_url as mutable but since there is not strictly related to my change I would prefer not to do this in this patch.

Comment on attachment 185989 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=185989&action=review > Source/WebKit2/UIProcess/API/efl/ewk_view.h:383 > +EAPI Evas_Object *ewk_view_favicon_get(const Evas_Object *o); I propose to rename this one to ewk_view_favicon_new(const Evas_Object *o) so that memory management is clear to the caller.

Created attachment 186033 [details] Patch Take Kenneth's feedback into consideration.

Comment on attachment 186033 [details] Patch Attachment 186033 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/16333096

Comment on attachment 186033 [details] Patch It seems win-ews is in a bad state. The warning seems unrelated.

Could I get (informal) review?

Comment on attachment 186033 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=186033&action=review > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:580 > +Evas_Object* ewk_view_favicon_new(const Evas_Object *ewkView) pointer star alignment? inconsistent in this line > Source/WebKit2/UIProcess/API/efl/ewk_view.h:52 > + * - "favicon,changed", void: reports that the view's favicon has changed. The favicon can be queried using > + * ewk_view_favicon_new(). I would split that line a bit before > Tools/MiniBrowser/efl/main.c:569 > + Evas_Object* favicon = ewk_view_favicon_new(ewk_view); > + update_view_favicon(window, favicon); > + > + if (favicon) > + evas_object_unref(favicon); so if it can be null does calling update_view_favicon make sense? is it safe?

Comment on attachment 186033 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=186033&action=review >> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:580 >> +Evas_Object* ewk_view_favicon_new(const Evas_Object *ewkView) > > pointer star alignment? inconsistent in this line Ok. >> Source/WebKit2/UIProcess/API/efl/ewk_view.h:52 >> + * ewk_view_favicon_new(). > > I would split that line a bit before OK. >> Tools/MiniBrowser/efl/main.c:569 >> + evas_object_unref(favicon); > > so if it can be null does calling update_view_favicon make sense? is it safe? Yes, it is safe and it is needed to make sure the previous favicon actually gets removed. Otherwise, you may move from a page that has a favicon, to a page that doesn't have one and still display the previous page's favicon.

Created attachment 187865 [details] Patch Take kenneth's feedback into consideration.

Comment on attachment 187865 [details] Patch LGTM

Comment on attachment 187865 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=187865&action=review > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:-758 > -void EwkView::informIconChange() nice that we don't have this method anymore > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1021 > + return ewk_favicon_database_icon_get(iconDatabase, m_url, sd->base.evas); smartData()->base.evas It's looks a bit weird to have "ewk_favicon_database_icon_get" in "create" function..are you sure the naming is fully OK?

Created attachment 187919 [details] Patch Take Mikhail's feedback into consideration. Note that I renamed ewk_view_favicon_new() to ewk_view_favicon_get() since it seems to be according to EFL style and it is documented that the returned value needs to be freed. Also, it is consistent with ewk_favicon_database_icon_get().

Comment on attachment 187919 [details] Patch LGTM

Created attachment 188136 [details] Patch Rebase on master.

Brady, could you please take a look at this patch?

(In reply to comment #20) > Brady, could you please take a look at this patch? It's 100% efl/ewk stuff, not sure I feel qualified to make any judgements. Looks fine, without having a deep understanding of how EFL works.

This is fine for another reviewer to take a look at.

Created attachment 188648 [details] Patch Rebase on master.

Comment on attachment 188648 [details] Patch Kenneth, could you please review it?

Comment on attachment 188648 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=188648&action=review > Source/WebKit2/UIProcess/API/efl/ewk_favicon_database.h:51 > * Retrieves asynchronously from the database the favicon for the given @a page_url Should this be synchronous instead ?

Comment on attachment 188648 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=188648&action=review >> Source/WebKit2/UIProcess/API/efl/ewk_favicon_database.h:51 >> * Retrieves asynchronously from the database the favicon for the given @a page_url > > Should this be synchronous instead ? Right, good catch.

Created attachment 188768 [details] Patch Take Laszlo's feedback into consideration.

Comment on attachment 188768 [details] Patch r=me as signed off by Brady

Comment on attachment 188768 [details] Patch Clearing flags on attachment: 188768 Committed r143190: <http://trac.webkit.org/changeset/143190>

All reviewed patches have been landed. Closing bug.