Bug 91053

Created attachment 151978 [details] Patch

Comment on attachment 151978 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=151978&action=review > Source/WebKit2/ChangeLog:12 > + The Ewk_Cookie_Manager instance can be retrieve Nit: s/retrieve/retrieved/ > Source/WebKit2/UIProcess/API/efl/ewk_cookie_manager.cpp:78 > + g_return_if_fail(filename); Isn't it better not to mix glib calls here?

(In reply to comment #2) > (From update of attachment 151978 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=151978&action=review > > > Source/WebKit2/ChangeLog:12 > > + The Ewk_Cookie_Manager instance can be retrieve > > Nit: s/retrieve/retrieved/ > > > Source/WebKit2/UIProcess/API/efl/ewk_cookie_manager.cpp:78 > > + g_return_if_fail(filename); > > Isn't it better not to mix glib calls here? Good catches, thanks. I'll fix those right now :)

Created attachment 152021 [details] Patch

LGTM

I don't know much about cookies, but maybe Jocelyn would be kind to have a quick look.

Comment on attachment 152021 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152021&action=review Cookie-wise that looks fine to me, it basically just wraps the C API. > Source/WebKit2/UIProcess/API/efl/ewk_cookie_manager.h:136 > +EAPI void ewk_cookie_manager_async_domains_with_cookies_get(Ewk_Cookie_Manager *manager, Ewk_Cookie_Manager_Async_Domains_Get_Cb callback, void *data); Technically those are host names and not always domains since they can be specified as IP addresses. It's just a detail though.

Created attachment 152218 [details] Patch Take Jocelyn's feedback into consideration, thanks for checking.

Created attachment 152245 [details] Patch Make new ewk header installable.

Created attachment 152298 [details] Patch Rebase on master.

Comment on attachment 152298 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152298&action=review > Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:46 > +Ewk_Cookie_Manager* ewk_context_cookie_manager_get(Ewk_Context* ewkContext) Use *const* for _get(). > Source/WebKit2/UIProcess/API/efl/ewk_context.h:54 > + * @return Ewk_Cookie_Manager object instance. I think you need to mention what is returned on failure. > Source/WebKit2/UIProcess/API/efl/ewk_cookie_manager.cpp:83 > +void ewk_cookie_manager_accept_policy_set(Ewk_Cookie_Manager *manager, Ewk_Cookie_Accept_Policy policy) Nit : Move '*' to type of variable side. > Source/WebKit2/UIProcess/API/efl/ewk_cookie_manager.cpp:107 > +void ewk_cookie_manager_async_accept_policy_get(Ewk_Cookie_Manager* manager, Ewk_Cookie_Manager_Async_Policy_Get_Cb callback, void* data) Use *const* keyword for _get(). > Source/WebKit2/UIProcess/API/efl/ewk_cookie_manager.cpp:109 > + EWK_COOKIE_MANAGER_WK_GET_OR_RETURN(manager, wkManager); Do you really need to return wkManager in *void* return type ? There is no description related to return values in API description.

Comment on attachment 152298 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152298&action=review I'll fix the nits and reupload, thanks Gyuyoung for catching them. >> Source/WebKit2/UIProcess/API/efl/ewk_cookie_manager.cpp:109 >> + EWK_COOKIE_MANAGER_WK_GET_OR_RETURN(manager, wkManager); > > Do you really need to return wkManager in *void* return type ? There is no description related to return values in API description. This macro does not return the wkManager, there would be a third argument if there was a return value. wkManager is the name of the WK typed variable that is retrieved from manager.

Created attachment 152423 [details] Patch Fix nits spotted by Gyuyoung.

Kenneth, can you please land this patch?

Are you writing unit tests in a separated bug?

Comment on attachment 152423 [details] Patch Clearing flags on attachment: 152423 Committed r122969: <http://trac.webkit.org/changeset/122969>

All reviewed patches have been landed. Closing bug.

(In reply to comment #15) > Are you writing unit tests in a separated bug? Yes, I'm making good progress on it. I'll file the bug soon.