Bug 163909 – Replace IDBKeyPath with a WTF::Variant

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Chris Dumez]

Replace IDBKeyPath with a WTF::Variant and use a (DOMString or sequence<DOMString>) union on IDL side, as per the specification:
- https://www.w3.org/TR/IndexedDB/#idl-def-IDBObjectStoreParameters
- https://www.w3.org/TR/IndexedDB/#dfn-valid-key-path

I'll likely go incrementally.

[Chris Dumez]

Created attachment 292869 [details]
WIP patch

[Chris Dumez]

Created attachment 292871 [details]
Patch

[Darin Adler]

Comment on attachment 292871 [details]
Patch

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

> Source/WebCore/Modules/indexeddb/IDBKeyPath.h:37
> +bool isIDBKeyPathValid(const IDBKeyPath&);

This function does need IDBKeyPath in its name, because it’s not something that is a general operation on any variant that happens to have a string plus a vector of strings.

> Source/WebCore/Modules/indexeddb/IDBKeyPath.h:48
> +IDBKeyPath IDBKeyPathIsolatedCopy(const IDBKeyPath&);
> +inline Optional<IDBKeyPath> IDBKeyPathIsolatedCopy(const Optional<IDBKeyPath>& variant)

The functions in this overloaded pair do not need IDBKeyPath in their names and should just be named isolatedCopy. The algorithm would be correct for any other variant combining String and Vector<String>, this could be overloaded for other types, and in fact we might eventually want to replace this with a generic implementation that can traverse the variants and do isolated copy on each piece.

> Source/WebCore/Modules/indexeddb/server/IDBSerialization.h:38
> +RefPtr<SharedBuffer> serializeIDBKeyPath(const Optional<IDBKeyPath>&);

Why does this return RefPtr, not Ref? Can it return null?

[Chris Dumez]

(In reply to comment #3)
> Comment on attachment 292871 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=292871&action=review
> 
> > Source/WebCore/Modules/indexeddb/IDBKeyPath.h:37
> > +bool isIDBKeyPathValid(const IDBKeyPath&);
> 
> This function does need IDBKeyPath in its name, because it’s not something
> that is a general operation on any variant that happens to have a string
> plus a vector of strings.
> 
> > Source/WebCore/Modules/indexeddb/IDBKeyPath.h:48
> > +IDBKeyPath IDBKeyPathIsolatedCopy(const IDBKeyPath&);
> > +inline Optional<IDBKeyPath> IDBKeyPathIsolatedCopy(const Optional<IDBKeyPath>& variant)
> 
> The functions in this overloaded pair do not need IDBKeyPath in their names
> and should just be named isolatedCopy. The algorithm would be correct for
> any other variant combining String and Vector<String>, this could be
> overloaded for other types, and in fact we might eventually want to replace
> this with a generic implementation that can traverse the variants and do
> isolated copy on each piece.

I initially called it isolatedCopy(). However, this is usually called from classes that already have an isolatedCopy() method and the compiler complains about it. The compiler want me to call ::WebCore::isolatedCopy() then, which looked horrible. This is why I renamed. What do you think?

> 
> > Source/WebCore/Modules/indexeddb/server/IDBSerialization.h:38
> > +RefPtr<SharedBuffer> serializeIDBKeyPath(const Optional<IDBKeyPath>&);
> 
> Why does this return RefPtr, not Ref? Can it return null?

It relies on KeyedEncoder.finishEncoding() internally which returns a PassRefPtr at the moment :( It seems KeyedEncoder.finishEncoding() can return null if CFPropertyListCreateData() return null.

[Darin Adler]

Comment on attachment 292871 [details]
Patch

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

>>> Source/WebCore/Modules/indexeddb/IDBKeyPath.h:37
>>> +bool isIDBKeyPathValid(const IDBKeyPath&);
>> 
>> This function does need IDBKeyPath in its name, because it’s not something that is a general operation on any variant that happens to have a string plus a vector of strings.
> 
> I initially called it isolatedCopy(). However, this is usually called from classes that already have an isolatedCopy() method and the compiler complains about it. The compiler want me to call ::WebCore::isolatedCopy() then, which looked horrible. This is why I renamed. What do you think?

You can use WebCore::isolatedCopy; don’t need the extra :: prefix, and I probably would do that.

Longer term I think an elegant solution is to use the overall pattern of a free function named isolatedCopy rather than a member function, using friend as needed, which can be used on a whole data structure or on a single object like a String.

But using this key-path-specific name in the short term is also OK, even though I do not think it is forward looking. Easy to rename later.

[Darin Adler]

Comment on attachment 292871 [details]
Patch

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

>>> Source/WebCore/Modules/indexeddb/server/IDBSerialization.h:38
>>> +RefPtr<SharedBuffer> serializeIDBKeyPath(const Optional<IDBKeyPath>&);
>> 
>> Why does this return RefPtr, not Ref? Can it return null?
> 
> It relies on KeyedEncoder.finishEncoding() internally which returns a PassRefPtr at the moment :( It seems KeyedEncoder.finishEncoding() can return null if CFPropertyListCreateData() return null.

Here’s what I do in situations like this: I try to find out if the callers check for null. If they don’t, then it’s just about moving the null-dereference crash around. So I try to move it down to as low level as possible.

If some callers check for null, then I try to make sure that either all of them do it, or there is some rationale instead.

[Chris Dumez]

(In reply to comment #6)
> Comment on attachment 292871 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=292871&action=review
> 
> >>> Source/WebCore/Modules/indexeddb/server/IDBSerialization.h:38
> >>> +RefPtr<SharedBuffer> serializeIDBKeyPath(const Optional<IDBKeyPath>&);
> >> 
> >> Why does this return RefPtr, not Ref? Can it return null?
> > 
> > It relies on KeyedEncoder.finishEncoding() internally which returns a PassRefPtr at the moment :( It seems KeyedEncoder.finishEncoding() can return null if CFPropertyListCreateData() return null.
> 
> Here’s what I do in situations like this: I try to find out if the callers
> check for null. If they don’t, then it’s just about moving the
> null-dereference crash around. So I try to move it down to as low level as
> possible.
> 
> If some callers check for null, then I try to make sure that either all of
> them do it, or there is some rationale instead.

All call sites check for null.

[Chris Dumez]

Created attachment 292984 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 292984 [details]
Patch

Clearing flags on attachment: 292984

Committed r207931: <http://trac.webkit.org/changeset/207931>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.