[webkit-reviews] review denied: [Bug 231880] [GTK][WPE] Support setting status code and getting HTTP method in custom URI scheme handlers : [Attachment 442080] Patch

[bugzilla-daemon at webkit.org]

Carlos Garcia Campos <cgarcia at igalia.com> has denied liushuyu011 at gmail.com's
request for review:
Bug 231880: [GTK][WPE] Support setting status code and getting HTTP method in
custom URI scheme handlers
https://bugs.webkit.org/show_bug.cgi?id=231880

Attachment 442080: Patch

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




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