[webkit-reviews] review denied: [Bug 34994] Add sync bindings for Worker access to DB : [Attachment 55292] patch #2: IDL files + JSC and V8 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 55292: patch #2: IDL files + JSC and V8 bindings
https://bugs.webkit.org/attachment.cgi?id=55292&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
Sorry for nit picking this patch to death, but writing custom bindings
correctly is nigh unto impossible.  Mostly r- for the lack of a toString call
in the V8 bindings.  You can test this with an object that has a toString
function that actually returns a string.

WebCore/bindings/js/JSSQLTransactionSyncCustom.cpp:54
 +	    setDOMException(exec, TYPE_MISMATCH_ERR);
It's unclear to me whether its better to magically transmute the exception to
TYPE_MISMATCH_ERR or whether it's better to re-throw the same exception.

WebCore/bindings/v8/custom/V8DOMWindowCustom.cpp:724
 +	if (!args[0]->IsString() || !args[1]->IsString() ||
!args[2]->IsString() || !args[3]->IsUint32())
Why don't we call toString() on these args like we do in the JSC version?

WebCore/bindings/v8/custom/V8DOMWindowCustom.cpp:732
 +	String name = toWebCoreString(args[0]);
I think you want to skip the check above and check for an exception here
instead.

WebCore/bindings/v8/custom/V8DatabaseSyncCustom.cpp:50
 +	if (!(args[0]->IsString() && args[1]->IsString()))
Same comment here about calling toString()

WebCore/bindings/v8/custom/V8SQLTransactionSyncCustom.cpp:63
 +		v8::Local<v8::Value> lengthGetter;
Is this the getter or the gotten value?

WebCore/bindings/v8/custom/V8SQLTransactionSyncCustom.cpp:80
 +			v8::TryCatch block;
Can we condense this somehow?  It's going to be a lot of code everywhere
otherwise.

WebCore/bindings/v8/custom/V8SQLTransactionSyncCustom.cpp:89
 +			sqlValues.append(SQLValue(value->NumberValue()));
Does this call valueOf?  If so, what if there's an exception?

WebCore/bindings/v8/custom/V8SQLTransactionSyncCustom.cpp:91
 +			sqlValues.append(SQLValue(toWebCoreString(value)));
Exception?

WebCore/bindings/v8/custom/V8WorkerContextCustom.cpp:150
 +	if (!args[0]->IsString() || !args[1]->IsString() ||
!args[2]->IsString() || !args[3]->IsUint32())
String comment.

WebCore/bindings/v8/custom/V8WorkerContextCustom.cpp:159
 +	unsigned long estimatedSize = args[3]->Uint32Value();
Does this call valueOf?  Should we be calling valueOf?	Does this match JSC?