Bug 18987 – [GTK] Implement SharedBuffer::createWithContentsOfFile and KURL::fileSystemPath

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Marco Barisione]

SharedBuffer::createWithContentsOfFile and KURL::fileSystemPath are not implemented, this prevents user stylesheets to work.

I'm writing a patch for this.

[Marco Barisione]

Created attachment 21057 [details]
Implement the missing functions

After this change "user-stylesheet-uri" works again.

[Holger Freyther]

(In reply to comment #1)
> Created an attachment (id=21057) [edit]
> Implement the missing functions
> 
> After this change "user-stylesheet-uri" works again.


The userstyle path is local by definition, why didn't you use g_file_get_contents? 

[Marco Barisione]

(In reply to comment #2)
> The userstyle path is local by definition, why didn't you use
> g_file_get_contents?

g_file_get_contents() allocates a new buffer that then must be copied in ther SharedBuffer. Maybe this is not a problem for a css file but this function is generic so I wanted to avoid a useless memory allocation.

[Marco Barisione]

Hm, I realised that it will not work on Windows because I use g_io_channel_unix_get_fd.

The function is used only to read the template for directory listing when using FTP and to read CSS, so the files should be small enough to just use g_file_get_contents().

[Marco Barisione]

Created attachment 21080 [details]
Implement the missing functions and use g_file_contents() to read the file

[Holger Freyther]

Comment on attachment 21080 [details]
Implement the missing functions and use g_file_contents() to read the file

Sorry, I have to say r=- on this one. I will be on irc tomorrow we can resolve this and land the patch without waiting another month.

comments:
   - the goto is unusual, we tend to use early exits
   - KURL::fileSystemPath does not work with utf8 encoded paths as String path(char*) works only on latin1.

[Marco Barisione]

(In reply to comment #6)
> comments:
>    - the goto is unusual, we tend to use early exits

The first version of the patch was based on the win port that uses gotos, when rewriting it to use g_file_get_contents() I din't change the structure of the code. I can fix it in the next version of the patch.

>    - KURL::fileSystemPath does not work with utf8 encoded paths as String
> path(char*) works only on latin1.

What do you mean?

[Unavowed]

(In reply to comment #7)
> (In reply to comment #6)
> > comments:
> >    - the goto is unusual, we tend to use early exits
> 
> The first version of the patch was based on the win port that uses gotos, when
> rewriting it to use g_file_get_contents() I din't change the structure of the
> code. I can fix it in the next version of the patch.
> 
> >    - KURL::fileSystemPath does not work with utf8 encoded paths as String
> > path(char*) works only on latin1.
> 
> What do you mean?
> 

I think he means you should use String::fromUTF8() in String KURL::fileSystemPath(), instead of the normal String constructor.

[Marco Barisione]

Created attachment 22679 [details]
Implement missing functions

I should have adressed all the problems.

[Holger Freyther]

(In reply to comment #9)
> Created an attachment (id=22679) [edit]
> Implement missing functions
> 
> I should have adressed all the problems.

Okay, SharedBuffer::createWithContentsOfFile is ready, if you want to land this go ahead.

KURL::fileSystemPath... in the windows port and other places early exits are popular. e.g.

if (!filename)
  return String();

other_code

also glib documentation and code do not agree.

+    gchar* filename = g_filename_from_uri(m_string.utf8().data(), 0, 0);

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?

[Mark Rowe (bdash)]

Comment on attachment 22679 [details]
Implement missing functions

A few comments:

In KURL::fileSystemPath, there's no need for the "else" since the "if" branch returns.

In SharedBuffer::createWithContentsOfFile, "result" should be declared at the point it is first used rather than towards the top of the function.

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.

[Eric Seidel (no email)]

Comment on attachment 22679 [details]
Implement missing functions

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!

[Marco Barisione]

Created attachment 23169 [details]
Implement the missing functions

(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.

[Marco Barisione]

Created attachment 23170 [details]
Implement missing functions

Sorry, I forgot the ChangeLog entry in the previous

[Holger Freyther]

Comment on attachment 23170 [details]
Implement missing functions

The implementations look sane. I assume KURL::fileSystemPath needs to be touched after/when handling bug #23441?

[Jan Alonzo]

Landed in r36812. Thanks for the patch and review!

zecke: bug # 23441?