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

[bugzilla-daemon at webkit.org]

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

Adrian Perez <aperez at igalia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |aperez at igalia.com

--- Comment #5 from Adrian Perez <aperez at igalia.com> ---
Comment on attachment 323310
  --> https://bugs.webkit.org/attachment.cgi?id=323310
Patch proposal

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

Informal review: r- 

It would be r+ if there wasn't doubts about error handling (is it missing?
is it uneeded?). Please check the inline comments below regarding this.

Otherwise, the added API looks good to me, and you did a good job adding
the tests for the API. Thanks a lot for working on this patch!

> Source/WebKit/UIProcess/API/glib/WebKitCookieManager.cpp:246
> + * request specified by @uri, which must be either and HTTP or an HTTP URL. In case the

Probably you meant “either an HTTP or an HTTPS URL”.

> Source/WebKit/UIProcess/API/glib/WebKitCookieManager.cpp:280
> +        g_task_return_boolean(task.get(), error == CallbackBase::Error::None);

What if the error is other than CallbackBase::Error::None? This is never
returning an error in that case e.g. using g_task_return_new_error(). I
would expect this to be something like:

  if (error == CallbackBase::Error::None) {
      g_task_return_boolen(task.get(), TRUE);
  } else {
      g_task_return_error(task.get(), toGError(error));
  }

If the current behavior is intended, you can remove the GError argument to
webkit_cookie_manager_add_cookie_finish() — read move below.

> Source/WebKit/UIProcess/API/glib/WebKitCookieManager.cpp:294
> +gboolean webkit_cookie_manager_add_cookie_finish(WebKitCookieManager *manager, GAsyncResult *result, GError **error)

The GError argument may not be needed if the WebCore/WebKit error
will not be converted to a GError, as argumented above. Either
remove it here, or add the conversion to GError above as suggested.

> Source/WebKit/UIProcess/API/glib/WebKitCookieManager.cpp:305
> + * @uri: the #SoupURI for which the list of cookies will be retrieved from

I think this should have the same parameter description as the
webkit_cookie_manager_add_cookie() function above, for consistency:

  @uri: the #SoupURI with the URI of the originating request

(Also, as a bonus this wording parses more easily in my head.)

> Source/WebKit/UIProcess/API/glib/WebKitCookieManager.cpp:311
> + * must be either and HTTP or an HTTP URL.

Ditto, probably you wanted “either an HTTP or an HTTPS URL” here as well.

> Source/WebKit/UIProcess/API/glib/WebKitCookieManager.cpp:328
> +    processPools[0]->supplement<WebCookieManagerProxy>()->getCookies(sessionID, WebCore::URL(uri), [task = WTFMove(task)](const Vector<WebCore::Cookie>& cookies, CallbackBase::Error error) {

The “error” argument is never checked. If it's guaranteed to always
indicate a successful operation, then please add an assertion:

  ASSERT_EQ(error, CallbackBase::Error::None);

otherwise, please add the needed bits to convert the error codes to
GError values and to report them to the code using this API.

> Source/WebKit/UIProcess/API/glib/WebKitCookieManager.cpp:395
> +        g_task_return_boolean(task.get(), error == CallbackBase::Error::None);

Same comment as for webkit_cookie_manager_add_cookie(): How about converting
errors to GError, or removing the GError parameter to the _finish() function
(whichever fits in this case)?

-- 
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/20171011/3638e6aa/attachment.html>