[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