Bug 40328 – GtkLauncher fails to build: Undefined references to openFile, readFromFile, and seekFile

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Clemmitt Sigler]

Hi,

In trying to build WebKitGtk on generic Linux (a Gentoo Linux installation) on x86_64, linking GtkLauncher fails with these three errors in particular:

./.libs/libwebkit-1.0.so: undefined reference to `WebCore::openFile(WebCore::String const&, WebCore::FileOpenMode)'
./.libs/libwebkit-1.0.so: undefined reference to `WebCore::readFromFile(int, char*, int)'
./.libs/libwebkit-1.0.so: undefined reference to `WebCore::seekFile(int, long long, WebCore::FileSeekOrigin)'

(Other linking errors are dealt with in Bug 40326.)

These file access problems seem to be related to WebCore/platform/gtk/FileSystemGtk.cpp.  These three methods aren't defined in this file as they are in, e.g., WebCore/platform/posix/FileSystemPOSIX.cpp.  Of course, when I tried including the posix module in the build, there were namespace collisions.  It seems the fix may be to add these missing methods to WebCore/platform/gtk/FileSystemGtk.cpp.

I'm not sure how to go about coding the fix for this problem.  I just hope some of the Gtk devs. read this report and can add these required methods.  Thanks.

Clemmitt

[Clemmitt Sigler]

(In reply to comment #0)
<snip>
> ./.libs/libwebkit-1.0.so: undefined reference to
> `WebCore::openFile(WebCore::String const&, WebCore::FileOpenMode)'
> ./.libs/libwebkit-1.0.so: undefined reference to
> `WebCore::readFromFile(int, char*, int)'
> ./.libs/libwebkit-1.0.so: undefined reference to
> `WebCore::seekFile(int, long long, WebCore::FileSeekOrigin)'
<snip>
> These file access problems seem to be related to
> WebCore/platform/gtk/FileSystemGtk.cpp.  These three
> methods aren't defined in this file as they are in,
> e.g., WebCore/platform/posix/FileSystemPOSIX.cpp.

Additional info:

As a QnD test I've added dummy stubs that return -1 (error) for these three methods to WebCore/platform/gtk/FileSystemGtk.cpp.  Doing that at least gets the build to complete w/o error.

A little investigation leads me to believe that:

1.) To implement 'PlatformFileHandle openFile(const String& path, FileOpenMode mode)' for the GTK platform, a wrapper for 'int g_open(const gchar *filename, int flags, int mode)' is needed.

2.) To implement 'int readFromFile(PlatformFileHandle handle, char* data, int length)' for the GTK platform, a wrapper for 'gboolean g_file_get_contents(const gchar *filename, gchar **contents, gsize *length, GError **error)' is needed.

3.) To implement 'long long seekFile(PlatformFileHandle handle, long long offset, FileSeekOrigin origin)' for the GTK platform, a wrapper for 'GIOStatus g_io_channel_seek_position(GIOChannel *channel, gint64 offset, GSeekType type, GError **error)' is needed.

I'm not a low-level GTK developer, having only done some simple GUI design/use, so I'd say I'm not the best person to implement these.  Any GTK developer out there, for whom this is a half-hour hack?  TIA!

Clemmitt

[Clemmitt Sigler]

