Bug 103243 – [EFL][WK2] Don't use the C API internally in ewk_cookie_manager

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Jinwoo Song]

Used the C++ classes directly instead of the C API wrappers to avoid a lot of toImpl/toAPI casts.

[Jinwoo Song]

Created attachment 175972 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 175972 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=175972&action=review

> Source/WebKit2/UIProcess/API/efl/ewk_cookie_manager_private.h:78
> +    PassRefPtr<WebKit::WebCookieManagerProxy> m_cookieManager;

Why do you use PassRefPtr instead of RefPtr ? If m_cookieManager based on PassRefPtr is assigned by other variables, m_cookieManager will be null. Beside WebKit recommends to use PassRefPtr only for function argument and result types, copying arguments into RefPtr local variables.

http://www.webkit.org/coding/RefPtr.html

[Mikhail Pozdnyakov]

Comment on attachment 175972 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=175972&action=review

> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:137
> +        m_cookieManager = Ewk_Cookie_Manager::create(m_context.get()->cookieManagerProxy());

just m_context->cookieManagerProxy()

> Source/WebKit2/UIProcess/API/efl/ewk_cookie_manager.cpp:43
> +    : m_cookieManager(cookieManager.get())

.get not needed

> Source/WebKit2/UIProcess/API/efl/ewk_cookie_manager.cpp:56
> +        m_cookieManager.get()->stopObservingCookieChanges();

.get() not needed

> Source/WebKit2/UIProcess/API/efl/ewk_cookie_manager.cpp:63
> +        m_cookieManager.get()->stopObservingCookieChanges();

ditto

> Source/WebKit2/UIProcess/API/efl/ewk_cookie_manager.cpp:68
> +        m_cookieManager.get()->startObservingCookieChanges();

ditto

> Source/WebKit2/UIProcess/API/efl/ewk_cookie_manager.cpp:73
> +    m_cookieManager.get()->setHTTPCookieAcceptPolicy(toHTTPCookieAcceptPolicy(policy));

ditto

> Source/WebKit2/UIProcess/API/efl/ewk_cookie_manager.cpp:78
> +    m_cookieManager.get()->deleteCookiesForHostname(hostname);

ditto

> Source/WebKit2/UIProcess/API/efl/ewk_cookie_manager.cpp:83
> +    m_cookieManager.get()->deleteAllCookies();

ditto

> Source/WebKit2/UIProcess/API/efl/ewk_cookie_manager.cpp:91
> +        m_cookieManager.get()->startObservingCookieChanges();

ditto

> Source/WebKit2/UIProcess/API/efl/ewk_cookie_manager.cpp:93
> +        m_cookieManager.get()->stopObservingCookieChanges();

ditto

> Source/WebKit2/UIProcess/API/efl/ewk_cookie_manager.cpp:103
> +    m_cookieManager.get()->getHostnamesWithCookies(ArrayCallback::create(userData, callback));

ditto

> Source/WebKit2/UIProcess/API/efl/ewk_cookie_manager.cpp:108
> +    m_cookieManager.get()->getHTTPCookieAcceptPolicy(HTTPCookieAcceptPolicyCallback::create(userData, callback));

ditto

>> Source/WebKit2/UIProcess/API/efl/ewk_cookie_manager_private.h:78
>> +    PassRefPtr<WebKit::WebCookieManagerProxy> m_cookieManager;
> 
> Why do you use PassRefPtr instead of RefPtr ? If m_cookieManager based on PassRefPtr is assigned by other variables, m_cookieManager will be null. Beside WebKit recommends to use PassRefPtr only for function argument and result types, copying arguments into RefPtr local variables.
> 
> http://www.webkit.org/coding/RefPtr.html

Exactly! you must use RefPtr here!

[Jinwoo Song]

Comment on attachment 175972 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=175972&action=review

>>> Source/WebKit2/UIProcess/API/efl/ewk_cookie_manager_private.h:78
>>> +    PassRefPtr<WebKit::WebCookieManagerProxy> m_cookieManager;
>> 
>> Why do you use PassRefPtr instead of RefPtr ? If m_cookieManager based on PassRefPtr is assigned by other variables, m_cookieManager will be null. Beside WebKit recommends to use PassRefPtr only for function argument and result types, copying arguments into RefPtr local variables.
>> 
>> http://www.webkit.org/coding/RefPtr.html
> 
> Exactly! you must use RefPtr here!

Yes, it's my mistake and I'll fix it. Thanks for review, Gyuyoung and Mikhail!

[Jinwoo Song]

Created attachment 177709 [details]
Patch

[Mikhail Pozdnyakov]

Comment on attachment 177709 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=177709&action=review

looks OK

> Source/WebKit2/UIProcess/API/efl/ewk_cookie_manager.cpp:43
> +    : m_cookieManager(cookieManager)

nit: maybe assertion for m_cookieManager?

[Chris Dumez]

Comment on attachment 177709 [details]
Patch

LGTM (although addressing comment from Mikhail would be a plus).

[Jinwoo Song]

Created attachment 177725 [details]
Added assertion for m_cookieManager

[Gyuyoung Kim]

Comment on attachment 177725 [details]
Added assertion for m_cookieManager

r=me

[WebKit Review Bot]

Comment on attachment 177725 [details]
Added assertion for m_cookieManager

Clearing flags on attachment: 177725

Committed r136787: <http://trac.webkit.org/changeset/136787>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.