RESOLVED FIXED 103484
IndexedDB: Implement IndexedDB bindings for JSC
https://bugs.webkit.org/show_bug.cgi?id=103484
Summary IndexedDB: Implement IndexedDB bindings for JSC
Michael Pruett
Reported 2012-11-27 23:05:13 PST
Implement IndexedDB bindings for JSC.
Attachments
Patch (27.22 KB, patch)
2012-11-28 10:47 PST, Michael Pruett
no flags
Patch (27.63 KB, patch)
2012-11-29 14:38 PST, Michael Pruett
haraken: review-
Patch (24.87 KB, patch)
2012-12-04 17:56 PST, Michael Pruett
haraken: review+
buildbot: commit-queue-
Patch (28.79 KB, patch)
2012-12-04 21:20 PST, Michael Pruett
no flags
Michael Pruett
Comment 1 2012-11-28 10:47:11 PST
Joshua Bell
Comment 2 2012-11-28 11:52:31 PST
Comment on attachment 176519 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176519&action=review > Source/WebCore/ChangeLog:6 > + Reviewed by NOBODY (OOPS!). Please include more of an explanation of the changes after the Reviewed line. > Source/WebCore/ChangeLog:8 > + Tests: storage/indexeddb/* Can you indicate roughly how many tests pass/fail with just this patch when INDEXED_DATABASE is enabled locally? > Source/WebCore/ChangeLog:13 > + * Modules/indexeddb/IDBVersionChangeRequest.idl: FYI, IDBVersionChangeRequest should be going away shortly (now that setVersion is gone, deleteDatabase can switch over to IDBOpenDBRequest). But that shouldn't block landing this. > Source/WebCore/ChangeLog:23 > + * bindings/js/IDBBindingUtilities.cpp: You could mention here that these all match the v8 implementations. > Source/WebCore/bindings/js/JSIDBOpenDBRequestCustom.cpp:40 > +void JSIDBOpenDBRequest::visitChildren(JSCell* cell, JSC::SlotVisitor& visitor) I don't know the JSC binding code well but after a brief glance it looks like this is doing the right thing. Unfortunate that this boilerplate is needed; we should probably put a bug in to add WebKitIDL attributes to autogenerate the different visitChildren variations (there only seem to be a few different patterns).
Michael Pruett
Comment 3 2012-11-28 16:40:31 PST
(In reply to comment #2) Joshua, thanks for your feedback. I'll update the patch with an explanation of the changes. > Can you indicate roughly how many tests pass/fail with just this patch when INDEXED_DATABASE is enabled locally? With only this patch and another fixing bug 103554, WebKitGTK+ passes 112 of the 196 layout tests in storage/indexeddb. I have patches which fix additional failures, but those patches are independent of the change proposed here.
Joshua Bell
Comment 4 2012-11-28 16:44:44 PST
(In reply to comment #3) > With only this patch and another fixing bug 103554, WebKitGTK+ passes 112 of the 196 layout tests in storage/indexeddb. I think that would be worth including in the ChangeLog.
Michael Pruett
Comment 5 2012-11-29 14:38:26 PST
Created attachment 176820 [details] Patch I've updated the patch to incorporate the suggestions I've received thus far.
Kentaro Hara
Comment 6 2012-12-02 21:17:17 PST
Comment on attachment 176820 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176820&action=review The approach looks reasonable. I hope JSC experts also take a look. > Source/WebCore/bindings/js/IDBBindingUtilities.cpp:130 > + if (currentValue.isString() && keyPathElements[i] == "length") > + return jsNumber(currentValue.toString(exec)->length()); > + if (!currentValue.isObject()) > + return jsUndefined(); This behavior looks different from V8's one. > Source/WebCore/bindings/js/IDBBindingUtilities.cpp:144 > + Don't you need to set up a new scope here? APIEntryShim entryShim(exec); > Source/WebCore/bindings/js/IDBBindingUtilities.cpp:147 > + jsValue = getNthValueOnKeyPath(exec, jsValue, keyPathElements, > + keyPathElements.size()); Nit: you don't need to wrap the line. > Source/WebCore/bindings/js/IDBBindingUtilities.cpp:148 > + if (jsValue.isUndefined()) This should be jsValue.isEmpty()? > Source/WebCore/bindings/js/IDBBindingUtilities.cpp:204 > + ExecState* exec = requestState->exec(); > + APIEntryShim entryShim(exec); You can use DOMRequestState::Scope. > Source/WebCore/bindings/js/IDBBindingUtilities.cpp:208 > + if (!jsValue.isObject()) > + return false; Remove this. > Source/WebCore/bindings/js/IDBBindingUtilities.cpp:211 > + JSValue parent = ensureNthValueOnKeyPath(exec, asObject(jsValue), > + keyPathElements, keyPathElements.size() - 1); asObject(jsValue) => jsValue. Nit: You don't need to wrap the line. > Source/WebCore/bindings/js/IDBBindingUtilities.cpp:212 > + if (parent.isUndefined()) This should be parent.isEmpty()? > Source/WebCore/bindings/js/IDBBindingUtilities.cpp:216 > + if (!set(exec, parent, keyPathElements.last(), > + toJS(exec, jsCast<JSDOMGlobalObject*>(exec->lexicalGlobalObject()), key.get()))) Nit: Don't wrap the line. > Source/WebCore/bindings/js/IDBBindingUtilities.cpp:228 > + ExecState* exec = requestState->exec(); > + APIEntryShim entryShim(exec); You can use DOMRequestState::Scope. BTW, V8 binding isn't creating a scope of DOMRequestState::Scope, which looks wrong. > Source/WebCore/bindings/js/IDBBindingUtilities.cpp:260 > + JSC::ExecState* exec = requestState->exec(); > + APIEntryShim entryShim(exec); You can use DOMRequestState::Scope. BTW, V8 binding isn't creating a scope of DOMRequestState::Scope, which looks wrong. > Source/WebCore/bindings/js/IDBBindingUtilities.cpp:262 > + return canInjectNthValueOnKeyPath(exec, scriptValue.jsValue(), > + keyPathElements, keyPathElements.size() - 1); Nit: don't wrap the line. > Source/WebCore/bindings/js/IDBBindingUtilities.cpp:268 > + ExecState* exec = requestState->exec(); > + APIEntryShim entryShim(exec); You can use DOMRequestState::Scope. > Source/WebCore/bindings/js/IDBBindingUtilities.cpp:278 > + ExecState* exec = requestState->exec(); > + APIEntryShim entryShim(exec); You can use DOMRequestState::Scope. BTW, V8 binding isn't creating a scope of DOMRequestState::Scope, which looks wrong. > Source/WebCore/bindings/js/IDBBindingUtilities.cpp:280 > + return ScriptValue(exec->globalData(), > + toJS(exec, jsCast<JSDOMGlobalObject*>(exec->lexicalGlobalObject()), key.get())); Nit: don't wrap the line. > Source/WebCore/bindings/js/JSIDBAnyCustom.cpp:-88 > - case IDBAny::SerializedScriptValueType: > - return idbAny->serializedScriptValue()->deserialize(exec, globalObject); Why can you remove this? > Source/WebCore/bindings/js/JSIDBAnyCustom.cpp:88 > + case IDBAny::StringType: > + return jsStringWithCache(exec, idbAny->string()); Just to confirm: Is there any reason why you moved this code from above to here? > Source/WebCore/bindings/js/JSIDBOpenDBRequestCustom.cpp:48 > +void JSIDBOpenDBRequest::visitChildren(JSCell* cell, JSC::SlotVisitor& visitor) > +{ > + JSIDBOpenDBRequest* jsRequest = jsCast<JSIDBOpenDBRequest*>(cell); > + ASSERT_GC_OBJECT_INHERITS(jsRequest, &s_info); > + COMPILE_ASSERT(StructureFlags & OverridesVisitChildren, OverridesVisitChildrenWithoutSettingFlag); > + ASSERT(jsRequest->structure()->typeInfo().overridesVisitChildren()); > + Base::visitChildren(jsRequest, visitor); > + static_cast<IDBOpenDBRequest*>(jsRequest->impl())->visitJSEventListeners(visitor); > +} Why do you need this? Event listeners are already cared by JS bindings, aren't they? > Source/WebCore/bindings/js/JSIDBVersionChangeRequestCustom.cpp:47 > +void JSIDBVersionChangeRequest::visitChildren(JSCell* cell, JSC::SlotVisitor& visitor) > { > - // FIXME: Implement this. > + JSIDBVersionChangeRequest* jsRequest = jsCast<JSIDBVersionChangeRequest*>(cell); > + ASSERT_GC_OBJECT_INHERITS(jsRequest, &s_info); > + COMPILE_ASSERT(StructureFlags & OverridesVisitChildren, OverridesVisitChildrenWithoutSettingFlag); > + ASSERT(jsRequest->structure()->typeInfo().overridesVisitChildren()); > + Base::visitChildren(jsRequest, visitor); > + static_cast<IDBVersionChangeRequest*>(jsRequest->impl())->visitJSEventListeners(visitor); > } Ditto.
Joshua Bell
Comment 7 2012-12-03 09:16:05 PST
Comment on attachment 176820 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176820&action=review >> Source/WebCore/bindings/js/IDBBindingUtilities.cpp:228 >> + APIEntryShim entryShim(exec); > > You can use DOMRequestState::Scope. > > BTW, V8 binding isn't creating a scope of DOMRequestState::Scope, which looks wrong. All of the call sites to this function are within a script scope already, so dcarney didn't add it here. I agree it's odd for what looks like a general purpose utility function. >> Source/WebCore/bindings/js/IDBBindingUtilities.cpp:260 >> + APIEntryShim entryShim(exec); > > You can use DOMRequestState::Scope. > > BTW, V8 binding isn't creating a scope of DOMRequestState::Scope, which looks wrong. Ditto. >> Source/WebCore/bindings/js/IDBBindingUtilities.cpp:278 >> + APIEntryShim entryShim(exec); > > You can use DOMRequestState::Scope. > > BTW, V8 binding isn't creating a scope of DOMRequestState::Scope, which looks wrong. And this is only called from places that have already set up a DOMRequestState::Scope. >> Source/WebCore/bindings/js/JSIDBAnyCustom.cpp:-88 >> - return idbAny->serializedScriptValue()->deserialize(exec, globalObject); > > Why can you remove this? SerializedScriptValues were removed from IDBAny in favor of just ScriptValues, to avoid unnecessary de/serializations.
Michael Pruett
Comment 8 2012-12-04 17:56:31 PST
Created attachment 177628 [details] Patch With this latest patch, I've removed APIEntryShim scopes from IDBBindingUtilities.cpp and removed customized bindings for IDBOpenDBRequest and IDBVersionChangeRequest.
Michael Pruett
Comment 9 2012-12-04 18:14:48 PST
(In reply to comment #6) > > Source/WebCore/bindings/js/IDBBindingUtilities.cpp:130 > > + if (currentValue.isString() && keyPathElements[i] == "length") > > + return jsNumber(currentValue.toString(exec)->length()); > > + if (!currentValue.isObject()) > > + return jsUndefined(); > > This behavior looks different from V8's one. Thanks, I neglected to remove this unnecessary code when I cleaned up this method. > > Source/WebCore/bindings/js/IDBBindingUtilities.cpp:148 > > + if (jsValue.isUndefined()) > > This should be jsValue.isEmpty()? I've used jsUndefined() as the error return value in ensureNthValueOnKeyPath() and getNthValueOnKeyPath(), so I believe the appropriate test is isUndefined() rather than isEmpty(). > > Source/WebCore/bindings/js/IDBBindingUtilities.cpp:228 > > + ExecState* exec = requestState->exec(); > > + APIEntryShim entryShim(exec); > > You can use DOMRequestState::Scope. > > BTW, V8 binding isn't creating a scope of DOMRequestState::Scope, which looks wrong. I've removed all APIEntryShim scopes from IDBBindingUtilities.cpp. The V8 implementation of IDBBindingUtilities.cpp assumes that the proper DOMRequestState scope has already been established in the core IDB code, and the same now holds true for the JSC implementation. > > Source/WebCore/bindings/js/JSIDBAnyCustom.cpp:88 > > + case IDBAny::StringType: > > + return jsStringWithCache(exec, idbAny->string()); > > Just to confirm: Is there any reason why you moved this code from above to here? Sorry, I've undone this unnecessary change. > > Source/WebCore/bindings/js/JSIDBOpenDBRequestCustom.cpp:48 > > +void JSIDBOpenDBRequest::visitChildren(JSCell* cell, JSC::SlotVisitor& visitor) > > +{ > > + JSIDBOpenDBRequest* jsRequest = jsCast<JSIDBOpenDBRequest*>(cell); > > + ASSERT_GC_OBJECT_INHERITS(jsRequest, &s_info); > > + COMPILE_ASSERT(StructureFlags & OverridesVisitChildren, OverridesVisitChildrenWithoutSettingFlag); > > + ASSERT(jsRequest->structure()->typeInfo().overridesVisitChildren()); > > + Base::visitChildren(jsRequest, visitor); > > + static_cast<IDBOpenDBRequest*>(jsRequest->impl())->visitJSEventListeners(visitor); > > +} > > Why do you need this? Event listeners are already cared by JS bindings, aren't they? With the binding generator change in bug 103908, the visitChildren() methods of IDBOpenDBRequest and IDBVersionChangeRequest are now automatically generated, and these custom visitChildren() methods are unnecessary.
Build Bot
Comment 10 2012-12-04 20:16:05 PST
Kentaro Hara
Comment 11 2012-12-04 20:26:58 PST
Comment on attachment 177628 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=177628&action=review Looks OK. This will behave consistently with V8's one. > > > Source/WebCore/bindings/js/IDBBindingUtilities.cpp:228 > > > + ExecState* exec = requestState->exec(); > > > + APIEntryShim entryShim(exec); > > > > You can use DOMRequestState::Scope. > > > > BTW, V8 binding isn't creating a scope of DOMRequestState::Scope, which looks wrong. > > I've removed all APIEntryShim scopes from IDBBindingUtilities.cpp. The V8 implementation of IDBBindingUtilities.cpp assumes that the proper DOMRequestState scope has already been established in the core IDB code, and the same now holds true for the JSC implementation. OK, then why does V8 binding need to create 'v8::HandleScope handleScope;' in IDBBindingUtilities.cpp ? (e.g. createIDBKeyFromScriptValueAndKeyPath()) BTW, maybe I'm wrong but do we really need DOMRequestState? What we need is just to keep a ScriptExecutionContext, isn't it? > Source/WebCore/bindings/js/IDBBindingUtilities.cpp:87 > + unsigned length = array->length(); Nit: unsigned => size_t The same comment for all unsigneds in this patch. > Source/WebCore/bindings/js/IDBBindingUtilities.cpp:247 > + return canInjectNthValueOnKeyPath(exec, scriptValue.jsValue(), > + keyPathElements, keyPathElements.size() - 1); Nit: Don't wrap the line.
Kentaro Hara
Comment 12 2012-12-04 20:30:08 PST
(In reply to comment #10) > (From update of attachment 177628 [details]) > Attachment 177628 [details] did not pass mac-ews (mac): > Output: http://queues.webkit.org/results/15132726 It looks like you need to remove JSIDBVersionChangeRequestCustom.cpp from Xcode build files. Please grep "JSIDBVersionChangeRequestCustom" under WebCore/.
Kentaro Hara
Comment 13 2012-12-04 20:42:26 PST
(In reply to comment #11) > > I've removed all APIEntryShim scopes from IDBBindingUtilities.cpp. The V8 implementation of IDBBindingUtilities.cpp assumes that the proper DOMRequestState scope has already been established in the core IDB code, and the same now holds true for the JSC implementation. > > BTW, maybe I'm wrong but do we really need DOMRequestState? What we need is just to keep a ScriptExecutionContext, isn't it? We discussed offline. We need DOMRequestState. Ignore this comment:)
Michael Pruett
Comment 14 2012-12-04 21:20:54 PST
Created attachment 177663 [details] Patch I've removed JSIDBVersionChangeRequestCustom.cpp from WebCore.xcodeproj and have changed unsigned to size_t for array lengths.
WebKit Review Bot
Comment 15 2012-12-05 07:38:39 PST
Comment on attachment 177663 [details] Patch Clearing flags on attachment: 177663 Committed r136686: <http://trac.webkit.org/changeset/136686>
WebKit Review Bot
Comment 16 2012-12-05 07:38:45 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.