Allow FileSystem API implementation to pass snapshot metadata at File creation time
authorkinuko@chromium.org <kinuko@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 17 May 2012 12:07:55 +0000 (12:07 +0000)
committerkinuko@chromium.org <kinuko@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 17 May 2012 12:07:55 +0000 (12:07 +0000)
https://bugs.webkit.org/show_bug.cgi?id=78879

Reviewed by Jian Li.

Source/WebCore:

We query File metadata (e.g. size and modifiedTime) when File.size,
lastModifiedTime or webkitSlice() is accessed / called, but in some
platform-specific filesystems it may not be feasible since synchronous
metadata query could take very long time.

This patch adds new File constructor which takes metadata argument
to allow each FileSystem API implementation to pass snapshot metadata
so that File object could cache the given metadata not to make
synchronous query.

We only call this constructor if the filesystem type is neither
Temporary nor Persistent, therefore this patch should not affect
existing code behavior.

Test: fast/filesystem/file-read-after-write.html

* Modules/filesystem/DOMFileSystem.cpp:
(WebCore::DOMFileSystem::createFile):
* Modules/filesystem/DOMFileSystemSync.cpp:
* fileapi/Blob.cpp:
(WebCore::Blob::webkitSlice): Updated implementation.
* fileapi/Blob.h:
* fileapi/File.cpp:
(WebCore::File::File): Added new constructor.
(WebCore::File::lastModifiedDate): Updated implementation.
(WebCore::File::size): Updated implementation.
(WebCore::File::captureSnapshot): Updated implementation.
* fileapi/File.h:
(WebCore::File::createForFileSystemFile): Added.
* platform/AsyncFileSystem.h:
(AsyncFileSystem): Updated comment.

LayoutTests:

Added tests for making sure metadata is not cached in the regular
temporary filesystem.

* fast/filesystem/file-metadata-after-write-expected.txt: Added.
* fast/filesystem/file-metadata-after-write.html: Added.

git-svn-id: http://svn.webkit.org/repository/webkit/trunk@117432 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/fast/filesystem/file-metadata-after-write-expected.txt [new file with mode: 0644]
LayoutTests/fast/filesystem/file-metadata-after-write.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/Modules/filesystem/DOMFileSystem.cpp
Source/WebCore/Modules/filesystem/DOMFileSystemSync.cpp
Source/WebCore/fileapi/File.cpp
Source/WebCore/fileapi/File.h
Source/WebCore/platform/AsyncFileSystem.h

index 559f65f..eb03d20 100644 (file)
@@ -1,3 +1,16 @@
+2012-05-11  Kinuko Yasuda  <kinuko@chromium.org>
+
+        Allow FileSystem API implementation to pass snapshot metadata at File creation time
+        https://bugs.webkit.org/show_bug.cgi?id=78879
+
+        Reviewed by Jian Li.
+
+        Added tests for making sure metadata is not cached in the regular
+        temporary filesystem.
+
+        * fast/filesystem/file-metadata-after-write-expected.txt: Added.
+        * fast/filesystem/file-metadata-after-write.html: Added.
+
 2012-05-17  Kinuko Yasuda  <kinuko@chromium.org>
 
         Unreviewed, updating chrome test expectations.
