Bug 46405

Created attachment 68597 [details] Patch

Comment on attachment 68597 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=68597&action=review > WebCore/bindings/js/JSDirectoryEntrySyncCustom.cpp:59 > + JSValue jsCreate = object->get(exec, Identifier(exec, "create")); This call re-enters JavaScript and can do arbitrary things. How do we know that |object| hasn't been deallocated? What about |imp|? > WebCore/bindings/v8/custom/V8DirectoryEntrySyncCustom.cpp:75 > + } else { > + EXCEPTION_BLOCK(Flags*, tmp_flags, V8Flags::HasInstance(args[1]) ? V8Flags::toNative(v8::Handle<v8::Object>::Cast(args[1])) : 0); > + flags = tmp_flags; > + } Bad indent.

It seems that at this point some test should be possible. If nothing else, some tests for invalid values and verifying that appropriate erros are given.

Created attachment 69366 [details] Patch

(In reply to comment #2) > (From update of attachment 68597 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=68597&action=review > > > WebCore/bindings/js/JSDirectoryEntrySyncCustom.cpp:59 > > + JSValue jsCreate = object->get(exec, Identifier(exec, "create")); > > This call re-enters JavaScript and can do arbitrary things. How do we know that |object| hasn't been deallocated? What about |imp|? Per webkit-dev's feedback, they must be referenced by the current executing code and shouldn't be deallocated while they are being executed. > > WebCore/bindings/v8/custom/V8DirectoryEntrySyncCustom.cpp:75 > > + } else { > > + EXCEPTION_BLOCK(Flags*, tmp_flags, V8Flags::HasInstance(args[1]) ? V8Flags::toNative(v8::Handle<v8::Object>::Cast(args[1])) : 0); > > + flags = tmp_flags; > > + } > > Bad indent. Fixed. (In reply to comment #3) > It seems that at this point some test should be possible. If nothing else, some tests for invalid values and verifying that appropriate erros are given. Added basic tests that test requestFileSystem and requestFileSystemSync in workers (and in non-workers too for the former). The patch size has increased some... I hope it'd be ok.

Created attachment 69370 [details] Patch Updated ChangeLog according and rebased.

Created attachment 69371 [details] Patch Cleaned up unnecessary headers.

Created attachment 69393 [details] Patch Updated test expectations for simple-{temporary-persistent}-sync as the entry method is not yet exposed in this patch. For now they should contain reference errors for the method (requestFileSystemSync).

Created attachment 69547 [details] Patch

Created attachment 69567 [details] Patch Did some more cleanup.

Comment on attachment 69567 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=69567&action=review > WebCore/bindings/js/JSDirectoryEntrySyncCustom.cpp:89 > + flags = toFlags(exec->argument(1)); Up to here it is the same as the previous function. Please consider a common function to do this stuff. This appears to be missing: if (exec->hadException()) return jsUndefined(); here. > WebCore/bindings/v8/custom/V8DirectoryEntrySyncCustom.cpp:56 > + STRING_TO_V8PARAMETER_EXCEPTION_BLOCK(V8Parameter<>, path, args[0]); indent. > WebCore/bindings/v8/custom/V8DirectoryEntrySyncCustom.cpp:109 > + } Duplicate code. consider a common function. > WebCore/fileapi/DOMFileSystemSync.cpp:43 > + return adoptRef(new DOMFileSystemSync(fileSystem->m_name, fileSystem->m_asyncFileSystem.leakPtr())); This shouldn't be a leakPtr(). This should be a releasePtr. leakPtr is only use when you want the OwnPtr to stop holding onto to the point and you want the raw pointer. You will manage the memory in some other way. Here you are passing the point to a PassOwnPtr, so releasePtr is perfect. > WebCore/fileapi/DOMFileSystemSync.cpp:60 > +} // namespace These // namespace comments aren't necessary in WebKit as far as I know. However, if you are going to include it, make it correct :). > WebCore/fileapi/DOMFileSystemSync.h:51 > + // This call is destructive; the argument fileSystem will become unusable. This sounds scary. It makes me wonder why it doesn't take a PassOwnPtr for fileSystem. If fileSystem is unusable afterward. is there a way to help callers to do the right thing and not use it again? > WebCore/fileapi/DOMFileSystemSync.h:62 > +} // namespace Ditto. > WebCore/fileapi/DirectoryEntrySync.h:61 > + DirectoryEntrySync(DOMFileSystemBase* fileSystem, const String& fullPath); Please remove variable names which add no information. fileSystem > WebCore/fileapi/DirectoryReader.h:60 > + DirectoryReader(DOMFileSystemBase* fileSystem, const String& fullPath); Please remove variable names which add no information. fileSystem > WebCore/fileapi/DirectoryReaderBase.h:46 > + void setHasMore(bool hasMore) { m_hasMore = hasMore; } What does "hasMore" mean? has more what? (My initial reaction is cowbell since that is the current meme.) > WebCore/fileapi/DirectoryReaderSync.h:54 > + DirectoryReaderSync(DOMFileSystemBase* fileSystem, const String& fullPath); fileSystem param name. > WebCore/fileapi/EntryArraySync.h:53 > + void set(unsigned index, PassRefPtr<EntrySync> entry); Please remove variable names which add no information. entry > WebCore/fileapi/EntrySync.h:59 > + EntrySync(DOMFileSystemBase* fileSystem, const String& fullPath); Please remove variable names which add no information. fileSystem > WebCore/fileapi/FileEntrySync.h:54 > + FileEntrySync(DOMFileSystemBase* fileSystem, const String& fullPath); Please remove variable names which add no information. fileSystem > WebCore/fileapi/FileEntrySync.idl:40 > + // File file() raises (FileException); Please remove commented out code.

