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

------- Additional Comments from Dumitru Daniliuc <dumi at chromium.org>
tested the patch on webkit/win, webkit/mac and chromium/win.

(In reply to comment #35)
> 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.

all these callbacks are auto-generated now.

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

done. this code is auto-generated.

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

done.

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

this is on my to-do list too.

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

fixed in all DB bindings.

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

yes, executeSQL() takes at least one mandatory argument.

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

changed to isUndefinedOrNull().

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

i don't think so. isNumber() makes sure it's a number, and uncheckedGetNumber()
gets it.

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

yes, openDatabaseSync() has 4 mandatory arguments.

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

done.

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

agree, will work on this in parallel to implementing the rest of the sync DB
API.

> Where's the V8 implementation of the custom bindings?

added.