Instead of wrapping a SoupMessage these classes should wrap the corresponding WebCore classes. This prevents unecessary conversion of the WebCore classes to a SoupMessage and makes the code simpler. If WebKitURIRequest needs a SoupMessage at a later time, it can create one lazily. Patch will follow.
Created attachment 119364 [details] Patch
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
Created attachment 119366 [details] How this affects the patch on bug 72952 Carlos asked me to post a patch showing how this affects his patch at https://bugs.webkit.org/show_bug.cgi?id=72952. I've done that.
Comment on attachment 119364 [details] Patch Attachment 119364 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/10905139
Comment on attachment 119364 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=119364&action=review > Source/WebKit2/UIProcess/API/gtk/WebKitURIRequest.cpp:105 > + WebKitURIRequest* request = WEBKIT_URI_REQUEST(g_object_new(WEBKIT_TYPE_URI_REQUEST, NULL)); > + request->priv->resourceRequest.setURL(KURL(KURL(), uri)); > + return request; I prefer to keep the property as construct only and move the .SetURL to the constructed method. That way this will work if g_object_new is called directly instead of webkit_uri_request_new(), which is what the bindings do (I think) > Source/WebKit2/UIProcess/API/gtk/WebKitURIRequest.cpp:126 > + request->priv->uri = request->priv->resourceRequest.url().string().utf8(); > return request->priv->uri.data(); I wonder if we really need priv->uri. When the request is created with an uri we set the URL of the ResourseRequest, so we could just return request->priv->resourceRequest.url().string().utf8() always here, no? > Source/WebKit2/UIProcess/API/gtk/WebKitURIResponse.cpp:136 > + response->priv->uri = response->priv->resourceResponse.url().string().utf8(); > return response->priv->uri.data(); Ditto.
(In reply to comment #4) > (From update of attachment 119364 [details]) > Attachment 119364 [details] did not pass gtk-ews (gtk): > Output: http://queues.webkit.org/results/10905139 Looks like webkit-patch skipped the new file again.
(In reply to comment #5) > I prefer to keep the property as construct only and move the .SetURL to the constructed method. That way this will work if g_object_new is called directly instead of webkit_uri_request_new(), which is what the bindings do (I think) Sure. That seems like a reasonable change. I forgot that we might support constructing and using WebKitURIRequests. > > > Source/WebKit2/UIProcess/API/gtk/WebKitURIRequest.cpp:126 > > + request->priv->uri = request->priv->resourceRequest.url().string().utf8(); > > return request->priv->uri.data(); > > I wonder if we really need priv->uri. When the request is created with an uri we set the URL of the ResourseRequest, so we could just return request->priv->resourceRequest.url().string().utf8() always here, no? The problem is that utf8() is a temporary here so returning utf8().data() would return a pointer to memory that was already freed. It's a bit annoying that we have to do the conversion to utf8 every time, but WebKit is gradually moving to supporting 8-bit strings.
(In reply to comment #7) > > I wonder if we really need priv->uri. When the request is created with an uri we set the URL of the ResourseRequest, so we could just return request->priv->resourceRequest.url().string().utf8() always here, no? > > The problem is that utf8() is a temporary here so returning utf8().data() would return a pointer to memory that was already freed. It's a bit annoying that we have to do the conversion to utf8 every time, but WebKit is gradually moving to supporting 8-bit strings. Right!, I always forget it, yes I also prefer to keep the uri twice and be able to return a const gchar *
Created attachment 121553 [details] Updated version of the patch
Comment on attachment 121553 [details] Updated version of the patch Looks good to me!
(In reply to comment #10) > (From update of attachment 121553 [details]) > Looks good to me! I didn't r+ this only because another +1 is needed from another GTK reviewer, unless I missed it :)
Comment on attachment 121553 [details] Updated version of the patch LGTM too, ftr I haven't been too strict regarding guaranteeing 2 reviewers for a webkit2 API change when it has been discussed already, feel free to assume my nod in those cases =)
(In reply to comment #12) > (From update of attachment 121553 [details]) > LGTM too, ftr I haven't been too strict regarding guaranteeing 2 reviewers for a webkit2 API change when it has been discussed already, feel free to assume my nod in those cases =) Yes, WK2 API hasn't been released as stable yet, so we can break API/ABI if needed at any time, and there's still a lot of work to do so there's no reason to make the development even slower. It's already enough difficult to find one reviewer. Once the first stable (or even beta) version is released I agree on requiring +1 from two reviewers for new API.
Committed r104850: <http://trac.webkit.org/changeset/104850>