diff --git a/LayoutTests/fast/filesystem/file-metadata-after-write-expected.txt b/LayoutTests/fast/filesystem/file-metadata-after-write-expected.txt
new file mode 100644 (file)
index 0000000..d6c591a
--- /dev/null
@@ -0,0 +1,10 @@
+This verifies File.size (for a file from FileSystem API) always returns the fresh size even after the file is modified.
+Writing 1234567890 to the file...
+Created a writer.
+Write succeeded.
+PASS testFile.size is testText1.length
+Writing abcdefghijklmnopqrstuvwxyz to the file...
+Created a writer.
+Write succeeded.
+PASS testFile.size is testText2.length
+
diff --git a/LayoutTests/fast/filesystem/file-metadata-after-write.html b/LayoutTests/fast/filesystem/file-metadata-after-write.html
new file mode 100644 (file)
index 0000000..94e52d0
--- /dev/null
@@ -0,0 +1,90 @@
+<!DOCTYPE html>
+<html>
+<head>
+<script src="../js/resources/js-test-pre.js"></script>
+</head>
+<body>
+<div>This verifies File.size (for a file from FileSystem API) always returns the fresh size even after the file is modified.</div>
+<div id='console'></div>
+<script>
+
+if (window.layoutTestController) {
+  layoutTestController.dumpAsText();
+  layoutTestController.waitUntilDone();
+}
+
+var fileEntryForCleanup;
+var testFile;
+var testText1 = '1234567890';
+var testText2 = 'abcdefghijklmnopqrstuvwxyz';
+
+webkitRequestFileSystem(
+    window.TEMPORARY, 100, function(fs) {
+        fs.root.getFile('test', {create:true}, function(entry) {
+            fileEntryForCleanup = entry;
+            writeTextToFile(entry, testText1, function() {
+                log('Write succeeded.');
+                entry.file(onWrittenFile.bind(this, entry), onError);
+            });
+        }, onError);
+    }, onError);
+
+function onWrittenFile(entry, file) {
+    testFile = file;
+    shouldBe('testFile.size', 'testText1.length');
+    // Write into this file again.
+    writeTextToFile(entry, testText2, function() {
+        log('Write succeeded.');
+        entry.file(function() {
+            // The file size should be updated.
+            shouldBe('testFile.size', 'testText2.length');
+            cleanup();
+        }, onError);
+    });
+}
+
+function writeTextToFile(entry, contents, successCallback)
+{
+    log('Writing ' + contents + ' to the file...');
+    entry.createWriter(function(writer) {
+        log('Created a writer.');
+        var builder = new WebKitBlobBuilder();
+        builder.append(contents);
+        writer.write(builder.getBlob('plain/text'));
+        writer.onwriteend = successCallback;
+        writer.onerror = onError;
+    });
+}
+
+function log(text)
+{
+    var console = document.getElementById('console');
+    console.appendChild(document.createTextNode(text));
+    console.appendChild(document.createElement('br'));
+}
+
+function onError(e)
+{
+    log('ERROR: ' + e.code);
+    console.log(e);
+    cleanup();
+}
+
+function cleanup()
+{
+    if (fileEntryForCleanup) {
+        fileEntryForCleanup.remove(endTest, endTest);
+        return;
+    }
+    endTest();
+}
+
+function endTest()
+{
+    if (layoutTestController)
+        layoutTestController.notifyDone();
+}
+
+</script>
+</body>
+</html>
index 1db7894..c94486e 100644 (file)
@@ -1,3 +1,42 @@
+2012-05-11  Kinuko Yasuda  <kinuko@chromium.org>
+
+        Allow FileSystem API implementation to pass snapshot metadata at File creation time
+        https://bugs.webkit.org/show_bug.cgi?id=78879
+
+        Reviewed by Jian Li.
+
+        We query File metadata (e.g. size and modifiedTime) when File.size,
+        lastModifiedTime or webkitSlice() is accessed / called, but in some
+        platform-specific filesystems it may not be feasible since synchronous
+        metadata query could take very long time.
+
+        This patch adds new File constructor which takes metadata argument
+        to allow each FileSystem API implementation to pass snapshot metadata
+        so that File object could cache the given metadata not to make
+        synchronous query.
+
+        We only call this constructor if the filesystem type is neither
+        Temporary nor Persistent, therefore this patch should not affect
+        existing code behavior.
+
+        Test: fast/filesystem/file-read-after-write.html
+
+        * Modules/filesystem/DOMFileSystem.cpp:
+        (WebCore::DOMFileSystem::createFile):
+        * Modules/filesystem/DOMFileSystemSync.cpp:
+        * fileapi/Blob.cpp:
+        (WebCore::Blob::webkitSlice): Updated implementation.
+        * fileapi/Blob.h:
+        * fileapi/File.cpp:
+        (WebCore::File::File): Added new constructor.
+        (WebCore::File::lastModifiedDate): Updated implementation.
+        (WebCore::File::size): Updated implementation.
+        (WebCore::File::captureSnapshot): Updated implementation.
+        * fileapi/File.h:
+        (WebCore::File::createForFileSystemFile): Added.
+        * platform/AsyncFileSystem.h:
+        (AsyncFileSystem): Updated comment.
+
 2012-05-17  Gyuyoung Kim  <gyuyoung.kim@samsung.com>
 
         Convert setDomainRelaxationForbiddenForURLScheme to use InternalSettings interface
