Bug 72353 – [SOUP][WK2] Implement the functions to manager cookies in CookieJar for WebKit2

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Jongseok Yang]

r79722 inserted the functions to manange cookies from web process.(getHostnamesWithCookies,deleteCookiesForHostname,deleteAllCookies)
The functions in CookieJarSoup.cpp are not implemented.

[Jongseok Yang]

Created attachment 115099 [details]
patch

[Martin Robinson]

Comment on attachment 115099 [details]
patch

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

Looks fine to me, but I have some style nits.

> Source/WebCore/platform/network/soup/CookieJarSoup.cpp:131
> +    GSList* cookies;
> +    GSList* item;
> +    SoupCookie* soupCookie;
> +    char* domain;
> +

In WebKit we typically declare variables when they are first used and in the scope they are used.

> Source/WebCore/platform/network/soup/CookieJarSoup.cpp:150
> +    GSList* cookies;
> +    GSList* item;
> +    SoupCookie* soupCookie;
> +    char* domain;

Ditto.

> Source/WebCore/platform/network/soup/CookieJarSoup.cpp:168
> +    GSList* cookies;
> +    GSList* item;

Ditto.

[Martin Robinson]

Comment on attachment 115099 [details]
patch

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

> Source/WebCore/platform/network/soup/CookieJarSoup.cpp:158
> +        if (domain && domain == hostname)

Here you should first convert the hostname to a UTF-8 CString and compare the utf8Hostname.data() with g_str_equal. This is an error because it's comparing a system locale string with a UTF-8 string.

[Jongseok Yang]

Created attachment 116885 [details]
Patch

[Gyuyoung Kim]

CC'ing Martin.

[Martin Robinson]

Comment on attachment 116885 [details]
Patch

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

> Source/WebCore/platform/network/soup/CookieJarSoup.cpp:132
> +        char* domain = const_cast<char*>(soup_cookie_get_domain(soupCookie));
> +        if (domain)

You can just do:

if (char* domain = const_cast<char*>(soup_cookie_get_domain(soupCookie)))

> Source/WebCore/platform/network/soup/CookieJarSoup.cpp:133
> +            hostnames.add(domain);

Whoops. Seems I missed this in my first review. This you are implicitly converting from from a UTF-8 c string to a String here, which uses the system locale. For ths kind of thing you need to use String::fromUTF8.

> Source/WebCore/platform/network/soup/CookieJarSoup.cpp:148
> +        if (domain && g_str_equal(domain, hostNameString.data()))

I think this can be simplified to be if (domain == hostNameString) or !g_strcmp0(domain,  hostNameString.data()).

[Jongseok Yang]

Created attachment 116957 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 116957 [details]
Patch

LGTM.

[WebKit Review Bot]

Comment on attachment 116957 [details]
Patch

Clearing flags on attachment: 116957

Committed r101474: <http://trac.webkit.org/changeset/101474>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.