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

[bugzilla-daemon at webkit.org]

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

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




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

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

> Source/WebKit/UIProcess/API/glib/WebKitURISchemeRequest.cpp:73
> +    GUniquePtr<SoupMessageHeaders> headers;

This will cause the same issue, you have to include <WebCore/GUniquePtrSoup.h>
in this file. See below.

> Source/WebKit/UIProcess/API/glib/WebKitURISchemeRequest.cpp:202
> +	   // Unfortunately, SoupMessageHeaders is not a GObject. So we need to
use GUniquePtr instead of GRefPtr.

I don't see why it's unfortunate that SoupMessageHeaders is not a GObject, so
better remove this comment. We use unique_ptr for compatibility with soup2,
once soup2 code is removed we can use GRefPtr instead, even when it's not a
GObject. 

We can avoid the local variable:
request->priv->headers.reset(soup_message_headers_new(SOUP_MESSAGE_HEADERS_REQU
EST));
request->priv->task->request().updateSoupMessageHeaders(request->priv->headers.
get());

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

This should be #include <WebCore/GUniquePtrSoup.h>

> Source/WebKit/UIProcess/API/glib/WebKitURISchemeResponse.cpp:65
> +namespace WTF {
> +#if USE(SOUP2)
> +WTF_DEFINE_GPTR_DELETER(SoupMessageHeaders, soup_message_headers_free);
> +#else
> +WTF_DEFINE_GPTR_DELETER(SoupMessageHeaders, soup_message_headers_unref);
> +#endif
> +}

This is already defined in GUniquePtrSoup.h

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitWebContext.cpp:25
> +#include <libsoup/soup-message-headers.h>

I'm surprised this works, you should only include the soup main header.