Bug 79394 - [chromium] Add createSnapshotFile API to WebFileSystem to create File snapshots for filesystem files
Summary: [chromium] Add createSnapshotFile API to WebFileSystem to create File snapsho...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kinuko Yasuda
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-02-23 13:38 PST by Kinuko Yasuda
Modified: 2012-02-27 21:00 PST (History)
6 users (show)

See Also:


Attachments
Patch (4.67 KB, patch)
2012-02-23 13:42 PST, Kinuko Yasuda
no flags Details | Formatted Diff | Diff
Patch (5.39 KB, patch)
2012-02-23 14:00 PST, Kinuko Yasuda
no flags Details | Formatted Diff | Diff
Patch (7.06 KB, patch)
2012-02-23 14:25 PST, Kinuko Yasuda
no flags Details | Formatted Diff | Diff
Patch (7.18 KB, patch)
2012-02-23 16:31 PST, Kinuko Yasuda
no flags Details | Formatted Diff | Diff
Patch (3.50 KB, patch)
2012-02-24 18:15 PST, Kinuko Yasuda
no flags Details | Formatted Diff | Diff
Patch (3.07 KB, patch)
2012-02-27 17:03 PST, Kinuko Yasuda
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kinuko Yasuda 2012-02-23 13:38:04 PST
[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.
Comment 1 Kinuko Yasuda 2012-02-23 13:42:19 PST
Created attachment 128546 [details]
Patch
Comment 2 Kinuko Yasuda 2012-02-23 14:00:48 PST
Created attachment 128553 [details]
Patch

Fixed build errors.
Comment 3 Michael Nordman 2012-02-23 14:13:12 PST
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.
Comment 4 Kinuko Yasuda 2012-02-23 14:25:16 PST
Created attachment 128556 [details]
Patch
Comment 5 Kinuko Yasuda 2012-02-23 14:26:31 PST
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.
Comment 6 Michael Nordman 2012-02-23 14:29:19 PST
thnx, lgtm
Comment 7 Darin Fisher (:fishd, Google) 2012-02-23 16:06:33 PST
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.
Comment 8 Kinuko Yasuda 2012-02-23 16:31:23 PST
Created attachment 128595 [details]
Patch
Comment 9 Kinuko Yasuda 2012-02-23 17:12:11 PST
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.
Comment 10 Kinuko Yasuda 2012-02-23 17:27:00 PST
Comment on attachment 128595 [details]
Patch

(Obsoleting for now)
Comment 11 Kinuko Yasuda 2012-02-24 18:15:05 PST
Created attachment 128836 [details]
Patch
Comment 12 Darin Fisher (:fishd, Google) 2012-02-27 12:32:24 PST
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?
Comment 13 Kinuko Yasuda 2012-02-27 13:33:59 PST
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?
Comment 14 Michael Nordman 2012-02-27 13:55:33 PST
> 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.
Comment 15 Darin Fisher (:fishd, Google) 2012-02-27 16:03:25 PST
(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
Comment 16 Kinuko Yasuda 2012-02-27 17:03:53 PST
Created attachment 129138 [details]
Patch
Comment 17 WebKit Review Bot 2012-02-27 17:10:07 PST
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
Comment 18 WebKit Review Bot 2012-02-27 20:58:10 PST
The commit-queue encountered the following flaky tests while processing attachment 129138 [details]:

css3/filters/effect-invert-hw.html bug 79639 (author: cmarrin@apple.com)
The commit-queue is continuing to process your patch.
Comment 19 WebKit Review Bot 2012-02-27 21:00:07 PST
Comment on attachment 129138 [details]
Patch

Clearing flags on attachment: 129138

Committed r109070: <http://trac.webkit.org/changeset/109070>
Comment 20 WebKit Review Bot 2012-02-27 21:00:13 PST
All reviewed patches have been landed.  Closing bug.