[webkit-reviews] review denied: [Bug 34994] Add sync bindings for Worker access to DB : [Attachment 54661] patch #2: IDL files + JSC bindings

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Dumitru Daniliuc
<dumi at chromium.org>'s request for review:
Bug 34994: Add sync bindings for Worker access to DB
https://bugs.webkit.org/show_bug.cgi?id=34994

Attachment 54661: patch #2: IDL files + JSC bindings
https://bugs.webkit.org/attachment.cgi?id=54661&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
WebCore/bindings/js/JSCustomSQLTransactionSyncCallback.cpp:30
 +  #include "JSCustomSQLTransactionSyncCallback.h"
I think this file is misnamed.	We usually put the custom at the end of the
file name.

WebCore/bindings/js/JSCustomSQLTransactionSyncCallback.cpp:54
 +  void
JSCustomSQLTransactionSyncCallback::handleEvent(ScriptExecutionContext*
context, SQLTransactionSync* transaction, bool& raisedException)
This code is all copy/paste from somewhere else.  We need to abstract this code
and not have duplication.

WebCore/bindings/js/JSDatabaseSyncCustom.cpp:48
 +	String newVersion = ustringToString(args.at(1).toString(exec));
What about exceptions generated here?

WebCore/bindings/js/JSDatabaseSyncCustom.cpp:45
 +  JSValue JSDatabaseSync::changeVersion(ExecState* exec, const ArgList& args)

It's lame that we can't autogenerate this code, but I'll work on that in a
separate bug.

WebCore/bindings/js/JSDatabaseSyncCustom.cpp:51
 +	if (!(object = args.at(2).getObject())) {
Can we do the assignment and the test separate statements?  This is hard to
read.

WebCore/bindings/js/JSSQLTransactionSyncCustom.cpp:47
 +	    setDOMException(exec, SYNTAX_ERR);
Are you sure this is the right exception to throw here?

WebCore/bindings/js/JSSQLTransactionSyncCustom.cpp:76
 +		if (value.isNull())
Why Null and not Undefined?

WebCore/bindings/js/JSSQLTransactionSyncCustom.cpp:78
 +		else if (value.isNumber())
Do we need to call valueOf?

WebCore/bindings/js/JSWorkerContextCustom.cpp:152
 +	if (args.size() < 4)
Why do we need check the args.size() here?

WebCore/bindings/js/JSWorkerContextCustom.cpp:159
 +	unsigned long estimatedSize = args.at(3).toInt32(exec);
Exception handling?

WebCore/bindings/js/JSWorkerContextCustom.cpp:150
 +  JSValue JSWorkerContext::openDatabaseSync(ExecState* exec, const ArgList&
args)
It's lame that we can't autogenerate this code.  It's not doing anything
complicated.

Where's the V8 implementation of the custom bindings?