Remove custom bindings for the IndexedDB code
Created attachment 297729 [details] Patch
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 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
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 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
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 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
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
Created attachment 297749 [details] Patch
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.
Created attachment 297757 [details] Patch
Created attachment 297758 [details] Patch
Committed r210148: <http://trac.webkit.org/changeset/210148>