Bug 79394 – [chromium] Add createSnapshotFile API to WebFileSystem to create File snapshots for filesystem files

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Kinuko Yasuda]

[chromium] Add GetSnapshotFileInfo API to WebFileSystem to get file snapshot info for filesystem files.

File object is considered a snapshot of the underlying file and for files served by FileSystem API we need an async API to capture snapshot file info for a given file.  We used to use GetFileInfo() for the purpose but since we want to start explicitly capture the snapshot info for remote files we need a separate explicit API for that.

[Kinuko Yasuda]

Created attachment 128546 [details]
Patch

[Kinuko Yasuda]

Created attachment 128553 [details]
Patch

Fixed build errors.

[Michael Nordman]

Comment on attachment 128553 [details]
Patch

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

> Source/WebCore/platform/AsyncFileSystem.h:143
> +    // Gets the snapshot info for a given file at |path|.

Can you comment on how this method is different than readMetadata(path, callbacks) which also completes with a callback to didReadmetadata()? I'm guessing that readMetadata() will not fill the resulting FileMetadata.platformPath value in all cases but getSnapshotFileInfo() will.

[Kinuko Yasuda]

Created attachment 128556 [details]
Patch

[Kinuko Yasuda]

Comment on attachment 128553 [details]
Patch

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

>> Source/WebCore/platform/AsyncFileSystem.h:143
>> +    // Gets the snapshot info for a given file at |path|.
> 
> Can you comment on how this method is different than readMetadata(path, callbacks) which also completes with a callback to didReadmetadata()? I'm guessing that readMetadata() will not fill the resulting FileMetadata.platformPath value in all cases but getSnapshotFileInfo() will.

Good point, done.

[Michael Nordman]

thnx, lgtm

[Darin Fisher (:fishd, Google)]

Comment on attachment 128556 [details]
Patch

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

> Source/WebKit/chromium/public/platform/WebFileSystem.h:124
> +    virtual void getSnapshotFileInfo(const WebURL& path, WebFileSystemCallbacks*) { WEBKIT_ASSERT_NOT_REACHED(); }

I think you should spell out the differences between this and getMetadata more clearly.

It almost feels like getFile in the remote filesystem case, should just coin a blob.  By reusing the blob system, we should be able to automagically solve the lifetime of the temporary file issues.

[Kinuko Yasuda]

Created attachment 128595 [details]
Patch

[Kinuko Yasuda]

Comment on attachment 128556 [details]
Patch

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

>> Source/WebKit/chromium/public/platform/WebFileSystem.h:124
>> +    virtual void getSnapshotFileInfo(const WebURL& path, WebFileSystemCallbacks*) { WEBKIT_ASSERT_NOT_REACHED(); }
> 
> I think you should spell out the differences between this and getMetadata more clearly.
> 
> It almost feels like getFile in the remote filesystem case, should just coin a blob.  By reusing the blob system, we should be able to automagically solve the lifetime of the temporary file issues.

Added more specific comments.

And yes it looks most likely that we can just use the blob system (since File is blob) to release the temporary file when we're done with it.

[Kinuko Yasuda]

Comment on attachment 128595 [details]
Patch

(Obsoleting for now)

[Kinuko Yasuda]

Created attachment 128836 [details]
Patch

[Darin Fisher (:fishd, Google)]

Comment on attachment 128836 [details]
Patch

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

> Source/WebKit/chromium/public/platform/WebFileSystem.h:125
> +    virtual void createSnapshotFile(const WebURL& blobURL, const WebURL& path, WebFileSystemCallbacks*) { WEBKIT_ASSERT_NOT_REACHED(); }

nit: perhaps this should be called createSnapshotFileAndReadMetadata.  that way it is clearer that the result you are looking for is metadata.

> Source/WebKit/chromium/public/platform/WebFileSystem.h:127
> +    // Called when the snapshot created by createSnapshotFile is successfully registered as a File. The implementation could use this method to unregister the |blobURL| (since at this point the File must have another (official) reference). This is done asynchronously and will not call any callbacks.

I'm a little confused here...

Does this happen in response to didReadMetadata?  It is not enough to call WebBlobRegister::unregisterBlobURL?

[Kinuko Yasuda]

