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

------- Additional Comments from Adam Barth <abarth at webkit.org>
No ChangeLog.  No tests!  There's tons of stuff you should be testing here.

WebCore/bindings/js/JSDOMWindowCustom.cpp:977
 +	  setDOMException(exec, TYPE_MISMATCH_ERR);
Wrong indent

WebCore/bindings/js/JSDatabaseSyncCustom.cpp:65
 +	RefPtr<SQLTransactionSyncCallback>
callback(JSSQLTransactionSyncCallback::create(object,
static_cast<JSDOMGlobalObject*>(exec->dynamicGlobalObject())));
Why the dynamic global object?

WebCore/bindings/js/JSDatabaseSyncCustom.cpp:93
 +	return createTransaction(exec, args, m_impl.get(),
static_cast<JSDOMGlobalObject*>(exec->dynamicGlobalObject()), false);
It seems more likely that we want the global object associated with the
JSDatabaseSync object.

WebCore/bindings/js/JSWorkerContextCustom.cpp:164
 +	const UString& name = args.at(0).toString(exec);
Exceptions?

WebCore/bindings/v8/custom/V8DOMWindowCustom.cpp:722
 +	    return throwError("openDatabaseSync() must be called with at least
4 parameters.", V8Proxy::SyntaxError);
These exception messages aren't localized.  Also, they differ from the JSC
versions.  I think we should just throw without a message.

WebCore/bindings/v8/custom/V8WorkerContextCustom.cpp:157
 +	String name = toWebCoreString(args[0]);
Again, no exception handling.