[webkit-reviews] review requested: [Bug 18987] [GTK] Implement SharedBuffer::createWithContentsOfFile and KURL::fileSystemPath : [Attachment 23169] Implement the missing functions

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Sep 4 06:35:35 PDT 2008


Marco Barisione <marco.barisione at collabora.co.uk> has asked  for review:
Bug 18987: [GTK] Implement SharedBuffer::createWithContentsOfFile and
KURL::fileSystemPath
https://bugs.webkit.org/show_bug.cgi?id=18987

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

------- Additional Comments from Marco Barisione
<marco.barisione at collabora.co.uk>
(In reply to comment #10)
> KURL::fileSystemPath... in the windows port and other places early exits are
> popular. e.g.

Fixed.

> g_filename_from_uri takes the URI as ASCII and returns in filesystem encoding

> (I assume that is utf8). Should we assume m_string is a properly encoded URL
> (all ascii?) and that utf8() is not creating any issue?

URIs should always be in ASCII, file names are in the glib file name encoding
(it depends on the locale and you can also have file names containing random
junk). This means that it's impossible to store every possible file name in a
String (while it would be possible to use a CString). All the code in WebKit
GTK currently assumes that the file names are encoded in UTF-8, I suggest that
we accept this patch as-is and I will write a patch for a separate bug to fix
it everywhere.


(In reply to comment #11)
> In KURL::fileSystemPath, there's no need for the "else" since the "if" branch

> returns.

Fixed.
 
> In SharedBuffer::createWithContentsOfFile, "result" should be declared at the

> point it is first used rather than towards the top of the function.

Fixed.

> It also seems bad that this results in allocating twice the file size in
> memory.  Avoiding the extra allocation and memcpy would be a good idea.

I know but it's the only easily portable solution. GIOChannel cannot be used as
it has no way of retrieving the file size unless you get the underlying file
descriptor, but this causes problems on Windows. Note that this function is
used only to read the content of user stylesheets and for sure they are not
huge.


(In reply to comment #12)
> 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.

Fixed.

> Namespaces don't need a ; after them.  All namespaces closes should have a
> comment though.

Fixed.

> Could we make up a GFreePtr?	Which just called g_free on its contents when
> they go out of scope?

I will write a separate patch for this. In the meantime I think that this patch
could go in and we could fix the g_free later.

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

Fixed.


More information about the webkit-reviews mailing list