Bug 36938 – Add basic FileSystem operations for FileReader/FileWriter support for POSIX (incl. Mac)

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Kinuko Yasuda]

WebCore/platform/FileSystem.h has some platform-dependent filesystem operations, but there are missing operations (like seek, truncate and read) or unimplemented ones.   Add basic implementations for FileAPI's FileReader/FileWriter support (bug 32624, bug 36567).

[Kinuko Yasuda]

Created attachment 52269 [details]
Patch

[Kent Tamura]

Comment on attachment 52269 [details]
Patch

> diff --git a/WebCore/platform/FileSystem.h b/WebCore/platform/FileSystem.h
> index 80023d4..3b61eb9 100644
> --- a/WebCore/platform/FileSystem.h
> +++ b/WebCore/platform/FileSystem.h
> +enum FileOpenMode {
> +    OPEN_FOR_READ = 0,
> +    OPEN_FOR_WRITE
> +};
> +
> +enum FileSeekOrigin {
> +    SEEK_FROM_BEGINNING = 0,
> +    SEEK_FROM_CURRENT,
> +    SEEK_FROM_END
> +};
> +

http://webkit.org/coding/coding-style.html
10. Enum members should user InterCaps with an initial capital letter.

[Kinuko Yasuda]

Created attachment 52274 [details]
Patch

[Kinuko Yasuda]

