Bug 92434 – IndexedDB: IDB*::keyPath should return IDBKeyPath, not IDBAny

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Alec Flett]

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.

[Alec Flett]

Created attachment 158410 [details]
Patch

[Alec Flett]

jsbell@ - maybe you remember this one...

[Joshua Bell]

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();

[Alec Flett]

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 :)

[Alec Flett]

Created attachment 158444 [details]
Patch

[Alec Flett]

tony@ - r?

[Tony Chang]

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.

[Alec Flett]

Created attachment 158582 [details]
Patch

[Alec Flett]

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.

[WebKit Review Bot]

Comment on attachment 158582 [details]
Patch

Clearing flags on attachment: 158582

Committed r125730: <http://trac.webkit.org/changeset/125730>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.