Bug 46405 – Add idl and mock classes for FileSystemSync for FileSystem API

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Kinuko Yasuda]

Add idl and mock classes for FileSystemSync for FileSystem API.

[Kinuko Yasuda]

Created attachment 68597 [details]
Patch

[Adam Barth]

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.

[David Levin]

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.

[Kinuko Yasuda]

Created attachment 69366 [details]
Patch

[Kinuko Yasuda]

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

[Kinuko Yasuda]

Created attachment 69370 [details]
Patch

Updated ChangeLog according and rebased.

[Kinuko Yasuda]

Created attachment 69371 [details]
Patch

Cleaned up unnecessary headers.

[Kinuko Yasuda]

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

[Kinuko Yasuda]

Created attachment 69547 [details]
Patch

[Kinuko Yasuda]

Created attachment 69567 [details]
Patch

Did some more cleanup.

[David Levin]

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.

[anton muhin]

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?

[Kinuko Yasuda]

Created attachment 69771 [details]
Patch

[Kinuko Yasuda]

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

[Kinuko Yasuda]

(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

[David Levin]

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.

[Kinuko Yasuda]

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