implemented ewk web storage API depending on currently existing WKAPI.
Created attachment 227413 [details] Patch
It's wrong. Check the ewk_storage_manager.cpp
(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.
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)*.
Created attachment 227627 [details] Patch
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.
Comment on attachment 227627 [details] Patch The API needs still lots of love. r-
Created attachment 227846 [details] Patch
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.
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>.
(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?
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
Created attachment 227913 [details] Patch
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.
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
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
Created attachment 227924 [details] Patch
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.
Created attachment 227928 [details] Patch
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
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
Created attachment 227936 [details] Patch
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) ?
Created attachment 228022 [details] Patch
Created attachment 228029 [details] Patch
(In reply to comment #25) > Created an attachment (id=228029) [details] > Patch LGTM
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 ?
Created attachment 228241 [details] Patch
Comment on attachment 228241 [details] Patch LGTM now.
Comment on attachment 228241 [details] Patch Clearing flags on attachment: 228241 Committed r166565: <http://trac.webkit.org/changeset/166565>
All reviewed patches have been landed. Closing bug.