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

[bugzilla-daemon at webkit.org]

Carlos Garcia Campos <cgarcia at igalia.com> has granted 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 442655: Patch

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




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

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

Looks good to me, but there are still a few minor issues to fix before landing.

> Source/WebKit/UIProcess/API/glib/WebKitURISchemeRequest.cpp:205
> +	   CString statusMessage =
WebKitURISchemeResponseGetStatusMessage(resp);

const CString& or const auto&

> Source/WebKit/UIProcess/API/glib/WebKitURISchemeResponse.cpp:25
> +#include "APIData.h"
> +#include "WebKitPrivate.h"
> +#include "WebURLSchemeTask.h"

Are these needed?

> Source/WebKit/UIProcess/API/glib/WebKitURISchemeResponse.cpp:29
> +#include <wtf/text/CString.h>

This is already included in WebKitURISchemeResponsePrivate.h

> Source/WebKit/UIProcess/API/glib/WebKitURISchemeResponse.cpp:65
> +    int statusCode = -1;

int statusCode { -1 };

> Source/WebKit/UIProcess/API/glib/WebKitURISchemeResponse.cpp:86
> +static void webkitURISchemeResponseGetProperty(GObject* object, guint
propId, GValue* value, GParamSpec* paramSpec)
> +{
> +    WebKitURISchemeResponse* response = WEBKIT_URI_SCHEME_RESPONSE(object);
> +
> +    switch (propId) {
> +    case PROP_STREAM:
> +	   g_value_set_object(value, response->priv->stream.get());
> +	   break;
> +    case PROP_STREAM_LENGTH:
> +	   g_value_set_int64(value, response->priv->streamLength);
> +	   break;
> +    default:
> +	   G_OBJECT_WARN_INVALID_PROPERTY_ID(object, propId, paramSpec);
> +    }
> +}

We don't have public getters, so maybe it's better to make the properties as
writable + construct_only and don't implement get_property.

> Source/WebKit/UIProcess/API/glib/WebKitURISchemeResponse.cpp:91
> +    gint64 streamLength;

Move this to the case PROP_STREAM_LENGTH

> Source/WebKit/UIProcess/API/glib/WebKitURISchemeResponse.cpp:98
> +	   streamLength = g_value_get_int64(value);

gint64 streamLength = g_value_get_int64(value);

> Source/WebKit/UIProcess/API/glib/WebKitURISchemeResponse.cpp:117
> +	*/

Since: 2.36

> Source/WebKit/UIProcess/API/glib/WebKitURISchemeResponse.cpp:130
> +	*/

Since: 2.36

> Source/WebKit/UIProcess/API/glib/WebKitURISchemeResponse.cpp:153
> +const CString WebKitURISchemeResponseGetStatusMessage(const
WebKitURISchemeResponse* response)

const CString&

> Source/WebKit/UIProcess/API/glib/WebKitURISchemeResponse.cpp:158
> +const CString WebKitURISchemeResponseGetContentType(const
WebKitURISchemeResponse* response)

Ditto.

> Source/WebKit/UIProcess/API/glib/WebKitURISchemeResponse.cpp:185
> +    WebKitURISchemeResponse* response =
WEBKIT_URI_SCHEME_RESPONSE(g_object_new(WEBKIT_TYPE_URI_SCHEME_RESPONSE,
"stream", inputStream, "stream-length", streamLength, nullptr));
> +    return response;

just return
WEBKIT_URI_SCHEME_RESPONSE(g_object_new(WEBKIT_TYPE_URI_SCHEME_RESPONSE,
"stream", inputStream, "stream-length", streamLength, nullptr));