[Webkit-unassigned] [Bug 18608] [Gtk] WebKitNetworkRequest needs to be finished

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=18608


marco.barisione at collabora.co.uk changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |marco.barisione at collabora.co
                   |                            |.uk




------- Comment #10 from marco.barisione at collabora.co.uk  2008-07-18 11:36 PDT -------
(In reply to comment #6)
> ok, so this is my proposal for landing WebKitNetworkRequest (along with an
> initial WebKitNetworkResponse); comments? =)

I would omit request-redirected/request-started for now and add it when we have
both this patch and the one to clean the frame loader signals, see bug #17066.
Moreover, having two signals means that people will forget to connect to both,
a single signal is better IMHO. Suggestions for the name?. Another problem is
that we don't pass the object indentified by the identifier argument.

>+        ResourceResponse responseCopy = resourceResponse;
>+        WebKitNetworkResponse* response;
>+    
>+        response = webkit_network_response_new_from_resource_response(responseCopy);

Initialisation should be on the same line as declaration.


>struct _WebKitNetworkRequestPrivate {
>+    ResourceRequest request;
>+
>     gchar* uri;
>+    WebKitReqCachePolicy cache_policy;
>+    gdouble timeout_interval;
>+    gchar* main_document_uri;
>+    gchar* http_method;
>+    GHashTable *http_headers;
>+    gboolean allow_http_cookies;
>+    gchar *body;
>+    gsize body_size;
> };

You should always use the WebKit style for things that are not exposed in the
public API.

Why are you keeping in WebKitNetworkRequestPrivate both the ResourceRequest and
the values (like cache_policy) that can be easily extracted from it? Maybe it
makes sense for strings to avoid conversions and duplications but not for just
integers.


>+static WebKitReqCachePolicy cache_policy_from_resource_request(const ResourceRequest& resourceRequest)
>+{
>+    ResourceRequestCachePolicy cache_policy = resourceRequest.cachePolicy();
>+    
>+    switch(cache_policy)
>+        {

This could just be something like:

static WebKitReqCachePolicy kit(ResourceRequestCachePolicy cachePolicy)
{
    return (WebKitReqCachePolicy)cachePolicy;
}

Then add a comment to the public header to keep the enums in sync.

>+void webkit_network_request_set_cache_policy(WebKitNetworkRequest* request, const WebKitReqCachePolicy cache_policy)
>+{

Ditto, just define a core() function that casts the enum.


>+    priv->http_headers = g_hash_table_new_full(g_str_hash, g_str_equal, g_free, g_free);
>+    HTTPHeaderMap headers = resourceRequest.httpHeaderFields();
>+    if (!headers.isEmpty()) {
>+        HTTPHeaderMap::const_iterator end = headers.end();
>+        for (HTTPHeaderMap::const_iterator it = headers.begin(); it != end; ++it)
>+            g_hash_table_insert(priv->http_headers, (gpointer)g_strdup(it->first.utf8().data()), (gpointer)g_strdup(it->second.utf8().data()));
>+    }

Maybe it's better to lazily initialise the hash table only when needed.


>+/**
>+ * webkit_network_request_new:
>+ * @uri: an already encoded URI
>+ *
>+ * Creates a new #WebKitNetworkRequest initialized with an URI.
>+ *
>+ * Returns: a new #WebKitNetworkRequest
>+ */
> WebKitNetworkRequest* webkit_network_request_new(const gchar* uri)

Why is this function public? Does it make sense to use it in an application? I
cannot think to any use case so maybe we could think about deprecating it.

What do you mean with "already encoded URI"?


>+const GHashTable* webkit_network_request_get_http_headers(WebKitNetworkRequest* request)
>+{
>+    g_return_val_if_fail(WEBKIT_IS_NETWORK_REQUEST(request), NULL);
>+
>+    WebKitNetworkRequestPrivate* priv = request->priv;
>+    return priv->http_headers;
>+}

The documentation is missing.
Maybe it could be useful to be able to keep the hash table somewhere after
calling _ref, but in this case the return value should not be const and maybe
people would think that they can overwrite values directly on the hash table.
What do you think?


>+/**
>+ * WebKitReqCachePolicy:
>+ * @WEBKIT_REQ_CACHE_POLICY_NONE: invalid, marker policy
>[...]
>+ */
>+typedef enum
>+{
>+  WEBKIT_REQ_CACHE_POLICY_NONE,

I don't think we need it.


Do we need all the getters and setters for headers? Like
webkit_network_request_[gs]et_http_user_agent().


Probably the various things for which we have getters and setters (excluding
the hash table for headers and probably also the body as it needs also the
length to be usable) should also be properties.


-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.