index 090bcc0..7bb0a67 100644 (file)
@@ -123,11 +123,11 @@ void DOMFileSystem::createWriter(const FileEntry* fileEntry, PassRefPtr<FileWrit
 
 namespace {
 
-class GetPathCallback : public FileSystemCallbacksBase {
+class GetMetadataCallback : public FileSystemCallbacksBase {
 public:
-    static PassOwnPtr<GetPathCallback> create(PassRefPtr<DOMFileSystem> filesystem, const String& name, PassRefPtr<FileCallback> successCallback, PassRefPtr<ErrorCallback> errorCallback)
+    static PassOwnPtr<GetMetadataCallback> create(PassRefPtr<DOMFileSystem> filesystem, const String& name, PassRefPtr<FileCallback> successCallback, PassRefPtr<ErrorCallback> errorCallback)
     {
-        return adoptPtr(new GetPathCallback(filesystem, name, successCallback, errorCallback));
+        return adoptPtr(new GetMetadataCallback(filesystem, name, successCallback, errorCallback));
     }
 
     virtual void didReadMetadata(const FileMetadata& metadata)
@@ -136,12 +136,18 @@ public:
         if (!m_successCallback)
             return;
 
-        m_successCallback->handleEvent(File::createWithName(metadata.platformPath, m_name).get());
+        // For regular filesystem types (temporary or persistent), we should not cache file metadata as it could change File semantics.
+        // For other filesystem types (which could be platform-specific ones), there's a chance that the files are on remote filesystem. If the port has returned metadata just pass it to File constructor (so we may cache the metadata).
+        if (m_filesystem->type() == FileSystemTypeTemporary || m_filesystem->type() == FileSystemTypePersistent)
+            m_successCallback->handleEvent(File::createWithName(metadata.platformPath, m_name).get());
+        else
+            m_successCallback->handleEvent(File::createForFileSystemFile(m_name, metadata).get());
+
         m_successCallback.release();
     }
 
 private:
-    GetPathCallback(PassRefPtr<DOMFileSystem> filesystem, const String& name,  PassRefPtr<FileCallback> successCallback, PassRefPtr<ErrorCallback> errorCallback)
+    GetMetadataCallback(PassRefPtr<DOMFileSystem> filesystem, const String& name,  PassRefPtr<FileCallback> successCallback, PassRefPtr<ErrorCallback> errorCallback)
         : FileSystemCallbacksBase(errorCallback)
         , m_filesystem(filesystem)
         , m_name(name)
@@ -158,7 +164,7 @@ private:
 
 void DOMFileSystem::createFile(const FileEntry* fileEntry, PassRefPtr<FileCallback> successCallback, PassRefPtr<ErrorCallback> errorCallback)
 {
-    m_asyncFileSystem->createSnapshotFileAndReadMetadata(createFileSystemURL(fileEntry), GetPathCallback::create(this, fileEntry->name(), successCallback, errorCallback));
+    m_asyncFileSystem->createSnapshotFileAndReadMetadata(createFileSystemURL(fileEntry), GetMetadataCallback::create(this, fileEntry->name(), successCallback, errorCallback));
 }
 
 } // namespace WebCore
index 8a57d00..bc36cae 100644 (file)
@@ -98,9 +98,9 @@ public:
         friend class WTF::RefCounted<CreateFileResult>;
     };
 
-    static PassOwnPtr<CreateFileHelper> create(PassRefPtr<CreateFileResult> result, const String& name)
+    static PassOwnPtr<CreateFileHelper> create(PassRefPtr<CreateFileResult> result, const String& name, FileSystemType type)
     {
-        return adoptPtr(new CreateFileHelper(result, name));
+        return adoptPtr(new CreateFileHelper(result, name, type));
     }
 
     virtual void didFail(int code)
@@ -115,17 +115,24 @@ public:
 
     void didReadMetadata(const FileMetadata& metadata)
     {
-        m_result->m_file = File::createWithName(metadata.platformPath, m_name);
+        // For regular filesystem types (temporary or persistent), we should not cache file metadata as it could change File semantics.
+        // For other filesystem types (which could be platform-specific ones), there's a chance that the files are on remote filesystem. If the port has returned metadata just pass it to File constructor (so we may cache the metadata).
+        if (m_type == FileSystemTypeTemporary || m_type == FileSystemTypePersistent)
+            m_result->m_file = File::createWithName(metadata.platformPath, m_name);
+        else
+            m_result->m_file = File::createForFileSystemFile(m_name, metadata);
     }
 private:
-    CreateFileHelper(PassRefPtr<CreateFileResult> result, const String& name)
+    CreateFileHelper(PassRefPtr<CreateFileResult> result, const String& name, FileSystemType type)
         : m_result(result)
         , m_name(name)
+        , m_type(type)
     {
     }
 
     RefPtr<CreateFileResult> m_result;
     String m_name;
+    FileSystemType m_type;
 };
 
 } // namespace
