Bug 163909 - Replace IDBKeyPath with a WTF::Variant
Summary: Replace IDBKeyPath with a WTF::Variant
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords:
Depends on: 163916 163935
Blocks:
  Show dependency treegraph
 
Reported: 2016-10-24 13:56 PDT by Chris Dumez
Modified: 2016-10-26 20:16 PDT (History)
6 users (show)

See Also:


Attachments
WIP patch (41.00 KB, patch)
2016-10-25 20:04 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (49.83 KB, patch)
2016-10-25 20:45 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (49.75 KB, patch)
2016-10-26 19:19 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2016-10-24 13:56:50 PDT
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.
Comment 1 Chris Dumez 2016-10-25 20:04:00 PDT
Created attachment 292869 [details]
WIP patch
Comment 2 Chris Dumez 2016-10-25 20:45:57 PDT
Created attachment 292871 [details]
Patch
Comment 3 Darin Adler 2016-10-26 16:38:41 PDT
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?
Comment 4 Chris Dumez 2016-10-26 16:46:00 PDT
(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.
Comment 5 Darin Adler 2016-10-26 16:58:46 PDT
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.
Comment 6 Darin Adler 2016-10-26 17:36:09 PDT
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.
Comment 7 Chris Dumez 2016-10-26 19:19:01 PDT
(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.
Comment 8 Chris Dumez 2016-10-26 19:19:48 PDT
Created attachment 292984 [details]
Patch
Comment 9 WebKit Commit Bot 2016-10-26 20:16:13 PDT
Comment on attachment 292984 [details]
Patch

Clearing flags on attachment: 292984

Committed r207931: <http://trac.webkit.org/changeset/207931>
Comment 10 WebKit Commit Bot 2016-10-26 20:16:18 PDT
All reviewed patches have been landed.  Closing bug.