(In reply to comment #2)
> http://webkit.org/coding/coding-style.html
> 10. Enum members should user InterCaps with an initial capital letter.

Oh... thanks for pointing it out!  Updated the patch.

[Kinuko Yasuda]

A minimal patch.  (We'll add implementations for chromium later)

[Kinuko Yasuda]

Created attachment 52460 [details]
Patch

[Jian Li]

Comment on attachment 52460 [details]
Patch

> --- a/WebCore/platform/FileSystem.h
> +++ b/WebCore/platform/FileSystem.h
> @@ -141,8 +152,12 @@ inline bool isHandleValid(const PlatformFileHandle& handle) { return handle != i
>  
>  // Prefix is what the filename should be prefixed with, not the full path.
>  WTF::CString openTemporaryFile(const char* prefix, PlatformFileHandle&);
> +PlatformFileHandle openFile(const String& path, FileOpenMode);
>  void closeFile(PlatformFileHandle&);
> +long long seekFile(PlatformFileHandle, long long offset, FileSeekOrigin);
> +bool truncateFile(PlatformFileHandle, long long offset);
>  int writeToFile(PlatformFileHandle, const char* data, int length);
> +int readFromFile(PlatformFileHandle, char* data, int length);
Better to add comment saying that return value less than 0 indicates an error.
>  
>  // Methods for dealing with loadable modules
>  bool unloadModule(PlatformModule);
> diff --git a/WebCore/platform/posix/FileSystemPOSIX.cpp b/WebCore/platform/posix/FileSystemPOSIX.cpp
> index d6804fb..57a36b8 100644
> --- a/WebCore/platform/posix/FileSystemPOSIX.cpp
> +++ b/WebCore/platform/posix/FileSystemPOSIX.cpp
> +void closeFile(PlatformFileHandle& handle)
> +{
> +    close(handle);
Since handle parameter is passed by reference, you need to set it to invalidPlatformFileHandle if closing is successful, like the following:
    if (isHandleValid(handle)) {
        close(handle);
        handle = invalidPlatformFileHandle;
    }
> +}
> +
> +long long seekFile(PlatformFileHandle handle, long long offset, FileSeekOrigin origin)
> +{
> +    int whence = SEEK_SET;
> +    switch (origin) {
> +    case SeekFromBeginning: whence = SEEK_SET; break;
Per WebKit coding style, each statement should get its own line.
> +    case SeekFromCurrent: whence = SEEK_CUR; break;
ditto.
> +    case SeekFromEnd: whence = SEEK_END; break;
ditto.
> +    default: ASSERT_NOT_REACHED();
ditto.
> +    }
> +    return static_cast<long long>(lseek(handle, offset, whence));
> +}
> +
> +bool truncateFile(PlatformFileHandle handle, long long offset)
> +{
> +    if (ftruncate(handle, offset) < 0)
> +        return false;
> +    return true;
Simpler to say:
       return ftruncate(handle, offset) == 0;
> +}
> +
> +int writeToFile(PlatformFileHandle handle, const char* data, int length)
> +{
> +    int totalBytesWritten = 0;
> +    while (totalBytesWritten < length) {
> +        int bytesWritten = HANDLE_EINTR(write(handle, data + totalBytesWritten, length - totalBytesWritten));
Using HANDLE_EINTR might not be necessary and it causes the complexity to understand the code.
How about something like the following:
           int bytesWritten = write(handle, data + totalBytesWritten, static_cast<size_t>(length - totalBytesWritten));
           if (bytesWritten < 0 && errno != EINTR)
               return -1;
In addition, better to do static_cast to size_t from int.
> +        if (bytesWritten < 0)
> +            return -1;
> +        totalBytesWritten += bytesWritten;
> +    }
> +
> +    return totalBytesWritten;
> +}
> +
> +int readFromFile(PlatformFileHandle handle, char* data, int length)
> +{
> +    int totalBytesRead = 0;
> +    while (totalBytesRead < length) {
> +        int bytesRead = HANDLE_EINTR(read(handle, data + totalBytesRead, length - totalBytesRead));
ditto.
> +        if (bytesRead <= 0)
> +            break;
> +        totalBytesRead += bytesRead;
> +    }
> +
> +    return totalBytesRead;
> +}
> +
>  bool deleteEmptyDirectory(const String& path)
>  {
>      CString fsRep = fileSystemRepresentation(path);

[Kinuko Yasuda]

Created attachment 52666 [details]
Patch

[Jian Li]

Comment on attachment 52666 [details]
Patch

> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> index fe8d9a4..dfe25d7 100644
> --- a/WebCore/ChangeLog
> +++ b/WebCore/ChangeLog
> @@ -1,3 +1,22 @@
> [phone]  Kinuko Yasuda  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Add basic FileSystem operations for FileReader/FileWriter support
> +        https://bugs.webkit.org/show_bug.cgi?id=36938
> +
> +        No new tests; tests will be added after we implement upper layers.
> +
> +        * platform/FileSystem.h:
> +        (WebCore::):
> +        * platform/posix/FileSystemPOSIX.cpp:
> +        (WebCore::openFile):
> +        (WebCore::closeFile):
> +        (WebCore::seekFile):
> +        (WebCore::truncateFile):
> +        (WebCore::writeToFile):
> +        (WebCore::readFromFile):
> +
>  2010-04-02  Laszlo Gombos  <[e-mail]>
>  
>          Unreviewed build fix when building --no-svg.
> diff --git a/WebCore/platform/FileSystem.h b/WebCore/platform/FileSystem.h
> index 80023d4..663458f 100644
> --- a/WebCore/platform/FileSystem.h
> +++ b/WebCore/platform/FileSystem.h
> @@ -26,7 +26,7 @@
>   * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF
>   * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>   */
> - 
> +
>  #ifndef FileSystem_h
>  #define FileSystem_h
>  
> @@ -122,6 +122,17 @@ typedef int PlatformFileHandle;
>  const PlatformFileHandle invalidPlatformFileHandle = -1;
>  #endif
>  
> +enum FileOpenMode {
> +    OpenForRead = 0,
> +    OpenForWrite
> +};
> +
> +enum FileSeekOrigin {
> +    SeekFromBeginning = 0,
> +    SeekFromCurrent,
> +    SeekFromEnd
> +};
> +
>  bool fileExists(const String&);
>  bool deleteFile(const String&);
>  bool deleteEmptyDirectory(const String&);
> @@ -141,8 +152,15 @@ inline bool isHandleValid(const PlatformFileHandle& handle) { return handle != i
>  
>  // Prefix is what the filename should be prefixed with, not the full path.
>  WTF::CString openTemporaryFile(const char* prefix, PlatformFileHandle&);
> +PlatformFileHandle openFile(const String& path, FileOpenMode);
>  void closeFile(PlatformFileHandle&);
> +// Returns seeked offset if successful, -1 otherwise.
> +long long seekFile(PlatformFileHandle, long long offset, FileSeekOrigin);
> +bool truncateFile(PlatformFileHandle, long long offset);
> +// Returns number of bytes actually read if successful, -1 otherwise.
>  int writeToFile(PlatformFileHandle, const char* data, int length);
> +// Returns number of bytes actually written if successful, -1 otherwise.
> +int readFromFile(PlatformFileHandle, char* data, int length);
>  
>  // Methods for dealing with loadable modules
>  bool unloadModule(PlatformModule);
> diff --git a/WebCore/platform/posix/FileSystemPOSIX.cpp b/WebCore/platform/posix/FileSystemPOSIX.cpp
> index d6804fb..5427948 100644
> --- a/WebCore/platform/posix/FileSystemPOSIX.cpp
> +++ b/WebCore/platform/posix/FileSystemPOSIX.cpp
> @@ -30,11 +30,11 @@
>  #include "FileSystem.h"
>  
>  #include "PlatformString.h"
> -#include <wtf/text/CString.h>
> -
> -#include <sys/stat.h>
> +#include <errno.h>
>  #include <libgen.h>
> +#include <sys/stat.h>
>  #include <unistd.h>
> +#include <wtf/text/CString.h>
>  
>  namespace WebCore {
>  
> @@ -65,6 +65,77 @@ bool deleteFile(const String& path)
>      return !unlink(fsRep.data());
>  }
>  
> +PlatformFileHandle openFile(const String& path, FileOpenMode mode)
> +{
> +    int platformFlag = 0;
> +    if (mode == OpenForRead)
> +        platformFlag |= O_RDONLY;
> +    else if (mode == OpenForWrite)
> +        platformFlag |= (O_WRONLY | O_CREAT | O_TRUNC);
> +    return open(path.utf8().data(), platformFlag, 0666);
> +}
> +
> +void closeFile(PlatformFileHandle& handle)
> +{
> +    if (isHandleValid(handle)) {
> +        close(handle);
> +        handle = invalidPlatformFileHandle;
> +    }
> +}
> +
> +long long seekFile(PlatformFileHandle handle, long long offset, FileSeekOrigin origin)
> +{
> +    int whence = SEEK_SET;
> +    switch (origin) {
> +    case SeekFromBeginning:
> +        whence = SEEK_SET;
> +        break;
> +    case SeekFromCurrent:
> +        whence = SEEK_CUR;
> +        break;
> +    case SeekFromEnd:
> +        whence = SEEK_END;
> +        break;
> +    default:
> +        ASSERT_NOT_REACHED();
> +    }
> +    return static_cast<long long>(lseek(handle, offset, whence));
> +}
> +
> +bool truncateFile(PlatformFileHandle handle, long long offset)
Why do we need to introduce truncateFile? I am not seeing any reference in your FileWriter patch.
> +{
> +    // A return value 0 indicates the call succeeds.
The comment is a little bit confusing because it might also mean the return value of truncateFile. How about something like: 
       // ftruncate returns 0 to indicates the success.
> +    return !ftruncate(handle, offset);
> +}
> +
> +int writeToFile(PlatformFileHandle handle, const char* data, int length)
> +{
> +    int totalBytesWritten = 0;
> +    while (totalBytesWritten < length) {
> +        int bytesWritten = write(handle, data + totalBytesWritten, static_cast<size_t>(length - totalBytesWritten));
> +        if (bytesWritten < 0 && errno != EINTR)
> +            return -1;
> +        if (errno != EINTR)
It would be safe to say:
           if (bytesWritten > 0)
> +            totalBytesWritten += bytesWritten;
> +    }
> +
> +    return totalBytesWritten;
> +}
> +
> +int readFromFile(PlatformFileHandle handle, char* data, int length)
> +{
> +    int totalBytesRead = 0;
> +    while (totalBytesRead < length) {
> +        int bytesRead = read(handle, data + totalBytesRead, static_cast<size_t>(length - totalBytesRead));
> +        if (bytesRead <= 0 && errno != EINTR)
> +            break;
> +        if (errno != EINTR)
ditto.
> +            totalBytesRead += bytesRead;
> +    }
> +
> +    return totalBytesRead;
> +}
> +
>  bool deleteEmptyDirectory(const String& path)
>  {
>      CString fsRep = fileSystemRepresentation(path);

[Kinuko Yasuda]

Created attachment 52673 [details]
Patch

[Kinuko Yasuda]

Thanks for reviewing very quickly.

(In reply to comment #9)
> (From update of attachment 52666 [details])
> > +bool truncateFile(PlatformFileHandle handle, long long offset)
> Why do we need to introduce truncateFile? I am not seeing any reference in your
> FileWriter patch.

I haven't implemented in my private patch yet but FileWriter spec has
truncate() API and I anticipate I will need that.

> > +int writeToFile(PlatformFileHandle handle, const char* data, int length)
> > +{
> > +    int totalBytesWritten = 0;
> > +    while (totalBytesWritten < length) {
> > +        int bytesWritten = write(handle, data + totalBytesWritten, static_cast<size_t>(length - totalBytesWritten));
> > +        if (bytesWritten < 0 && errno != EINTR)
> > +            return -1;
> > +        if (errno != EINTR)
> It would be safe to say:
>            if (bytesWritten > 0)
> > +            totalBytesWritten += bytesWritten;

Good suggestion, fixed it (here and in readFrmoFile).

[Jian Li]

Comment on attachment 52673 [details]
Patch

Noticed another problem in read file logic plus a couple minor comment suggestions. Almost there.

> +// Returns seeked offset if successful, -1 otherwise.
Probably need to rephrase a little bit, like:
    // Returns the resulting offset from the beginning of the file if successful, -1 otherwise.
> +long long seekFile(PlatformFileHandle, long long offset, FileSeekOrigin);

> +bool truncateFile(PlatformFileHandle handle, long long offset)
> +{
> +    // ftruncate return 0 to indicate the success.
r/return/returns
> +    return !ftruncate(handle, offset);
> +}
> +

> +int readFromFile(PlatformFileHandle handle, char* data, int length)
> +{
> +    int totalBytesRead = 0;
> +    while (totalBytesRead < length) {
> +        int bytesRead = read(handle, data + totalBytesRead, static_cast<size_t>(length - totalBytesRead));
> +        if (bytesRead <= 0 && errno != EINTR)
> +            break;
If we encounter non-EINTR error and break out of the loop, we will return totalBytesRead that is not -1.
In addition, we should not continue the reading if bytesRead >= 0.
I think the block can be reorganized like the following:
           if (bytesRead >= 0) {
               totalBytesRead += bytesRead;
               break;
           }
           if (errno != EINTR)
               return -1;

writeToFile could also do the similar thing for better readerability.

> +        if (bytesRead > 0)
> +            totalBytesRead += bytesRead;
> +    }
> +
> +    return totalBytesRead;
> +}

[Kinuko Yasuda]

Created attachment 52687 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 2
Last 3072 characters of output:
s
  any such category.  The filter input "-whitespace,+whitespace/braces" fails
  the category "whitespace/tab" and passes "whitespace/braces".

  Examples: --filter=-whitespace,+whitespace/braces
            --filter=-whitespace,-runtime/printf,+runtime/printf_format
            --filter=-,+build/include_what_you_use

Paths:
  Certain style-checking behavior depends on the paths relative to
  the WebKit source root of the files being checked.  For example,
  certain types of errors may be handled differently for files in
  WebKit/gtk/webkit/ (e.g. by suppressing "readability/naming" errors
  for files in this directory).

  Consequently, if the path relative to the source root cannot be
  determined for a file being checked, then style checking may not
  work correctly for that file.  This can occur, for example, if no
  WebKit checkout can be found, or if the source root can be detected,
  but one of the files being checked lies outside the source tree.

  If a WebKit checkout can be detected and all files being checked
  are in the source tree, then all paths will automatically be
  converted to paths relative to the source root prior to checking.
  This is also useful for display purposes.

  Currently, this command can detect the source root only if the
  command is run from within a WebKit checkout (i.e. if the current
  working directory is below the root of a checkout).  In particular,
  it is not recommended to run this script from a directory outside
  a checkout.

  Running this script from a top-level WebKit source directory and
  checking only files in the source tree will ensure that all style
  checking behaves correctly -- whether or not a checkout can be
  detected.  This is because all file paths will already be relative
  to the source root and so will not need to be converted.

Options:
  -h, --help            show this help message and exit
  -f RULES, --filter-rules=RULES
                        set a filter to control what categories of style errors
                        to report.  Specify a filter using a comma-delimited
                        list of boolean filter rules, for example "--filter
                        -whitespace,+whitespace/braces".  To display all
                        categories and which are enabled by default, pass no
                        value (e.g. '-f ""' or '--filter=').
  -g COMMIT, --git-commit=COMMIT, --git-diff=COMMIT, --git-since=COMMIT
                        check all changes after the given git commit.
  -m INT, --min-confidence=INT
                        set the minimum confidence of style errors to report.
                        Can be an integer 1-5, with 1 displaying all errors.
                        Defaults to 1.
  -o FORMAT, --output-format=FORMAT
                        set the output format, which can be "emacs" or "vs7"
                        (for Visual Studio).  Defaults to "emacs".
  -v, --verbose         enable verbose logging.

This script can miss errors and does not substitute for code review.

ERROR: no such option: --no-squash


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

[Jian Li]

Comment on attachment 52687 [details]
Patch

You can land it given that the ChangeLog and bug title need to be updated to reflect that the FileSystem implementations are just for POSIX.

r=me

[Kinuko Yasuda]

Created attachment 52688 [details]
Patch

[Kinuko Yasuda]

(In reply to comment #15)
> (From update of attachment 52687 [details])
> You can land it given that the ChangeLog and bug title need to be updated to
> reflect that the FileSystem implementations are just for POSIX.

Thanks, made one more update for ChangeLog.

[Jian Li]

Comment on attachment 52688 [details]
Patch

ChangeLog change looks good.

r=me

[WebKit Commit Bot]

Comment on attachment 52688 [details]
Patch

Clearing flags on attachment: 52688

Committed r57182: <http://trac.webkit.org/changeset/57182>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.