Bug 102853

Created attachment 175316 [details] Patch

Comment on attachment 175316 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175316&action=review > Source/WebKit2/UIProcess/API/efl/ewk_application_cache_manager.cpp:47 > + WKApplicationCacheManagerGetApplicationCacheOrigins(toAPI(m_applicationCacheManager.get()), context, callback); You may use getApplicationCacheOrigins(PassRefPtr<ArrayCallback>) instead of WK APIs. > Source/WebKit2/UIProcess/API/efl/ewk_application_cache_manager.cpp:68 > +struct Ewk_Application_Cache_Origins_Async_Get_Context { I prefer to follow the WebKit coding style except in the public header files. > Source/WebKit2/UIProcess/API/efl/ewk_application_cache_manager.cpp:84 > + OwnPtr<Ewk_Error> ewkError = Ewk_Error::create(wkError); s/Ewk_Error/EwkError > Source/WebKit2/UIProcess/API/efl/ewk_application_cache_manager_private.h:43 > + return adoptPtr(new Ewk_Application_Cache_Manager(context->applicationCacheManagerProxy())); s/Ewk_Application_Cache_Manager/EwkApplicationCacheManager > Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:138 > + return m_applicationCacheManager.get(); Inline function would be better. > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_application_cache_manager.cpp:29 > +#include "UnitTestUtils/EWK2UnitTestEnvironment.h" unneeded include. Already included in EWK2UnitTestBase.h. > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_application_cache_manager.cpp:32 > +#include <EWebKit2.h> ditto. > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_application_cache_manager.cpp:33 > +#include <Ecore.h> ditto. > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_context.cpp:53 > + ASSERT_EQ(applicationCacheManager, ewk_context_application_cache_manager_get(context)); This comparison seems to be meaningless.

Comment on attachment 175316 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175316&action=review > Source/WebKit2/PlatformEfl.cmake:47 > + UIProcess/API/efl/ewk_application_cache_manager.cpp You need to add the new header to the installed headers below in the same file. > Source/WebKit2/UIProcess/API/efl/ewk_application_cache_manager.cpp:62 > + originList = eina_list_append(originList, origin.release().leakRef()); calling .release() here will reset the pointer the NULL in the wrapperCache. This is probably not what you want. Probably it should be something like eina_list_append(originList, ewk_object_ref(origin.get())) > Source/WebKit2/UIProcess/API/efl/ewk_application_cache_manager.cpp:83 > + Eina_List* originList = webApplicationCacheContext->manager->createOriginList(origins); This list is never freed (leak). You should unref the list items and free the list at this end of this function. > Source/WebKit2/UIProcess/API/efl/ewk_application_cache_manager.cpp:93 > + Ewk_Application_Cache_Origins_Async_Get_Context* context = new Ewk_Application_Cache_Origins_Async_Get_Context(ewkApplicationCacheManager, callback, userData); context is never freed? > Source/WebKit2/UIProcess/API/efl/ewk_application_cache_manager.h:51 > + * the Eina_List and its items should be freed after use. Use ewk_security_origin_unref() No, we usually don't pass ownership of callback arguments to the client. The client should clone the list (and ref its items) if it wishes to keep them outside the callback. This is less leak prone and common practice I believe. >> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_application_cache_manager.cpp:29 >> +#include "UnitTestUtils/EWK2UnitTestEnvironment.h" > > unneeded include. Already included in EWK2UnitTestBase.h. ? >> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_application_cache_manager.cpp:32 >> +#include <EWebKit2.h> > > ditto. So? This file is using Ewk types so it should include EWebKit2.h. Relying on other headers includes is bad practice. >> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_application_cache_manager.cpp:33 >> +#include <Ecore.h> > > ditto. So? This file is using ecore function so it should include Ecore.h. Relying on other headers includes is bad practice. > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_application_cache_manager.cpp:81 > +static void getApplicationCacheOriginsCallback(Eina_List* origins, Ewk_Error* error, void* userData) Note that you're not freeing the origins list here, which proves my point that the current API is leak prone. > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_application_cache_manager.cpp:111 > + Eina_Strbuf* body = eina_strbuf_new(); Should be inside the if case otherwise it leaks if the if condition is false. > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_application_cache_manager.cpp:128 > + } There should probably be an else case here which sets status to SOUP_STATUS_NOT_FOUND. > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_application_cache_manager.cpp:157 > + const char* applicationCacheTestUrl = httpServer->getURLForPath("/application_cache.html").data(); You keep a pointer to memory owned by a temporary object (CString), this is bad and may crash. Get rid of this useless variable and update next line to read: ewk_view_url_set(view, httpServer->getURLForPath("/application_cache.html").data()); > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_application_cache_manager.cpp:158 > + ewk_view_url_set(view, applicationCacheTestUrl); There is loadUrlSync() to do this in 1 line instead of 2. >> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_context.cpp:53 >> + ASSERT_EQ(applicationCacheManager, ewk_context_application_cache_manager_get(context)); > > This comparison seems to be meaningless. It is not meaningless, it makes sure we return the same pointer every time. This is consistent with our other tests.

