[webkit-reviews] review denied: [Bug 19656] [SOUP] The gio code should call didFail() instead of didFinishLoading() in case of error : [Attachment 21819] Call didFail in case of error

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Marco Barisione
<marco.barisione at collabora.co.uk>'s request for review:
Bug 19656: [SOUP] The gio code should call didFail() instead of
didFinishLoading() in case of error
https://bugs.webkit.org/show_bug.cgi?id=19656

Attachment 21819: Call didFail in case of error
https://bugs.webkit.org/attachment.cgi?id=21819&action=edit

------- Additional Comments from Eric Seidel <eric at webkit.org>
We really should have a GOwnPtr which knows how to call g_free on the
associated pointer.  In that case:
      gchar* uri = g_file_get_uri (d->m_gfile);
 519	     ResourceError resourceError("webkit-network-error",
ERROR_TRANSPORT, uri, String::fromUTF8(error->message));
 520	     g_free(uri);

would not need the g_free line.  Stupid c-style memory management.  Bleh.  (No
offense to your code, I just have a general hatred towards languages and APIs
which make it easy for you to stab yourself in the foot, or leak memory.)

WebKit style does not have a space between the function name and the (
+	 gchar* uri = g_file_get_uri (d->m_gfile);

So we make 2 copies of the buffer just to send this URI off to ResourceError. 
Is it possible to avoid this?

You should consider adding an inline function which created the resource error
from the gfile handle and returned it to you.	That would eliminate 6 lines of
copy-paste code.

Then these calls just become:

client->didFail(handle, networkErrorForFile(d->m_gfile, error));

static inline ResourceError networkErrorForFile(gfile* file, gerror* error)
{
    gchar* uri = g_file_get_uri(d->m_gfile);
    ResourceError resourceError("webkit-network-error", ERROR_TRANSPORT, uri,
String::fromUTF8(error->message));
    g_free(uri);
   return resourceError;
}

r- for the style errors.  And I would encourage you to use a static inline to
eliminate this copy-paste code.