[EFL][WK2] Refactor Ewk_Favicon code and stop relying on internal C++ API

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Chris Dumez]

The Ewk_Favicon code still relies on internal C++ API that has no C API equivalent. We likely need to refactor the code to stop relying on this internal API.

[Chris Dumez]

Created attachment 185989 [details]
Patch

[Kenneth Rohde Christiansen]

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

[Chris Dumez]

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.

[Mikhail Pozdnyakov]

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

[Mikhail Pozdnyakov]

(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

[Chris Dumez]

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.

[Chris Dumez]

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.

[Chris Dumez]

Created attachment 186033 [details]
Patch

Take Kenneth's feedback into consideration.

[Build Bot]

Comment on attachment 186033 [details]
Patch

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

[Chris Dumez]

Comment on attachment 186033 [details]
Patch

It seems win-ews is in a bad state. The warning seems unrelated.

[Chris Dumez]

Could I get (informal) review?

[Kenneth Rohde Christiansen]

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?

[Chris Dumez]

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.

[Chris Dumez]

Created attachment 187865 [details]
Patch

Take kenneth's feedback into consideration.

[Kenneth Rohde Christiansen]

Comment on attachment 187865 [details]
Patch

LGTM

[Mikhail Pozdnyakov]

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?

[Chris Dumez]

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

[Mikhail Pozdnyakov]

Comment on attachment 187919 [details]
Patch

LGTM

[Chris Dumez]

Created attachment 188136 [details]
Patch

Rebase on master.

[Chris Dumez]

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

[Brady Eidson]

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

[Brady Eidson]

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

[Chris Dumez]

Created attachment 188648 [details]
Patch

Rebase on master.

[Chris Dumez]

Comment on attachment 188648 [details]
Patch

Kenneth, could you please review it?

[Laszlo Gombos]

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 ?

[Chris Dumez]

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.

[Chris Dumez]

Created attachment 188768 [details]
Patch

Take Laszlo's feedback into consideration.

[Kenneth Rohde Christiansen]

Comment on attachment 188768 [details]
Patch

r=me as signed off by Brady

[WebKit Review Bot]

Comment on attachment 188768 [details]
Patch

Clearing flags on attachment: 188768

Committed r143190: <http://trac.webkit.org/changeset/143190>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.