[webkit-reviews] review denied: [Bug 231880] [GTK][WPE] Support setting status code and getting HTTP method in custom URI scheme handlers : [Attachment 442335] 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 442335: Patch

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




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