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

• Component: WebKit API
• Status: RESOLVED FIXED

Currently, there is no way for the URI scheme handler to get the request HTTP method or set the response status code.

However, those functionalities are necessary, because:

* The same functionality already exists in the macOS version of the WebKit implementation (by manipulating `NSHTTPURLResponse`)
* API consistency: similar functions exist for `WebKitURIRequest` but not for `WebKitURISchemeRequest`
* From the web application standpoint, URI scheme requests are just normal XHRs so applications will handle them as such
* There are already feature requests for this: https://bugs.webkit.org/show_bug.cgi?id=231564

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

Created attachment 441554 [details]
Patch

[EWS Watchlist]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See https://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

Created attachment 441555 [details]
Patch

[Michael Catanzaro]

Comment on attachment 441555 [details]
Patch

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

Looks pretty good overall.

> Source/WebKit/UIProcess/API/glib/WebKitURISchemeRequest.cpp:173
> + * Since: 2.35

2.36

(2.35 is a development release leading up to 2.36. It's confusing.)

> Source/WebKit/UIProcess/API/glib/WebKitURISchemeRequest.cpp:175
> +void webkit_uri_scheme_request_set_status_code(WebKitURISchemeRequest* request, int statusCode)

Since this is public API, it use gint. (Yes, they're exactly the same. It's just style.)

> Source/WebKit/UIProcess/API/glib/WebKitURISchemeRequest.cpp:190
> + * Since: 2.35

2.36

> Source/WebKit/UIProcess/API/glib/WebKitURISchemeRequest.cpp:192
> +const char* webkit_uri_scheme_request_get_http_method(WebKitURISchemeRequest* request)

Since this is public API, it should use gchar*.

> Source/WebKit/UIProcess/API/glib/WebKitURISchemeRequest.cpp:196
> +    return request->priv->task->request().httpMethod().utf8().data();

This is a use-after-free: after the function call returns, httpMethod().utf8() is destroyed and the caller is left with a dangling pointer. To avoid this, you'll need to cache the result in a CString object that you add to the priv struct. You can see three other examples of how to do it.

I remember making the exact same mistake in one of my first patches to WebKit....

> Source/WebKit/UIProcess/API/glib/WebKitURISchemeRequest.cpp:-184
> -        response.setHTTPStatusText("OK"_s);

Maybe run this if the status code is 200?

> Source/WebKit/UIProcess/API/gtk/WebKitURISchemeRequest.h:76
> +WEBKIT_API void
> +webkit_uri_scheme_request_set_status_code (WebKitURISchemeRequest* request,
> +                                           int status_code);

The public headers use GNOME style, different from the WebKit style used in the C++ sources:

WEBKIT_API void
webkit_uri_scheme_request_set_status_code (WebKitURISchemeRequest *request,
                                                                                                gint                                                   status_code);

Note the parameter alignment is different, * hangs on the variable name, and int -> gint.

> Source/WebKit/UIProcess/API/gtk/WebKitURISchemeRequest.h:79
> +webkit_uri_scheme_request_get_http_method (WebKitURISchemeRequest* request);

WebKitURISchemeRequest *request

> Source/WebKit/UIProcess/API/wpe/WebKitURISchemeRequest.h:80
> +WEBKIT_API void
> +webkit_uri_scheme_request_set_status_code (WebKitURISchemeRequest* request, 
> +                                           int status_code);
> +
> +WEBKIT_API const gchar *
> +webkit_uri_scheme_request_get_http_method (WebKitURISchemeRequest* request);

Same style nits here.

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitWebContext.cpp:106
> +        URISchemeHandler(const char* reply, int replyLength, const char* mimeType, int statusCode)

Use a default argument:

URISchemeHandler(const char* reply, int replyLength, const char* mimeType, int statusCode = 200)

Then you don't need two separate overloads.

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitWebContext.cpp:137
> +
> +

Only one blank line here.

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitWebContext.cpp:181
> +    void registerURISchemeHandler(const char* scheme, const char* reply, int replyLength, const char* mimeType, int statusCode)

Again, you only need one implementation if you use a default argument:

void registerURISchemeHandler(const char* scheme, const char* reply, int replyLength, const char* mimeType, int statusCode = 200)

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitWebContext.cpp:325
> +    test->registerURISchemeHandler("notfound", kBarHTML, strlen(kBarHTML), "text/html", 404);
> +    test->m_loadEvents.clear();
> +    test->loadURI("notfound:blank");
> +    test->waitUntilLoadFinished();
> +    g_assert_true(test->m_loadEvents.contains(LoadTrackingTest::LoadFailed));

What happens if you pass 200 as the status code? Does the test still pass (load fails with LoadTrackingTest::LoadFailed), or does the test fail? I'm nervous because it looks like the load would fail because "notfound:blank" is not a registered URI scheme, not because of the 404 status code. The test is only useful if it fails without your change, right?

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #4)
> The public headers use GNOME style, different from the WebKit style used in
> the C++ sources:

Trying again to show the correct alignment:

WEBKIT_API void
webkit_uri_scheme_request_set_status_code (WebKitURISchemeRequest *request,
                                           gint                    status_code);

Created attachment 441685 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 441685 [details]
Patch

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

> Source/WebKit/ChangeLog:8
> +        Add new public APIs to set status code and get HTTP method in custom URI scheme handlers.

Why do you need to get the HTTP method? custom uri schemes are not actually HTTP, so only GET is supported.

> Source/WebKit/UIProcess/API/glib/WebKitURISchemeRequest.cpp:89
> +    request->priv->statusCode = 200;

This is weird, the status code doesn't belong to a request, but to a response. So, this should only have a value after the response is received (in this case the first time webkitURISchemeRequestReadCallback is called).

> Source/WebKit/UIProcess/API/glib/WebKitURISchemeRequest.cpp:168
> + * webkit_uri_scheme_request_set_status_code:

Same here, setting a response value in the request looks weird. I think the status code should be set when finalizing the request like other response parameters (stream, content type, content length, etc.). Problem here is that we can't add more parameters to webkit_uri_scheme_request_finish(). We have several options here:

 a) add webkit_uri_scheme_request_finish_with_status() and add status code and status text parameters.
 b) add a more generic webkit_uri_sheme_request_finish_full taking var args like if they were properties.
 c) add WebKitURISchemeResponse + webkit_uri_scheme_request_finish_with_response(). 