Comment on attachment 175316 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175316&action=review >>> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_application_cache_manager.cpp:29 >>> +#include "UnitTestUtils/EWK2UnitTestEnvironment.h" >> >> unneeded include. Already included in EWK2UnitTestBase.h. > > ? I refactored EWK2UnitTestBase.h to include the EWK2UnitTestEnvironment.h, Ecore.h, Eina.h headers in http://trac.webkit.org/changeset/135349, as they are used commonly in API test cases. Actually, I started this work as I have been pointed out that why <gtest/gtest.h> should be included in the test files even though it is already included in EWK2UnitTestBase.h file. Then according to Laszlo's comments, I moved out the duplicated headers in to EWK2UnitTestBase.h. >>> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_context.cpp:53 >>> + ASSERT_EQ(applicationCacheManager, ewk_context_application_cache_manager_get(context)); >> >> This comparison seems to be meaningless. > > It is not meaningless, it makes sure we return the same pointer every time. This is consistent with our other tests. I got it.

(In reply to comment #4) > > >> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_application_cache_manager.cpp:32 > > >> +#include <EWebKit2.h> > > > > > > ditto. > > > > So? This file is using Ewk types so it should include EWebKit2.h. Relying on other headers includes is bad practice. > I refactored EWK2UnitTestBase.h to include the EWK2UnitTestEnvironment.h, Ecore.h, Eina.h headers in http://trac.webkit.org/changeset/135349, as they are used commonly in API test cases. I agree with Christophe in general that relying on other header includes is bad practice, but when it is justified it might make sense to break the general rule. As an example most WebKit files rely on Platform.h to include Compiler.h - as we made an exception with config.h (and even added this exception to the style checker). The idea of EWK2UnitTestBase.h is similar to config.h - EWK2UnitTestBase.h should be included first in every EFL API test-case. I do not feel strongly about EWK2UnitTestBase.h, but wanted to take this chance to explain why I supported r135349. We should either follow this new practice going forward for future API tests as well or revert r135349 for consistency.

(In reply to comment #2) > (From update of attachment 175316 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=175316&action=review > > > Source/WebKit2/UIProcess/API/efl/ewk_application_cache_manager.cpp:47 > > + WKApplicationCacheManagerGetApplicationCacheOrigins(toAPI(m_applicationCacheManager.get()), context, callback); > > You may use getApplicationCacheOrigins(PassRefPtr<ArrayCallback>) instead of WK APIs. Yes, I'll do. > > > Source/WebKit2/UIProcess/API/efl/ewk_application_cache_manager.cpp:68 > > +struct Ewk_Application_Cache_Origins_Async_Get_Context { > > I prefer to follow the WebKit coding style except in the public header files. I think private header file is better than public header file. > > > Source/WebKit2/UIProcess/API/efl/ewk_application_cache_manager.cpp:84 > > + OwnPtr<Ewk_Error> ewkError = Ewk_Error::create(wkError); > > s/Ewk_Error/EwkError > > > Source/WebKit2/UIProcess/API/efl/ewk_application_cache_manager_private.h:43 > > + return adoptPtr(new Ewk_Application_Cache_Manager(context->applicationCacheManagerProxy())); > > s/Ewk_Application_Cache_Manager/EwkApplicationCacheManager > > > Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:138 > > + return m_applicationCacheManager.get(); > > Inline function would be better. > Yes I'll do. > > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_application_cache_manager.cpp:29 > > +#include "UnitTestUtils/EWK2UnitTestEnvironment.h" > > unneeded include. Already included in EWK2UnitTestBase.h. > > > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_application_cache_manager.cpp:32 > > +#include <EWebKit2.h> > > ditto. > > > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_application_cache_manager.cpp:33 > > +#include <Ecore.h> > > ditto. > > > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_context.cpp:53 > > + ASSERT_EQ(applicationCacheManager, ewk_context_application_cache_manager_get(context)); > > This comparison seems to be meaningless.

(In reply to comment #3) > (From update of attachment 175316 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=175316&action=review > > > Source/WebKit2/PlatformEfl.cmake:47 > > + UIProcess/API/efl/ewk_application_cache_manager.cpp > > You need to add the new header to the installed headers below in the same file. > > > Source/WebKit2/UIProcess/API/efl/ewk_application_cache_manager.cpp:62 > > + originList = eina_list_append(originList, origin.release().leakRef()); > > calling .release() here will reset the pointer the NULL in the wrapperCache. This is probably not what you want. > Probably it should be something like eina_list_append(originList, ewk_object_ref(origin.get())) > > > Source/WebKit2/UIProcess/API/efl/ewk_application_cache_manager.cpp:83 > > + Eina_List* originList = webApplicationCacheContext->manager->createOriginList(origins); > > This list is never freed (leak). You should unref the list items and free the list at this end of this function. > > > Source/WebKit2/UIProcess/API/efl/ewk_application_cache_manager.cpp:93 > > + Ewk_Application_Cache_Origins_Async_Get_Context* context = new Ewk_Application_Cache_Origins_Async_Get_Context(ewkApplicationCacheManager, callback, userData); > > context is never freed? > > > Source/WebKit2/UIProcess/API/efl/ewk_application_cache_manager.h:51 > > + * the Eina_List and its items should be freed after use. Use ewk_security_origin_unref() > > No, we usually don't pass ownership of callback arguments to the client. The client should clone the list (and ref its items) if it wishes to keep them outside the callback. > This is less leak prone and common practice I believe. > > Yes, I'll do. Thank you. >> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_application_cache_manager.cpp:29 > >> +#include "UnitTestUtils/EWK2UnitTestEnvironment.h" > > > > unneeded include. Already included in EWK2UnitTestBase.h. > > ? > > >> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_application_cache_manager.cpp:32 > >> +#include <EWebKit2.h> > > > > ditto. > > So? This file is using Ewk types so it should include EWebKit2.h. Relying on other headers includes is bad practice. > > >> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_application_cache_manager.cpp:33 > >> +#include <Ecore.h> > > > > ditto. > > So? This file is using ecore function so it should include Ecore.h. Relying on other headers includes is bad practice. > > > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_application_cache_manager.cpp:81 > > +static void getApplicationCacheOriginsCallback(Eina_List* origins, Ewk_Error* error, void* userData) > > Note that you're not freeing the origins list here, which proves my point that the current API is leak prone. > Yes, I'll fix. > > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_application_cache_manager.cpp:111 > > + Eina_Strbuf* body = eina_strbuf_new(); > > Should be inside the if case otherwise it leaks if the if condition is false. > I added eina_strbuf_free in front of soup_message_body_complete and remove eina_strbuf_free in if case. > > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_application_cache_manager.cpp:128 > > + } > > There should probably be an else case here which sets status to SOUP_STATUS_NOT_FOUND. > > > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_application_cache_manager.cpp:157 > > + const char* applicationCacheTestUrl = httpServer->getURLForPath("/application_cache.html").data(); > > You keep a pointer to memory owned by a temporary object (CString), this is bad and may crash. > > Get rid of this useless variable and update next line to read: > ewk_view_url_set(view, httpServer->getURLForPath("/application_cache.html").data()); > > > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_application_cache_manager.cpp:158 > > + ewk_view_url_set(view, applicationCacheTestUrl); > > There is loadUrlSync() to do this in 1 line instead of 2. > I'll fix. > >> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_context.cpp:53 > >> + ASSERT_EQ(applicationCacheManager, ewk_context_application_cache_manager_get(context)); > > > > This comparison seems to be meaningless. > > It is not meaningless, it makes sure we return the same pointer every time. This is consistent with our other tests.

Created attachment 177596 [details] Patch

(In reply to comment #8) > Created an attachment (id=177596) [details] > Patch Before making patch, please rebase you local first.

Created attachment 177636 [details] Patch

Comment on attachment 177636 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=177636&action=review > Source/WebKit2/UIProcess/API/efl/ewk_application_cache_manager.cpp:78 > + ewk_object_unref(static_cast<Ewk_Security_Origin*>(item)); I don't understand why you delete Ewk_Security_Origin via ewk_object_unref. AFAIK, ewk_object_unref is meaningful when item is based on EwkObject. > Source/WebKit2/UIProcess/API/efl/ewk_application_cache_manager_private.h:38 > +struct Ewk_Application_Cache_Origins_Async_Get_Context { Why do you use *_Get_* in struct name ? I don't know what does *Get* means here. Is Ewk_Application_Cache_Origins_Async_Context enough ? Beside this struct is for internal usage, right ? If so, I prefer not to use _ in struct name. > Source/WebKit2/UIProcess/API/efl/ewk_context.h:154 > + * @param context context object to query. We don't use . at the end of line in @param field. > Source/WebKit2/UIProcess/API/efl/ewk_context.h:156 > + * @return Ewk_Application_Cache_Manager object instance or @c NULL in case of failure. ditto. > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_application_cache_manager.cpp:78 > +const char* applicationCacheCss = "output { font:2em sans-serif; }"; s/= /= /g > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_application_cache_manager.cpp:138 > + Unneeded line.

Comment on attachment 177636 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=177636&action=review > Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestServer.h:34 > + unsigned getPort() const; strange, is port() correct for coding style? > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_application_cache_manager.cpp:31 > +#include "ewk_security_origin_private.h" private.h is required? > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_application_cache_manager.cpp:33 > +#include <Ecore.h> I am not sure whether we should add EWebKit2.h and Ecore.h In other test cases, it was omitted because EWK2UnitTestBase.h include. > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_application_cache_manager.cpp:62 > +const char* applicationCacheHtml = Should it be globa? If then, please add static. And const char XXX[] = is prefered. > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_application_cache_manager.cpp:126 > + eina_strbuf_append_printf(body, applicationCacheHtml, 0); > + const size_t bodyLength = eina_strbuf_length_get(body); > + soup_message_body_append(message->response_body, SOUP_MEMORY_TAKE, eina_strbuf_string_steal(body), bodyLength); > + } else if (!strcmp(path, applicationCacheManifestFile)) { > + eina_strbuf_append_printf(body, applicationCacheManifest, 0); > + const size_t bodyLength = eina_strbuf_length_get(body); > + soup_message_body_append(message->response_body, SOUP_MEMORY_TAKE, eina_strbuf_string_steal(body), bodyLength); > + } else if (!strcmp(path, applicationCacheCssFile)) { > + eina_strbuf_append_printf(body, applicationCacheCss, 0); > + const size_t bodyLength = eina_strbuf_length_get(body); > + soup_message_body_append(message->response_body, SOUP_MEMORY_TAKE, eina_strbuf_string_steal(body), bodyLength); > + } else > + soup_message_set_status(message, SOUP_STATUS_NOT_FOUND); How about reducing the duplication?

(In reply to comment #11) > (From update of attachment 177636 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=177636&action=review > > > Source/WebKit2/UIProcess/API/efl/ewk_application_cache_manager.cpp:78 > > + ewk_object_unref(static_cast<Ewk_Security_Origin*>(item)); > > I don't understand why you delete Ewk_Security_Origin via ewk_object_unref. AFAIK, ewk_object_unref is meaningful when item is based on EwkObject. Ewk_Security_Origin also based on EwkObject. But I'll change ewk_object_unref(static_cast<Ewk_Object*>(item)); > > > Source/WebKit2/UIProcess/API/efl/ewk_application_cache_manager_private.h:38 > > +struct Ewk_Application_Cache_Origins_Async_Get_Context { > > Why do you use *_Get_* in struct name ? I don't know what does *Get* means here. Is Ewk_Application_Cache_Origins_Async_Context enough ? > > Beside this struct is for internal usage, right ? If so, I prefer not to use _ in struct name. This struct is for get application cache origins Callback. So I added *Get*. And other struct name also "Get_Policy_Async_Data", "Get_Hostnames_Async_Data ". > > > Source/WebKit2/UIProcess/API/efl/ewk_context.h:154 > > + * @param context context object to query. > > We don't use . at the end of line in @param field. > > > Source/WebKit2/UIProcess/API/efl/ewk_context.h:156 > > + * @return Ewk_Application_Cache_Manager object instance or @c NULL in case of failure. > > ditto. > > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_application_cache_manager.cpp:78 > > +const char* applicationCacheCss = "output { font:2em sans-serif; }"; > > s/= /= /g > > > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_application_cache_manager.cpp:138 > > + > > Unneeded line. I'll fix.

(In reply to comment #13) > (In reply to comment #11) > > (From update of attachment 177636 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=177636&action=review > > > > > Source/WebKit2/UIProcess/API/efl/ewk_application_cache_manager.cpp:78 > > > + ewk_object_unref(static_cast<Ewk_Security_Origin*>(item)); > > > > I don't understand why you delete Ewk_Security_Origin via ewk_object_unref. AFAIK, ewk_object_unref is meaningful when item is based on EwkObject. > Ewk_Security_Origin also based on EwkObject. > But I'll change ewk_object_unref(static_cast<Ewk_Object*>(item)); Ah, my source was out of date. Ok, I got it.

(In reply to comment #12) > (From update of attachment 177636 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=177636&action=review > > > Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestServer.h:34 > > + unsigned getPort() const; > > strange, is port() correct for coding style? > Yes, I'll fix. > > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_application_cache_manager.cpp:31 > > +#include "ewk_security_origin_private.h" > > private.h is required? > I'll change ewk_security_origin.h to ewk_security_origin_private.h > > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_application_cache_manager.cpp:33 > > +#include <Ecore.h> > > I am not sure whether we should add EWebKit2.h and Ecore.h > In other test cases, it was omitted because EWK2UnitTestBase.h include. > I'll remove. > > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_application_cache_manager.cpp:62 > > +const char* applicationCacheHtml = > > Should it be globa? > If then, please add static. > > And const char XXX[] = is prefered. > I'll fix. > > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_application_cache_manager.cpp:126 > > + eina_strbuf_append_printf(body, applicationCacheHtml, 0); > > + const size_t bodyLength = eina_strbuf_length_get(body); > > + soup_message_body_append(message->response_body, SOUP_MEMORY_TAKE, eina_strbuf_string_steal(body), bodyLength); > > + } else if (!strcmp(path, applicationCacheManifestFile)) { > > + eina_strbuf_append_printf(body, applicationCacheManifest, 0); > > + const size_t bodyLength = eina_strbuf_length_get(body); > > + soup_message_body_append(message->response_body, SOUP_MEMORY_TAKE, eina_strbuf_string_steal(body), bodyLength); > > + } else if (!strcmp(path, applicationCacheCssFile)) { > > + eina_strbuf_append_printf(body, applicationCacheCss, 0); > > + const size_t bodyLength = eina_strbuf_length_get(body); > > + soup_message_body_append(message->response_body, SOUP_MEMORY_TAKE, eina_strbuf_string_steal(body), bodyLength); > > + } else > > + soup_message_set_status(message, SOUP_STATUS_NOT_FOUND); > > How about reducing the duplication? I'll reduce the duplication.

Created attachment 177678 [details] Patch

Comment on attachment 177678 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=177678&action=review > Source/WebKit2/UIProcess/API/efl/ewk_application_cache_manager.cpp:43 > + : m_applicationCacheManager(applicationCacheManager) assert? > Source/WebKit2/UIProcess/API/efl/ewk_application_cache_manager.cpp:54 > + const size_t length = WKArrayGetSize(origins); const should not be used with local vars, due to coding style. > Source/WebKit2/UIProcess/API/efl/ewk_application_cache_manager.cpp:80 > + delete webApplicationCacheContext; if you used own_ptr for 'webApplicationCacheContext' with adoption, manual delete would be avoided > Source/WebKit2/UIProcess/API/efl/ewk_application_cache_manager_private.h:38 > +struct _Ewk_Application_Cache_Origins_Async_Get_Context { Do we have to use "_" for private object names? > Source/WebKit2/UIProcess/API/efl/ewk_application_cache_manager_private.h:39 > + const Ewk_Application_Cache_Manager* manager; isn't it EwkApplicationCacheManager ? > Source/WebKit2/UIProcess/API/efl/ewk_context_private.h:58 > + inline EwkApplicationCacheManager* applicationCacheManager() const { return m_applicationCacheManager.get(); } inline is unneeded here, as function is defined in header. And again const member returns mutable pointer :(

Comment on attachment 177678 [details] Patch r- based on many comments.

Created attachment 226870 [details] Patch

Created attachment 226872 [details] Patch

Created attachment 226882 [details] Patch

Comment on attachment 226882 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=226882&action=review LGTM except for a wrong description. > Source/WebKit2/UIProcess/API/efl/ewk_application_cache_manager.h:73 > + * @return @c EINA_TRUE on successful request or @c EINA_FALSE on failure This API returns *void*, not boolean.

Committed r165718: <http://trac.webkit.org/changeset/165718>