Bug 146337 – [EFL] Implement ewk_favicon_database_clear() API

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Hyungwook Lee]

We need to provide ewk_favicon_database_clear() API for make able to clear all favicon data.

[Hyungwook Lee]

Created attachment 255627 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 255627 [details]
Patch

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

Patch looks good to me except for my comment.

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_favicon_database.cpp:124
> +    data.icon = ewk_favicon_database_icon_get(database, httpServer->getURLForPath("/").data(), evas_object_evas_get(webView()));

Though I'm not sure whether I fully understand ewk_favicon_database_clear() behavior, It seems that this test is not enough to test this new API. In order to test this API, I think we need to register some favicons for a couple of urls, then we call ewk_favicon_database_clear(). After calling it, we have to check each favicon for each URL to verify if all favicons are cleared successfully. It would be good if you add new test for the test as below,

TEST_F(EWK2FaviconDatabaseTest, ewk_favicon_database_clear)
{
    ...
}

[Gyuyoung Kim]

Comment on attachment 255627 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_favicon_database.h:100
> + * @param database database object to clear all favicon data.

Do not add a period in @param line.

[Hyungwook Lee]

Comment on attachment 255627 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/efl/ewk_favicon_database.h:100
>> + * @param database database object to clear all favicon data.
> 
> Do not add a period in @param line.

Done

>> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_favicon_database.cpp:124
>> +    data.icon = ewk_favicon_database_icon_get(database, httpServer->getURLForPath("/").data(), evas_object_evas_get(webView()));
> 
> Though I'm not sure whether I fully understand ewk_favicon_database_clear() behavior, It seems that this test is not enough to test this new API. In order to test this API, I think we need to register some favicons for a couple of urls, then we call ewk_favicon_database_clear(). After calling it, we have to check each favicon for each URL to verify if all favicons are cleared successfully. It would be good if you add new test for the test as below,
> 
> TEST_F(EWK2FaviconDatabaseTest, ewk_favicon_database_clear)
> {
>     ...
> }

To test ewk_favicon_database_clear(), it required favicon data.
ewk_favicon_database_async_icon_get test case already has favicon data that retrieved by ewk_favicon_database_icon_change_callback_add().
Hence I think ewk_favicon_database_async_icon_get() is good position to test ewk_favicon_database_clear() API.

[Hyungwook Lee]

Created attachment 255814 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 255627 [details]
Patch

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

>>> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_favicon_database.cpp:124
>>> +    data.icon = ewk_favicon_database_icon_get(database, httpServer->getURLForPath("/").data(), evas_object_evas_get(webView()));
>> 
>> Though I'm not sure whether I fully understand ewk_favicon_database_clear() behavior, It seems that this test is not enough to test this new API. In order to test this API, I think we need to register some favicons for a couple of urls, then we call ewk_favicon_database_clear(). After calling it, we have to check each favicon for each URL to verify if all favicons are cleared successfully. It would be good if you add new test for the test as below,
>> 
>> TEST_F(EWK2FaviconDatabaseTest, ewk_favicon_database_clear)
>> {
>>     ...
>> }
> 
> To test ewk_favicon_database_clear(), it required favicon data.
> ewk_favicon_database_async_icon_get test case already has favicon data that retrieved by ewk_favicon_database_icon_change_callback_add().
> Hence I think ewk_favicon_database_async_icon_get() is good position to test ewk_favicon_database_clear() API.

Now I understand, ewk_favicon_database_clear() can remove all favicon datas, right ? So I meant that we have to test if ewk_favicon_database_clear() removes all favicon datas. However this test seems to test only one favicon. If not, I agree to land current implementation.

[Hyungwook Lee]

Created attachment 255819 [details]
Patch

[Hyungwook Lee]

(In reply to comment #6)
> Comment on attachment 255627 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=255627&action=review
> 
> >>> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_favicon_database.cpp:124
> >>> +    data.icon = ewk_favicon_database_icon_get(database, httpServer->getURLForPath("/").data(), evas_object_evas_get(webView()));
> >> 
> >> Though I'm not sure whether I fully understand ewk_favicon_database_clear() behavior, It seems that this test is not enough to test this new API. In order to test this API, I think we need to register some favicons for a couple of urls, then we call ewk_favicon_database_clear(). After calling it, we have to check each favicon for each URL to verify if all favicons are cleared successfully. It would be good if you add new test for the test as below,
> >> 
> >> TEST_F(EWK2FaviconDatabaseTest, ewk_favicon_database_clear)
> >> {
> >>     ...
> >> }
> > 
> > To test ewk_favicon_database_clear(), it required favicon data.
> > ewk_favicon_database_async_icon_get test case already has favicon data that retrieved by ewk_favicon_database_icon_change_callback_add().
> > Hence I think ewk_favicon_database_async_icon_get() is good position to test ewk_favicon_database_clear() API.
> 
> Now I understand, ewk_favicon_database_clear() can remove all favicon datas,
> right ? So I meant that we have to test if ewk_favicon_database_clear()
> removes all favicon datas. However this test seems to test only one favicon.
> If not, I agree to land current implementation.

I've update the test case to support multiple favicon loading.

[Gyuyoung Kim]

Comment on attachment 255819 [details]
Patch

Patch looks better ! However I would like to recommend to add new test for the ewk_favicon_database_clear() because I think TEST_F(EWK2FaviconDatabaseTest, ewk_favicon_database_async_icon_get) is not a proper place to test all icons removals. This test is just to test if we can get a favicon correctly. Thus how about adding a new test instead of modifying the test as below ?

TEST_F(EWK2FaviconDatabaseTest, ewk_favicon_database_clear)

[Hyungwook Lee]

Created attachment 255820 [details]
Patch

[Hyungwook Lee]

(In reply to comment #9)
> Comment on attachment 255819 [details]
> Patch
> 
> Patch looks better ! However I would like to recommend to add new test for
> the ewk_favicon_database_clear() because I think
> TEST_F(EWK2FaviconDatabaseTest, ewk_favicon_database_async_icon_get) is not
> a proper place to test all icons removals. This test is just to test if we
> can get a favicon correctly. Thus how about adding a new test instead of
> modifying the test as below ?
> 
> TEST_F(EWK2FaviconDatabaseTest, ewk_favicon_database_clear)

Done

[Gyuyoung Kim]

Comment on attachment 255820 [details]
Patch

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

LGTM.

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_favicon_database.cpp:147
> +    // Runs another httpServer for loading another favicon

Missing a period at the end of line.

[Hyungwook Lee]

Created attachment 255821 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 255821 [details]
Patch

Clearing flags on attachment: 255821

Committed r186153: <http://trac.webkit.org/changeset/186153>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.