Bug 79193 – [EFL] Implements FileSystem on EFL port

• Component: WebKit EFL
• Status: RESOLVED INVALID

[Dongwoo Joshua Im (dwim)]

Below two File API are needed to be implemented on EFL port.

* File API: Writer - http://www.w3.org/TR/file-writer-api/
* File API: Directories and System - http://dev.w3.org/2009/dap/file-system/file-dir-sys.html

I'm working on this patch now.

[Chris Dumez]

Any news on this? This bug has been open for a long time...
Someone else can take over if you don't have time to work on this.

[Dongwoo Joshua Im (dwim)]

Oh, 
I think I can upload the first patch in the week after next week.

[Chris Dumez]

(In reply to comment #2)
> Oh, 
> I think I can upload the first patch in the week after next week.

Any news on this? It's been 2 weeks. As I said, we are happy to work on this if you don't have time.

[Dongwoo Joshua Im (dwim)]

I've been long business trip. ;)
Now, I have some time to make a patch for this.

Actually, I've already implemented this feature in our local repository,
so I don't think it will take much time.

[Dongwoo Joshua Im (dwim)]

Created attachment 165121 [details]
First patch

This is my first patch to implement the File System API on EFL port.

[Chris Dumez]

Comment on attachment 165121 [details]
First patch

if you don't unskip any test case, how do we know your patch works?

[Kenneth Rohde Christiansen]

Comment on attachment 165121 [details]
First patch

View in context: https://bugs.webkit.org/attachment.cgi?id=165121&action=review

This is a huge patch backed by no tests whatsoever. Can it be split into minor patches with actual tests?

> Source/WebCore/ChangeLog:17
> +        (WebCore::AsyncFileSystem::deleteFileSystem):
> +        (WebCore::LocalFileSystem::deleteFileSystem):

No comment why you changed the RefPtr to an OwnPtr

> Source/WebCore/platform/efl/AsyncFileSystemCallbacksEfl.cpp:138
> +void AsyncFileSystemCallbacksEfl::didReadDirectoryEntries(bool hasMore)
> +{
> +    if (hasMore)
> +        ASSERT_NOT_REACHED();

Why does it have the argument then?

> Source/WebCore/platform/efl/AsyncFileSystemEfl.cpp:52
> +    storageIdentifier = storageIdentifier + ":";

+ ':'

> Source/WebCore/platform/efl/AsyncFileSystemEfl.cpp:56
> +    rootURL.append("filesystem:");

appendLiteral

> Source/WebCore/platform/efl/AsyncFileSystemEfl.cpp:60
> +    rootURL.append(typeString + "/" + identifier + "/");

'/' should be faster

[Chris Dumez]

Comment on attachment 165121 [details]
First patch

View in context: https://bugs.webkit.org/attachment.cgi?id=165121&action=review

> Source/WebCore/platform/efl/AsyncFileSystemCallbacksEfl.cpp:137
> +    if (hasMore)

ASSERT(!hasMore) ?

> Source/WebCore/platform/efl/AsyncFileSystemCallbacksEfl.cpp:141
> +    for (Vector<DirectoryEntry>::iterator it = m_directoryEntries.begin(); it != end; ++it)

Coding style says we should prefer indexes over iterators to improve readability.

> Source/WebCore/platform/efl/AsyncFileSystemCallbacksEfl.h:68
> +        DirectoryEntry() : isDirectory(false) { }

How about a constructor which sets the name and isDirectory members?

> Source/WebCore/platform/efl/AsyncFileSystemEfl.cpp:51
> +    String storageIdentifier = identifier;

Please use StringBuilder (http://trac.webkit.org/wiki/EfficientStrings)

> Source/WebCore/platform/efl/AsyncFileSystemEfl.cpp:56
> +    rootURL.append("filesystem:");

appendLiteral()

> Source/WebCore/platform/efl/AsyncFileSystemEfl.cpp:59
> +        rootURL.append("/");

append('/')

> Source/WebCore/platform/efl/AsyncFileSystemEfl.cpp:63
> +        makeAllDirectories(rootURL.toString().substring(11));

Maybe a comment to explain why the substring?

> Source/WebCore/platform/efl/AsyncFileSystemEfl.cpp:242
> +    if (removeDirectoryEntries(path))

This is not really async, we should probably use EIO at some point.

> Source/WebCore/platform/efl/AsyncFileSystemEfl.cpp:288
> +    PlatformFileHandle handle;

Why 2 lines?

> Source/WebCore/platform/efl/AsyncFileSystemEfl.cpp:434
> +    if (!virtualPath.string().startsWith("filesystem:"))

!virtualPath.protocolIs("filesystem") ?

> Source/WebCore/platform/efl/AsyncFileSystemTaskControllerEfl.cpp:39
> +    else

I would put { } since it is on several lines.

> Source/WebCore/platform/efl/AsyncFileSystemTaskControllerEfl.cpp:56
> +    else

Ditto.

> Source/WebCore/platform/efl/AsyncFileSystemTaskControllerEfl.cpp:70
> +    else

Ditto.

> Source/WebCore/platform/efl/AsyncFileSystemTaskControllerEfl.cpp:88
> +#if ENABLE(WORKERS)

Ditto.

> Source/WebCore/platform/efl/AsyncFileSystemTaskControllerEfl.h:45
> +    String uniqueMode();

const?

> Source/WebCore/platform/efl/AsyncFileSystemTaskControllerEfl.h:52
> +    FileSystemSynchronousType synchronousType() { return m_synchronousType; }

const?

> Source/WebCore/platform/efl/AsyncFileWriterEfl.cpp:46
> +    int bytesWritten;

Please define this in the scope where it is needed.

> Source/WebCore/platform/efl/AsyncFileWriterEfl.cpp:47
> +    PlatformFileHandle handle;

Why 2 lines?

> Source/WebCore/platform/efl/AsyncFileWriterEfl.cpp:52
> +        for (int i = 0; i < blobStorage->items().size(); i++) {

1. We should probably cache blobStorage->items() and blobStorage->items().size() to avoid calling them at each iteration.
2. Also, shouldn't we use size_t instead of int? 
3. Please use preincrement ++i

> Source/WebCore/platform/efl/AsyncFileWriterEfl.cpp:92
> +    PlatformFileHandle handle;

Why on 2 lines?

> Source/WebCore/platform/posix/FileSystemPOSIX.cpp:291
> +        return false;

Shouldn't you close srcFile before returning?

> Source/WebCore/platform/posix/FileSystemPOSIX.cpp:293
> +    int readSize;

Can be defined inside the loop condition

> Source/WebCore/platform/posix/FileSystemPOSIX.cpp:294
> +    int writeSize;

Can be defined inside the if condition

> Source/WebCore/platform/posix/FileSystemPOSIX.cpp:298
> +            return false;

Shouldn't you close the files before returning?

> Source/WebCore/platform/posix/FileSystemPOSIX.cpp:305
> +bool removeDirectory(const String& path)

The function to remove a file is called deleteFile(), shouldn't we call this one deleteDirectory() for consistency?

> Source/WebCore/platform/posix/FileSystemPOSIX.cpp:321
> +        String absolutePath = pathByAppendingComponent(path, String(directoryEntry->d_name));

String::fromUTF8(directoryEntry->d_name) otherwise it only works for ascii.

> Source/WebCore/platform/posix/FileSystemPOSIX.cpp:324
> +            return false;

Shouldn't you close the directory before returning?

> Source/WebCore/platform/posix/FileSystemPOSIX.cpp:333
> +            return false;

You're returning without closing the directory.

[Dongwoo Joshua Im (dwim)]

(In reply to comment #6)
> (From update of attachment 165121 [details])
> if you don't unskip any test case, how do we know your patch works?

Actually, I've run the sample tests on HTML5Rocks sites.
 - http://www.html5rocks.com/en/tutorials/file/filesystem/

And, I cannot find "Layouttest/fast/filesystem" in both Skipped and TestExpectations files.

[Dongwoo Joshua Im (dwim)]

(In reply to comment #7)
> (From update of attachment 165121 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=165121&action=review
> 
> This is a huge patch backed by no tests whatsoever. Can it be split into minor patches with actual tests?
> 

I will try to split this.

[Chris Dumez]

(In reply to comment #9)
> (In reply to comment #6)
> > (From update of attachment 165121 [details] [details])
> > if you don't unskip any test case, how do we know your patch works?
> 
> Actually, I've run the sample tests on HTML5Rocks sites.
>  - http://www.html5rocks.com/en/tutorials/file/filesystem/
> 
> And, I cannot find "Layouttest/fast/filesystem" in both Skipped and TestExpectations files.

$ git grep filesystem LayoutTests/platform/efl*
LayoutTests/platform/efl/Skipped:http/tests/security/contentSecurityPolicy/filesystem-urls-match-self.html
LayoutTests/platform/efl/TestExpectations:webkit.org/b/68591 fast/filesystem
LayoutTests/platform/efl/TestExpectations:webkit.org/b/68591 fast/mutation/filesystem-callback-delivery.html
LayoutTests/platform/efl/TestExpectations:webkit.org/b/68591 http/tests/filesystem
LayoutTests/platform/efl/TestExpectations:webkit.org/b/68591 http/tests/security/filesystem-iframe-from-remote.html
LayoutTests/platform/efl/TestExpectations:webkit.org/b/68591 http/tests/security/mixedContent/filesystem-url-in-iframe.html
LayoutTests/platform/efl/TestExpectations:webkit.org/b/68591 http/tests/inspector/filesystem

[Taiju Tsuiki]

Comment on attachment 165121 [details]
First patch

View in context: https://bugs.webkit.org/attachment.cgi?id=165121&action=review

> Source/WebCore/platform/efl/AsyncFileSystemEfl.cpp:173
> +    if (link(sourcePath.utf8().data(), destinationPath.utf8().data()) == -1)

Does it make hard link?
If we copy A to B and write something to A, should B also be overwritten?

> Source/WebCore/platform/efl/AsyncFileWriterEfl.cpp:48
> +    handle = openFile(path, OpenForWrite);

position seems not to be used. Maybe, we need seekFile here.

> Source/WebCore/platform/efl/AsyncFileWriterEfl.cpp:65
> +                if (copyFile(blobItem.path, path))

Does this write to correct position? Looks like writing from head of the file.

[Dongwoo Joshua Im (dwim)]

Wow..
Huge patch, and huge comments! ;-)

My first task might be split this patch into 2 or 3 patches.
I will apply those comments below into the new patches.
(And, for sure, I will leave 'done' comments here, also.)

[Dongwoo Joshua Im (dwim)]

(In reply to comment #7)
> (From update of attachment 165121 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=165121&action=review
> 
> This is a huge patch backed by no tests whatsoever. Can it be split into minor patches with actual tests?
> 

How about this?

To easy review, I will upload a skeleton code for the first patch, such as https://bugs.webkit.org/show_bug.cgi?id=91187 which I uploaded before.
First patch will be uploaded without enabling no new test cases.

Then, I will add some implementation as another patch with enabling test cases for that.
I will try to make the patches in proper size to review at once.

[Gyuyoung Kim]

If you follow efl patch contribution style(e.g public API with unit test, public file with installation and so on.), I prefer to split huge patch into smaller one personally.

[Dongwoo Joshua Im (dwim)]

I've upload a skeleton code at https://bugs.webkit.org/show_bug.cgi?id=91187.
Please review this patch first.

Real implementation patches will follow this patch, in proper size to easy review.

[Zan Dobersek]

(Reposting this from bug #58443)

For what it's worth, the File API: Directories and System specification[1] is being considered for a move to the Note track[2].

Translated, it means the work on this particular specification will probably cease. There are already considerations of a similar but simplified approach though[3][4].

[1] http://dev.w3.org/2009/dap/file-system/file-dir-sys.html
[2] http://lists.w3.org/Archives/Public/public-webapps/2012JulSep/0791.html
[3] http://lists.w3.org/Archives/Public/public-webapps/2012JulSep/0823.html
[4] http://lists.w3.org/Archives/Public/public-webapps/2012JulSep/0827.html

[Dongwoo Joshua Im (dwim)]

(In reply to comment #17)
> (Reposting this from bug #58443)
> 
> For what it's worth, the File API: Directories and System specification[1] is being considered for a move to the Note track[2].
> 
> Translated, it means the work on this particular specification will probably cease. There are already considerations of a similar but simplified approach though[3][4].
> 
> [1] http://dev.w3.org/2009/dap/file-system/file-dir-sys.html
> [2] http://lists.w3.org/Archives/Public/public-webapps/2012JulSep/0791.html
> [3] http://lists.w3.org/Archives/Public/public-webapps/2012JulSep/0823.html
> [4] http://lists.w3.org/Archives/Public/public-webapps/2012JulSep/0827.html

I don't think it is worthless.

Because, there are already some web applications use this File System API to read/write files.
And, there is no other way to do this kind of things until new file system spec is  announced.

So, I think still it is valuable to implement this File System API in WebKit.

What do you think?

[Zan Dobersek]

(In reply to comment #18)
> (In reply to comment #17)
> > (Reposting this from bug #58443)
> > 
> > For what it's worth, the File API: Directories and System specification[1] is being considered for a move to the Note track[2].
> > 
> > Translated, it means the work on this particular specification will probably cease. There are already considerations of a similar but simplified approach though[3][4].
> > 
> > [1] http://dev.w3.org/2009/dap/file-system/file-dir-sys.html
> > [2] http://lists.w3.org/Archives/Public/public-webapps/2012JulSep/0791.html
> > [3] http://lists.w3.org/Archives/Public/public-webapps/2012JulSep/0823.html
> > [4] http://lists.w3.org/Archives/Public/public-webapps/2012JulSep/0827.html
> 
> I don't think it is worthless.
> 
> Because, there are already some web applications use this File System API to read/write files.
> And, there is no other way to do this kind of things until new file system spec is  announced.
> 
> So, I think still it is valuable to implement this File System API in WebKit.
> 
> What do you think?

To sum up a bit longer answer that's at https://bugs.webkit.org/show_bug.cgi?id=58443#c52 - it's definitely not worthless. (Let's try to keep any further discussion of this in one thread, please - simplifies everyone's work.)

[Chris Dumez]

Any update on this? There hasn't seem to be much progress recently.

[Dongwoo Joshua Im (dwim)]

(In reply to comment #20)
> Any update on this? There hasn't seem to be much progress recently.

Ye.. little progress, but not much, though.

I don't have much time to concentrate to File System currently,
that's why this bug is still delayed.

Do you have any plan to use this feature in the near future?

[Chris Dumez]

(In reply to comment #21)
> (In reply to comment #20)
> > Any update on this? There hasn't seem to be much progress recently.
> 
> Ye.. little progress, but not much, though.
> 
> I don't have much time to concentrate to File System currently,
> that's why this bug is still delayed.
> 
> Do you have any plan to use this feature in the near future?

Well, this is an important API so we would like it upstream sooner rather than later. We were planning to work on this but since you have a patch I think it is better you upstream it first before we make contributions to this area.

Personally, I think it would be nice to use eio library for AsyncFileSystem instead of faking asynchronism. I would like to work on that.

[Kenneth Rohde Christiansen]

(In reply to comment #22)
> (In reply to comment #21)
> > (In reply to comment #20)
> > > Any update on this? There hasn't seem to be much progress recently.
> > 
> > Ye.. little progress, but not much, though.
> > 
> > I don't have much time to concentrate to File System currently,
> > that's why this bug is still delayed.
> > 
> > Do you have any plan to use this feature in the near future?
> 
> Well, this is an important API so we would like it upstream sooner rather than later. We were planning to work on this but since you have a patch I think it is better you upstream it first before we make contributions to this area.
> 
> Personally, I think it would be nice to use eio library for AsyncFileSystem instead of faking asynchronism. I would like to work on that.

The FileSystem is pretty much dead (talk to Wayne). I think that indexedDB was said to cover most of the same use cases.

[Dongwoo Joshua Im (dwim)]

(In reply to comment #23)
> The FileSystem is pretty much dead (talk to Wayne). I think that indexedDB was said to cover most of the same use cases.

We already discussed about this issue with Zan Dobersek on https://bugs.webkit.org/show_bug.cgi?id=79193#c17, 
https://bugs.webkit.org/show_bug.cgi?id=79193#c19, 
and https://bugs.webkit.org/show_bug.cgi?id=58443#c52.

It seems there are some number of proposals which can replace the File System API.

But, I still think this implementation could be valuable because there are already some web applications use this File System API to read/write files, and there is no other way to do this kind of things until new file system spec is  announced.

What do you think, Kenneth?

[Kenneth Rohde Christiansen]

(In reply to comment #24)
> (In reply to comment #23)
> > The FileSystem is pretty much dead (talk to Wayne). I think that indexedDB was said to cover most of the same use cases.
> 
> We already discussed about this issue with Zan Dobersek on https://bugs.webkit.org/show_bug.cgi?id=79193#c17, 
> https://bugs.webkit.org/show_bug.cgi?id=79193#c19, 
> and https://bugs.webkit.org/show_bug.cgi?id=58443#c52.
> 
> It seems there are some number of proposals which can replace the File System API.
> 
> But, I still think this implementation could be valuable because there are already some web applications use this File System API to read/write files, and there is no other way to do this kind of things until new file system spec is  announced.
> 
> What do you think, Kenneth?

I think that people are backing away from this, and then we should as well, or otherwise the web becomes a very fragmented place.

FileSystem is a bad name because it only allows you to create files in a sandbox, and apparently indexedDB does more or less the same - I think that was the conclusion from this years W3C TPAC meeting, but I wasn't there myself.

[Kenneth Rohde Christiansen]

More info: https://hacks.mozilla.org/2012/07/why-no-filesystem-api-in-firefox/

If apps can't migrate to indexedDB, there is a polyfill (using indexedDB) here: http://ericbidelman.tumblr.com/post/21649963613/idb-filesystem-js-bringing-the-html5-filesystem-api

[Kinuko Yasuda]

(In reply to comment #26)
> More info: https://hacks.mozilla.org/2012/07/why-no-filesystem-api-in-firefox/
> 
> If apps can't migrate to indexedDB, there is a polyfill (using indexedDB) here: http://ericbidelman.tumblr.com/post/21649963613/idb-filesystem-js-bringing-the-html5-filesystem-api

I wouldn't strongly object, but to clarify: 1) the TPAC's voting results seem to indicate the community still supports further iteration as there's some interest in a (but not the current) 'file system' API. 2) IndexedDB could cover most usage but in general it's not designed to store huge mutable data, so we should keep watching if it (the spec and possible implementation) could really cover such scenario well.