Bug 103484 – IndexedDB: Implement IndexedDB bindings for JSC

• Component: New Bugs
• Status: RESOLVED FIXED

[Michael Pruett]

Implement IndexedDB bindings for JSC.

[Michael Pruett]

Created attachment 176519 [details]
Patch

[Joshua Bell]

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]

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

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

Created attachment 176820 [details]
Patch

I've updated the patch to incorporate the suggestions I've received thus far.

[Kentaro Hara]

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 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]

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]

(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 on attachment 177628 [details]
Patch

Attachment 177628 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15132726

[Kentaro Hara]

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]

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

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

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 on attachment 177663 [details]
Patch

Clearing flags on attachment: 177663

Committed r136686: <http://trac.webkit.org/changeset/136686>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.