[Webkit-unassigned] [Bug 231880] [GTK][WPE] Support setting status code and getting HTTP method in custom URI scheme handlers

[bugzilla-daemon at webkit.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #442080|review?                     |review-
              Flags|                            |

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

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

Looks much better but still needs a bit more work.

> Source/WebKit/UIProcess/API/glib/WebKitURISchemeRequest.cpp:194
> +    WebKitURISchemeResponsePrivate* respPriv = priv->response->priv;

Better add getters to WebKitURISchemeResponsePrivate.h and keep the private struct in the cpp file.

> Source/WebKit/UIProcess/API/glib/WebKitURISchemeRequest.cpp:207
> +            response.setHTTPStatusText(soup_status_get_phrase((guint)respPriv->statusCode));

Don't use C style casts, use C++ static_cast instead.

> Source/WebKit/UIProcess/API/glib/WebKitURISchemeRequest.cpp:239
> +    request->priv->response = webkit_uri_scheme_response_new(inputStream, streamLength, contentType);

You need to use adoptGRef to not leak the response. Since the contentType is optional, I prefer to add a specific setter for it instead of pass it to the constructor.

> Source/WebKit/UIProcess/API/glib/WebKitURISchemeRequest.cpp:242
>      g_input_stream_read_async(inputStream, request->priv->readBuffer, gReadBufferSize, RunLoopSourcePriority::AsyncIONetwork, request->priv->cancellable.get(),
>          reinterpret_cast<GAsyncReadyCallback>(webkitURISchemeRequestReadCallback), g_object_ref(request));

Instead fo setting the response here and calling g_input_stream_read() use a local variable and call webkit_uri_scheme_request_finish_with_response().

auto response = adoptGRef(webkit_uri_scheme_response_new(inputStream, streamLength));
if (contentType)
    webkit_uri_scheme_response_set_content_type(response.get(), contentType);
webkit_uri_scheme_request_finish_with_response(request, response.get());

> Source/WebKit/UIProcess/API/glib/WebKitURISchemeRequest.cpp:259
> +    request->priv->response = response;

You need to create the cancellable here.

> Source/WebKit/UIProcess/API/glib/WebKitURISchemeRequest.cpp:280
> +    priv->response->priv->stream = nullptr;

priv->response = nullptr

> Source/WebKit/UIProcess/API/glib/WebKitURISchemeResponse.cpp:31
> +#include "APIData.h"
> +#include "WebKitPrivate.h"
> +#include "WebURLSchemeTask.h"
> +#include <WebCore/HTTPParsers.h>
> +#include <WebCore/MIMETypeRegistry.h>
> +#include <wtf/glib/GRefPtr.h>
> +#include <wtf/glib/RunLoopSourcePriority.h>
> +#include <wtf/glib/WTFGType.h>
> +#include <wtf/text/CString.h>

Too many headers included here, I'm sure you don't need all of them.

> Source/WebKit/UIProcess/API/glib/WebKitURISchemeResponse.cpp:50
> + * webkit_uri_request_finish_with_response with it to return the response.

webkit_uri_request_finish_with_response()

> Source/WebKit/UIProcess/API/glib/WebKitURISchemeResponse.cpp:54
> +WEBKIT_DEFINE_TYPE(WebKitURISchemeResponse, webkit_uri_scheme_response, G_TYPE_OBJECT);

The ; isn't needed here.

> Source/WebKit/UIProcess/API/glib/WebKitURISchemeResponse.cpp:77
> +    response->priv->statusCode = 200;

Why do we want to initialize this to 200? Since there's a setter for it, I would init this to -1 indicating, no status has been set. To keep the compatibility I would add webkit_uri_scheme_response_set_status(response, 200, "OK") to webkit_uri_scheme_request_finish().

> Source/WebKit/UIProcess/API/glib/WebKitURISchemeResponse.cpp:81
> +    response->priv->bytesRead = 0;

I would leave this in the request which is the one actually reading.

> Source/WebKit/UIProcess/API/glib/WebKitURISchemeResponse.cpp:97
> +void webkit_uri_scheme_response_set_status(WebKitURISchemeResponse* response, gint statusCode, const gchar* statusMessage)

I think statusCode should be unsigned

> Source/WebKit/UIProcess/API/glib/WebKitURISchemeResponse.cpp:102
> +    response->priv->statusMessage = statusMessage;

Maybe we can set the fallback status message here, so that the caller only need to check whether it's null

> Source/WebKit/UIProcess/API/glib/WebKitURISchemeResponsePrivate.h:31
> +    uint64_t bytesRead;

This is already in the request.

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitWebContext.cpp:109
> +        int statusCode;

Where is this used? I don't see where you are using the new api in the test.

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitWebContext.cpp:127
> +        const char* method = webkit_uri_scheme_request_get_http_method(request);
> +        g_assert_nonnull(method);
> +        g_assert_cmpstr(method, ==, "GET");

I still would like to see a test where method doesn't return GET. Maybe we can split the patch and leave the method for a different bug, I'm still not sure it's actually useful.

-- 
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/20211022/638d67c3/attachment.htm>