[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

--- Comment #7 from Mario Sanchez Prada <mario at webkit.org> ---
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

>> 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”.

Oops! Will fix that

>> 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.

Another "Ooops!", but this one is actually worse. TL;DR: I think you're right, I'll change it

>> 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.

I think the GError should be filled, even if it's with a generic G_IO_ERROR_CANCELLED, in case something went wrong, so I'll keep it.

>> 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.)

Fair enough

>> 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.

Yep

>> 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.

I think I'll add the needed bits to return an GError in case it failed, for the same reasons explained above

>> 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)?

Providing a proper GError sounds good

-- 
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/20171030/4d4e28dd/attachment.html>