Bug 164504 – IndexedDB 2.0: Encapsulate cursor iteration parameters for easy future expansion

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

[Brady Eidson]

IndexedDB 2.0: Encapsulate cursor iteration parameters for easy future expansion

This is needed to support https://bugs.webkit.org/show_bug.cgi?id=164404

[Brady Eidson]

Created attachment 294136 [details]
Patch

[Darin Adler]

Comment on attachment 294136 [details]
Patch

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

> Source/WebCore/Modules/indexeddb/shared/IDBIterateCursorData.h:31
> +#include <wtf/Optional.h>

No need for this include. Optional is not used in this file.

> Source/WebCore/Modules/indexeddb/shared/IDBIterateCursorData.h:35
> +struct IDBIterateCursorData {

Where does this name come from? The phrase "iterate cursor data" seems grammatically peculiar, since "iterate" is a verb. I guess it’s "'iterate cursor' function data"? Some places you called it "cursor iteration data". Is there a phrase in more colloquial normal language that describes what this is?

> Source/WebCore/Modules/indexeddb/shared/IDBIterateCursorData.h:37
> +    unsigned long count;

In WebKit we use "unsigned" for this type, not "unsigned long". My guess is someone was working with IDL, and in IDL, the type is "unsigned long", and so that’s why this type is used.

> Source/WebCore/Modules/indexeddb/shared/IDBIterateCursorData.h:42
> +    template<class Encoder> void encode(Encoder&) const;
> +    template<class Decoder> static bool decode(Decoder&, IDBIterateCursorData&);

Do these need to be members? Is that the usual Encoder/Decoder convention?

> Source/WebCore/Modules/indexeddb/shared/IDBIterateCursorData.h:48
> +    encoder << keyData << static_cast<uint64_t>(count);

Why uint64_t instead of uint32_t?

> Source/WebCore/Modules/indexeddb/shared/IDBIterateCursorData.h:64
> +    uint64_t count;
> +    if (!decoder.decode(count))
> +        return false;
> +
> +    if (count > std::numeric_limits<unsigned long>::max())
> +        return false;
> +
> +    iteratorCursorData.count = static_cast<unsigned long>(count);

This seems like an unnecessarily complicated way to encode/decode a 32-bit integer.

[Brady Eidson]

(In reply to comment #2)
> Comment on attachment 294136 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=294136&action=review
> 
> > Source/WebCore/Modules/indexeddb/shared/IDBIterateCursorData.h:31
> > +#include <wtf/Optional.h>
> 
> No need for this include. Optional is not used in this file.

Done.

> 
> > Source/WebCore/Modules/indexeddb/shared/IDBIterateCursorData.h:35
> > +struct IDBIterateCursorData {
> 
> Where does this name come from? The phrase "iterate cursor data" seems
> grammatically peculiar, since "iterate" is a verb. I guess it’s "'iterate
> cursor' function data"? Some places you called it "cursor iteration data".
> Is there a phrase in more colloquial normal language that describes what
> this is?

This is a convention used with other examples such as "GetRecordData" and "GetAllRecordsData", meaning "data for GetRecord" and "data for GetAllRecords", as you figured with "data for IterateCursor"

I only happened to call it "cursor iteration parameters" in the bugzilla title (And therefore the changelog)

I'm not sure that's better...?

> 
> > Source/WebCore/Modules/indexeddb/shared/IDBIterateCursorData.h:37
> > +    unsigned long count;
> 
> In WebKit we use "unsigned" for this type, not "unsigned long". My guess is
> someone was working with IDL, and in IDL, the type is "unsigned long", and
> so that’s why this type is used.

True.

> > Source/WebCore/Modules/indexeddb/shared/IDBIterateCursorData.h:42
> > +    template<class Encoder> void encode(Encoder&) const;
> > +    template<class Decoder> static bool decode(Decoder&, IDBIterateCursorData&);
> 
> Do these need to be members? Is that the usual Encoder/Decoder convention?

It is the usual convention of how we expose WK2's encoder/decoder functionality to WebCore.

They do need to be members to support all of the automatic template magic behind ArgumentEncoder/Decoder as they fallback to calling ::encode and ::decode on the object they're trying to work with.

> 
> > Source/WebCore/Modules/indexeddb/shared/IDBIterateCursorData.h:48
> > +    encoder << keyData << static_cast<uint64_t>(count);
> 
> Why uint64_t instead of uint32_t?

The same encoder/decoder template magic mentioned above only "automatically" knows how to encode explicitly 64-bit integers.

I'm not *sure* why this is, but suspect it's related to the magic to automatically encode/decode enum types, and preventing that from causing specialization ambiguity.

But, again, not sure - I just know I'm used to jumping through this hoop because, without doing so, I get compiler errors with it trying to call "::encode" on the type unsigned.

> 
> > Source/WebCore/Modules/indexeddb/shared/IDBIterateCursorData.h:64
> > +    uint64_t count;
> > +    if (!decoder.decode(count))
> > +        return false;
> > +
> > +    if (count > std::numeric_limits<unsigned long>::max())
> > +        return false;
> > +
> > +    iteratorCursorData.count = static_cast<unsigned long>(count);
> 
> This seems like an unnecessarily complicated way to encode/decode a 32-bit
> integer.

Agreed, ditto to the above but with ::decode on the integer type.

I'll look in to other options before landing this.

[Brady Eidson]

https://trac.webkit.org/changeset/208486

I'm open to renaming the new object (and the other IDB objects with similar naming schemes) in a followup if we come up with good names!