Bug 74478 - [GTK] [WebKit2] WebKitURIRequest and WebKitURIResponse should wrap the corresponding WebCore classes
Summary: [GTK] [WebKit2] WebKitURIRequest and WebKitURIResponse should wrap the corres...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Martin Robinson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-12-13 19:52 PST by Martin Robinson
Modified: 2012-01-12 13:16 PST (History)
5 users (show)

See Also:


Attachments
Patch (11.30 KB, patch)
2011-12-14 19:59 PST, Martin Robinson
no flags Details | Formatted Diff | Diff
How this affects the patch on bug 72952 (1.98 KB, patch)
2011-12-14 20:30 PST, Martin Robinson
no flags Details | Formatted Diff | Diff
Updated version of the patch (12.01 KB, patch)
2012-01-07 09:24 PST, Martin Robinson
gns: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Robinson 2011-12-13 19:52:30 PST
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.
Comment 1 Martin Robinson 2011-12-14 19:59:55 PST
Created attachment 119364 [details]
Patch
Comment 2 WebKit Review Bot 2011-12-14 20:02:39 PST
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 3 Martin Robinson 2011-12-14 20:30:38 PST
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 4 Gustavo Noronha (kov) 2011-12-14 22:32:00 PST
Comment on attachment 119364 [details]
Patch

Attachment 119364 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10905139
Comment 5 Carlos Garcia Campos 2011-12-14 23:52:18 PST
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.
Comment 6 Martin Robinson 2011-12-15 01:43:32 PST
(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.
Comment 7 Martin Robinson 2011-12-15 01:46:20 PST
(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.
Comment 8 Carlos Garcia Campos 2011-12-15 01:49:16 PST
(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 *
Comment 9 Martin Robinson 2012-01-07 09:24:25 PST
Created attachment 121553 [details]
Updated version of the patch
Comment 10 Philippe Normand 2012-01-09 05:06:40 PST
Comment on attachment 121553 [details]
Updated version of the patch

Looks good to me!
Comment 11 Philippe Normand 2012-01-10 08:38:39 PST
(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 12 Gustavo Noronha (kov) 2012-01-10 09:16:15 PST
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 =)
Comment 13 Carlos Garcia Campos 2012-01-10 23:32:19 PST
(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.
Comment 14 Martin Robinson 2012-01-12 13:16:46 PST
Committed r104850: <http://trac.webkit.org/changeset/104850>