(In reply to comment #1)
<snip>
> A little investigation leads me to believe that:
<snip>

I've got some more info now.  The only place the methods openFile, readFromFile and seekFile seem to be called is in WebCore/html/FileStream.cpp.  I observe that these changes were introduced in Rev. 58832 --> http://trac.webkit.org/changeset/58832

Therefore, it's my speculation that since that rev. -- when building GTK with all the bells and whistles turned on, at least -- Programs/GtkLauncher hasn't been able to link without error.  As for me, I hadn't run a build since rev. 55083 until recently.

{FWIW, the two FileStream methods that call these three are:

void FileStream::openForRead(Blob* blob)
void FileStream::read(char* buffer, int length)}

I'm still proposing that these three need to be implemented in WebCore/platform/gtk/FileSystemGtk.cpp by someone who knows what they're doing with basic GTK ops. to match similar implementation in, e.g., WebCore/platform/posix/FileSystemPOSIX.cpp as provided by Revision 57182.

(As a lark I may try rolling my tree back to rev. 58831 and see if this problem goes away, as it should.)

HTH some more.

Clemmitt

[Clemmitt Sigler]

(In reply to comment #1)

I found some time to look at stuff, so I just did an implementation of these methods myself.  Not sure if the code meets standards for GTK use.  Patch is coming next.  HTH.

Clemmitt

[Clemmitt Sigler]

Created attachment 58535 [details]
Patch to add missing methods to FileSystemGtk.cpp

Patch as described in previous comment.  HTH.

[Clemmitt Sigler]

Created attachment 58536 [details]
Patch to add missing methods to FileSystemGtk.cpp

Patch as described in previous comment.  HTH.

[Dirk Schulze]

Comment on attachment 58536 [details]
Patch to add missing methods to FileSystemGtk.cpp

This is not an official review, but i would like to give some hints.WebCore/ChangeLog:5
 +          Add to FileSystemGtk.cpp methods openFile, readFromFile
The order in Changelogs is normally bug title, bug link, description to the patch and added tests. The patch WebKitTools/Scripts/prepare-ChangeLog -bug <bug-number> will help you to create the changelog.

WebCore/platform/gtk/FileSystemGtk.cpp:34
 +  #include <unistd.h>
You added this include twice now.

WebCore/platform/gtk/FileSystemGtk.cpp:267
 +  // Following the implementation for closeFile above, implement openFile
Please indent all comments with the code.

WebCore/platform/gtk/FileSystemGtk.cpp:286
 +  // as the pattern for implementing readFromFile
dito.

WebCore/platform/gtk/FileSystemGtk.cpp:292
 +  // Handle EOF on read
dito.

WebCore/platform/gtk/FileSystemGtk.cpp:305
 +  // just use code from WebCore/platform/posix/FileSystemPOSIX.cpp
dito.

You either added a new feature, or fixed a regression. Both need layouttesting...

[Clemmitt Sigler]

(In reply to comment #6)
> (From update of attachment 58536 [details])
> This is not an official review, but i would like to give some hints.

Thank you very much, Dirk.  I'll make all these fixes.

As of now I don't know how to work with layouttests.  I need to review and digest the "Writing New Tests" webpage and then find time to do the work -- which may not happen until sometime next week :^(

Thanks again.

Clemmitt

[Kent Hansen]

(In reply to comment #7)
> (In reply to comment #6)
> > (From update of attachment 58536 [details] [details])
> > This is not an official review, but i would like to give some hints.
> 
> Thank you very much, Dirk.  I'll make all these fixes.
> 
> As of now I don't know how to work with layouttests.  I need to review and digest the "Writing New Tests" webpage and then find time to do the work -- which may not happen until sometime next week :^(
> 
> Thanks again.
> 
> Clemmitt

Hi Clemmitt,
Please turn off the review flag if you don't intend to have the current patch reviewed anymore. That stops it from showing up in the list of patches pending review.

I've also added the gtk keyword to this bug, since it's more likely to be spotted by gtk devs then.

As you commented already, the FileReader class was added in http://trac.webkit.org/changeset/58832, but it's still off by default. So the workaround until a gtk patch is landed is to build without that feature (--no-file-reader, or just don't pass --file-reader).

And I don't think you can write a LayoutTest for this; essentially the patch for the missing functions is needed in order to enable file-reader for the gtk port, and once that happens, the entry for fast/files/file-reader.html can be removed from platform/gtk/Skipped.

[Gustavo Noronha (kov)]

Comment on attachment 58536 [details]
Patch to add missing methods to FileSystemGtk.cpp

 304 // Since above methods are implemented without GLib calls and variables,
 305 // just use code from WebCore/platform/posix/FileSystemPOSIX.cpp

Will these work on Windows? Why not use the glib tools that enable portability here? Also, yes, I think you can just remove the test from the skipped list =)

I'll r- given krit's comments and my own.

[Gustavo Noronha (kov)]

Implementations for these seem to be written already. See r63308. I believe we should revisit FileSystemGtk, and implement all of it using GIO, though.