[webkit-reviews] review denied: [Bug 18608] [Gtk] WebKitNetworkRequest needs to be finished : [Attachment 30604] Make NetworkRequest carry a reference of the SoupMessage used by ResourceRequest

[bugzilla-daemon at webkit.org]

Jan Alonzo <jmalonzo at gmail.com> has denied Gustavo Noronha (kov)
<gns at gnome.org>'s request for review:
Bug 18608: [Gtk] WebKitNetworkRequest needs to be finished
https://bugs.webkit.org/show_bug.cgi?id=18608

Attachment 30604: Make NetworkRequest carry a reference of the SoupMessage used
by ResourceRequest
https://bugs.webkit.org/attachment.cgi?id=30604&action=review

------- Additional Comments from Jan Alonzo <jmalonzo at gmail.com>
> +	   ResourceRequest(SoupMessage* soupMessage)
> +	       : ResourceRequestBase()
> +	       , m_soupMessage(soupMessage)
> +	   {
> +	   }
> +

Would be nice if we can initialize ResourceRequestBast as
ResourceRequestBase(KURL(), UseProtocolCachePolicy)

> +void ResourceRequest::doUpdateResourceRequest()
> +{
> +    SoupURI* soupURI = soup_message_get_uri(m_soupMessage);

We need to null-check m_soupMessage here just in case updatePlatformRequest
hasn't been called.

> +    gchar* uri = soup_uri_to_string(soupURI, FALSE);
> +    m_url = KURL(KURL(), String::fromUTF8(uri));
> +    g_free(uri);

no need to create uri. Just put the call inside fromUTF8().

> +
> +    m_httpBody = FormData::create(m_soupMessage->request_body->data,
m_soupMessage->request_body->length);
> +}

Can we add a FIXME for the cookies stuff to be updated once we figure out how
to do that? Thanks.

> +
> +void ResourceRequest::doUpdatePlatformRequest()
> +{
> +    if (isNull()) {
> +	   if (m_soupMessage)
> +	       g_object_unref(m_soupMessage);
> +	   m_soupMessage = 0;
> +	   return;
> +    }
> +
> +    if (!m_soupMessage) {
> +	   m_soupMessage = soup_message_new(httpMethod().utf8().data(),
url().string().utf8().data());
> +
> +	   if (!m_soupMessage)
> +	       return;
> +    } else {
> +	   SoupURI* soupURI = soup_uri_new(url().string().utf8().data());
> +	   soup_message_set_uri(m_soupMessage, soupURI);
> +	   soup_uri_free(soupURI);

I think we can avoid initializing a soupURI here by moving soup_uri_new in the
call to soup_message_set_uri().

> +
> +	   g_object_set(m_soupMessage, "method", httpMethod().utf8().data(),
NULL);
> +
> +	   soup_message_headers_clear(m_soupMessage->request_headers);
> +    }

I think we can lose the else block since here since we only need to make sure
we have a m_soupMessage before doing anything.

> +
> +    HTTPHeaderMap headers = httpHeaderFields();
> +    SoupMessageHeaders* soupHeaders = m_soupMessage->request_headers;
> +    if (!headers.isEmpty()) {
> +	   HTTPHeaderMap::const_iterator end = headers.end();
> +	   for (HTTPHeaderMap::const_iterator it = headers.begin(); it != end;
++it)
> +	       soup_message_headers_append(soupHeaders,
it->first.string().utf8().data(), it->second.utf8().data());
> +    }
> +
> +    // Body data is only handlded at ResourceHandleSoup::startHttp

handlded -> handled. Maybe a good idea to put the rationale here why we're
doing it in startHttp().

Looking good. I'm going to r- this for now until the trivial issues above are
addressed. 

Cheers