@@ -134,7 +141,7 @@ PassRefPtr<File> DOMFileSystemSync::createFile(const FileEntrySync* fileEntry, E
 {
     ec = 0;
     RefPtr<CreateFileHelper::CreateFileResult> result(CreateFileHelper::CreateFileResult::create());
-    m_asyncFileSystem->createSnapshotFileAndReadMetadata(createFileSystemURL(fileEntry), CreateFileHelper::create(result, fileEntry->name()));
+    m_asyncFileSystem->createSnapshotFileAndReadMetadata(createFileSystemURL(fileEntry), CreateFileHelper::create(result, fileEntry->name(), type()));
     if (!m_asyncFileSystem->waitForOperationToComplete()) {
         ec = FileException::ABORT_ERR;
         return 0;
index 905291b..f2afa34 100644 (file)
@@ -26,6 +26,7 @@
 #include "config.h"
 #include "File.h"
 
+#include "FileMetadata.h"
 #include "FileSystem.h"
 #include "MIMETypeRegistry.h"
 #include <wtf/CurrentTime.h>
 
 namespace WebCore {
 
+static String getContentTypeFromFileName(const String& name)
+{
+    String type;
+    int index = name.reverseFind('.');
+    if (index != -1)
+        type = MIMETypeRegistry::getWellKnownMIMETypeForExtension(name.substring(index + 1));
+    return type;
+}
+
 static PassOwnPtr<BlobData> createBlobDataForFileWithType(const String& path, const String& contentType)
 {
     OwnPtr<BlobData> blobData = BlobData::create();
@@ -43,22 +53,24 @@ static PassOwnPtr<BlobData> createBlobDataForFileWithType(const String& path, co
 
 static PassOwnPtr<BlobData> createBlobDataForFile(const String& path)
 {
-    String type;
-    int index = path.reverseFind('.');
-    if (index != -1)
-        type = MIMETypeRegistry::getMIMETypeForExtension(path.substring(index + 1));
-    return createBlobDataForFileWithType(path, type);
+    return createBlobDataForFileWithType(path, getContentTypeFromFileName(path));
 }
 
 static PassOwnPtr<BlobData> createBlobDataForFileWithName(const String& path, const String& fileSystemName)
 {
-    String type;
-    int index = fileSystemName.reverseFind('.');
-    if (index != -1)
-        type = MIMETypeRegistry::getWellKnownMIMETypeForExtension(fileSystemName.substring(index + 1));
-    return createBlobDataForFileWithType(path, type);
+    return createBlobDataForFileWithType(path, getContentTypeFromFileName(fileSystemName));
 }
 
+#if ENABLE(FILE_SYSTEM)
+static PassOwnPtr<BlobData> createBlobDataForFileWithMetadata(const String& fileSystemName, const FileMetadata& metadata)
+{
+    OwnPtr<BlobData> blobData = BlobData::create();
+    blobData->setContentType(getContentTypeFromFileName(fileSystemName));
+    blobData->appendFile(metadata.platformPath, 0, metadata.length, metadata.modificationTime);
+    return blobData.release();
+}
+#endif
+
 #if ENABLE(DIRECTORY_UPLOAD)
 PassRefPtr<File> File::createWithRelativePath(const String& path, const String& relativePath)
 {
@@ -72,12 +84,20 @@ File::File(const String& path)
     : Blob(createBlobDataForFile(path), -1)
     , m_path(path)
     , m_name(pathGetFileName(path))
+#if ENABLE(FILE_SYSTEM)
+    , m_snapshotSize(-1)
+    , m_snapshotModificationTime(0)
+#endif
 {
 }
 
 File::File(const String& path, const KURL& url, const String& type)
     : Blob(url, type, -1)
     , m_path(path)
+#if ENABLE(FILE_SYSTEM)
+    , m_snapshotSize(-1)
+    , m_snapshotModificationTime(0)
+#endif
 {
     m_name = pathGetFileName(path);
     // FIXME: File object serialization/deserialization does not include
@@ -89,11 +109,31 @@ File::File(const String& path, const String& name)
     : Blob(createBlobDataForFileWithName(path, name), -1)
     , m_path(path)
     , m_name(name)
+#if ENABLE(FILE_SYSTEM)
+    , m_snapshotSize(-1)
+    , m_snapshotModificationTime(0)
+#endif
+{
+}
+
+#if ENABLE(FILE_SYSTEM)
+File::File(const String& name, const FileMetadata& metadata)
+    : Blob(createBlobDataForFileWithMetadata(name, metadata), metadata.length)
+    , m_path(metadata.platformPath)
+    , m_name(name)
+    , m_snapshotSize(metadata.length)
+    , m_snapshotModificationTime(metadata.modificationTime)
 {
 }
+#endif
 
 double File::lastModifiedDate() const
 {
+#if ENABLE(FILE_SYSTEM)
+    if (m_snapshotSize >= 0 && m_snapshotModificationTime)
+        return m_snapshotModificationTime * 1000.0;
+#endif
+
     time_t modificationTime;
     if (!getFileModificationTime(m_path, modificationTime))
         return 0;
@@ -104,6 +144,11 @@ double File::lastModifiedDate() const
 
 unsigned long long File::size() const
 {
+#if ENABLE(FILE_SYSTEM)
+    if (m_snapshotSize >= 0 && m_snapshotModificationTime)
+        return m_snapshotSize;
+#endif
+
     // FIXME: JavaScript cannot represent sizes as large as unsigned long long, we need to
     // come up with an exception to throw if file size is not representable.
     long long size;
@@ -114,6 +159,14 @@ unsigned long long File::size() const
 
 void File::captureSnapshot(long long& snapshotSize, double& snapshotModificationTime) const
 {
+#if ENABLE(FILE_SYSTEM)
+    if (m_snapshotSize >= 0 && m_snapshotModificationTime) {
+        snapshotSize = m_snapshotSize;
+        snapshotModificationTime = m_snapshotModificationTime;
+        return;
+    }
+#endif
+
     // Obtains a snapshot of the file by capturing its current size and modification time. This is used when we slice a file for the first time.
     // If we fail to retrieve the size or modification time, probably due to that the file has been deleted, 0 size is returned.
     // FIXME: Combine getFileSize and getFileModificationTime into one file system call.
index 8375671..fa0b223 100644 (file)
@@ -33,6 +33,7 @@
 
 namespace WebCore {
 
+class FileMetadata;
 class KURL;
 
 class File : public Blob {
@@ -52,6 +53,16 @@ public:
     static PassRefPtr<File> createWithRelativePath(const String& path, const String& relativePath);
 #endif
 
+#if ENABLE(FILE_SYSTEM)
+    // If filesystem files live in the remote filesystem, the port might pass the valid metadata (non-zero modificationTime and non-negative file size) and cache in the File object.
+    //
+    // Otherwise calling size(), lastModifiedTime() and webkitSlice() will synchronously query the file metadata.
+    static PassRefPtr<File> createForFileSystemFile(const String& name, const FileMetadata& metadata)
+    {
+        return adoptRef(new File(name, metadata));
+    }
+#endif
+
     // Create a file with a name exposed to the author (via File.name and associated DOM properties) that differs from the one provided in the path.
     static PassRefPtr<File> createWithName(const String& path, const String& name)
     {
@@ -81,8 +92,20 @@ private:
     File(const String& path, const KURL& srcURL, const String& type);
     File(const String& path, const String& name);
 
+# if ENABLE(FILE_SYSTEM)
+    File(const String& name, const FileMetadata&);
+#endif
+
     String m_path;
     String m_name;
+
+#if ENABLE(FILE_SYSTEM)
+    // If non-zero modificationTime and non-negative file size are given at construction time we use them in size(), lastModifiedTime() and webkitSlice().
+    // By default we initialize m_snapshotSize to -1 and m_snapshotModificationTime to 0 (so that we don't use them unless they are given).
+    const long long m_snapshotSize;
+    const double m_snapshotModificationTime;
+#endif
+
 #if ENABLE(DIRECTORY_UPLOAD)
     String m_relativePath;
 #endif
index 6e53a9a..c33d6a0 100644 (file)
@@ -127,6 +127,8 @@ public:
     // while in remote filesystem case the backend may download the file into a temporary snapshot file and return the metadata of the temporary file.
     // AsyncFileSystemCallbacks::didReadMetadata() is called when the metadata for the snapshot file is successfully returned.
     // AsyncFileSystemCallbacks::didFail() is called otherwise.
+    //
+    // Note: the returned metadata info is cached in the File object for non-regular filesystem types (neither Temporary nor Persistent). The port could return valid metadata if it wants File object to cache metadata (e.g. if the file body is on a remote server), but otherwise should NOT return valid metadata.
     virtual void createSnapshotFileAndReadMetadata(const KURL& path, PassOwnPtr<AsyncFileSystemCallbacks>) = 0;
 
 protected: