Bug 39145 - Add v8 bindings for async DB API in workers
Summary: Add v8 bindings for async DB API in workers
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Eric U.
URL:
Keywords:
Depends on: 34992 38742
Blocks:
  Show dependency treegraph
 
Reported: 2010-05-14 18:02 PDT by Eric U.
Modified: 2010-06-01 11:52 PDT (History)
7 users (show)

See Also:


Attachments
Reviewable, but not committable, patch. (24.24 KB, patch)
2010-05-14 19:10 PDT, Eric U.
no flags Details | Formatted Diff | Diff
Patch (28.98 KB, patch)
2010-05-19 18:13 PDT, Eric U.
no flags Details | Formatted Diff | Diff
Patch (25.17 KB, patch)
2010-05-27 10:54 PDT, Eric U.
no flags Details | Formatted Diff | Diff
patch: fix the world context (3.65 KB, patch)
2010-05-31 02:23 PDT, Dumitru Daniliuc
abarth: review+
dumi: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric U. 2010-05-14 18:02:55 PDT
It's in the commit queue for JSC right now, as is the needed support for canEstablishDatabase.
Comment 1 Eric U. 2010-05-14 19:10:19 PDT
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.
Comment 2 Early Warning System Bot 2010-05-14 19:17:24 PDT
Attachment 56130 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/2287081
Comment 3 Adam Barth 2010-05-14 21:27:31 PDT
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.
Comment 4 Eric U. 2010-05-19 18:13:38 PDT
Created attachment 56544 [details]
Patch
Comment 5 Eric U. 2010-05-19 18:16:56 PDT
(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.
Comment 6 Eric U. 2010-05-26 11:22:59 PDT
Ping?
Incidentally, all the dependencies have landed, so the latest patch should be committable.
Comment 7 Adam Barth 2010-05-26 11:24:45 PDT
Looking.
Comment 8 Adam Barth 2010-05-26 11:29:08 PDT
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?
Comment 9 Eric U. 2010-05-26 11:35:59 PDT
(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 10 Adam Barth 2010-05-26 11:56:12 PDT
Comment on attachment 56544 [details]
Patch

It hasn't changed.  If dglazkov said that's right, then it is.  Thanks!
Comment 11 WebKit Commit Bot 2010-05-27 02:24:44 PDT
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
Comment 12 Eric U. 2010-05-27 10:54:26 PDT
Created attachment 57258 [details]
Patch
Comment 13 Eric U. 2010-05-27 10:55:19 PDT
Comment on attachment 57258 [details]
Patch

Resolved recent changes; this is the exact same patch.  It should apply cleanly now [fingers crossed].
Comment 14 WebKit Commit Bot 2010-05-27 16:22:04 PDT
Comment on attachment 57258 [details]
Patch

Clearing flags on attachment: 57258

Committed r60330: <http://trac.webkit.org/changeset/60330>
Comment 15 WebKit Commit Bot 2010-05-27 16:22:11 PDT
All reviewed patches have been landed.  Closing bug.
Comment 16 Dumitru Daniliuc 2010-05-31 02:23:13 PDT
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.
Comment 17 Dumitru Daniliuc 2010-05-31 02:28:45 PDT
Reopening bug until the latest patch is landed.
Comment 18 Adam Barth 2010-06-01 11:43:54 PDT
Comment on attachment 57443 [details]
patch: fix the world context

That makes more sense to me.  Thanks.
Comment 19 Dumitru Daniliuc 2010-06-01 11:52:03 PDT
The fix to V8CustomVoidCallback was landed in r60492. Re-closing the bug.