Bug 79928 – Use the new createSnapshotFileAndReadMetadata API for FileEntry.file()

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Kinuko Yasuda]

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).

[Kinuko Yasuda]

Created attachment 129521 [details]
Patch

[Kinuko Yasuda]

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)

[Kinuko Yasuda]

Created attachment 129524 [details]
Patch

Updated the ChangeLog.

[Michael Nordman]

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?

[Kinuko Yasuda]

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.

[Michael Nordman]

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);

[Kinuko Yasuda]

Created attachment 129556 [details]
Patch

[Kinuko Yasuda]

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.

[Michael Nordman]

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.

[Kinuko Yasuda]

Created attachment 129564 [details]
Patch

[Kinuko Yasuda]

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.

[Michael Nordman]

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()?

[Michael Nordman]

lgtm fwiw

[WebKit Review Bot]

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.

[Kinuko Yasuda]

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.

[David Levin]

Comment on attachment 129564 [details]
Patch

Thanks Michael for doing the heavy lifting :).

[Michael Nordman]

(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 :)

[WebKit Review Bot]

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