IndexedDB: Support overloaded methods that take IDBKey or IDBKeyRange

• Component: New Bugs
• Status: RESOLVED FIXED

[Joshua Bell]

IndexedDB: Support overloaded methods that take IDBKey or IDBKeyRange

[Joshua Bell]

Created attachment 126600 [details]
Patch

[Joshua Bell]

Comment on attachment 126600 [details]
Patch

This is just a stub, to demonstrate the binding code generator and IDL changes that would be required.

[Joshua Bell]

The following Chromium bugs depend on this:

http://crbug.com/101384 - IDBObjectStore.delete()
http://crbug.com/107916 - IDBObjectStore.count() / IDBIndex.count()
http://crbug.com/92046 - IDBObjectStore.openCursor() / IDBIndex.openCursor() / IDBIndex.openKeyCursor()
http://crbug.com/92047 - IDBObjectStore.get() / IDBIndex.get() / IDBIndex.getKey()

[Kentaro Hara]

Comment on attachment 126600 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=126600&action=review

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3212
> +        if ($parameter->type eq "IDBKey" || $parameter->type eq "NodeFilter" || $parameter->type eq "XPathNSResolver") {

- I am not sure why this change becomes necessary NOW. How has the binding for add(..., PassRefPtr<IDBKey> key, ...) or put(..., PassRefPtr<IDBKey> key, ...) worked so far?
- If possible, we do not want to hard-code '... eq "IDLKey"' in code generators. Can we change the method signature to deleteFunction(ScriptExecutionContext*, IDBKey*, ExceptionCode&) so that the hard-coding goes away?
- Don't we need similar change in CodeGeneratorJS.pm?
- How about IDBKeyRange?

[WebKit Review Bot]

Comment on attachment 126600 [details]
Patch

Attachment 126600 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11487911

New failing tests:
storage/indexeddb/objectStore-required-arguments.html

[Joshua Bell]

(In reply to comment #4)
> (From update of attachment 126600 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=126600&action=review
> 
> > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3212
> > +        if ($parameter->type eq "IDBKey" || $parameter->type eq "NodeFilter" || $parameter->type eq "XPathNSResolver") {
> 
> - I am not sure why this change becomes necessary NOW. How has the binding for add(..., PassRefPtr<IDBKey> key, ...) or put(..., PassRefPtr<IDBKey> key, ...) worked so far?

The overloads take PassRefPtr<IDBKeyRange> and PassRefPtr<IDBKey> respectively. Passing RefPtr<IDBKey> in results in an ambiguity since these both derive from a common base.

> - If possible, we do not want to hard-code '... eq "IDLKey"' in code generators. Can we change the method signature to deleteFunction(ScriptExecutionContext*, IDBKey*, ExceptionCode&) so that the hard-coding goes away?

That probably works too - I'll play around. I agree, hard-coding is evil. This CL is more a stake in the ground to investigate further than a serious suggestion (which is why I removed the r? flag), since a minimal change had eluded me when I looked at it a few months ago.

> - Don't we need similar change in CodeGeneratorJS.pm?

The IDB implementation is only enabled in Chromium/V8, so nothing builds via that path. Adding similar changes would be flying blind.

> - How about IDBKeyRange?

Looking at the generated code, it's held locally in an IDBKeyRange* not a RefPtr<> so the overload isn't ambiguous when called that way.

[Joshua Bell]

> > - If possible, we do not want to hard-code '... eq "IDLKey"' in code generators. Can we change the method signature to deleteFunction(ScriptExecutionContext*, IDBKey*, ExceptionCode&) so that the hard-coding goes away?
> 
> That probably works too - I'll play around. I agree, hard-coding is evil. 

Nope, doesn't work. The generated code is:

  EXCEPTION_BLOCK(RefPtr<IDBKey>, key, createIDBKeyFromValue(MAYBE_MISSING_PARAMETER(args, 0, DefaultIsUndefined)));
  ...
  RefPtr<IDBRequest> result = imp->deleteFunction(scriptContext, key, ec);

It looks like the types in that if() clause - IDBKey, NodeFilter, and XPathNSResolver are all types that may be created within the callback function. See notes elsewhere in the script...

  # temporary hack
  return "RefPtr<NodeFilter>" if $type eq "NodeFilter";
  ...  
  # necessary as resolvers could be constructed on fly.
  return "RefPtr<XPathNSResolver>" if $type eq "XPathNSResolver";

Possibly refactor and introduce a IsConstructedInCallback() function that is tested instead? Other suggestions appreciated.

[Kentaro Hara]

Thanks jsbell.

(In reply to comment #7)
>   EXCEPTION_BLOCK(RefPtr<IDBKey>, key, createIDBKeyFromValue(MAYBE_MISSING_PARAMETER(args, 0, DefaultIsUndefined)));
>   ...
>   RefPtr<IDBRequest> result = imp->deleteFunction(scriptContext, key, ec);
> 
> It looks like the types in that if() clause - IDBKey, NodeFilter, and XPathNSResolver are all types that may be created within the callback function. See notes elsewhere in the script...
> 
>   # temporary hack
>   return "RefPtr<IDBKey>" if $type eq "IDBKey";

I am not still sure why the native type of IDBKey needs to be RefPtr<IDBKey>, instead of IDBKey.

Would it be possible to change all "RefPtr<IDBKey>"s to "IDBKey"s in IDBObjectStore.h? For example, would it require a big change in WebCore implementation if we try to change the following current signatures:

    PassRefPtr<IDBRequest> get(ScriptExecutionContext*, PassRefPtr<IDBKey>, ExceptionCode&);
    PassRefPtr<IDBRequest> add(ScriptExecutionContext*, PassRefPtr<SerializedScriptValue>, PassRefPtr<IDBKey>, ExceptionCode&);
    PassRefPtr<IDBRequest> put(ScriptExecutionContext*, PassRefPtr<SerializedScriptValue>, PassRefPtr<IDBKey>, ExceptionCode&);

to the following signatures?:

    PassRefPtr<IDBRequest> get(ScriptExecutionContext*, IDBKey*, ExceptionCode&);
    PassRefPtr<IDBRequest> add(ScriptExecutionContext*, PassRefPtr<SerializedScriptValue>, IDBKey*, ExceptionCode&);
    PassRefPtr<IDBRequest> put(ScriptExecutionContext*, PassRefPtr<SerializedScriptValue>, IDBKey*, ExceptionCode&);

[Joshua Bell]

(In reply to comment #8)
> Thanks jsbell.
> 
> (In reply to comment #7)
> >   EXCEPTION_BLOCK(RefPtr<IDBKey>, key, createIDBKeyFromValue(MAYBE_MISSING_PARAMETER(args, 0, DefaultIsUndefined)));
> >   ...
> >   RefPtr<IDBRequest> result = imp->deleteFunction(scriptContext, key, ec);
> > 
> > It looks like the types in that if() clause - IDBKey, NodeFilter, and XPathNSResolver are all types that may be created within the callback function. See notes elsewhere in the script...
> > 
> >   # temporary hack
> >   return "RefPtr<IDBKey>" if $type eq "IDBKey";
> 
> I am not still sure why the native type of IDBKey needs to be RefPtr<IDBKey>, instead of IDBKey.

The createIDBKeyFromValue() is actually creating an new IDBKey; something needs to hold on to it, doesn't it?
 
> Would it be possible to change all "RefPtr<IDBKey>"s to "IDBKey"s in IDBObjectStore.h? 

That's fine but doesn't help. With *only* that change, the code generator tries to pass a RefPtr<IDBKey> to an IDBKey* argument. We still need the generated function to both (1) hold a RefPtr<T> and (2) pass a non-RefPtr (either T* or PassRefPtr<T>). 

> For example, would it require a big change in WebCore implementation if we try to change the following current signatures:

If this turns out to be what we want it's not a problem, these methods are only called from script via the IDL.

[Joshua Bell]

Created attachment 126862 [details]
Patch

[Kentaro Hara]

(In reply to comment #9)
> > I am not still sure why the native type of IDBKey needs to be RefPtr<IDBKey>, instead of IDBKey.
> 
> The createIDBKeyFromValue() is actually creating an new IDBKey; something needs to hold on to it, doesn't it?

Thank you for the clarification. From what we observed, it seems to me that allowing the "IDBKey" hard-coding might be a good approach in this patch.

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[Joshua Bell]

Comment on attachment 126862 [details]
Patch

This will need to be split up before landing into WebKit/Chromium API changes, the Chromium side, and finally the bulk of the implementation.

[Kentaro Hara]

r+ for the code generator change and IDL change. Would anyone review Indexed DB changes?

[Darin Fisher (:fishd, Google)]

Comment on attachment 126862 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=126862&action=review

> Source/WebKit/chromium/public/WebIDBObjectStore.h:72
> +    virtual void deleteFunction(const WebIDBKeyRange&, WebIDBCallbacks*, const WebIDBTransaction&, WebExceptionCode&) { WEBKIT_ASSERT_NOT_REACHED(); }

API change LGTM

[Tony Chang]

Comment on attachment 126862 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=126862&action=review

> Source/WebCore/storage/IDBObjectStoreBackendImpl.cpp:372
> +                if (!objectStore->m_backingStore->deleteIndexDataForRecord(objectStore->m_databaseId, objectStore->id(), it->second->id(), recordIdentifier.get()))
> +                    ASSERT_NOT_REACHED();

Nit: I think assigning to a bool and using ASSERT_UNUSED would be a bit clearer here.

[Joshua Bell]

(In reply to comment #16)
> Nit: I think assigning to a bool and using ASSERT_UNUSED would be a bit clearer here.

Done.

[Joshua Bell]

Created attachment 127637 [details]
Patch for landing

[Joshua Bell]

Created attachment 127641 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 127641 [details]
Patch for landing

Clearing flags on attachment: 127641

Committed r108150: <http://trac.webkit.org/changeset/108150>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Joshua Bell]

Reopening to attach new patch.

[Joshua Bell]

Created attachment 128603 [details]
Patch

[Joshua Bell]

Comment on attachment 128603 [details]
Patch

Oops, ignore this attachment. Need a new bug.