Bug 44361

Created attachment 67692 [details] Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=67692&action=prettypatch > WebCore/fileapi/DOMFileSystem.cpp:209 > + if (!DOMFilePath::isValidPath(file->fullPath())) { In this context the path is from a valid Entry... do we need to check the path? We may have entries that have restricted names/chars if they're copied from outside the system and obtained by DirectoryReader... do you mean we want to reject creating FileWriter for those files? > WebKit/chromium/src/AsyncFileSystemChromium.cpp:37 > #include "FileSystem.h" (It's from my code but I don't think we need this... would you mind deleting this line?) > WebKit/chromium/src/AsyncFileSystemChromium.cpp:38 > +#include "WebFileError.h" nits: this is included in WebFileSystemCallbacks.h > WebKit/chromium/src/AsyncFileSystemChromium.cpp:41 > +#include "WebFileSystemCallbacks.h" nits: this is included in WebFileSystemCallbacksImpl.h > WebKit/chromium/src/AsyncFileSystemChromium.cpp:145 > + OwnPtr<WebKit::WebFileWriter> webFileWriter = adoptPtr(m_webFileSystem->createFileWriter(path, asyncFileWriterChromium.get())); I'm kinda curious why you chose to create FileWriters in the calling path but not in the callback path. We 'initialize' the writers later in the callback path - is there a reason you did that? > WebKit/chromium/src/AsyncFileWriterChromium.h:54 > + void setWebFileWriter(PassOwnPtr<WebKit::WebFileWriter> writer); Will we call this other than in creation time? Can't it be a constructor argument?

(I think dumi's also going to extend WebFileInfo... +dumi) View in context: https://bugs.webkit.org/attachment.cgi?id=67692&action=prettypatch > WebKit/chromium/public/WebFileInfo.h:45 > + bool isFile; Do we need both isFile and isDirectory here? (Is there any chance we need to return non-regular files in the API?)

(In reply to comment #2) > View in context: https://bugs.webkit.org/attachment.cgi?id=67692&action=prettypatch > > > WebCore/fileapi/DOMFileSystem.cpp:209 > > + if (!DOMFilePath::isValidPath(file->fullPath())) { > In this context the path is from a valid Entry... do we need to check the path? > We may have entries that have restricted names/chars if they're copied from outside the system and obtained by DirectoryReader... do you mean we want to reject creating FileWriter for those files? No, that's a bug--thanks. Fixed. > > WebKit/chromium/src/AsyncFileSystemChromium.cpp:37 > > #include "FileSystem.h" > (It's from my code but I don't think we need this... would you mind deleting this line?) Done. > > WebKit/chromium/src/AsyncFileSystemChromium.cpp:38 > > +#include "WebFileError.h" > nits: this is included in WebFileSystemCallbacks.h Removed. > > WebKit/chromium/src/AsyncFileSystemChromium.cpp:41 > > +#include "WebFileSystemCallbacks.h" > nits: this is included in WebFileSystemCallbacksImpl.h Removed. > > WebKit/chromium/src/AsyncFileSystemChromium.cpp:145 > > + OwnPtr<WebKit::WebFileWriter> webFileWriter = adoptPtr(m_webFileSystem->createFileWriter(path, asyncFileWriterChromium.get())); > I'm kinda curious why you chose to create FileWriters in the calling path but not in the callback path. > We 'initialize' the writers later in the callback path - is there a reason you did that? I was on the fence about where to do what. The choice was between passing more arguments to the callbacks object and doing more work earlier, before a potential failure. Since you brought it up too, I reworked it to go the other way, so now it's maximally lazy. > > WebKit/chromium/src/AsyncFileWriterChromium.h:54 > > + void setWebFileWriter(PassOwnPtr<WebKit::WebFileWriter> writer); > Will we call this other than in creation time? Can't it be a constructor argument? It can't; the WebFileWriter and the AsyncFileWriterChromium each want to point at the other. One has to get created first, and in this case it's the AsyncFileWriterChromium.

(In reply to comment #3) > (I think dumi's also going to extend WebFileInfo... +dumi) > > View in context: https://bugs.webkit.org/attachment.cgi?id=67692&action=prettypatch > > > WebKit/chromium/public/WebFileInfo.h:45 > > + bool isFile; > Do we need both isFile and isDirectory here? (Is there any chance we need to return non-regular files in the API?) Dumi and I just discussed this offline. I'd like to keep it open just in case we ever have to deal with symlinks, pipes, or other special filesystem types. We should at least be able to report that something isn't a file or directory. However, we've decided that it might be better to use an enum than a list of booleans, so I'll change to that and then post a new patch shortly.

Created attachment 67751 [details] Patch

Comment on attachment 67692 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=67692&action=prettypatch > WebKit/chromium/src/AsyncFileSystemChromium.cpp:113 > + virtual void didSucceed() { ASSERT(0); delete this; } I think there's an ASSERT_NOT_REACHED() macro in webcore, would that fit better? > WebKit/chromium/src/AsyncFileSystemChromium.cpp:128 > + virtual void didCreateFileWriter(WebKit::WebFileWriter* writer) { ASSERT(0); delete this; } Under what circumstance is this particular WebFileSystemCallbacks method used? Do we need it in the interface? I would have expected it to be used to perform this function, but apparently its not is why i ask. > WebKit/chromium/src/AsyncFileWriterChromium.h:45 > +namespace WebCore { thnx for switching namespaces here!

(In reply to comment #7) > (From update of attachment 67692 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=67692&action=prettypatch > > > WebKit/chromium/src/AsyncFileSystemChromium.cpp:113 > > + virtual void didSucceed() { ASSERT(0); delete this; } > I think there's an ASSERT_NOT_REACHED() macro in webcore, would that fit better? Ah, yes. Fixed. > > WebKit/chromium/src/AsyncFileSystemChromium.cpp:128 > > + virtual void didCreateFileWriter(WebKit::WebFileWriter* writer) { ASSERT(0); delete this; } > Under what circumstance is this particular WebFileSystemCallbacks method used? Do we need it in the interface? I would have expected it to be used to perform this function, but apparently its not is why i ask. Ha--I removed it from the interface when it turned out not to be needed, but forgot to take out the dummy implementation. > > WebKit/chromium/src/AsyncFileWriterChromium.h:45 > > +namespace WebCore { > thnx for switching namespaces here!

Created attachment 67755 [details] Patch

(In reply to comment #4) > (In reply to comment #2) > > > WebKit/chromium/src/AsyncFileSystemChromium.cpp:145 > > > + OwnPtr<WebKit::WebFileWriter> webFileWriter = adoptPtr(m_webFileSystem->createFileWriter(path, asyncFileWriterChromium.get())); > > I'm kinda curious why you chose to create FileWriters in the calling path but not in the callback path. > > We 'initialize' the writers later in the callback path - is there a reason you did that? > > I was on the fence about where to do what. The choice was between passing more arguments to the callbacks object and doing more work earlier, before a potential failure. Since you brought it up too, I reworked it to go the other way, so now it's maximally lazy. Oh thanks for changing. Btw as for m_webFileSystem in the helper callback technically you can always get it by calling WebKit::WebKitClient()->fileSystem() in WebKit/chromium. Also if we do so it might be good to delay creating the FileWriter too. - FileWriterCallbacks holds scriptExecutionContext - FileWriterHelperCallbacks holds path and FileWriterCallbacks - in FileWriterHelperCallbacks' callback, instantiate AsyncFileWriter (without client) and WebFileWriter - in FileWriterCallbacks' callback, instantiate FileWriter with scriptExecutionContext, AsyncFileWriter and length (and set the AsyncFileWriter's client in FileWriter's constructor). Not sure worth doing this... I'll leave it to you. > > > WebKit/chromium/src/AsyncFileWriterChromium.h:54 > > > + void setWebFileWriter(PassOwnPtr<WebKit::WebFileWriter> writer); > > Will we call this other than in creation time? Can't it be a constructor argument? > > It can't; the WebFileWriter and the AsyncFileWriterChromium each want to point at the other. One has to get created first, and in this case it's the AsyncFileWriterChromium. Oh I see. AsyncFileWriter is WebFileWriter's client and AsyncFileWriter needs to ask WebFileWriter for operations. By the way 'FileWriterClient' actually means AsyncFileWriter's client right? If I'm correct can we rename it to AsyncFileWriterClient? (FileWriter is also FileWriterClient.. it confused me a bit.)

(In reply to comment #10) > (In reply to comment #4) > > (In reply to comment #2) > > > > WebKit/chromium/src/AsyncFileSystemChromium.cpp:145 > > > > + OwnPtr<WebKit::WebFileWriter> webFileWriter = adoptPtr(m_webFileSystem->createFileWriter(path, asyncFileWriterChromium.get())); > > > I'm kinda curious why you chose to create FileWriters in the calling path but not in the callback path. > > > We 'initialize' the writers later in the callback path - is there a reason you did that? > > > > I was on the fence about where to do what. The choice was between passing more arguments to the callbacks object and doing more work earlier, before a potential failure. Since you brought it up too, I reworked it to go the other way, so now it's maximally lazy. > > Oh thanks for changing. > > Btw as for m_webFileSystem in the helper callback technically you can always get it by calling WebKit::WebKitClient()->fileSystem() in WebKit/chromium. Is it better to do it that way? I was following the lead of AsyncFileSystemChromium; does it just hold a pointer for performance? > Also if we do so it might be good to delay creating the FileWriter too. > > - FileWriterCallbacks holds scriptExecutionContext > - FileWriterHelperCallbacks holds path and FileWriterCallbacks > - in FileWriterHelperCallbacks' callback, instantiate AsyncFileWriter (without client) and WebFileWriter > - in FileWriterCallbacks' callback, instantiate FileWriter with scriptExecutionContext, AsyncFileWriter and length (and set the AsyncFileWriter's client in FileWriter's constructor). > > Not sure worth doing this... I'll leave it to you. I could certainly do it that way if anyone has a preference. If I do, does the FileWriterCallbacks need a RefPtr to the ScriptExecutionContext? > > > > WebKit/chromium/src/AsyncFileWriterChromium.h:54 > > > > + void setWebFileWriter(PassOwnPtr<WebKit::WebFileWriter> writer); > > > Will we call this other than in creation time? Can't it be a constructor argument? > > > > It can't; the WebFileWriter and the AsyncFileWriterChromium each want to point at the other. One has to get created first, and in this case it's the AsyncFileWriterChromium. > > Oh I see. AsyncFileWriter is WebFileWriter's client and AsyncFileWriter needs to ask WebFileWriter for operations. > > By the way 'FileWriterClient' actually means AsyncFileWriter's client right? If I'm correct can we rename it to AsyncFileWriterClient? (FileWriter is also FileWriterClient.. it confused me a bit.) Where are you looking? You want to rename the interface FileWriterClient? Hmm...let me think about it, but I think I see what you're saying.

Eric, can you please also add the accessedTime and createdTime fields (or something similar) to WebFileInfo? We need those fields for pepper, and they could probably be useful for the FileSystem API in the future too.

> > Btw as for m_webFileSystem in the helper callback technically you can always get it by calling WebKit::WebKitClient()->fileSystem() in WebKit/chromium. > Is it better to do it that way? I was following the lead of AsyncFileSystemChromium; does it just hold a pointer for performance? Never mind, it'll be better to keep the pointer. (I was looking for the point where we could reduce the number of passing parameters) > > Also if we do so it might be good to delay creating the FileWriter too. > > > I could certainly do it that way if anyone has a preference. > If I do, does the FileWriterCallbacks need a RefPtr to the ScriptExecutionContext? It needs to do so if we do, I think. (Well we may not like keeping scriptExecutionContext) > > By the way 'FileWriterClient' actually means AsyncFileWriter's client right? If I'm correct can we rename it to AsyncFileWriterClient? (FileWriter is also FileWriterClient.. it confused me a bit.) > > Where are you looking? You want to rename the interface FileWriterClient? Hmm...let me think about it, but I think I see what you're saying. Yes that's what I was trying to say. It's a bit off-topic from this patch and this patch itself lgtm.

(In reply to comment #12) > Eric, can you please also add the accessedTime and createdTime fields (or something similar) to WebFileInfo? We need those fields for pepper, and they could probably be useful for the FileSystem API in the future too. I'm happy to add them if Pepper's going to need them and is going to use this struct [I haven't looked at where that hooks in]. But ctime and atime are unlikely to go into the FileSystem API any time soon, as they're not consistent or dependable from platform to platform.

(In reply to comment #14) > (In reply to comment #12) > > Eric, can you please also add the accessedTime and createdTime fields (or something similar) to WebFileInfo? We need those fields for pepper, and they could probably be useful for the FileSystem API in the future too. > > I'm happy to add them if Pepper's going to need them and is going to use this struct [I haven't looked at where that hooks in]. But ctime and atime are unlikely to go into the FileSystem API any time soon, as they're not consistent or dependable from platform to platform. pepper has a file system component, that is quite similar to the Web FileSystem API, so the idea is to reuse as much of the code in chromium/webkit API as possible, including this structure.

Comment on attachment 67755 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=67755&action=prettypatch a few style comments. > WebCore/fileapi/FileSystemCallbacks.cpp:97 > +void FileSystemCallbacksBase::didCreateFileWriter(PassOwnPtr<AsyncFileWriter>, long long) i think we should have a name for the 'long long' argument, since it's not obvious from the type what its purpose is. > WebKit/chromium/src/AsyncFileSystemChromium.cpp:110 > + , m_callbacks(callbacks) {} not 100% sure, but i think '{' and '}' should each go on a new line, even for an empty constructor. > WebKit/chromium/src/AsyncFileSystemChromium.cpp:128 > + virtual void didOpenFileSystem(const WebKit::WebString& name, const WebKit::WebString& rootPath) { ASSERT_NOT_REACHED(); delete this; } again, not 100% sure what coding style we use in chromium's webkit API, but i don't think it's ok to put 2 instructions on the same line (i'm looking at WebDatabase.cpp).

(In reply to comment #16) > (From update of attachment 67755 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=67755&action=prettypatch > > a few style comments. > > > WebCore/fileapi/FileSystemCallbacks.cpp:97 > > +void FileSystemCallbacksBase::didCreateFileWriter(PassOwnPtr<AsyncFileWriter>, long long) > > i think we should have a name for the 'long long' argument, since it's not obvious from the type what its purpose is. The name appears in the header file and in the implementation. It's omitted in this dummy function to avoid unused variable warnings. > > WebKit/chromium/src/AsyncFileSystemChromium.cpp:110 > > + , m_callbacks(callbacks) {} > > not 100% sure, but i think '{' and '}' should each go on a new line, even for an empty constructor. Fixed. > > WebKit/chromium/src/AsyncFileSystemChromium.cpp:128 > > + virtual void didOpenFileSystem(const WebKit::WebString& name, const WebKit::WebString& rootPath) { ASSERT_NOT_REACHED(); delete this; } > > again, not 100% sure what coding style we use in chromium's webkit API, but i don't think it's ok to put 2 instructions on the same line (i'm looking at WebDatabase.cpp). Fixed.

(In reply to comment #15) > (In reply to comment #14) > > (In reply to comment #12) > > > Eric, can you please also add the accessedTime and createdTime fields (or something similar) to WebFileInfo? We need those fields for pepper, and they could probably be useful for the FileSystem API in the future too. > > > > I'm happy to add them if Pepper's going to need them and is going to use this struct [I haven't looked at where that hooks in]. But ctime and atime are unlikely to go into the FileSystem API any time soon, as they're not consistent or dependable from platform to platform. > > pepper has a file system component, that is quite similar to the Web FileSystem API, so the idea is to reuse as much of the code in chromium/webkit API as possible, including this structure. As per offline discussion, we'll leave those fields out until we're sure what we need.

(In reply to comment #13) > > > Btw as for m_webFileSystem in the helper callback technically you can always get it by calling WebKit::WebKitClient()->fileSystem() in WebKit/chromium. > > Is it better to do it that way? I was following the lead of AsyncFileSystemChromium; does it just hold a pointer for performance? > > Never mind, it'll be better to keep the pointer. (I was looking for the point where we could reduce the number of passing parameters) > > > > Also if we do so it might be good to delay creating the FileWriter too. > > > > > I could certainly do it that way if anyone has a preference. > > If I do, does the FileWriterCallbacks need a RefPtr to the ScriptExecutionContext? > > It needs to do so if we do, I think. (Well we may not like keeping scriptExecutionContext) I've just looked at that a bit, and it doesn't seem to have a big clear win. It just complicates things slightly differently and inverts the construction order a little. If there are no objections, I'm going to leave it as-is for now. > > > By the way 'FileWriterClient' actually means AsyncFileWriter's client right? If I'm correct can we rename it to AsyncFileWriterClient? (FileWriter is also FileWriterClient.. it confused me a bit.) > > > > Where are you looking? You want to rename the interface FileWriterClient? Hmm...let me think about it, but I think I see what you're saying. > > Yes that's what I was trying to say. It's a bit off-topic from this patch and this patch itself lgtm. I think you're right--it makes more sense as AsyncFileWriterClient. But I'd like to do it in a separate patch. I'll post my style fixes shortly.

Created attachment 67844 [details] Patch

Comment on attachment 67844 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=67844&action=prettypatch > WebKit/chromium/src/AsyncFileSystemChromium.cpp:122 > + didFail(WebKit::WebFileErrorInvalidState); Can we put a comment something like "didFail() deletes 'this'", or put 'ASSERT(m_callbacks)' and 'delete this' outside the if/else and replace this line with m_callbacks->didFail(error)? Other parts I commented lgtm.

Created attachment 67991 [details] Patch

Comment on attachment 67991 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=67991&action=review > WebKit/chromium/public/WebFileInfo.h:45 > + enum EntryType { nit: EntryType -> Type since this is already scoped by WebFileInfo. Or, change the enum values to be prefixed with EntryType*.

Comment on attachment 67991 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=67991&action=review r=me, based on kinuko's review. please address all pending comments before submitting. > WebKit/chromium/ChangeLog:10 > + This makes a chain of contruction-and-linking-up for various sorts of s/contruction/construction/.

Created attachment 68393 [details] Patch

(In reply to comment #24) > (From update of attachment 67991 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=67991&action=review > > r=me, based on kinuko's review. please address all pending comments before submitting. > > > WebKit/chromium/ChangeLog:10 > > + This makes a chain of contruction-and-linking-up for various sorts of > > s/contruction/construction/. I've changed EntryType to Type and fixed the typo in the ChangeLog.

Hold on--there may have been a merge conflict. Investigating.

Created attachment 68404 [details] Patch

(In reply to comment #27) > Hold on--there may have been a merge conflict. Investigating. It wasn't a merge conflict, just a trivial bug [didn't fill in the proper error in a call in AsyncFileSystemChromium.cpp]; not sure how I let it get in there. Anyway, it's fixed now. Dumi, would you CQ+? I don't have commit.

Will r+/cq+ as soon as the chromium EWS bot is done with this patch.

Comment on attachment 68404 [details] Patch Clearing flags on attachment: 68404 Committed r68101: <http://trac.webkit.org/changeset/68101>

All reviewed patches have been landed. Closing bug.