Bug 92434 - IndexedDB: IDB*::keyPath should return IDBKeyPath, not IDBAny
Summary: IndexedDB: IDB*::keyPath should return IDBKeyPath, not IDBAny
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alec Flett
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-07-26 16:02 PDT by Alec Flett
Modified: 2012-08-15 17:58 PDT (History)
5 users (show)

See Also:

Attachments
Patch (9.28 KB, patch)
2012-08-14 13:59 PDT, Alec Flett 		no flags Details | Formatted Diff | Diff
Patch (9.26 KB, patch)
2012-08-14 16:45 PDT, Alec Flett 		no flags Details | Formatted Diff | Diff
Patch (9.29 KB, patch)
2012-08-15 09:43 PDT, Alec Flett 		no flags Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Alec Flett 2012-07-26 16:02:54 PDT 
This is used from JS and from C++ - the JS (IDL) should be calling through to a keyPathAny() and the C++ should call keyPath() and get an IDBKeyPath.
Comment 1 Alec Flett 2012-08-14 13:59:57 PDT 
Created attachment 158410 [details]
Patch
Comment 2 Alec Flett 2012-08-14 14:00:25 PDT 
jsbell@ - maybe you remember this one...
Comment 3 Joshua Bell 2012-08-14 14:11:58 PDT 
Comment on attachment 158410 [details]
Patch

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

Yep, I remember. LGTM with a couple suggestions.

> Source/WebCore/ChangeLog:9
> +        objects. This gets rid of some implicit conversion from

Well, not quite. The IDBKeyPath is not held directly with a new type (like the other encapsulated objects) but decomposed into a more IDL-friendly type (null, DOMString, or DOMStringList).

Just quibbling over the comment, though - I think this is good.

> Source/WebCore/Modules/indexeddb/IDBAny.cpp:216
> +        m_type = NullType;

Manually assign m_type/m_XXX here (and below), or use setNull()/set(XXX)?

> Source/WebCore/Modules/indexeddb/IDBAny.cpp:227
> +        m_domStringList = keyPaths;

Can do keyPaths.release();
Comment 4 Alec Flett 2012-08-14 16:45:23 PDT 
Comment on attachment 158410 [details]
Patch

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

>> Source/WebCore/Modules/indexeddb/IDBAny.cpp:216
>> +        m_type = NullType;
> 
> Manually assign m_type/m_XXX here (and below), or use setNull()/set(XXX)?

I started down that route but then hit some annoying ambiguity errors, esp when converting the DOMStringList, that couldn't be fixed without some really disgusting casting. I was a little disappointed in clang, for the first time :)
Comment 5 Alec Flett 2012-08-14 16:45:34 PDT 
Created attachment 158444 [details]
Patch
Comment 6 Alec Flett 2012-08-14 16:46:32 PDT 
tony@ - r?
Comment 7 Tony Chang 2012-08-14 17:51:26 PDT 
Comment on attachment 158444 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        No new tests (OOPS!).

Can this be tested?

> Source/WebCore/Modules/indexeddb/IDBAny.h:48
>  class IDBTransaction;
>  class SerializedScriptValue;
> +class IDBKeyPath;

Nit: Sort.
Comment 8 Alec Flett 2012-08-15 09:43:00 PDT 
Created attachment 158582 [details]
Patch
Comment 9 Alec Flett 2012-08-15 09:44:38 PDT 
tony@ - nits addressed. This can't really be tested because what you'd have to tests is if some kind of implicit conversion was executed, but I've removed the conversion routines that were being invoked implicitly.
Comment 10 WebKit Review Bot 2012-08-15 17:58:09 PDT 
Comment on attachment 158582 [details]
Patch

Clearing flags on attachment: 158582

Committed r125730: <http://trac.webkit.org/changeset/125730>
Comment 11 WebKit Review Bot 2012-08-15 17:58:13 PDT 
All reviewed patches have been landed.  Closing bug.