Bug 79928

Summary: Use the new createSnapshotFileAndReadMetadata API for FileEntry.file()
Product: WebKit Reporter: Kinuko Yasuda <kinuko>
Component: WebCore Misc.Assignee: Kinuko Yasuda <kinuko>
Status: RESOLVED FIXED    
Severity: Normal CC: fishd, jianli, levin, levin+threading, michaeln, tzik, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description Kinuko Yasuda 2012-02-29 11:18:08 PST
Use the new createSnapshotFileAndReadMetadata API for FileEntry.file()

We used to call readMetadata() to get the platform file name when we create a File object from FileEntry, but we should use new platform-specific specialized API for creating a snapshot File, i.e. createSnapshotFileAndReadMetadata, so that the platform can make more appropriate action, e.g. it may simply return the metadata as we used to, or may want to create a temporary snapshot file from the original file (so that it can make sure it gets immutable File object).
Comment 1 Kinuko Yasuda 2012-02-29 14:35:39 PST
Created attachment 129521 [details]
Patch
Comment 2 Kinuko Yasuda 2012-02-29 14:46:32 PST
Comment on attachment 129521 [details]
Patch

The cr-linux tests will fail until we submit the corresponding DRT changes.

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

> Source/WebKit/chromium/ChangeLog:13
> +        registers the created snapshot file with the given internal Blob URL.

...and in the callback chain we call File::createWithName() to create a new File by supplying the file path and the internal Blob URL as a source Blob, and then call unregisterBlobURL() to clean up the internal Blob URL.

(I'll add more description in the ChangeLog)
Comment 3 Kinuko Yasuda 2012-02-29 14:50:13 PST
Created attachment 129524 [details]
Patch

Updated the ChangeLog.
Comment 4 Michael Nordman 2012-02-29 15:39:39 PST
Comment on attachment 129524 [details]
Patch

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

> Source/WebCore/platform/AsyncFileSystem.h:144
> +    // In local filesystem cases the backend may simply return the metadata of the file itself (as well as readMetadata does), while in remote filesystem case the backend may download the file into a temporary snapshot file and return the metadata of the temporary file.

some lines may be too long even for webcore... is this one of them :)

> Source/WebKit/chromium/src/AsyncFileSystemChromium.cpp:74
> +        // Now that we've registered the snapshot file, we can unregister our internalBlobURL which has played a placeholder for the file during the IPC.

Just want to verify that in the 'worker' case, this method is executing on the background worker thread, is that right?
Comment 5 Kinuko Yasuda 2012-02-29 15:47:29 PST
Comment on attachment 129524 [details]
Patch

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

>> Source/WebCore/platform/AsyncFileSystem.h:144
>> +    // In local filesystem cases the backend may simply return the metadata of the file itself (as well as readMetadata does), while in remote filesystem case the backend may download the file into a temporary snapshot file and return the metadata of the temporary file.
> 
> some lines may be too long even for webcore... is this one of them :)

Yep will split them... at least will do so for this one.

>> Source/WebKit/chromium/src/AsyncFileSystemChromium.cpp:74
>> +        // Now that we've registered the snapshot file, we can unregister our internalBlobURL which has played a placeholder for the file during the IPC.
> 
> Just want to verify that in the 'worker' case, this method is executing on the background worker thread, is that right?

Yes, in the worker case we first bridge the request to the main thread, call createSnapshotFileAndReadMetadata WebKit API on the main thread, and are called back on the main thread, come back to the worker thread and then run this code snippet.
Comment 6 Michael Nordman 2012-02-29 15:55:01 PST
Comment on attachment 129524 [details]
Patch

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

> Source/WebCore/ChangeLog:22
> +        * platform/AsyncFileSystem.h:

If we could fully encapsulate details of the 'internalBlobUrl' within the AsyncFileSystem class, that could be an improvement.

AsyncFileSystem::createSnapshotFile(path, callbacks);
AsyncFileSystemCallbacks::didCreateSnapshotFile(file);
Comment 7 Kinuko Yasuda 2012-02-29 16:56:44 PST
Created attachment 129556 [details]
Patch
Comment 8 Kinuko Yasuda 2012-02-29 16:58:40 PST
Comment on attachment 129524 [details]
Patch

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

