Bug 112160 - [GTK] Add webkit_uri_request_get_http_headers to WebKit2 GTK+ API
Summary: [GTK] Add webkit_uri_request_get_http_headers to WebKit2 GTK+ API
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on: 83681 111288
Blocks:
  Show dependency treegraph
 
Reported: 2013-03-12 09:36 PDT by Carlos Garcia Campos
Modified: 2013-04-23 08:07 PDT (History)
12 users (show)

See Also:


Attachments
Patch (20.54 KB, patch)
2013-03-12 09:46 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Update patch addressing review comments (20.54 KB, patch)
2013-03-12 10:23 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Another update (21.48 KB, patch)
2013-03-12 10:32 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Rebased patch (21.13 KB, patch)
2013-04-20 08:28 PDT, Carlos Garcia Campos
kling: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2013-03-12 09:36:22 PDT
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.
Comment 1 Carlos Garcia Campos 2013-03-12 09:46:02 PDT
Created attachment 192756 [details]
Patch
Comment 2 Martin Robinson 2013-03-12 09:54:16 PDT
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.
Comment 3 Carlos Garcia Campos 2013-03-12 10:03:24 PDT
(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.
Comment 4 Carlos Garcia Campos 2013-03-12 10:23:52 PDT
Created attachment 192760 [details]
Update patch addressing review comments
Comment 5 Carlos Garcia Campos 2013-03-12 10:32:57 PDT
Created attachment 192761 [details]
Another update

Sorry I forgot to add the changes of one of the unit tests files.
Comment 6 Carlos Garcia Campos 2013-04-20 08:28:19 PDT
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?
Comment 7 WebKit Commit Bot 2013-04-20 08:30:51 PDT
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
Comment 8 Andreas Kling 2013-04-23 04:09:03 PDT
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.
Comment 9 Carlos Garcia Campos 2013-04-23 05:03:12 PDT
(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.
Comment 10 Carlos Garcia Campos 2013-04-23 08:07:27 PDT
Committed r148966: <http://trac.webkit.org/changeset/148966>