Bug 39145

Created attachment 56130 [details] Reviewable, but not committable, patch. This patch is a standalone, testable implementation of the V8 bindings, which will apply, build, and run in Chromium [although not quite build in WebKit+JSC]. This bug is blocked on two CLs currently in the commit queue. Once they're in, I'll reformulate this patch to assume their existence, and then everything will coexist happily. But I wanted to get this up for general review without waiting for the queue, which currently has quite a backlog.

Attachment 56130 [details] did not build on qt: Build output: http://webkit-commit-queue.appspot.com/results/2287081

Comment on attachment 56130 [details] Reviewable, but not committable, patch. 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.

Created attachment 56544 [details] Patch

(In reply to comment #3) > (From update of attachment 56130 [details]) > 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. Done. Wow, I didn't know about that. > 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)" Fixed; I think that may have happened when I brought the changelist between machines. > 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". Renamed, in a number of places. > WebCore/bindings/v8/custom/V8CustomVoidCallback.cpp:96 > + } > Why did you remove this namespace comment? We have these in most files. Accident--fixed. > 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. Ditto. > WebCore/bindings/v8/custom/V8CustomVoidCallback.h:55 > + V8CustomVoidCallback(v8::Local<v8::Object>, ScriptExecutionContext *context); > ditto 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" Ditto. > WebCore/bindings/v8/custom/V8NotificationCenterCustom.cpp:93 > + callback = V8CustomVoidCallback::create(args[0], context); > Can these be different? Where does the context come from? Can what be different? I'm not sure what you're talking about. > 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. In another changelist perhaps, that covers openDatabaseCallback and openDatabaseSyncCallback? > 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. No worries--that's code that was about to go away due to one of the CLs in the commit queue. It's all gone now.

Ping? Incidentally, all the dependencies have landed, so the latest patch should be committable.

Looking.

Comment on attachment 56544 [details] Patch Fantastic. I love it. WebCore/bindings/scripts/CodeGeneratorV8.pm:2150 + static PassRefPtr<${className}> create(v8::Local<v8::Value> value) Nice. Whenever we can remove Frame* from the bindings, we're doing something right. WebCore/bindings/v8/custom/V8CustomVoidCallback.cpp:55 + v8::Handle<v8::Context> v8Context = toV8Context(m_scriptExecutionContext.get(), WorldContextHandle(UseCurrentWorld)); I'm not sure the "WorldContextHandle(UseCurrentWorld)" part is right. Is there other code that uses this pattern?

(In reply to comment #8) > (From update of attachment 56544 [details]) > Fantastic. I love it. > > WebCore/bindings/scripts/CodeGeneratorV8.pm:2150 > + static PassRefPtr<${className}> create(v8::Local<v8::Value> value) > Nice. Whenever we can remove Frame* from the bindings, we're doing something right. > > WebCore/bindings/v8/custom/V8CustomVoidCallback.cpp:55 > + v8::Handle<v8::Context> v8Context = toV8Context(m_scriptExecutionContext.get(), WorldContextHandle(UseCurrentWorld)); > I'm not sure the "WorldContextHandle(UseCurrentWorld)" part is right. Is there other code that uses this pattern? IIRC I had to ask dglazkov [+CC] what to use there, and that's what he suggested. That was 4-5 months ago, though, so I don't know if that's changed. BTW I'm not a committer, so I'll need you to CQ+ or land this for me if the WorldContextHandle isn't a blocker.

Comment on attachment 56544 [details] Patch It hasn't changed. If dglazkov said that's right, then it is. Thanks!

Comment on attachment 56544 [details] Patch Rejecting patch 56544 from commit-queue. Failed to run "[u'/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Adam Barth', u'--force']" exit_code: 1 Last 500 characters of output: s/v8/custom/V8DOMWindowCustom.cpp patching file WebCore/bindings/v8/custom/V8DatabaseCustom.cpp patching file WebCore/bindings/v8/custom/V8DatabaseSyncCustom.cpp patching file WebCore/bindings/v8/custom/V8NotificationCenterCustom.cpp patching file WebCore/bindings/v8/custom/V8SQLTransactionCustom.cpp patching file WebCore/bindings/v8/custom/V8WorkerContextCustom.cpp patching file WebKit/chromium/ChangeLog Hunk #1 succeeded at 1 with fuzz 3. patching file WebKit/chromium/src/DatabaseObserver.cpp Full output: http://webkit-commit-queue.appspot.com/results/2502064

Created attachment 57258 [details] Patch

Comment on attachment 57258 [details] Patch Resolved recent changes; this is the exact same patch. It should apply cleanly now [fingers crossed].

Comment on attachment 57258 [details] Patch Clearing flags on attachment: 57258 Committed r60330: <http://trac.webkit.org/changeset/60330>

All reviewed patches have been landed. Closing bug.

Created attachment 57443 [details] patch: fix the world context > WebCore/bindings/v8/custom/V8CustomVoidCallback.cpp:55 > + v8::Handle<v8::Context> v8Context = toV8Context(m_scriptExecutionContext.get(), WorldContextHandle(UseCurrentWorld)); > I'm not sure the "WorldContextHandle(UseCurrentWorld)" part is right. Is there other code that uses this pattern? Turns out Adam was right. We should save the context in which the callback is created and run the callback in that context, instead of the context in which handleEvent() is called.

Reopening bug until the latest patch is landed.

Comment on attachment 57443 [details] patch: fix the world context That makes more sense to me. Thanks.

The fix to V8CustomVoidCallback was landed in r60492. Re-closing the bug.