[Webkit-unassigned] [Bug 102853] [EFL][WK2] Add ewk_application_cache_manager APIs

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=102853





--- Comment #7 from Jiyeon Kim <jiyeon0402.kim at samsung.com>  2012-12-04 16:07:59 PST ---
(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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.