Comment on attachment 69567 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=69567&action=review Drive by comments > WebCore/bindings/v8/custom/V8DirectoryEntrySyncCustom.cpp:72 > + EXCEPTION_BLOCK(Flags*, tmp_flags, V8Flags::HasInstance(args[1]) ? V8Flags::toNative(v8::Handle<v8::Object>::Cast(args[1])) : 0); I don't think you need to wrap this code into EXCEPTION_BLOCK as there is nothing that can throw an exception (unless I miss something) >> WebCore/bindings/v8/custom/V8DirectoryEntrySyncCustom.cpp:109 >> + } > > Duplicate code. consider a common function. As another thought: why you need that to be custom? Maybe you should just extend code generator?

Created attachment 69771 [details] Patch

(In reply to comment #11) > (From update of attachment 69567 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=69567&action=review > > > WebCore/bindings/js/JSDirectoryEntrySyncCustom.cpp:89 > > + flags = toFlags(exec->argument(1)); > > Up to here it is the same as the previous function. Please consider a common function to do this stuff. Fixed. (Introduced a static getFlags function) > This appears to be missing: > if (exec->hadException()) > return jsUndefined(); > > here. Fixed. > > WebCore/bindings/v8/custom/V8DirectoryEntrySyncCustom.cpp:56 > > + STRING_TO_V8PARAMETER_EXCEPTION_BLOCK(V8Parameter<>, path, args[0]); > > indent. Fixed. > > WebCore/bindings/v8/custom/V8DirectoryEntrySyncCustom.cpp:109 > > + } > > Duplicate code. consider a common function. Fixed. (Introduced a static getFlags function) > > WebCore/fileapi/DOMFileSystemSync.cpp:43 > > + return adoptRef(new DOMFileSystemSync(fileSystem->m_name, fileSystem->m_asyncFileSystem.leakPtr())); > > This shouldn't be a leakPtr(). This should be a releasePtr. > leakPtr is only use when you want the OwnPtr to stop holding onto to the point and you want the raw pointer. You will manage the memory in some other way. Here you are passing the point to a PassOwnPtr, so releasePtr is perfect. Got it. (For now I removed this code -- I'll think over the constructor.) > > WebCore/fileapi/DOMFileSystemSync.cpp:60 > > +} // namespace > > These // namespace comments aren't necessary in WebKit as far as I know. However, if you are going to include it, make it correct :). Removed the comment. > > WebCore/fileapi/DOMFileSystemSync.h:51 > > + // This call is destructive; the argument fileSystem will become unusable. > > This sounds scary. It makes me wonder why it doesn't take a PassOwnPtr for fileSystem. If fileSystem is unusable afterward. is there a way to help callers to do the right thing and not use it again? I removed this constructor for now. > > WebCore/fileapi/DOMFileSystemSync.h:62 > > +} // namespace > > Ditto. Fixed. > > WebCore/fileapi/DirectoryEntrySync.h:61 > > + DirectoryEntrySync(DOMFileSystemBase* fileSystem, const String& fullPath); > > Please remove variable names which add no information. fileSystem Fixed. > > WebCore/fileapi/DirectoryReader.h:60 > > + DirectoryReader(DOMFileSystemBase* fileSystem, const String& fullPath); > > Please remove variable names which add no information. fileSystem Fixed. > > WebCore/fileapi/DirectoryReaderBase.h:46 > > + void setHasMore(bool hasMore) { m_hasMore = hasMore; } > > What does "hasMore" mean? has more what? (My initial reaction is cowbell since that is the current meme.) Changed the method and variable names to hasMoreEntries. > > WebCore/fileapi/DirectoryReaderSync.h:54 > > + DirectoryReaderSync(DOMFileSystemBase* fileSystem, const String& fullPath); > > fileSystem param name. Fixed. > > WebCore/fileapi/EntryArraySync.h:53 > > + void set(unsigned index, PassRefPtr<EntrySync> entry); > > Please remove variable names which add no information. entry Fixed. (Actually set() wasn't implemented anywhere. Removed) > > WebCore/fileapi/EntrySync.h:59 > > + EntrySync(DOMFileSystemBase* fileSystem, const String& fullPath); > > Please remove variable names which add no information. fileSystem Fixed. > > WebCore/fileapi/FileEntrySync.h:54 > > + FileEntrySync(DOMFileSystemBase* fileSystem, const String& fullPath); > > Please remove variable names which add no information. fileSystem Fixed. > > WebCore/fileapi/FileEntrySync.idl:40 > > + // File file() raises (FileException); > > Please remove commented out code. Fixed.

(In reply to comment #12) > (From update of attachment 69567 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=69567&action=review > > Drive by comments > > > WebCore/bindings/v8/custom/V8DirectoryEntrySyncCustom.cpp:72 > > + EXCEPTION_BLOCK(Flags*, tmp_flags, V8Flags::HasInstance(args[1]) ? V8Flags::toNative(v8::Handle<v8::Object>::Cast(args[1])) : 0); > > I don't think you need to wrap this code into EXCEPTION_BLOCK as there is nothing that can throw an exception (unless I miss something) Thanks for the comment, fixed. > >> WebCore/bindings/v8/custom/V8DirectoryEntrySyncCustom.cpp:109 > >> + } > > > > Duplicate code. consider a common function. > > As another thought: why you need that to be custom? Maybe you should just extend code generator? Right that would be ideal. I have a proposing patch for this, which somehow works (but slow)... it'd be great if you could take a look at it as well. https://bugs.webkit.org/show_bug.cgi?id=45237

Comment on attachment 69771 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=69771&action=review Just a few things to address. > WebCore/bindings/js/JSDirectoryEntrySyncCustom.cpp:72 > + RefPtr<Flags> flags = getFlags(exec, exec->argument(1)); Test for flags being 0? Due to if (argument.isNull() || argument.isUndefined() || !argument.isObject()) return 0; > WebCore/bindings/js/JSDirectoryEntrySyncCustom.cpp:89 > + RefPtr<Flags> flags = getFlags(exec, exec->argument(1)); Test for flags being 0? > WebCore/bindings/v8/custom/V8DirectoryEntrySyncCustom.cpp:48 > +#define GET_FLAGS_EXCEPTION_BLOCK(type, var, value) \ Macros are bad in so many ways. Please get rid of the macro and just write out the code. I guess this is already done for strings, but I'd rather we no do it again here. > WebCore/bindings/v8/custom/V8DirectoryEntrySyncCustom.cpp:53 > + if (block.HasCaught()) { \ This shouldn't have {} (single line statement). > WebCore/bindings/v8/custom/V8DirectoryEntrySyncCustom.cpp:88 > + RefPtr<Flags> flags = getFlags(args[1]); Shouldn't this check for flags being 0? Is there a layout test for this error case? If not, please add one. > WebCore/bindings/v8/custom/V8DirectoryEntrySyncCustom.cpp:103 > + RefPtr<Flags> flags = getFlags(args[1]); Test for flags being 0.

Committed r69178: <http://trac.webkit.org/changeset/69178>