Bug 112160 – [GTK] Add webkit_uri_request_get_http_headers to WebKit2 GTK+ API

• Component: WebKit2
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

It allows to load requests with custom headers, or to upsdate the headers of a request before being sent to the server in the WebKitWebPage::send-request callback.

[Carlos Garcia Campos]

Created attachment 192756 [details]
Patch

[Martin Robinson]

Comment on attachment 192756 [details]
Patch

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

Great stuff! Now we just need the owner stamp.

> Source/WebKit2/UIProcess/API/gtk/WebKitURIRequest.cpp:157
> + *    or %NULL if @request is not an HTTP request.

Maybe it would be slightly more accurate to say "if @request is not a #WebKitURIRequest." ?

> Source/WebKit2/UIProcess/API/gtk/WebKitURIRequest.cpp:169
> +    request->priv->httpHeaders.set(soup_message_headers_new (SOUP_MESSAGE_HEADERS_REQUEST));

A space snuck in after soup_message_headers_new.

> Source/WebKit2/UIProcess/API/gtk/tests/TestLoaderClient.cpp:351
> +    // Load a request with no do not track header.

Really tiny nitpick: Do Not Track is a proper noun, which isn't a big deal, but it makes this comment easier to read if you capitalize it.

[Carlos Garcia Campos]

(In reply to comment #2)
> (From update of attachment 192756 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=192756&action=review
> 
> Great stuff! Now we just need the owner stamp.

Thanks! Adding owners to CC.

> > Source/WebKit2/UIProcess/API/gtk/WebKitURIRequest.cpp:157
> > + *    or %NULL if @request is not an HTTP request.
> 
> Maybe it would be slightly more accurate to say "if @request is not a #WebKitURIRequest." ?

Nope, we can create a WebKitURIRequest for a non HTTP URI, there's a test case in the unit tests that checks this returns NULL for a file:// URI request.

> > Source/WebKit2/UIProcess/API/gtk/WebKitURIRequest.cpp:169
> > +    request->priv->httpHeaders.set(soup_message_headers_new (SOUP_MESSAGE_HEADERS_REQUEST));
> 
> A space snuck in after soup_message_headers_new.

I wonder why the style checker didn't catch this one.

> > Source/WebKit2/UIProcess/API/gtk/tests/TestLoaderClient.cpp:351
> > +    // Load a request with no do not track header.
> 
> Really tiny nitpick: Do Not Track is a proper noun, which isn't a big deal, but it makes this comment easier to read if you capitalize it.

Sure, I'll upload a new patch.

[Carlos Garcia Campos]

Created attachment 192760 [details]
Update patch addressing review comments

[Carlos Garcia Campos]

Created attachment 192761 [details]
Another update

Sorry I forgot to add the changes of one of the unit tests files.

[Carlos Garcia Campos]

Created attachment 198944 [details]
Rebased patch

Patch rebased to current git master and removed the soup hack for SoupMessageHeaders include since we already bumped libsoup requirements. 

Other patches blocking this one landed already, so this can land now. Could a WebKit2 owner review it, please?

[WebKit Commit Bot]

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

[Andreas Kling]

Comment on attachment 198944 [details]
Rebased patch

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

> Source/WebCore/platform/network/soup/ResourceRequestSoup.cpp:60
> +    g_object_set(soupMessage, SOUP_MESSAGE_METHOD, httpMethod().utf8().data(), NULL);

Does g_object_set() make a copy of the string passed here? (httpMethod().utf8().data())
Otherwise this looks wrong.

[Carlos Garcia Campos]

(In reply to comment #8)
> (From update of attachment 198944 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=198944&action=review
> 
> > Source/WebCore/platform/network/soup/ResourceRequestSoup.cpp:60
> > +    g_object_set(soupMessage, SOUP_MESSAGE_METHOD, httpMethod().utf8().data(), NULL);
> 
> Does g_object_set() make a copy of the string passed here? (httpMethod().utf8().data())
> Otherwise this looks wrong.

Yes, it copies the string.

[Carlos Garcia Campos]

Committed r148966: <http://trac.webkit.org/changeset/148966>