Bug 40328 - GtkLauncher fails to build: Undefined references to openFile, readFromFile, and seekFile
Summary: GtkLauncher fails to build: Undefined references to openFile, readFromFile, a...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on:
Blocks:
 
Reported: 2010-06-08 13:28 PDT by Clemmitt Sigler
Modified: 2010-08-11 05:36 PDT (History)
2 users (show)

See Also:


Attachments
Patch to add missing methods to FileSystemGtk.cpp (2.93 KB, patch)
2010-06-11 18:38 PDT, Clemmitt Sigler
no flags Details | Formatted Diff | Diff
Patch to add missing methods to FileSystemGtk.cpp (2.93 KB, patch)
2010-06-11 18:39 PDT, Clemmitt Sigler
gns: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Clemmitt Sigler 2010-06-08 13:28:06 PDT
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
Comment 1 Clemmitt Sigler 2010-06-09 06:32:28 PDT
(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
Comment 2 Clemmitt Sigler 2010-06-09 14:12:26 PDT
(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
Comment 3 Clemmitt Sigler 2010-06-11 18:37:09 PDT
(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
Comment 4 Clemmitt Sigler 2010-06-11 18:38:36 PDT
Created attachment 58535 [details]
Patch to add missing methods to FileSystemGtk.cpp

Patch as described in previous comment.  HTH.
Comment 5 Clemmitt Sigler 2010-06-11 18:39:28 PDT
Created attachment 58536 [details]
Patch to add missing methods to FileSystemGtk.cpp

Patch as described in previous comment.  HTH.
Comment 6 Dirk Schulze 2010-06-11 23:19:21 PDT
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...
Comment 7 Clemmitt Sigler 2010-06-12 07:21:09 PDT
(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
Comment 8 Kent Hansen 2010-07-23 07:04:13 PDT
(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.
Comment 9 Gustavo Noronha (kov) 2010-08-11 05:28:26 PDT
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.
Comment 10 Gustavo Noronha (kov) 2010-08-11 05:36:49 PDT
Implementations for these seem to be written already. See r63308. I believe we should revisit FileSystemGtk, and implement all of it using GIO, though.