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
Kinuko Yasuda
2012-02-29 11:18:08 PST
Created attachment 129521 [details]
Patch
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) Created attachment 129524 [details]
Patch
Updated the ChangeLog.
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 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 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); Created attachment 129556 [details]
Patch
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 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. Created attachment 129564 [details]
Patch
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 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()? lgtm fwiw 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 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 on attachment 129564 [details]
Patch
Thanks Michael for doing the heavy lifting :).
(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 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 |