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

------- Additional Comments from Dumitru Daniliuc <dumi at chromium.org>
(In reply to comment #51)
> 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.

reverted, keeping the original exception around.

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

i believe toString() is automatically called when needed.

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

done here and everywhere else.

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

done.

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

changed to 'length'.

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

added a macro to V8BindingMacros.h.

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

wrapped in an EXCEPTION_BLOCK.

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

handled.

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

potential exceptions should be handled. JSC was changed to return a uint32 too.