[Webkit-unassigned] [Bug 177932] [GTK] New API to add, retrieve and delete cookies via WebKitCookieManager

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Nov 16 03:55:40 PST 2017


https://bugs.webkit.org/show_bug.cgi?id=177932

Carlos Garcia Campos <cgarcia at igalia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #326469|review?                     |review+, commit-queue-
              Flags|                            |

--- Comment #18 from Carlos Garcia Campos <cgarcia at igalia.com> ---
Comment on attachment 326469
  --> https://bugs.webkit.org/attachment.cgi?id=326469
Patch proposal

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

> Source/WebCore/platform/network/soup/CookieSoup.cpp:43
> +Cookie::Cookie(SoupCookie* cookie)
> +{
> +    name = String::fromUTF8(cookie->name);
> +    value = String::fromUTF8(cookie->value);
> +    domain = String::fromUTF8(cookie->domain);
> +    path = String::fromUTF8(cookie->path);
> +    expires = cookie->expires ? static_cast<double>(soup_date_to_time_t(cookie->expires)) * 1000 : 0;
> +    httpOnly = cookie->http_only;
> +    secure = cookie->secure;
> +    session = !cookie->expires;

Use the member initialization instead

Cookie::Cookie(SoupCookie* cookie)
    : name(String::fromUTF8(cookie->name))
    , value(String::fromUTF8(cookie->value))
....

> Source/WebCore/platform/network/soup/CookieSoup.cpp:60
> +    if (name.isNull() || value.isNull() || domain.isNull() || path.isNull())
> +        return nullptr;

Could we use isNull()?

> Source/WebKit/UIProcess/API/glib/WebKitCookieManager.cpp:246
> + * Asynchronously add a #SoupCookie to the underlying storage, which is not persistent
> + * unless you explicitly call webkit_cookie_manager_set_persistent_storage().

Asynchronously add @cookie to @cookie_manager ? I don't think we need to clarify the persistance here

> Source/WebKit/UIProcess/API/glib/WebKitCookieManager.cpp:250
> + */

Since: 2.20

> Source/WebKit/UIProcess/API/glib/WebKitCookieManager.cpp:284
> + */

Since: 2.20

> Source/WebKit/UIProcess/API/glib/WebKitCookieManager.cpp:296
> + * @uri: the #SoupURI for the domain of the cookies to be retrieved

We don't expose SoupURI in our API, better use char* and create the soupURI internally if needed.

> Source/WebKit/UIProcess/API/glib/WebKitCookieManager.cpp:302
> + * Asynchronously get a list of #SoupCookie from @cookie_manager associated with @uri, which
> + * must be either and HTTP or an HTTPS URL.

and -> an

> Source/WebKit/UIProcess/API/glib/WebKitCookieManager.cpp:306
> + */

Since: 2.20

> Source/WebKit/UIProcess/API/glib/WebKitCookieManager.cpp:327
> +        GList* cookies_list = nullptr;

cookies_list -> cookieList

> Source/WebKit/UIProcess/API/glib/WebKitCookieManager.cpp:346
> + * Returns: (element-type SoupCookie) (transfer full): A #GSList of #SoupCookie instances
> + *    or %NULL in case of error.

If NULL means error, how can we return an empty list?

> Source/WebKit/UIProcess/API/glib/WebKitCookieManager.cpp:347
> + */

Since: 2.20

> Source/WebKit/UIProcess/API/glib/WebKitCookieManager.cpp:361
> + * @name: the name the cookie to be deleted
> + * @domain: the domain of the cookie to be deleted
> + * @path: the path of the cookie to be deleted

It's weird that this doesn't receive a SoupCookie, maybe we can document that name, domain and patch will be used to match the cookie.

> Source/WebKit/UIProcess/API/glib/WebKitCookieManager.cpp:371
> + */

Since: 2.20

> Source/WebKit/UIProcess/API/glib/WebKitCookieManager.cpp:412
> + * Returns: %TRUE if the cookie was added or %FALSE in case of error.

added -> deleted

> Source/WebKit/UIProcess/API/glib/WebKitCookieManager.cpp:413
> + */

Since: 2.20

> Source/WebKit/UIProcess/API/wpe/WebKitCookieManager.h:131
> +WEBKIT_API GList*

GList* -> GList *

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestCookieManager.cpp:122
> +        test->m_cookieAdded = added;

Does this make sense? we are asserting before if there's an error which is the only case where the cookie isn't added no?

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestCookieManager.cpp:148
> +        g_list_free_full(m_cookies, reinterpret_cast<GDestroyNotify>(soup_cookie_free));

You should free the cookies in the destructor too.

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestCookieManager.cpp:149
> +        m_cookies = 0;

nullptr

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestCookieManager.cpp:287
> +    test->setAcceptPolicy(WEBKIT_COOKIE_POLICY_ACCEPT_NO_THIRD_PARTY);

No third party is already the default.

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestCookieManager.cpp:332
> +    g_assert_nonnull(foundCookies);
> +    g_assert_cmpint(g_list_length(foundCookies), ==, 2);

g_list_length returns 0 in case list is null, so we don't need the previous assert that isn't used in previous calls to getCookies() either.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20171116/e3c440a9/attachment-0001.html>


More information about the webkit-unassigned mailing list