Comment on attachment 128836 [details]
Patch

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

>> Source/WebKit/chromium/public/platform/WebFileSystem.h:125
>> +    virtual void createSnapshotFile(const WebURL& blobURL, const WebURL& path, WebFileSystemCallbacks*) { WEBKIT_ASSERT_NOT_REACHED(); }
> 
> nit: perhaps this should be called createSnapshotFileAndReadMetadata.  that way it is clearer that the result you are looking for is metadata.

Sounds better, I'll do so.

>> Source/WebKit/chromium/public/platform/WebFileSystem.h:127
>> +    // Called when the snapshot created by createSnapshotFile is successfully registered as a File. The implementation could use this method to unregister the |blobURL| (since at this point the File must have another (official) reference). This is done asynchronously and will not call any callbacks.
> 
> I'm a little confused here...
> 
> Does this happen in response to didReadMetadata?  It is not enough to call WebBlobRegister::unregisterBlobURL?

I was planning to have the internal blob URL reference in the FileSystem backend in the browser (i.e. FileSystemDispatcherHost) while regular blob references are kept in BlobMessageFilter (note: all other blob registration code is centralized in BlobStorageController and we use it for this snapshot case too).  That's why I wanted to add this another IPC/API.

...but Michael has just suggested that probably we can just combine FileSystemDispatcherHost and BlobMessageFilter and naming it FileAndBlobMessageFilter or something.  Then we can just get rid of this extra API.  WDYT?

[Michael Nordman]

> I was planning to have the internal blob URL reference in the FileSystem backend in the browser (i.e. FileSystemDispatcherHost) while regular blob references are kept in BlobMessageFilter (note: all other blob registration code is centralized in BlobStorageController and we use it for this snapshot case too).  That's why I wanted to add this another IPC/API.
> 
> ...but Michael has just suggested that probably we can just combine FileSystemDispatcherHost and BlobMessageFilter and naming it FileAndBlobMessageFilter or something.  Then we can just get rid of this extra API.  WDYT?

Just looking more closely at BlobMessageFilter and FileSystemDispatcherHost sources. Let's merge those classes together. The extra API is an artifact of those things not being properly integrated in the backend. The BlobMessageFilter is just mechanical plumbing, there are a small number of IPC messages to dispatch into the 'controller'. The FileSystemDispatcherHost is similarly mechanical (which plumbs IPCs into FileSystemOperations), although it handles a larger set of messages.

Ultimately, i think it makes pretty good sense to merge the webkit/blob and webkit/fileapi libraries. Merging the ipc filter classes in advance of that is fine by me.

[Darin Fisher (:fishd, Google)]

(In reply to comment #14)
> > I was planning to have the internal blob URL reference in the FileSystem backend in the browser (i.e. FileSystemDispatcherHost) while regular blob references are kept in BlobMessageFilter (note: all other blob registration code is centralized in BlobStorageController and we use it for this snapshot case too).  That's why I wanted to add this another IPC/API.
> > 
> > ...but Michael has just suggested that probably we can just combine FileSystemDispatcherHost and BlobMessageFilter and naming it FileAndBlobMessageFilter or something.  Then we can just get rid of this extra API.  WDYT?
> 
> Just looking more closely at BlobMessageFilter and FileSystemDispatcherHost sources. Let's merge those classes together. The extra API is an artifact of those things not being properly integrated in the backend. The BlobMessageFilter is just mechanical plumbing, there are a small number of IPC messages to dispatch into the 'controller'. The FileSystemDispatcherHost is similarly mechanical (which plumbs IPCs into FileSystemOperations), although it handles a larger set of messages.
> 
> Ultimately, i think it makes pretty good sense to merge the webkit/blob and webkit/fileapi libraries. Merging the ipc filter classes in advance of that is fine by me.

SGTM

[Kinuko Yasuda]

Created attachment 129138 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[WebKit Review Bot]

The commit-queue encountered the following flaky tests while processing attachment 129138 [details]:

css3/filters/effect-invert-hw.html bug 79639 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Review Bot]

Comment on attachment 129138 [details]
Patch

Clearing flags on attachment: 129138

Committed r109070: <http://trac.webkit.org/changeset/109070>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.