Bug 79394

Created attachment 128546 [details] Patch

Created attachment 128553 [details] Patch Fixed build errors.

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.

Created attachment 128556 [details] Patch

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.

thnx, lgtm

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.

Created attachment 128595 [details] Patch

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 on attachment 128595 [details] Patch (Obsoleting for now)

Created attachment 128836 [details] Patch

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

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

(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

Created attachment 129138 [details] Patch

Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.

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 on attachment 129138 [details] Patch Clearing flags on attachment: 129138 Committed r109070: <http://trac.webkit.org/changeset/109070>

All reviewed patches have been landed. Closing bug.