Bug 74478 – [GTK] [WebKit2] WebKitURIRequest and WebKitURIResponse should wrap the corresponding WebCore classes

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Martin Robinson]

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.

[Martin Robinson]

Created attachment 119364 [details]
Patch

[WebKit Review 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

[Martin Robinson]

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.

[Gustavo Noronha (kov)]

Comment on attachment 119364 [details]
Patch

Attachment 119364 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10905139

[Carlos Garcia Campos]

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.

[Martin Robinson]

(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.

[Martin Robinson]

(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.

[Carlos Garcia Campos]

(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 *

[Martin Robinson]

Created attachment 121553 [details]
Updated version of the patch

[Philippe Normand]

Comment on attachment 121553 [details]
Updated version of the patch

Looks good to me!

[Philippe Normand]

(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 :)

[Gustavo Noronha (kov)]

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 =)

[Carlos Garcia Campos]

(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.

[Martin Robinson]

Committed r104850: <http://trac.webkit.org/changeset/104850>