Good thing of c) is that we can add more properties to the response in the future if needed without chanigng the request API.

> Source/WebKit/UIProcess/API/glib/WebKitURISchemeRequest.cpp:184
> + * webkit_uri_scheme_request_get_http_method:

Doesn't this always return "GET"?

> Source/WebKit/UIProcess/API/glib/WebKitURISchemeRequest.cpp:224
> +        response.setHTTPStatusCode(priv->statusCode);
> +        if (priv->statusCode >= 0)
> +            response.setHTTPStatusText(soup_status_get_phrase((guint)priv->statusCode));

If we allow to set a status code, we should allow to optionally set a status text too and just use soup_status_get_phrase() as fallback.

> Why do you need to get the HTTP method? custom uri schemes are not actually HTTP, so only GET is supported.

Sorry, I may not understand this correctly. I would like to know what would happen when the web application makes an XHR or fetch() request to the custom scheme URI? Will it return an error when the method specified is not "GET"? (Currently, I am getting CORS errors with any XHR requests)

>  c) add WebKitURISchemeResponse + webkit_uri_scheme_request_finish_with_response(). 

Yeah, I think this is better. However, I am not sure how to approach this? Shouldn't I place a `WebKitURISchemeResponse` type field inside `WebKitURISchemeRequest` as well since it's a callback function? I would imagine it needs to be here to temporarily store the response struct before `webkitURISchemeRequestReadCallback` is called?

[Carlos Garcia Campos]

(In reply to liushuyu011 from comment #8)
> > Why do you need to get the HTTP method? custom uri schemes are not actually HTTP, so only GET is supported.
> 
> Sorry, I may not understand this correctly. I would like to know what would
> happen when the web application makes an XHR or fetch() request to the
> custom scheme URI? Will it return an error when the method specified is not
> "GET"? (Currently, I am getting CORS errors with any XHR requests)

I don't know, that's a good point. It would be a great to have a unit test for this.

> >  c) add WebKitURISchemeResponse + webkit_uri_scheme_request_finish_with_response(). 
> 
> Yeah, I think this is better. However, I am not sure how to approach this?
> Shouldn't I place a `WebKitURISchemeResponse` type field inside
> `WebKitURISchemeRequest` as well since it's a callback function? I would
> imagine it needs to be here to temporarily store the response struct before
> `webkitURISchemeRequestReadCallback` is called?

We need to add a new object WebKitURISchemeResponse with api to set the response properties (stream, content length, content type, status code and text). The user would have to create an instance before calling webkit_uri_request_finish_with_response and pass it as argument.

Created attachment 441978 [details]
Patch

