Bug 166451 – [WebIDL] Remove (most) custom bindings for the IndexedDB code

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Sam Weinig]

Remove custom bindings for the IndexedDB code

[Sam Weinig]

Created attachment 297729 [details]
Patch

[Sam Weinig]

I am a little sad that I couldn't take this all the way. Got stuck up with IDBRequest::result(), which wanted to return a type of:

ExceptionOr<std::optional<Variant<RefPtr<IDBCursor>, RefPtr<IDBDatabase>, JSC::Strong<JSC::Unknown>>>>

The compiler really didn't like this type. So, in the interest of moving forward in reviewable chunks, and not diving into the implementation of Variant in the same patch, I present, most of it.

[Build Bot]

Comment on attachment 297729 [details]
Patch

Attachment 297729 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2777956

New failing tests:
storage/indexeddb/exceptions.html
storage/indexeddb/index-basics-private.html
storage/indexeddb/exceptions-private.html
storage/indexeddb/objectstore-cursor.html
storage/indexeddb/objectstore-cursor-private.html
storage/indexeddb/index-basics.html
storage/indexeddb/index-basics-workers.html

[Build Bot]

Created attachment 297731 [details]
Archive of layout-test-results from ews103 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 297729 [details]
Patch

Attachment 297729 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2777967

New failing tests:
storage/indexeddb/exceptions.html
storage/indexeddb/index-basics-private.html
storage/indexeddb/exceptions-private.html
storage/indexeddb/objectstore-cursor.html
storage/indexeddb/objectstore-cursor-private.html
storage/indexeddb/index-basics.html
storage/indexeddb/index-basics-workers.html

[Build Bot]

Created attachment 297732 [details]
Archive of layout-test-results from ews107 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 297729 [details]
Patch

Attachment 297729 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2777968

New failing tests:
storage/indexeddb/exceptions.html
storage/indexeddb/index-basics-private.html
storage/indexeddb/exceptions-private.html
storage/indexeddb/objectstore-cursor.html
storage/indexeddb/objectstore-cursor-private.html
storage/indexeddb/index-basics.html
storage/indexeddb/index-basics-workers.html

[Build Bot]

Created attachment 297733 [details]
Archive of layout-test-results from ews115 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Sam Weinig]

Created attachment 297749 [details]
Patch

[Darin Adler]

Comment on attachment 297749 [details]
Patch

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

r=me assuming you figure out how to fix the EFL build failure

> Source/WebCore/Modules/indexeddb/IDBCursor.cpp:112
> +IDBCursor::Source IDBCursor::source() const

I guess we are not inlining this to minimize the number of things that need to be in the header? Otherwise, seems like it would be nice to inline.

> Source/WebCore/Modules/indexeddb/IDBCursor.cpp:115
> +        return Source { m_index };

If you could omit the "Source" then I would. If you can’t then, obviously fine like this.

> Source/WebCore/Modules/indexeddb/IDBCursor.cpp:118
> +    return Source { m_objectStore };

Ditto.

> Source/WebCore/Modules/indexeddb/IDBCursor.cpp:121
> +IDBCursorDirection IDBCursor::direction() const

I guess we are not inlining this to minimize the number of things that need to be in the header? Otherwise, seems like it would be nice to inline.

> Source/WebCore/Modules/indexeddb/IDBIndex.h:59
> +    ExceptionOr<Ref<IDBRequest>> openCursor(JSC::ExecState&, IDBKeyRange*, IDBCursorDirection);

Not IDBKeyRange&?

> Source/WebCore/Modules/indexeddb/IDBKeyRange.idl:35
> -    [ImplementationReturnType=IDBKey] readonly attribute any lower;
> -    [ImplementationReturnType=IDBKey] readonly attribute any upper;
> +    // NOTE: The 'lower' and 'upper' attributes are specified to be of type 'any'
> +    // but we use the extension type IDBKey, to allow code generator to know our
> +    // which conversion to use based on our actual implementation type.
> +    readonly attribute IDBKey lower;
> +    readonly attribute IDBKey upper;

Is the new syntax better than the old? I’m not 100% sure.

> Source/WebCore/Modules/indexeddb/IDBObjectStore.cpp:165
> +    auto info = IDBCursorInfo::objectStoreCursor(m_transaction, m_info.identifier(), range.get(), direction, IndexedDB::CursorType::KeyAndValue);
>      return m_transaction.requestOpenCursor(execState, *this, info);

