Bug 91053

Summary: [EFL][WK2] Add Ewk class for cookie manager
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: WebKit2Assignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: gns, gyuyoung.kim, js45.yang, jturcotte, kenneth, rakuco, ryuan.choi, sw0524.lee, tmpsantos, tonikitoo, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 61838    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Chris Dumez 2012-07-12 00:01:50 PDT
We need a cookie manager Ewk class so that the client can manage cookies.
Comment 1 Chris Dumez 2012-07-12 09:52:09 PDT
Created attachment 151978 [details]
Patch
Comment 2 Raphael Kubo da Costa (:rakuco) 2012-07-12 12:15:40 PDT
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?
Comment 3 Chris Dumez 2012-07-12 12:25:28 PDT
(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 :)
Comment 4 Chris Dumez 2012-07-12 12:26:26 PDT
Created attachment 152021 [details]
Patch
Comment 5 Raphael Kubo da Costa (:rakuco) 2012-07-12 12:45:12 PDT
LGTM
Comment 6 Kenneth Rohde Christiansen 2012-07-12 20:18:38 PDT
I don't know much about cookies, but maybe Jocelyn would be kind to have a quick look.
Comment 7 Jocelyn Turcotte 2012-07-13 04:18:46 PDT
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.
Comment 8 Chris Dumez 2012-07-13 04:39:14 PDT
Created attachment 152218 [details]
Patch

Take Jocelyn's feedback into consideration, thanks for checking.
Comment 9 Chris Dumez 2012-07-13 06:36:56 PDT
Created attachment 152245 [details]
Patch

Make new ewk header installable.
Comment 10 Chris Dumez 2012-07-13 10:43:53 PDT
Created attachment 152298 [details]
Patch

Rebase on master.
Comment 11 Gyuyoung Kim 2012-07-14 05:41:37 PDT
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 12 Chris Dumez 2012-07-14 07:21:14 PDT
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.
Comment 13 Chris Dumez 2012-07-14 08:01:54 PDT
Created attachment 152423 [details]
Patch

Fix nits spotted by Gyuyoung.
Comment 14 Chris Dumez 2012-07-18 05:49:58 PDT
Kenneth, can you please land this patch?
Comment 15 Thiago Marcos P. Santos 2012-07-18 07:35:02 PDT
Are you writing unit tests in a separated bug?
Comment 16 WebKit Review Bot 2012-07-18 07:41:33 PDT
Comment on attachment 152423 [details]
Patch

Clearing flags on attachment: 152423

Committed r122969: <http://trac.webkit.org/changeset/122969>
Comment 17 WebKit Review Bot 2012-07-18 07:41:40 PDT
All reviewed patches have been landed.  Closing bug.
Comment 18 Chris Dumez 2012-07-18 09:57:11 PDT
(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.