[webkit-reviews] review denied: [Bug 43134] Add idl and mock implementation for HTML5 FileSystem API : [Attachment 62864] Patch (depends on 43130)

[bugzilla-daemon at webkit.org]

Dumitru Daniliuc <dumi at chromium.org> has denied Kinuko Yasuda
<kinuko at chromium.org>'s request for review:
Bug 43134: Add idl and mock implementation for HTML5 FileSystem API
https://bugs.webkit.org/show_bug.cgi?id=43134

Attachment 62864: Patch (depends on 43130)
https://bugs.webkit.org/attachment.cgi?id=62864&action=review

------- Additional Comments from Dumitru Daniliuc <dumi at chromium.org>
i think you might need to modify JSValueToNative in CodeGeneratorV8.pm to make
the patch build in chromium. for [Callback] parameters, i think you would want
to return "$value".

WebCore/bindings/scripts/CodeGenerator.pm:363
 +	$ret =~ s/^eXCLUSIVE/exclusiveFlag/ if $ret =~ /^eXCLUSIVE$/;
i think isCreate() or isCreateFlag() would be a better name for a boolean
getter. same for exclusiveFlag(). also, you should add a test for these cases
to WebCore/bindings/scripts/test/TestObj.idl, and change the expected outputs
in WebCore/bindings/scripts/test/{JS|V8|...}/. Please make sure
WebKitTools/Scripts/run-bindings-tests passes.

WebCore/storage/DOMFileSystem.cpp:35
 +  #include "DOMFileSystem.h"
i believe the common pattern is:

#include "config.h"
#include "DOMFileSystem.h"

#if ENABLE(FILE_SYSTEM)

WebCore/storage/DOMFileSystem.cpp:38
 +  #include "EntryCallback.h"
looks like this include is not yet needed.

WebCore/storage/DOMFileSystem.cpp:39
 +  #include "ErrorCallback.h"
ditto.

WebCore/storage/DOMFileSystem.h:44
 +  class ErrorCallback;
these 2 classes don't need to be forward-declared just yet.

WebCore/storage/DOMFileSystem.h:45
 +  class ScriptExecutionContext;
ditto.

WebCore/storage/Entry.cpp:34
 +  #include "Entry.h"
same comment about moving this outside #if ENABLE(FILE_SYSTEM)

WebCore/storage/Entry.cpp:50
 +	    m_name = fullPath.substring(index);
should there be a "else m_name = fullPath;" here?

WebCore/storage/Entry.h:34
 +  #include "DOMFileSystem.h"
either remove this include, or the forward-declaration below.

WebCore/storage/ErrorCallback.h:43
 +  class ErrorCallback : public ThreadSafeShared<ErrorCallback> {
this callback is ThreadSafeShared, and all other callbacks are RefCounted. is
this intended?

WebCore/storage/Flags.h:50
 +	void setEXCLUSIVE(bool exclusive) { m_exclusive = exclusive; }
you should modify WK_ucfirst in CodeGenerator.pm the same way you modified
WK_lcfirst, and convert setCREATE() and setEXCLUSIVE() to setCreate() and
setExclusive().

WebCore/storage/Metadata.h:53
 +	double m_modificationTime;
empty line before this field.