Bug 78399 - IndexedDB: Support overloaded methods that take IDBKey or IDBKeyRange
Summary: IndexedDB: Support overloaded methods that take IDBKey or IDBKeyRange
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: Joshua Bell
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-02-10 16:17 PST by Joshua Bell
Modified: 2012-02-23 16:51 PST (History)
10 users (show)

See Also:


Attachments
Patch (4.46 KB, patch)
2012-02-10 16:17 PST, Joshua Bell
no flags Details | Formatted Diff | Diff
Patch (36.01 KB, patch)
2012-02-13 16:31 PST, Joshua Bell
no flags Details | Formatted Diff | Diff
Patch for landing (36.05 KB, patch)
2012-02-17 13:02 PST, Joshua Bell
no flags Details | Formatted Diff | Diff
Patch for landing (35.53 KB, patch)
2012-02-17 13:15 PST, Joshua Bell
no flags Details | Formatted Diff | Diff
Patch (17.22 KB, patch)
2012-02-23 16:49 PST, Joshua Bell
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joshua Bell 2012-02-10 16:17:12 PST
IndexedDB: Support overloaded methods that take IDBKey or IDBKeyRange
Comment 1 Joshua Bell 2012-02-10 16:17:34 PST
Created attachment 126600 [details]
Patch
Comment 2 Joshua Bell 2012-02-10 16:18:10 PST
Comment on attachment 126600 [details]
Patch

This is just a stub, to demonstrate the binding code generator and IDL changes that would be required.
Comment 3 Joshua Bell 2012-02-10 16:26:51 PST
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()
Comment 4 Kentaro Hara 2012-02-10 17:06:43 PST
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?
Comment 5 WebKit Review Bot 2012-02-10 17:21:00 PST
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
Comment 6 Joshua Bell 2012-02-10 17:23:53 PST
(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.
Comment 7 Joshua Bell 2012-02-13 11:20:15 PST
> > - 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.
Comment 8 Kentaro Hara 2012-02-13 16:12:22 PST
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&);
Comment 9 Joshua Bell 2012-02-13 16:27:48 PST
(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.
Comment 10 Joshua Bell 2012-02-13 16:31:50 PST
Created attachment 126862 [details]
Patch
Comment 11 Kentaro Hara 2012-02-13 16:35:47 PST
(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.
Comment 12 WebKit Review Bot 2012-02-13 16:37:40 PST
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
Comment 13 Joshua Bell 2012-02-13 16:38:09 PST
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.
Comment 14 Kentaro Hara 2012-02-13 16:40:14 PST
r+ for the code generator change and IDL change. Would anyone review Indexed DB changes?
Comment 15 Darin Fisher (:fishd, Google) 2012-02-14 11:22:19 PST
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
Comment 16 Tony Chang 2012-02-14 16:21:46 PST
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.
Comment 17 Joshua Bell 2012-02-17 12:41:47 PST
(In reply to comment #16)
> Nit: I think assigning to a bool and using ASSERT_UNUSED would be a bit clearer here.

Done.
Comment 18 Joshua Bell 2012-02-17 13:02:38 PST
Created attachment 127637 [details]
Patch for landing
Comment 19 Joshua Bell 2012-02-17 13:15:41 PST
Created attachment 127641 [details]
Patch for landing
Comment 20 WebKit Review Bot 2012-02-17 18:46:32 PST
Comment on attachment 127641 [details]
Patch for landing

Clearing flags on attachment: 127641

Committed r108150: <http://trac.webkit.org/changeset/108150>
Comment 21 WebKit Review Bot 2012-02-17 18:46:38 PST
All reviewed patches have been landed.  Closing bug.
Comment 22 Joshua Bell 2012-02-23 16:49:53 PST
Reopening to attach new patch.
Comment 23 Joshua Bell 2012-02-23 16:49:55 PST
Created attachment 128603 [details]
Patch
Comment 24 Joshua Bell 2012-02-23 16:51:42 PST
Comment on attachment 128603 [details]
Patch

Oops, ignore this attachment. Need a new bug.