[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 #3 from Christophe Dumez <christophe.dumez at intel.com>  2012-11-21 05:49:37 PST ---
(From update of attachment 175316)
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.

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