Bug 130577 – [EFL][WK2] Implement ewk APIs of web local storage which have deletion functions of the local storage entries

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Joonghun Park]

implemented ewk web storage API depending on currently existing WKAPI.

[Joonghun Park]

Created attachment 227413 [details]
Patch

[Ryuan Choi]

It's wrong.

Check the ewk_storage_manager.cpp

[Joonghun Park]

(In reply to comment #2)
> It's wrong.
> 
> Check the ewk_storage_manager.cpp

I checked ewk_storage_manager.cpp and saw that you already had reimplemented it.
So, I will reconsider about this patch if it is valueable to revise or not.

Thank you for your comment.

[Jinwoo Song]

Comment on attachment 227413 [details]
Patch

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

You should add unit tests for newly added APIs.

> Source/WebKit2/UIProcess/API/efl/ewk_context.h:362
> +typedef void (*Ewk_Web_Storage_Origins_Get_Callback)(Eina_List* origins, void *user_data);

Duplicated with Ewk_Storage_Origins_Async_Get_Cb in ewk_stroage_manager.h

> Source/WebKit2/UIProcess/API/efl/ewk_context.h:371
> +EAPI Eina_Bool ewk_context_web_storage_delete_all(Ewk_Context *context);

It would be better to move into ewk_storage_manager and change the name to *ewk_storage_manager_all_entries_delete(Ewk_Storage_Manager *manager)*.

> Source/WebKit2/UIProcess/API/efl/ewk_context.h:380
> +EAPI Eina_Bool ewk_context_web_storage_origin_delete(Ewk_Context *context, Ewk_Security_Origin *origin);

It would be better to move into ewk_storage_manager and change the name to *ewk_storage_manager_entries_for_origin_delete(Ewk_Storage_Manager *manager, Ewk_Security_Origin *origin)*.

> Source/WebKit2/UIProcess/API/efl/ewk_context.h:394
> +EAPI Eina_Bool ewk_context_web_storage_origins_get(Ewk_Context *context, Ewk_Web_Storage_Origins_Get_Callback callback, void *user_data);

Duplicated with ewk_storage_manager_origins_async_get() in ewk_stroage_manager.h

> Source/WebKit2/UIProcess/API/efl/ewk_context.h:397
> + * Requests for setting web storage path.

This API sets the local storage path but does not set the session storage path. So it is right to comment *local* not *web*.

> Source/WebKit2/UIProcess/API/efl/ewk_context.h:400
> + * @param path web storage path to set

ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_context.h:404
> +EAPI Eina_Bool ewk_context_web_storage_path_set(Ewk_Context *context, const char *path);

It would better to rename to *ewk_context_local_storage_path_set(Ewk_Context *context, const char *path)*.

[Joonghun Park]

Created attachment 227627 [details]
Patch

[Ryuan Choi]

Comment on attachment 227627 [details]
Patch

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

> Source/WebKit2/ChangeLog:4
> +        Implement ewk web storage APIs which has delete entries functionality.
> +        https://bugs.webkit.org/show_bug.cgi?id=130577.

Bug title is wrong.
Please change title of bug.

And add test case.

> Source/WebKit2/UIProcess/API/efl/ewk_storage_manager.cpp:57
> +void EwkStorageManager::deleteAllStorageEntries()
> +{
> +    WKKeyValueStorageManagerDeleteAllEntries(m_storageManager.get());
> +}
> +
> +void EwkStorageManager::deleteStorageEntriesForOrigin(WKSecurityOriginRef origin)
> +{
> +    WKKeyValueStorageManagerDeleteEntriesForOrigin(m_storageManager.get(), origin);
> +}

Should we add this? IMO, we'd better to avoid these dummy methods.

> Source/WebKit2/UIProcess/API/efl/ewk_storage_manager.cpp:125
> +    WKRetainPtr<WKStringRef> hostRef(AdoptWK, WKStringCreateWithUTF8CString(ewk_security_origin_host_get(origin)));
> +    WKRetainPtr<WKStringRef> protocolRef(AdoptWK, WKStringCreateWithUTF8CString(ewk_security_origin_protocol_get(origin)));
> +    WKRetainPtr<WKSecurityOriginRef> securityOriginRef(AdoptWK, WKSecurityOriginCreate(protocolRef.get(), hostRef.get(), ewk_security_origin_port_get(origin)));

Use EwkSecurityOrigin::wkSecurityOrigin() to get WKSecurityOrigin.

> Source/WebKit2/UIProcess/API/efl/ewk_storage_manager.h:87
> + * @param context context object

wrong.

> Source/WebKit2/UIProcess/API/efl/ewk_storage_manager.h:94
> + * Deletes origin and DBs related with the origin that is stored in web storage db.

It looks wrong.

> Source/WebKit2/UIProcess/API/efl/ewk_storage_manager.h:96
> + * @param context context object

wrong.

[Gyuyoung Kim]

Comment on attachment 227627 [details]
Patch

The API needs still lots of love. r-

[Joonghun Park]

Created attachment 227846 [details]
Patch

[Ryuan Choi]

Comment on attachment 227846 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_storage_manager.h:91
> +EAPI Eina_Bool ewk_storage_manager_all_entries_delete(Ewk_Storage_Manager *manager);

How about ewk_storage_manager_delete_all ?

> Source/WebKit2/UIProcess/API/efl/ewk_storage_manager.h:101
> +EAPI Eina_Bool ewk_storage_manager_entries_for_origin_delete(Ewk_Storage_Manager *manager, Ewk_Security_Origin *origin);

How about just ewk_storage_manager_delete ?

> Source/WebKit2/UIProcess/API/efl/ewk_storage_manager_private.h:45
> +    inline WKRetainPtr<WKKeyValueStorageManagerRef> getStorageManager()
> +    {
> +        return m_storageManager;
> +    }

sing line looks better.
inline is unnecessary.

And wkStorageManager looks better, IMO.

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_storage_manager.cpp:53
> +        originData->originList = 0;

originData->originList = nullptr;

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_storage_manager.cpp:61
> +            fprintf(stderr, "%s\n", ewk_security_origin_host_get(origin));

maybe, mistake?

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_storage_manager.cpp:133
> +TEST_F(EWK2StorageManagerTest, ewk_storage_manager_all_entries_delete)

Instead of adding this, we can test them using current test because you called it in _origins_async_get.

[Jinwoo Song]

Comment on attachment 227846 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/efl/ewk_storage_manager.h:91
>> +EAPI Eina_Bool ewk_storage_manager_all_entries_delete(Ewk_Storage_Manager *manager);
> 
> How about ewk_storage_manager_delete_all ?

According to EFL coding convention, function namespace needs to describe *object* and *specialization*.

  - namespaced code: <module>_<object>_<specializations>_<action>, example evas_object_color_get(). 

In this API, I think *ewk_storage_manager* is a module and *all_entries* is an object. Actually, this API does not delete *ewk_storage_manager* object but delete *entries* in the ewk_storage_manager object.
One ambiguous thing is the location of *all* word. The name may be 'ewk_storage_manager_entries_delete_all'.

>> Source/WebKit2/UIProcess/API/efl/ewk_storage_manager.h:101
>> +EAPI Eina_Bool ewk_storage_manager_entries_for_origin_delete(Ewk_Storage_Manager *manager, Ewk_Security_Origin *origin);
> 
> How about just ewk_storage_manager_delete ?

In this case, *for origin* maybe a <specializatoins>.

[Ryuan Choi]

(In reply to comment #10)
> (From update of attachment 227846 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=227846&action=review
> 
> >> Source/WebKit2/UIProcess/API/efl/ewk_storage_manager.h:91
> >> +EAPI Eina_Bool ewk_storage_manager_all_entries_delete(Ewk_Storage_Manager *manager);
> > 
> > How about ewk_storage_manager_delete_all ?
> 
> According to EFL coding convention, function namespace needs to describe *object* and *specialization*.
> 
>   - namespaced code: <module>_<object>_<specializations>_<action>, example evas_object_color_get(). 
> 
> In this API, I think *ewk_storage_manager* is a module and *all_entries* is an object. Actually, this API does not delete *ewk_storage_manager* object but delete *entries* in the ewk_storage_manager object.
> One ambiguous thing is the location of *all* word. The name may be 'ewk_storage_manager_entries_delete_all'.
> 
After discussed with jinwoo,
ewk_storage_manager_entries_clear looks better.

> >> Source/WebKit2/UIProcess/API/efl/ewk_storage_manager.h:101
> >> +EAPI Eina_Bool ewk_storage_manager_entries_for_origin_delete(Ewk_Storage_Manager *manager, Ewk_Security_Origin *origin);
> > 
> > How about just ewk_storage_manager_delete ?
> 
> In this case, *for origin* maybe a <specializatoins>.

ewk_storage_manager_entries_for_origin_del looks better.

any other suggestion?

[Jinwoo Song]

Comment on attachment 227846 [details]
Patch

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

>>>> Source/WebKit2/UIProcess/API/efl/ewk_storage_manager.h:91
>>>> +EAPI Eina_Bool ewk_storage_manager_all_entries_delete(Ewk_Storage_Manager *manager);
>>> 
>>> How about ewk_storage_manager_delete_all ?
>> 
>> According to EFL coding convention, function namespace needs to describe *object* and *specialization*.
>> 
>>   - namespaced code: <module>_<object>_<specializations>_<action>, example evas_object_color_get(). 
>> 
>> In this API, I think *ewk_storage_manager* is a module and *all_entries* is an object. Actually, this API does not delete *ewk_storage_manager* object but delete *entries* in the ewk_storage_manager object.
>> One ambiguous thing is the location of *all* word. The name may be 'ewk_storage_manager_entries_delete_all'.
> 
> After discussed with jinwoo,
> ewk_storage_manager_entries_clear looks better.

+1

>>>> Source/WebKit2/UIProcess/API/efl/ewk_storage_manager.h:101
>>>> +EAPI Eina_Bool ewk_storage_manager_entries_for_origin_delete(Ewk_Storage_Manager *manager, Ewk_Security_Origin *origin);
>>> 
>>> How about just ewk_storage_manager_delete ?
>> 
>> In this case, *for origin* maybe a <specializatoins>.
> 
> ewk_storage_manager_entries_for_origin_del looks better.
> 
> any other suggestion?

+1

[Joonghun Park]

Created attachment 227913 [details]
Patch

[Ryuan Choi]

Comment on attachment 227913 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_storage_manager.cpp:129
> +    EXPECT_EQ(1, eina_list_count(originData.originList));

Release originList.

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_storage_manager.cpp:164
> +    EXPECT_EQ(3, eina_list_count(originData.originList));

Release originList.

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_storage_manager.cpp:178
> +    ASSERT_FALSE(checkOrigin(originData.originList, &origin, "http", "www.storagetest1.com", 0));

Release originList.

[Build Bot]

Comment on attachment 227913 [details]
Patch

Attachment 227913 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5911403034574848

New failing tests:
media/adopt-node-crash.html

[Build Bot]

Created attachment 227919 [details]
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-16  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Joonghun Park]

Created attachment 227924 [details]
Patch

[Jinwoo Song]

Comment on attachment 227924 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_storage_manager.cpp:128
> +        ecore_timer_del(storage_timer);

You should not free storage_timer again because timer is already fired in the callback. 
Instead you should initialize the timer variable as null.

storage_timer = nullptr;

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_storage_manager.cpp:165
>          ecore_timer_del(storage_timer);

Please fix here, too.

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_storage_manager.cpp:180
> +        ecore_timer_del(storage_timer);

ditto.

[Joonghun Park]

Created attachment 227928 [details]
Patch

[Ryuan Choi]

Comment on attachment 227928 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_storage_manager.cpp:55
> +        void* originItem;
> +        EINA_LIST_FREE(originData->originList, originItem)
> +            ewk_object_unref((Ewk_Object*)originItem);

IMO, it's not wrong but it's not good.

These cleanup are for previous logic. so it's not proper to put these in `getStorageOriginsCallback`

Why don't you just release the list after used.

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_storage_manager.cpp:169
> +    EXPECT_EQ(3, eina_list_count(originData.originList));
> +

here

[Ryuan Choi]

Comment on attachment 227928 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_storage_manager.cpp:66
>                      originData->isSynchronized = true;

originData->originList = origins;
return;

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_storage_manager.cpp:68
>          }

// free origins

[Joonghun Park]

Created attachment 227936 [details]
Patch

[Ryuan Choi]

Comment on attachment 227936 [details]
Patch

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

> Source/WebKit2/ChangeLog:14
> +        adding API unittest

I think that you need more description.

you not only added test case for ewk_storage_manager_entries_clear/ewk_storage_manager_entries_for_origin_del and also fixed leak of legacy test case.

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_storage_manager.cpp:69
> +            ewk_object_unref((Ewk_Object*)originItem);

can we use static_cast<Ewk_Object*>(originItem) ?

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_storage_manager.cpp:135
> +        ewk_object_unref((Ewk_Object*)originItem);

can we use static_cast<Ewk_Object*>(originItem) ?

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_storage_manager.cpp:147
> +    const char* storageHTML =
> +        "<html><head><title>original title</title></head>"
> +        "<body>"
> +        "<script type='text/javascript'>"
> +        " localStorage.setItem('item', 'storage');"
> +        "</script>"
> +        "</body></html>";

It's duplicated in ewk_storage_manager_origins_async_get/ewk_storage_manager_entries_for_origin_delete.
How about moving it out of source code?
static char storageHTML[] = "...";

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_storage_manager.cpp:171
> +    ewk_storage_manager_entries_for_origin_del(originData.manager, static_cast<Ewk_Security_Origin*>(eina_list_nth(originData.originList, 0)));

Just question,
Can we guarantee first item is for www.storagetest1.com ?

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_storage_manager.cpp:175
> +        ewk_object_unref((Ewk_Object*)originItem);

can we use static_cast<Ewk_Object*>(originItem) ?

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_storage_manager.cpp:189
> +        ewk_object_unref((Ewk_Object*)originItem);

can we use static_cast<Ewk_Object*>(originItem) ?

[Joonghun Park]

Created attachment 228022 [details]
Patch

[Joonghun Park]

Created attachment 228029 [details]
Patch

[Ryuan Choi]

(In reply to comment #25)
> Created an attachment (id=228029) [details]
> Patch

LGTM

[Gyuyoung Kim]

Comment on attachment 228029 [details]
Patch

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

> Source/WebKit2/ChangeLog:3
> +        [EFL][WK2]Implement ewk web local storage APIs which has delete function of the local storage entries

[EFL][WK2] Implement ewk APIs of web local storage which have deletion functions of the local storage entries ?

> Source/WebKit2/ChangeLog:14
> +        adding API unittest and fixing memory leak of the existing test case

It looks you just update existing API test. So, I think it is a little correct to change *adding* with *updating*. Besides do you add API unit test to ewk_storage_manager_private.h ?

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_storage_manager.cpp:189
> +    storage_timer = ecore_timer_add(1, reinterpret_cast<Ecore_Task_Cb>(timerCallback), &originData);

s/storage_timer/storageTimer/g ?

[Joonghun Park]

Created attachment 228241 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 228241 [details]
Patch

LGTM now.

[WebKit Commit Bot]

Comment on attachment 228241 [details]
Patch

Clearing flags on attachment: 228241

Committed r166565: <http://trac.webkit.org/changeset/166565>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.