[webkit-reviews] review denied: [Bug 39145] Add v8 bindings for async DB API in workers : [Attachment 56130] Reviewable, but not committable, patch.

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Eric U. <ericu at chromium.org>'s
request for review:
Bug 39145: Add v8 bindings for async DB API in workers
https://bugs.webkit.org/show_bug.cgi?id=39145

Attachment 56130: Reviewable, but not committable, patch.
https://bugs.webkit.org/attachment.cgi?id=56130&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
I really like the direction this patch is going.  Please run-bindings-tests
--reset-results so we can see the effect of your change to the CodeGenerator in
the review.  Minor complains below.

WebCore/bindings/scripts/CodeGeneratorV8.pm:2274
 +		push(@implContent, "	return !invokeCallback(m_callback, " . 
scalar(@params). ", argv, callbackReturnValue, context);\n");
Why did you add a second space before "scalar"?  Also, I think we need a space
"after params)"

WebCore/bindings/v8/custom/V8CustomVoidCallback.cpp:65
 +  bool invokeCallback(v8::Persistent<v8::Object> callback, int argc,
v8::Handle<v8::Value> argv[], bool& callbackReturnValue,
ScriptExecutionContext* executionContext)
I think we either call it a "context" or a "scriptExecutionContext" but not
really an "executionContext".

WebCore/bindings/v8/custom/V8CustomVoidCallback.cpp:96
 +  }
Why did you remove this namespace comment?  We have these in most files.

WebCore/bindings/v8/custom/V8CustomVoidCallback.h:45
 +	static PassRefPtr<V8CustomVoidCallback> create(v8::Local<v8::Value>
value, ScriptExecutionContext *context)
The * goes next to ScriptExecutionContext not next to context.

WebCore/bindings/v8/custom/V8CustomVoidCallback.h:55
 +	V8CustomVoidCallback(v8::Local<v8::Object>, ScriptExecutionContext
*context);
ditto

WebCore/bindings/v8/custom/V8CustomVoidCallback.h:62
 +  bool invokeCallback(v8::Persistent<v8::Object> callback, int argc,
v8::Handle<v8::Value> argv[], bool& callbackReturnValue,
ScriptExecutionContext* executionContext);
Same comment about "executionContext"

WebCore/bindings/v8/custom/V8NotificationCenterCustom.cpp:93
 +	    callback = V8CustomVoidCallback::create(args[0], context);
Can these be different?  Where does the context come from?

WebCore/bindings/v8/custom/V8WorkerContextCustom.cpp:147
 +  v8::Handle<v8::Value> V8WorkerContext::openDatabaseCallback(const
v8::Arguments& args)
This method looks like it can be autogenerated.

WebKit/chromium/src/DatabaseObserver.cpp:54
 +	// ASSERT(scriptExecutionContext->isDocument());
Please don't comment out code.	If this assert is now bogus, we can remove it.