[webkit-reviews] review requested: [Bug 79193] [EFL] Implements FileSystem on EFL port : [Attachment 165121] First patch

[bugzilla-daemon at webkit.org]

Christophe Dumez <christophe.dumez at intel.com> has asked  for review:
Bug 79193: [EFL] Implements FileSystem on EFL port
https://bugs.webkit.org/show_bug.cgi?id=79193

Attachment 165121: First patch
https://bugs.webkit.org/attachment.cgi?id=165121&action=review

------- Additional Comments from Christophe Dumez <christophe.dumez at intel.com>
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.