[webkit-reviews] review denied: [Bug 203273] [GTK][WPE] Support setting response headers in custom URI scheme handlers : [Attachment 443262] Patch

[bugzilla-daemon at webkit.org]

Michael Catanzaro <mcatanzaro at gnome.org> has denied liushuyu011 at gmail.com's
request for review:
Bug 203273: [GTK][WPE] Support setting response headers in custom URI scheme
handlers
https://bugs.webkit.org/show_bug.cgi?id=203273

Attachment 443262: Patch

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




--- Comment #14 from Michael Catanzaro <mcatanzaro at gnome.org> ---
Comment on attachment 443262
  --> https://bugs.webkit.org/attachment.cgi?id=443262
Patch

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

> Source/WebKit/UIProcess/API/glib/WebKitURISchemeRequest.cpp:202
> +	   auto headers =
soup_message_headers_new(SOUP_MESSAGE_HEADERS_REQUEST);
> +	   request->priv->task->request().updateSoupMessageHeaders(headers);
> +	   request->priv->headers = headers;

You're leaking the SoupMessageHeaders here. You have a ref, then assign it to
the GRefPtr, which itself takes a ref. Now there are two refs, but only one is
owned and the other is lost. When creating a new GObject, you almost always
need to use adoptGRef():

request->priv->headers =
adoptGRef(soup_message_headers_new(SOUP_MESSAGE_HEADERS_REQUEST));
request->priv->task->request().updateSoupMessageHeaders(request->priv->headers.
get());

In general, avoid ever holding ownership in a raw pointer, always store
ownership in a smart pointer -- even for short periods of time -- and use raw
pointers only for unowned access. This makes it much harder to make mistakes.

One more thing: our code style is to write auto* rather than plain auto, to
make more clear that you're working with a raw pointer.

> Source/WebKit/UIProcess/API/glib/WebKitURISchemeRequest.cpp:241
> +	   SoupMessageHeaders* headers =
WebKitURISchemeResponseGetHeaders(resp);
> +	   if (headers)
> +	       response.updateFromSoupMessageHeaders(headers);

if (auto* headers = webKitURISchemeResponseGetHeaders(resp))
    response.updateFromSoupMessageHeaders(headers);

Also note: lowercase 'w'...

> Source/WebKit/UIProcess/API/glib/WebKitURISchemeResponse.cpp:153
> +SoupMessageHeaders*
WebKitURISchemeResponseGetHeaders(WebKitURISchemeResponse* response)

Ooops, we missed this in the previous rounds of review, but these private
functions are supposed to follow WebKit coding style, which uses camelCase like
this. So it should be named webkitURISchemeResponseGetHeaders with an initial
lowercase w. If WebKit were a more normal software project that followed
generally-accepted commit granularity rules, I would ask you to fix these all
in a separate commit, but in WebKit it is actually typical to land small
mildly-related fixes all in one large patch. So you can go ahead and fix them
all as part of this patch.

> Source/WebKit/UIProcess/API/glib/WebKitURISchemeResponse.cpp:203
> +void webkit_uri_scheme_response_set_http_headers(WebKitURISchemeResponse*
response, SoupMessageHeaders* headers)

I'm not sure about the name of this function. Problem is the name "set" implies
the previous value will be replaced with the new value. But you're not doing a
replacement, you're just appending. I think I would call this
webkit_uri_scheme_response_append_http_headers().

I would also add a warning that it is an error to append a header that already
exists if it is not a list-valued header, same as when using
soup_message_headers_append() directly.

Do we actually need this at all? Is allowing access to the SoupMessageHeaders
not enough for applications to do what they want with it?

> Source/WebKit/UIProcess/API/glib/WebKitURISchemeResponse.cpp:209
> +	   auto headers =
soup_message_headers_new(SOUP_MESSAGE_HEADERS_RESPONSE);
> +	   response->priv->headers = headers;

Leaking here too. Fix it with adoptGRef:

response->priv->headers =
adoptGRef(soup_message_headers_new(SOUP_MESSAGE_HEADERS_RESPONSE));

> Source/WebKit/UIProcess/API/glib/WebKitURISchemeResponse.cpp:212
> +    // iterate over headers and append them to our response's headers'

"headers" is not possessive, so remove the stray apostrophe. Also, we like to
write comments as sentences, so we'd start with a capital I and end with a
period. That said, we have one more rule: we generally use comments to explain
*why* the code is doing things, not to explain *what* it is that the code is
doing (unless the code is necessarily unusually complex or confusing, which is
not the case here). So actually we would not use a comment here at all. You can
just remove it.

> Source/WebKit/UIProcess/API/glib/WebKitURISchemeResponse.cpp:213
> +    soup_message_headers_foreach(headers, [](const char *name, const char
*value, gpointer user_data) {

Remember the * hangs on the type:

const char* name, const char* value

And remember everything has to use camelCase with the exception of the public
API functions that use underscores, so user_data -> userData.

> Source/WebKit/UIProcess/API/gtk/WebKitURISchemeRequest.h:28
> +#include <libsoup/soup-message-headers.h>

I think a forward declaration would be enough:

typedef struct _SoupMessageHeaders SoupMessageHeaders;

Does that work?

If not, add it alphabetically to the list of includes rather than adding it at
the bottom.

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitWebContext.cpp:133
> +	       auto headers =
webkit_uri_scheme_request_get_http_headers(request);

auto* headers =

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitWebContext.cpp:348
> +    static const char* headersHTML =
"<html><body><script>fetch('headers:data', {headers: {'X-Test':
'A'}})</script></body></html>";
> +    test->registerURISchemeHandler("headers", nullptr, 0,
"application/json", 204);
> +    test->m_loadEvents.clear();
> +    test->loadHtml(headersHTML, "headers:form");
> +    test->waitUntilLoadFinished();
> +   
g_assert_false(test->m_loadEvents.contains(LoadTrackingTest::ProvisionalLoadFai
led));
> +   
g_assert_false(test->m_loadEvents.contains(LoadTrackingTest::LoadFailed));

OK, this is a good start at a test.

I see that we cannot test what happens when you add a header that already
exists, because that behavior is undefined. And we cannot test removing or
replacing headers, because your API does not allow that (should it?). We could
test list-valued headers, though, to ensure that you're able to properly
construct a list using multiple headers. (See documentation of
soup_message_headers_get_list().)

The other little thing we need to test is...
webkit_uri_scheme_response_set_http_headers()! Did you forget about that? Oops.
:)