Add v8 bindings for async DB API in workers

• Component: DOM
• Status: RESOLVED FIXED

[Eric U.]

It's in the commit queue for JSC right now, as is the needed support for canEstablishDatabase.

[Eric U.]

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.

[Early Warning System Bot]

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

[Adam Barth]

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.

[Eric U.]

Created attachment 56544 [details]
Patch

[Eric U.]

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

[Eric U.]

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

[Adam Barth]

Looking.

[Adam Barth]

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?

[Eric U.]

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

[Adam Barth]

Comment on attachment 56544 [details]
Patch

It hasn't changed.  If dglazkov said that's right, then it is.  Thanks!

[WebKit Commit Bot]

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

[Eric U.]

Created attachment 57258 [details]
Patch

[Eric U.]

Comment on attachment 57258 [details]
Patch

Resolved recent changes; this is the exact same patch.  It should apply cleanly now [fingers crossed].

[WebKit Commit Bot]

Comment on attachment 57258 [details]
Patch

Clearing flags on attachment: 57258

Committed r60330: <http://trac.webkit.org/changeset/60330>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Dumitru Daniliuc]

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.

[Dumitru Daniliuc]

Reopening bug until the latest patch is landed.

[Adam Barth]

Comment on attachment 57443 [details]
patch: fix the world context

That makes more sense to me.  Thanks.

[Dumitru Daniliuc]

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