Bug 72946 – [GTK] Add WebKitURIResponse to WebKit2 GTK+ API

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Carlos Garcia Campos]

It will be needed by Download and Policy clients.

[Carlos Garcia Campos]

Created attachment 116213 [details]
Patch

It only returns uri, content_length and status_code for now. It doesn't include unit tests, because it will be covered by other unit tests like downloads and policy client.

[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]

Comment on attachment 116213 [details]
Patch

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

> Source/WebKit2/UIProcess/API/gtk/WebKitNetworkResponse.cpp:130
> + * the server. It will normally be a #SoupKnownStatusCode, eg,
> + * %SOUP_STATUS_OK, though of course it might actually be an unknown
> + * status code.

I would rephrase this slightly to: It will normally be a #SoupKnownStatusCode, for example %SOUP_STATUS_OK, though the server can respond with any unsigned integer. We need to look into how we can properly link to soup documentation so this doesn't produce errors during the gktdoc-fixxref run.

> Source/WebKit2/UIProcess/API/gtk/WebKitNetworkResponse.cpp:153
> +guint64 webkit_network_response_get_content_length(WebKitNetworkResponse* response)

Any reason this isn't a property? It is appropriate to use gsize here instead of guint64? Soup seems to use goffset, which I assume is the same data type as gsize.

> Source/WebKit2/UIProcess/API/gtk/WebKitNetworkResponse.cpp:170
> +    WebKitNetworkResponse* response = WEBKIT_NETWORK_RESPONSE(g_object_new(WEBKIT_TYPE_NETWORK_RESPONSE, "uri", uri.get(), NULL));

Ah, is this why you've chosen to use a property for uri, but not the others? I guess I would prefer that all private data be properties or none of them.

> Source/WebKit2/UIProcess/API/gtk/WebKitNetworkResponse.cpp:176
> +SoupMessage*
> +webkitNetworkResponseGetSoupMessage(WebKitNetworkResponse* response)

Extra newline here. :)

[Carlos Garcia Campos]

(In reply to comment #3)
> (From update of attachment 116213 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=116213&action=review
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitNetworkResponse.cpp:130
> > + * the server. It will normally be a #SoupKnownStatusCode, eg,
> > + * %SOUP_STATUS_OK, though of course it might actually be an unknown
> > + * status code.
> 
> I would rephrase this slightly to: It will normally be a #SoupKnownStatusCode, for example %SOUP_STATUS_OK, though the server can respond with any unsigned integer. 

I copied it literally from the libsoup api docs :-P

> We need to look into how we can properly link to soup documentation so this doesn't produce errors during the gktdoc-fixxref run.

Right.

> > Source/WebKit2/UIProcess/API/gtk/WebKitNetworkResponse.cpp:153
> > +guint64 webkit_network_response_get_content_length(WebKitNetworkResponse* response)
> 
> Any reason this isn't a property? It is appropriate to use gsize here instead of guint64? Soup seems to use goffset, which I assume is the same data type as gsize.

Because data length used by WebKit2 internal API is uint64_t

> > Source/WebKit2/UIProcess/API/gtk/WebKitNetworkResponse.cpp:170
> > +    WebKitNetworkResponse* response = WEBKIT_NETWORK_RESPONSE(g_object_new(WEBKIT_TYPE_NETWORK_RESPONSE, "uri", uri.get(), NULL));
> 
> Ah, is this why you've chosen to use a property for uri, but not the others? I guess I would prefer that all private data be properties or none of them.

Yes, because it's construct only.

> > Source/WebKit2/UIProcess/API/gtk/WebKitNetworkResponse.cpp:176
> > +SoupMessage*
> > +webkitNetworkResponseGetSoupMessage(WebKitNetworkResponse* response)
> 
> Extra newline here. :)

Ok.

[Dan Winship]

(In reply to comment #4)
> > We need to look into how we can properly link to soup documentation so this doesn't produce errors during the gktdoc-fixxref run.

I think if you have the libsoup gtk-docs installed in the prefix you are installing webkit into, it will find them automatically.

> > > +guint64 webkit_network_response_get_content_length(WebKitNetworkResponse* response)
> > 
> > Any reason this isn't a property? It is appropriate to use gsize here instead of guint64? Soup seems to use goffset, which I assume is the same data type as gsize.

(Not always. gsize is size_t and goffset is offset_t. On x86_64 they're both 64-bit but on x86 size_t is 32-bit.)

> > Ah, is this why you've chosen to use a property for uri, but not the others? I guess I would prefer that all private data be properties or none of them.
> 
> Yes, because it's construct only.

Having properties for everything is more language-bindings-friendly.

[Martin Robinson]

(In reply to comment #5)

> Having properties for everything is more language-bindings-friendly.

In this case, I think it makes sense to have all members be properties or none of them, but not only some of them.

[Carlos Garcia Campos]

(In reply to comment #6)
> (In reply to comment #5)
> 
> > Having properties for everything is more language-bindings-friendly.
> 
> In this case, I think it makes sense to have all members be properties or none of them, but not only some of them.

Good point, I agree, I'll update the wiki accordingly http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

btw, I'll rename this and NetworkRequest as WebKitURIRequest/Response if nobody objects.

[Martin Robinson]

(In reply to comment #7)

> btw, I'll rename this and NetworkRequest as WebKitURIRequest/Response if nobody objects.

I like WebKitURIRequest. It's more accurate and less typing. :)

[Carlos Garcia Campos]

Created attachment 116749 [details]
Updated patch

Updated according to review comments and renamed to WebKitURIResponse

[Martin Robinson]

Comment on attachment 116749 [details]
Updated patch

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

Nice.

> Source/WebKit2/UIProcess/API/gtk/WebKitURIResponse.cpp:98
> +     * The URI to which the response will be made.

Maybe change this to "The URI for which the response was made."

> Source/WebKit2/UIProcess/API/gtk/WebKitURIResponse.cpp:104
> +                                                        _("The URI to which the response will be made."),

Ditto.

> Source/WebKit2/UIProcess/API/gtk/WebKitURIResponse.cpp:182
> + * be 0 if the server provided incorrect or missing Content-Length.

You have "an" after provided.

[Carlos Garcia Campos]

Committed r101350: <http://trac.webkit.org/changeset/101350>