[webkit-reviews] review requested: [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]

Dumitru Daniliuc <dumi at chromium.org> has asked	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 Dumitru Daniliuc <dumi at chromium.org>
(In reply to comment #48)
> No ChangeLog.

oops, sorry about that. fixed.

> No tests!  There's tons of stuff you should be testing here.

added a test to test the parameters passed to openDatabaseSync(). i'm not sure
what else we can test at this point. once we have some implementation in place,
i'll add a similar test for db.{read}transaction(), as well as more functional
tests.

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

fixed.

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

i copy-pasted this code from another class. fixed in all DB bindings.

> 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.

fixed.

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

handled.

> 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.

removed the exception messages from all V8 DB bindings.

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

done.