[webkit-reviews] review denied: [Bug 18987] [GTK] Implement SharedBuffer::createWithContentsOfFile and KURL::fileSystemPath : [Attachment 22679] Implement missing functions

[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 18987: [GTK] Implement SharedBuffer::createWithContentsOfFile and
KURL::fileSystemPath
https://bugs.webkit.org/show_bug.cgi?id=18987

Attachment 22679: Implement missing functions
https://bugs.webkit.org/attachment.cgi?id=22679&action=edit

------- Additional Comments from Eric Seidel <eric at webkit.org>
We generally try to use a "no else after return" as well as "prefer early
return instead of long if blocks" coding style in WebCore.  I would have
written
+String KURL::fileSystemPath() const

instead as:

+String KURL::fileSystemPath() const
+{
+    gchar* filename = g_filename_from_uri(m_string.utf8().data(), 0, 0);
       if (!filename)
	   return String();
       String path = String::fromUTF8(filename);
+    g_free(filename);
+    return path;
+}

Namespaces don't need a ; after them.  All namespaces closes should have a
comment though.  So this is mostly right:
}; // namespace WebCore
just no need for the ;, and several other } namespace closes should have a
comment.

Could we make up a GFreePtr?  Which just called g_free on its contents when
they go out of scope?  That would get rid of about half of the lines of this
patch... since they all deal with silly manual memory management. :(  You'd
have to add an accessor to GFreePtr to get a reference to the storage location
of the data pointer so you could pass it to these sorts of methods which return
raw pointers as argument references.

For example... does contents need to be free'd here?

 if (!g_file_get_contents(filename, &contents, &size, &error)) {
 43	    LOG_ERROR("Failed to fully read contents of file %s - %s",
filePath.utf8().data(), error->message);
 44	    g_error_free(error);
 45	    g_free(filename);
 46	    return 0;
 47	}

IF we had a smart pointer, we wouldn't need to care. :(

bah!

 49	result = SharedBuffer::create();
 50	result->m_buffer.resize(size);
 51	if (result->m_buffer.size() != size) {
 52	    g_free(filename);
 53	    g_free(contents);
 54	    return 0;
 55	}

That's not the way I would write that.

Instead:

RefPtr<SharedBuffer> result = SharedBuffer::create(contents, size);
g_free(contents);
g_free(fillename);

return result.release();

createWithContentsOfFile needs to be fixed.  Please consider adding a smart
pointer to be used in the GTK port, this manual memory management is a good way
to crasher + leaks bugs down the road.

Thanks for the patch!