Bug 40209 – [GTK] Enabling File Reader/Writer APIs

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Joone Hur]

Created attachment 57976 [details]
Proposed Patch

Enabling FileAPI for GTK Port causes build error.
Because there are 3 functions not implemented yet in FileSystemGtk.cpp
 * CString fileSystemRepresentation(const String& path)
 * PlatformFileHandle openFile(const String& path, FileOpenMode mode)
 * int readFromFile(PlatformFileHandle handle, char* data, int length)

I copied these functions from other port and tweaked a bit.
However, it could not pass fast/files test case.

The problem is that fileBlob, the parameter of readAsBinaryString is null as follows,

#0  0x002c42d8 in WTF::RefPtr<WebCore::StringImpl>::operator! (this=0xc) at ../../JavaScriptCore/wtf/RefPtr.h:68
#1  0x002ccc27 in WebCore::String::length (this=0xc) at ../../JavaScriptCore/wtf/text/WTFString.h:123
#2  0x00ca4b6f in WebCore::String::utf8 (this=0xc) at ../../JavaScriptCore/wtf/text/WTFString.cpp:641
#3  0x005b19da in WebCore::FileReader::readAsBinaryString (this=0x85ab058, fileBlob=0x0) at ../../WebCore/html/FileReader.cpp:88
#4  0x00df4baf in WebCore::jsFileReaderPrototypeFunctionReadAsBinaryString (exec=0xb544e118) at DerivedSources/JSFileReader.cpp:390

Is there any idea for solving this problem?

[Kinuko Yasuda]

The patch lgtm, though I'm not a reviewer.
As for the test failure, how did you run the test?  If you're running fast/files/file-reader.html by manually choosing files under resources, you'd end up selecting 6 files while the test requires 7 file entries (including non-existent one) and the last test would always fail.
(If that's the case maybe we should make the test support manual testing?)

[Jian Li]

(In reply to comment #1)
> The patch lgtm, though I'm not a reviewer.
> As for the test failure, how did you run the test?  If you're running fast/files/file-reader.html by manually choosing files under resources, you'd end up selecting 6 files while the test requires 7 file entries (including non-existent one) and the last test would always fail.
> (If that's the case maybe we should make the test support manual testing?)

Does GTK support eventSender in DRT?

[Kinuko Yasuda]

(In reply to comment #2)
> (In reply to comment #1)
> > The patch lgtm, though I'm not a reviewer.
> > As for the test failure, how did you run the test?  If you're running fast/files/file-reader.html by manually choosing files under resources, you'd end up selecting 6 files while the test requires 7 file entries (including non-existent one) and the last test would always fail.
> > (If that's the case maybe we should make the test support manual testing?)
> 
> Does GTK support eventSender in DRT?

(As far as I believe) it does, though I don't exactly know which features are missing or supported.  Basic ones like eventSender.mouseUp seem to be working.

[Jian Li]

(In reply to comment #3)
> (As far as I believe) it does, though I don't exactly know which features are missing or supported.  Basic ones like eventSender.mouseUp seem to be working.

I mean eventSender.beginDragWithFiles.

[Kinuko Yasuda]

(In reply to comment #4)
> (In reply to comment #3)
> > (As far as I believe) it does, though I don't exactly know which features are missing or supported.  Basic ones like eventSender.mouseUp seem to be working.
> 
> I mean eventSender.beginDragWithFiles.

Oh I see.  Apparently it does not.  There's a "FIXME: Implement this comment" in the code.

[Kinuko Yasuda]

(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > (As far as I believe) it does, though I don't exactly know which features are missing or supported.  Basic ones like eventSender.mouseUp seem to be working.
> > 
> > I mean eventSender.beginDragWithFiles.
> 
> Oh I see.  Apparently it does not.  There's a "FIXME: Implement this comment" in the code.
I meant: there's a "FIXME: Implement this" comment in the code.

(And that's why I asked if Joone tested the feature by manually selecting the files.)

[Joone Hur]

> I meant: there's a "FIXME: Implement this" comment in the code.
> 
> (And that's why I asked if Joone tested the feature by manually selecting the files.)

I ran fast/files/file-reader.html by manually choosing UTF8.txt under resource directory.

[Kinuko Yasuda]

(In reply to comment #7)
> > I meant: there's a "FIXME: Implement this" comment in the code.
> > 
> > (And that's why I asked if Joone tested the feature by manually selecting the files.)
> 
> I ran fast/files/file-reader.html by manually choosing UTF8.txt under resource directory.

In that case the reason it's failing is because the GTK port doesn't support eventSender.beginDragWithFiles.   The patch itself looks ok to me - in my environment the file-reader.html ran successfully with your patch (with a small tweak in the test to work around beginDragWithFiles).

[Joone Hur]

Created attachment 61346 [details]
Updated patch with ChangeLog

Thanks, Kinuko, Jian for comments.

eventSender.beginDragWithFiles is not implemented yet for GTK.
https://bugs.webkit.org/show_bug.cgi?id=40833

fast/files/file-reader.html works well with the trunk by manually selecting the files.

[WebKit Review Bot]

Attachment 61346 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/platform/gtk/FileSystemGtk.cpp:29:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Joone Hur]

Created attachment 61347 [details]
test case

It loads a image file from disk and display the image.

[Joone Hur]

Created attachment 61351 [details]
Fixed style error

[Jian Li]

Comment on attachment 61351 [details]
Fixed style error

Generally looks good except one minor comment:

WebCore/ChangeLog:8
 +          No new tests, it needs to include fast/files test case.
This comment is a little bit confusing. Better to say the layout test fast/files will be enabled after eventSender.beginDragWithFiles is implemented for GTK.

By the way, do you want to land your patch via commit-queue after it is approved? If yes, please set commit-queue flag to "?".

[Joone Hur]

Created attachment 61444 [details]
updated the test case comment


Thanks Jian for the review

[Jian Li]

Comment on attachment 61444 [details]
updated the test case comment

r=me

[Jeremy Orlow]

Comment on attachment 61444 [details]
updated the test case comment

The commit queue seems to have gotten stuck on this for some reason.  removing cq bit to see if that fixes it.

[WebKit Commit Bot]

Comment on attachment 61444 [details]
updated the test case comment

Clearing flags on attachment: 61444

Committed r63308: <http://trac.webkit.org/changeset/63308>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Eric Seidel (no email)]

The commit-queue clearly wasn't stuck. :)  Just was taking a while.