[webkit-reviews] review granted: [Bug 46405] Add idl and mock classes for FileSystemSync for FileSystem API : [Attachment 69771] Patch

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has granted Kinuko Yasuda
<kinuko at chromium.org>'s request for review:
Bug 46405: Add idl and mock classes for FileSystemSync for FileSystem API
https://bugs.webkit.org/show_bug.cgi?id=46405

Attachment 69771: Patch
https://bugs.webkit.org/attachment.cgi?id=69771&action=review

------- Additional Comments from David Levin <levin at chromium.org>
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.