To a casual reader it seems like it should be WTFMove(info).

> Source/WebCore/Modules/indexeddb/IDBObjectStore.cpp:189
> +    auto info = IDBCursorInfo::objectStoreCursor(m_transaction, m_info.identifier(), range.get(), direction, IndexedDB::CursorType::KeyOnly);
>      return m_transaction.requestOpenCursor(execState, *this, info);

Ditto.

> Source/WebCore/Modules/indexeddb/IDBObjectStore.h:75
> +    ExceptionOr<Ref<IDBRequest>> openCursor(JSC::ExecState&, RefPtr<IDBKeyRange>, IDBCursorDirection);

A little surprising to see RefPtr<IDBKeyRange> here instead of IDBKeyRange*.

> Source/WebCore/Modules/indexeddb/IDBRequest.cpp:110
> +    auto source = cursor.source();
> +    WTF::switchOn(source,

Probably reads better without the local variable.

> Source/WebCore/Modules/indexeddb/IDBRequest.cpp:112
> +        [&] (RefPtr<IDBIndex>& index) { this->m_indexSource = index; },
> +        [&] (RefPtr<IDBObjectStore>& objectStore) { this->m_objectStoreSource = objectStore; }

Why the this-> here? Won’t it work without that?

Also, I would probably write [this] instead of [&].

> Source/WebCore/Modules/indexeddb/IDBRequest.cpp:170
> +    if (m_cursorSource)
> +        return Source { m_cursorSource };
> +    if (m_indexSource)
> +        return Source { m_indexSource };
> +    if (m_objectStoreSource)
> +        return Source { m_objectStoreSource };

Would be nice to replace the three data members with a single Variant m_source at some point. I tried doing that and failed, though.

> Source/WebCore/Modules/indexeddb/IDBRequest.cpp:172
> +    return std::nullopt;

Is this truly reachable?

> Source/WebCore/Modules/indexeddb/IndexedDB.h:49
>  enum class CursorDirection {
>      Next = 0,
>      NextNoDuplicate = 1,
> +    Nextunique = 1,
>      Prev = 2,
>      PrevNoDuplicate = 3,
> +    Prevunique = 3,
>  };

Synonyms might need a "why" comment, also might be nicer to list them last instead of mixed in, in numeric order. Also unclear why we have to have specific constant values instead of letting them get generated.

> Source/WebCore/bindings/scripts/CodeGenerator.pm:887
>      return 1 if $type->name eq "object";
>      return 1 if $type->name eq "BufferSource";
>      return 1 if $type->name eq "Promise";
> -    return 1 if $type->name eq "XPathNSResolver";    
> -    return 1 if $type->name eq "EventListener";    
> -    return 1 if $type->name eq "SerializedScriptValue";    
> -    return 1 if $type->name eq "JSON";    
> +    return 1 if $type->name eq "XPathNSResolver";
> +    return 1 if $type->name eq "IDBKey";
> +    return 1 if $type->name eq "EventListener";
> +    return 1 if $type->name eq "SerializedScriptValue";
> +    return 1 if $type->name eq "JSON";

Sort in alphabetical order? Use a hash instead of a cascading set of return statements?

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:5114
>          "Date" => "IDLDate",
> +        "JSON" => "IDLJSON",
>          "SerializedScriptValue" => "IDLSerializedScriptValue<SerializedScriptValue>",
>          "EventListener" => "IDLEventListener<JSEventListener>",
>          "XPathNSResolver" => "IDLXPathNSResolver<XPathNSResolver>",
> -        "JSON" => "IDLJSON",
> +        "IDBKey" => "IDLIDBKey<IDBKey>",

Sort in alphabetical order?

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:5340
>      return 1 if $type->name eq "Date";
>      return 1 if $type->name eq "SerializedScriptValue";
>      return 1 if $type->name eq "XPathNSResolver";
> +    return 1 if $type->name eq "IDBKey";
>      return 1 if $type->name eq "JSON";

Same questions as above.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:5358
>      return 1 if $codeGenerator->IsTypedArrayType($type);
>      return 1 if $type->name eq "SerializedScriptValue";
>      return 1 if $type->name eq "XPathNSResolver";
> +    return 1 if $type->name eq "IDBKey";

Same questions as above.

[Sam Weinig]

Created attachment 297757 [details]
Patch

[Sam Weinig]

Created attachment 297758 [details]
Patch

[Sam Weinig]

Committed r210148: <http://trac.webkit.org/changeset/210148>