Add DOMRequestState to maintain world/ScriptExecutionContext state
Created attachment 173919 [details] Patch
abarth@ - finally putting this in a bug. Things that are missing: 1) tests - I'm not sure how to test this but if you can point me in the right direction, I'll see what I can put together 2) JSC implementation. I'm going to take a crack at this but I welcome any pointers there as well. adamk@ - this is the patch we talked about monday, it's usable for your private build as-is, and you can use the IDB code here as a reference - the key is that the IDBRequest is created with JS on the stack, and then IDBRequest hangs out until an event is generated/dispatched without JS on the stack.
Comment on attachment 173919 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=173919&action=review > Source/WebCore/bindings/v8/DOMRequestState.h:39 > + DOMRequestState(ScriptExecutionContext *scriptExecutionContext) explicit "ScriptExecutionContext *" -> "ScriptExecutionContext* " > Source/WebCore/bindings/v8/DOMRequestState.h:47 > + Scope(v8::Handle<v8::Context> context) explicit > Source/WebCore/bindings/v8/DOMRequestState.h:48 > + : m_contextScope(context) { } The { and } each go on their own line. > Source/WebCore/bindings/v8/DOMRequestState.h:53 > + const Scope scope() Woah, I don't know what happens if we try to copy a v8::Context::Scope.... > Source/WebCore/bindings/v8/DOMRequestState.h:55 > + v8::HandleScope handleScope; /* do we need this for Local<> ? */ This is bad new bears. You've got a local handle which will die once this object goes out of scope. > Source/WebCore/bindings/v8/DOMRequestState.h:59 > + return Scope(context); Ok. So the way to do this is to have the DOMRequestState::Scope object create the HandleScope in its constructor. The caller looks like: DOMRequestState::Scope scope(m_requestState) The DOMRequestState::Scope class then is something like the following: class Scope { public: explicit Scope(DOMRequestState& state) : m_scope(static.context()) { } private: v8::HandleScope m_handleScope; v8::Context::Scope m_scope }; Notice that the HandleScope member precedes the v8::Context::Scope member so that it will be constructed first and destructed last. DOMRequestState::context is then just "return toV8Context(m_scriptExecutionContext, m_worldContextHandle)"
Created attachment 173940 [details] Patch
Comment on attachment 173940 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=173940&action=review > Source/WebCore/bindings/v8/DOMRequestState.h:48 > + : m_contextScope(state.context()) { } The { and } should each be on their own line. > Source/WebCore/bindings/v8/DOMRequestState.h:61 > + ScriptExecutionContext* m_scriptExecutionContext; How does this pointer get cleared when the ScriptExecutionContext is destroyed? Maybe we should clear it explicitly in IDBRequest::stop.
thanks for the suggestions. My only open issue at this point is if/where to attach context() - you can see I'm now passing DOMRequest* into my IDBBindingUtilities - I don't need it there, but I'm kind of assuming I will on the JSC side? And then I wonder if really I should be passing the DOMRequest::Scope* in there, and hanging context() off of scope? But I don't know if that violates conventions around scope-like objects, who usually just exist on the stack. I'm also wondering if I can narrow the scope in a few places
> I don't need it there, but I'm kind of assuming I will on the JSC side? We can always add it later if we find we need it then.
Created attachment 173951 [details] Patch for landing
Created attachment 173954 [details] Patch for landing
Comment on attachment 173954 [details] Patch for landing Rejecting attachment 173954 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: t/git/webkit-commit-queue/Source/WebKit/chromium/v8 --revision 12869 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' 50>At revision 12869. ________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' ________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' Updating webkit projects from gyp files... Full output: http://queues.webkit.org/results/14809977
Created attachment 173975 [details] Patch for landing
Comment on attachment 173975 [details] Patch for landing Clearing flags on attachment: 173975 Committed r134523: <http://trac.webkit.org/changeset/134523>
All reviewed patches have been landed. Closing bug.
Re-opened since this is blocked by bug 102218
This patch breaks Chromium windows build. The build succeeds in ninja but fails in MSVS (Sorry I'm now at home and cannot find the log). This would indicate that something is wrong with build files. See a comment from Jochen here (https://bugs.webkit.org/show_bug.cgi?id=102218#c2)
Here is a build log of the windows build: http://build.chromium.org/p/chromium.win/builders/Win%20Builder/builds/3509/steps/runhooks/logs/stdio
Created attachment 174202 [details] Patch for landing
Comment on attachment 174202 [details] Patch for landing Clearing flags on attachment: 174202 Committed r134632: <http://trac.webkit.org/changeset/134632>