(In reply to liushuyu011 from comment #10)
> Created attachment 441978 [details]
> Patch

This is a WIP patch.

I am not sure if my API design is reasonable, so the tests are not updated to save some work at this stage.

Created attachment 441990 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 441990 [details]
Patch

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

Thanks, this goes in the right direction, but we don't really want to duplicate the code to read the stream.

> Source/WebKit/UIProcess/API/glib/WebKitURISchemeRequest.cpp:262
> +    webkitURISchemeResponseSetTask(response, request->priv->task);
> +    webkitURISchemeResponseReadStart(response);

We don't want to move/duplicate the code to read the response, just use the new WebKitURISchemeResponse to store the response parameters and get them from here to reuse the exiting code. Current webkit_uri_scheme_request_finish should be rewritten to build a response and call this method.

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

We don't need the ; here

> Source/WebKit/UIProcess/API/glib/WebKitURISchemeResponse.cpp:76
> +WebKitURISchemeResponse* webkit_uri_scheme_response_new()

This could receive the stream and length at least, since the stream is mandatory.

> Source/WebKit/UIProcess/API/glib/WebKitURISchemeResponse.cpp:133
> +void webkitURISchemeResponseSetTask(WebKitURISchemeResponse* schemeResponse, RefPtr<WebURLSchemeTask> task)
> +{
> +    g_return_if_fail(WEBKIT_IS_URI_SCHEME_RESPONSE(schemeResponse));
> +    schemeResponse->priv->task = task;
> +}

We don't need the task here.

> Source/WebKit/UIProcess/API/glib/WebKitURISchemeResponse.cpp:144
> +void webkit_uri_scheme_response_set_status_code(WebKitURISchemeResponse* response, gint statusCode)

We could add a single method set_status() like liboup3 does, that receives an optional reason_phrase (or status text) parameter.

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

I think the body should be a constructor parameter and I would add a new setter for the content type

> Source/WebKit/UIProcess/API/gtk/docs/webkit2gtk-4.0-sections.txt:1295
> +WebKitURISchemeResponse

This doesn't belong here

(In reply to Carlos Garcia Campos from comment #13)

Thank you! I will make these changes.

(In reply to Carlos Garcia Campos from comment #13)
> Comment on attachment 441990 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=441990&action=review
> 
> Thanks, this goes in the right direction, but we don't really want to
> duplicate the code to read the stream.
> 
> > Source/WebKit/UIProcess/API/glib/WebKitURISchemeRequest.cpp:262
> > +    webkitURISchemeResponseSetTask(response, request->priv->task);
> > +    webkitURISchemeResponseReadStart(response);
> 
> We don't want to move/duplicate the code to read the response, just use the
> new WebKitURISchemeResponse to store the response parameters and get them
> from here to reuse the existing code. Current
> webkit_uri_scheme_request_finish should be rewritten to build a response and
> call this method.
> 
> > Source/WebKit/UIProcess/API/glib/WebKitURISchemeResponse.cpp:133
> > +void webkitURISchemeResponseSetTask(WebKitURISchemeResponse* schemeResponse, RefPtr<WebURLSchemeTask> task)
> > +{
> > +    g_return_if_fail(WEBKIT_IS_URI_SCHEME_RESPONSE(schemeResponse));
> > +    schemeResponse->priv->task = task;
> > +}
> 
> We don't need the task here.
> 

Sorry, I have a question here:

I can think of two approaches to read the input stream from the `WebKitURISchemeResponse` in the `WebKitURISchemeRequest` context:

1. Add a non-public function to get the `GInputStream` in the `WebKitURISchemeResponse`, then assign it to the `stream` field in `WebKitURISchemeRequest`;
2. Change `webkit_uri_scheme_request_finish_with_response` to accept a `GInputStream` parameter so `WebKitURISchemeResponse` only contains "metadata"

I am not sure which one is better in your opinion?

Thanks

[Carlos Garcia Campos]

(In reply to liushuyu011 from comment #15)
> (In reply to Carlos Garcia Campos from comment #13)
> > Comment on attachment 441990 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=441990&action=review
> > 
> > Thanks, this goes in the right direction, but we don't really want to
> > duplicate the code to read the stream.
> > 
> > > Source/WebKit/UIProcess/API/glib/WebKitURISchemeRequest.cpp:262
> > > +    webkitURISchemeResponseSetTask(response, request->priv->task);
> > > +    webkitURISchemeResponseReadStart(response);
> > 
> > We don't want to move/duplicate the code to read the response, just use the
> > new WebKitURISchemeResponse to store the response parameters and get them
> > from here to reuse the existing code. Current
> > webkit_uri_scheme_request_finish should be rewritten to build a response and
> > call this method.
> > 
> > > Source/WebKit/UIProcess/API/glib/WebKitURISchemeResponse.cpp:133
> > > +void webkitURISchemeResponseSetTask(WebKitURISchemeResponse* schemeResponse, RefPtr<WebURLSchemeTask> task)
> > > +{
> > > +    g_return_if_fail(WEBKIT_IS_URI_SCHEME_RESPONSE(schemeResponse));
> > > +    schemeResponse->priv->task = task;
> > > +}
> > 
> > We don't need the task here.
> > 
> 
> Sorry, I have a question here:
> 
> I can think of two approaches to read the input stream from the
> `WebKitURISchemeResponse` in the `WebKitURISchemeRequest` context:
> 
> 1. Add a non-public function to get the `GInputStream` in the
> `WebKitURISchemeResponse`, then assign it to the `stream` field in
> `WebKitURISchemeRequest`;
> 2. Change `webkit_uri_scheme_request_finish_with_response` to accept a
> `GInputStream` parameter so `WebKitURISchemeResponse` only contains
> "metadata"
> 
> I am not sure which one is better in your opinion?
> 
> Thanks

I'm not sure I understand, just keep the current code to read the stream, but instead of using request->priv->stream get the stream from the response. Move the response members to the response object (stream, streamLength and contentType) and add GRefPtr<WebKitURISchemeResponse> as a member of the request instead. In finish_with_response you just need to set the response (request->priv->response = response) and start reading the stream. In current finish implementation, create a response object with the given parameters and call finish_with_response

Created attachment 442078 [details]
Patch

Created attachment 442080 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 442080 [details]
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.

Created attachment 442282 [details]
Patch

Created attachment 442283 [details]
Patch

Created attachment 442335 [details]
Patch

[Michael Catanzaro]

Carlos, are you happy with this version?

[Carlos Garcia Campos]

Comment on attachment 442335 [details]
Patch

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

> Source/WebKit/ChangeLog:9
> +        * SourcesGTK.txt: Added WebKitURISchemeRequest.cpp
> +        * SourcesWPE.txt: Added WebKitURISchemeRequest.cpp

You mean WebKitURISchemeResponse.cpp

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

You shouldn't need this.

> Source/WebKit/UIProcess/API/glib/WebKitURISchemeRequest.cpp:201
> +        CString contentType = WebKitURISchemeResponseGetContentType(respPriv);

Private getter should receive a WebKitURISchemeResponse as parameter.

> Source/WebKit/UIProcess/API/glib/WebKitURISchemeRequest.cpp:208
> +        if (statusMessage.isNull())
> +            webkit_uri_scheme_response_set_status(priv->response.get(), 200, "OK");
> +        response.setHTTPStatusCode(WebKitURISchemeResponseGetStatusCode(respPriv));
> +        response.setHTTPStatusText(statusMessage.data());

I think you should set the status code and if message is not null set status text text too, but we don't need to call webkit_uri_scheme_response_set_status() in any case.

> Source/WebKit/UIProcess/API/glib/WebKitURISchemeRequestPrivate.h:23
> +#include "WebKitURISchemeResponsePrivate.h"

Include this from WebKitURISchemeRequest.cpp not from here.

> Source/WebKit/UIProcess/API/glib/WebKitURISchemeResponse.cpp:27
> +#include "APIData.h"
> +#include "WebKitPrivate.h"
> +#include "WebURLSchemeTask.h"
> +#include <WebCore/HTTPParsers.h>
> +#include <WebCore/MIMETypeRegistry.h>

I don't think these are needed.

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

Nor this one

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

const CString

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

Ditto

> Source/WebKit/UIProcess/API/glib/WebKitURISchemeResponse.cpp:98
> +void WebKitURISchemeResponseDisposeStream(WebKitURISchemeResponsePrivate* response)
> +{
> +    response->stream = nullptr;
> +}

This is unused.

> Source/WebKit/UIProcess/API/glib/WebKitURISchemeResponse.cpp:110
> +/**
> + * webkit_uri_scheme_response_set_status_body:
> + * @response: a #WebKitURISchemeResponse
> + * @stream: a #GInputStream to read the contents of the request
> + * @stream_length: the length of the stream or -1 if not known
> + * @content_type: (allow-none): the content type of the stream or %NULL if not known
> + *
> + * Sets the status message for the @response
> + *
> + * Since: 2.36
> + */

This documentation isn't correct, this is webkit_uri_scheme_response_new().

> Source/WebKit/UIProcess/API/glib/WebKitURISchemeResponse.cpp:120
> +    response->priv->statusCode = -1;
> +    response->priv->stream = inputStream;
> +    // We use -1 in the API for consistency with soup when the content length is not known, but 0 internally.
> +    response->priv->streamLength = streamLength == -1 ? 0 : streamLength;

I've just realized that this is a problem for bindings because this is a public constructor. The statusCode initialization could be done in the struct declaration and stream and length should be construct only properties.

> Source/WebKit/UIProcess/API/glib/WebKitURISchemeResponse.cpp:127
> + * @content_type: (allow-none): the content type of the stream or %NULL if not known

I'm not sure we should allow none now that we have a specific setter for this, if unknown don't call this function.

> Source/WebKit/UIProcess/API/glib/WebKitURISchemeResponsePrivate.h:26
> +#include "WebKitURISchemeResponse.h"
> +#include "WebKitWebContext.h"
> +#include "WebPageProxy.h"
> +#include "WebURLSchemeTask.h"
> +#include <WebCore/ResourceRequest.h>

You don't need all this headers here.

Created attachment 442650 [details]
Patch

Created attachment 442655 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 442655 [details]
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));

Comment on attachment 442655 [details]
Patch

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

>> Source/WebKit/UIProcess/API/glib/WebKitURISchemeResponse.cpp:91
>> +    gint64 streamLength;
> 
> Move this to the case PROP_STREAM_LENGTH

If I do this when compiling using Clang 12.0.1, the compiler will complain about (GCC does not report this one though):
(Line numbers are from the newer patch)

```
WebKitURISchemeResponse.cpp:83:13: warning: Cannot jump from switch statement to this case label clang(switch_into_protected_scope)
WebKitURISchemeResponse.cpp:78:16: note: Jump bypasses variable initialization
```

I am not sure if this is a false-positive from the compiler?

[Michael Catanzaro]

Comment on attachment 442655 [details]
Patch

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

>>> Source/WebKit/UIProcess/API/glib/WebKitURISchemeResponse.cpp:91
>>> +    gint64 streamLength;
>> 
>> Move this to the case PROP_STREAM_LENGTH
> 
> If I do this when compiling using Clang 12.0.1, the compiler will complain about (GCC does not report this one though):
> (Line numbers are from the newer patch)
> 
> ```
> WebKitURISchemeResponse.cpp:83:13: warning: Cannot jump from switch statement to this case label clang(switch_into_protected_scope)
> WebKitURISchemeResponse.cpp:78:16: note: Jump bypasses variable initialization
> ```
> 
> I am not sure if this is a false-positive from the compiler?

Well that warning message seems clearly wrong, but perhaps you can silence it by adding braces?

case PROP_STREAM_LENGTH: {
    // ...
    break;
}

This will limit the scope of the variable to only the 'case PROP_STREAM_LENGTH', which was the goal behind moving it anyway.

Good job iterating on all the feedback btw. The strict code style requirements keep things more maintainable.

(In reply to Michael Catanzaro from comment #29)
> Comment on attachment 442655 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=442655&action=review
> 
> >>> Source/WebKit/UIProcess/API/glib/WebKitURISchemeResponse.cpp:91
> >>> +    gint64 streamLength;
> >> 
> >> Move this to the case PROP_STREAM_LENGTH
> > 
> > If I do this when compiling using Clang 12.0.1, the compiler will complain about (GCC does not report this one though):
> > (Line numbers are from the newer patch)
> > 
> > ```
> > WebKitURISchemeResponse.cpp:83:13: warning: Cannot jump from switch statement to this case label clang(switch_into_protected_scope)
> > WebKitURISchemeResponse.cpp:78:16: note: Jump bypasses variable initialization
> > ```
> > 
> > I am not sure if this is a false-positive from the compiler?
> 
> Well that warning message seems clearly wrong, but perhaps you can silence
> it by adding braces?
> 
> case PROP_STREAM_LENGTH: {
>     // ...
>     break;
> }
> 
> This will limit the scope of the variable to only the 'case
> PROP_STREAM_LENGTH', which was the goal behind moving it anyway.
> 

Thank you! The solves it. I am not sure why Clang is having issues with switch scoping in this case though.

> Good job iterating on all the feedback btw. The strict code style
> requirements keep things more maintainable.

Thank you! I think iterating the patch is a good way of learning how bigger projects write and maintain their code.

Created attachment 442893 [details]
Patch

(In reply to liushuyu011 from comment #31)
> Created attachment 442893 [details]
> Patch

Is there anything I need to do before this patch could be merged?

[EWS]

Committed r285155 (243791@main): <https://commits.webkit.org/243791@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 442893 [details].

[Radar WebKit Bug Importer]

<rdar://problem/84920723>