>> Source/WebCore/ChangeLog:22
>> +        * platform/AsyncFileSystem.h:
> 
> If we could fully encapsulate details of the 'internalBlobUrl' within the AsyncFileSystem class, that could be an improvement.
> 
> AsyncFileSystem::createSnapshotFile(path, callbacks);
> AsyncFileSystemCallbacks::didCreateSnapshotFile(file);

As we chatted offline while this sounded a good idea we cannot do that because AsyncFileSystemCallbacks is defined in platform/ and we cannot refer File there.  Then we also realized that we can probably just not using the internal Blob URL when we create a new File.

I updated the patch along the line-- now it should look simpler.
Comment 9 Michael Nordman 2012-02-29 17:25:50 PST
Comment on attachment 129556 [details]
Patch

This looks much nicer.

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

> Source/WebCore/fileapi/DOMFileSystemSync.cpp:135
> +    m_asyncFileSystem->createSnapshotFileAndReadMetadata(fileEntry->fullPath(), GetPathHelper::create(result));

The File is created at the end of this method body, has 'unregister' happened prior to that? Looks like GetPathHelper::didReadMetadata() may need to construct the File in this case.
Comment 10 Kinuko Yasuda 2012-02-29 17:38:11 PST
Created attachment 129564 [details]
Patch
Comment 11 Kinuko Yasuda 2012-02-29 17:38:44 PST
Comment on attachment 129556 [details]
Patch

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

>> Source/WebCore/fileapi/DOMFileSystemSync.cpp:135
>> +    m_asyncFileSystem->createSnapshotFileAndReadMetadata(fileEntry->fullPath(), GetPathHelper::create(result));
> 
> The File is created at the end of this method body, has 'unregister' happened prior to that? Looks like GetPathHelper::didReadMetadata() may need to construct the File in this case.

Good catch, sounds true.  I updated the patch.
Comment 12 Michael Nordman 2012-02-29 17:40:36 PST
Comment on attachment 129564 [details]
Patch

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

> Source/WebCore/fileapi/DOMFileSystemSync.cpp:146
> +    return result->m_file;

result->m_file.release()?
Comment 13 Michael Nordman 2012-02-29 17:40:57 PST
lgtm fwiw
Comment 14 WebKit Review Bot 2012-02-29 17:41:05 PST
Attachment 129564 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
Source/JavaScriptCore/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 1 in 30 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 15 Kinuko Yasuda 2012-02-29 17:42:08 PST
Comment on attachment 129564 [details]
Patch

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

>> Source/WebCore/fileapi/DOMFileSystemSync.cpp:146
>> +    return result->m_file;
> 
> result->m_file.release()?

I was about to do so but it wouldn't make a big change... well anyway will change like that when I create a next patch.
Comment 16 David Levin 2012-02-29 17:48:57 PST
Comment on attachment 129564 [details]
Patch

Thanks Michael for doing the heavy lifting :).
Comment 17 Michael Nordman 2012-02-29 18:49:55 PST
(In reply to comment #16)
> (From update of attachment 129564 [details])
> Thanks Michael for doing the heavy lifting :).

i think kinuko did the heavy lifting :)
Comment 18 WebKit Review Bot 2012-03-01 01:22:07 PST
Comment on attachment 129564 [details]
Patch

Rejecting attachment 129564 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
eeded at 298 with fuzz 1 (offset 9 lines).
Hunk #2 succeeded at 371 with fuzz 1 (offset 14 lines).
patching file Source/WebKit/chromium/src/WorkerFileSystemCallbacksBridge.h
Hunk #1 FAILED at 94.
Hunk #2 FAILED at 117.
2 out of 2 hunks FAILED -- saving rejects to file Source/WebKit/chromium/src/WorkerFileSystemCallbacksBridge.h.rej

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'David Levin']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue/

Full output: http://queues.webkit.org/results/11757310