Bug 103484 - IndexedDB: Implement IndexedDB bindings for JSC
Summary: IndexedDB: Implement IndexedDB bindings for JSC
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 103554
Blocks: 45110
  Show dependency treegraph
 
Reported: 2012-11-27 23:05 PST by Michael Pruett
Modified: 2012-12-05 07:38 PST (History)
12 users (show)

See Also:


Attachments
Patch (27.22 KB, patch)
2012-11-28 10:47 PST, Michael Pruett
no flags Details | Formatted Diff | Diff
Patch (27.63 KB, patch)
2012-11-29 14:38 PST, Michael Pruett
haraken: review-
Details | Formatted Diff | Diff
Patch (24.87 KB, patch)
2012-12-04 17:56 PST, Michael Pruett
haraken: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
Patch (28.79 KB, patch)
2012-12-04 21:20 PST, Michael Pruett
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Pruett 2012-11-27 23:05:13 PST
Implement IndexedDB bindings for JSC.
Comment 1 Michael Pruett 2012-11-28 10:47:11 PST
Created attachment 176519 [details]
Patch
Comment 2 Joshua Bell 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).
Comment 3 Michael Pruett 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.
Comment 4 Joshua Bell 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.
Comment 5 Michael Pruett 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.
Comment 6 Kentaro Hara 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.
Comment 7 Joshua Bell 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.
Comment 8 Michael Pruett 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.
Comment 9 Michael Pruett 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.
Comment 10 Build Bot 2012-12-04 20:16:05 PST
Comment on attachment 177628 [details]
Patch

Attachment 177628 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15132726
Comment 11 Kentaro Hara 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.
Comment 12 Kentaro Hara 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/.
Comment 13 Kentaro Hara 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:)
Comment 14 Michael Pruett 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.
Comment 15 WebKit Review Bot 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>
Comment 16 WebKit Review Bot 2012-12-05 07:38:45 PST
All reviewed patches have been landed.  Closing bug.