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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Oct 19 03:33:59 PDT 2021


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 441685: Patch

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




--- Comment #7 from Carlos Garcia Campos <cgarcia at igalia.com> ---
Comment on attachment 441685
  --> https://bugs.webkit.org/attachment.cgi?id=441685
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.


More information about the webkit-reviews mailing list