Bug 166451 - [WebIDL] Remove (most) custom bindings for the IndexedDB code
Summary: [WebIDL] Remove (most) custom bindings for the IndexedDB code
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sam Weinig
URL:
Keywords:
Depends on:
Blocks: 166002
  Show dependency treegraph
 
Reported: 2016-12-22 17:15 PST by Sam Weinig
Modified: 2016-12-25 17:34 PST (History)
3 users (show)

See Also:


Attachments
Patch (66.69 KB, patch)
2016-12-23 20:17 PST, Sam Weinig
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-elcapitan (965.04 KB, application/zip)
2016-12-23 21:26 PST, Build Bot
no flags Details
Archive of layout-test-results from ews107 for mac-elcapitan-wk2 (960.02 KB, application/zip)
2016-12-23 21:30 PST, Build Bot
no flags Details
Archive of layout-test-results from ews115 for mac-elcapitan (1.70 MB, application/zip)
2016-12-23 21:36 PST, Build Bot
no flags Details
Patch (79.25 KB, patch)
2016-12-25 00:39 PST, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (88.56 KB, patch)
2016-12-25 16:10 PST, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (88.51 KB, patch)
2016-12-25 16:27 PST, Sam Weinig
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Weinig 2016-12-22 17:15:23 PST
Remove custom bindings for the IndexedDB code
Comment 1 Sam Weinig 2016-12-23 20:17:16 PST
Created attachment 297729 [details]
Patch
Comment 2 Sam Weinig 2016-12-23 20:20:17 PST
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.
Comment 3 Build Bot 2016-12-23 21:26:06 PST
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
Comment 4 Build Bot 2016-12-23 21:26:09 PST
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
Comment 5 Build Bot 2016-12-23 21:30:22 PST
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
Comment 6 Build Bot 2016-12-23 21:30:25 PST
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
Comment 7 Build Bot 2016-12-23 21:36:38 PST
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
Comment 8 Build Bot 2016-12-23 21:36:41 PST
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
Comment 9 Sam Weinig 2016-12-25 00:39:44 PST
Created attachment 297749 [details]
Patch
Comment 10 Darin Adler 2016-12-25 09:36:06 PST
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.
Comment 11 Sam Weinig 2016-12-25 16:10:36 PST
Created attachment 297757 [details]
Patch
Comment 12 Sam Weinig 2016-12-25 16:27:55 PST
Created attachment 297758 [details]
Patch
Comment 13 Sam Weinig 2016-12-25 17:34:19 PST
Committed r210148: <http://trac.webkit.org/changeset/210148>