IDB: ObjectStore cursor advance() support <rdar://problem/15779645>
Created attachment 222714 [details] Patch v1
Created attachment 222725 [details] Patch v2 - build fix
Comment on attachment 222725 [details] Patch v2 - build fix View in context: https://bugs.webkit.org/attachment.cgi?id=222725&action=review > Source/WebKit2/DatabaseProcess/IndexedDB/sqlite/UniqueIDBDatabaseBackingStoreSQLite.cpp:887 > + // There's no way to indicate an error to SQLite - we have to return a sorting decision. > + // We arbitrarily choose "A > B" > + return 1; I would suggest that even if the data is not deserializable, you still write a collation function that obeys the properties here: http://www.sqlite.org/c3ref/create_collation.html Otherwise, "the behavior of SQLite is undefined". I think it would be best if you put all un-deserializable things together, maybe before everything else. So you would always try to deserialize both things, even if the first one can't be deserialized. When comparing two un-deserializable things, just call memcmp. You may also want to consider performing sqlite3_interrupte(): http://sqlite.1065341.n5.nabble.com/Signaling-errors-from-collation-functions-td43419.html
Comment on attachment 222725 [details] Patch v2 - build fix View in context: https://bugs.webkit.org/attachment.cgi?id=222725&action=review > Source/WebKit2/DatabaseProcess/IndexedDB/IDBIdentifier.h:84 > + DatabaseProcessIDBConnection& connection() const { return *m_connection; } You should ASSERT(m_connection) here. > Source/WebKit2/DatabaseProcess/IndexedDB/sqlite/SQLiteIDBCursor.cpp:43 > + std::unique_ptr<SQLiteIDBCursor> cursor = std::unique_ptr<SQLiteIDBCursor>(new SQLiteIDBCursor(transaction, cursorIdentifier, objectStoreID, indexID, cursorDirection, cursorType, taskType, keyRange)); This should use auto and make_unique<> > Source/WebKit2/DatabaseProcess/IndexedDB/sqlite/SQLiteIDBTransaction.cpp:41 > +SQLiteIDBTransaction::SQLiteIDBTransaction(UniqueIDBDatabaseBackingStoreSQLite* backingStore, const IDBIdentifier& transactionIdentifier, IndexedDB::TransactionMode mode) Could backingStore be a reference?
(In reply to comment #4) > (From update of attachment 222725 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=222725&action=review > > > Source/WebKit2/DatabaseProcess/IndexedDB/IDBIdentifier.h:84 > > + DatabaseProcessIDBConnection& connection() const { return *m_connection; } > > You should ASSERT(m_connection) here. Fixed. > > Source/WebKit2/DatabaseProcess/IndexedDB/sqlite/SQLiteIDBCursor.cpp:43 > > + std::unique_ptr<SQLiteIDBCursor> cursor = std::unique_ptr<SQLiteIDBCursor>(new SQLiteIDBCursor(transaction, cursorIdentifier, objectStoreID, indexID, cursorDirection, cursorType, taskType, keyRange)); > > This should use auto and make_unique<> Using auto. To use make_unique<> requires a public constructor or finding the right syntax for friending std::make_unique. I can't figure out that syntax... Will skip this for now. > > > Source/WebKit2/DatabaseProcess/IndexedDB/sqlite/SQLiteIDBTransaction.cpp:41 > > +SQLiteIDBTransaction::SQLiteIDBTransaction(UniqueIDBDatabaseBackingStoreSQLite* backingStore, const IDBIdentifier& transactionIdentifier, IndexedDB::TransactionMode mode) > > Could backingStore be a reference? Yup.
http://trac.webkit.org/changeset/163113
Sorry I'd missed this comment before landing. (In reply to comment #3) > (From update of attachment 222725 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=222725&action=review > > > Source/WebKit2/DatabaseProcess/IndexedDB/sqlite/UniqueIDBDatabaseBackingStoreSQLite.cpp:887 > > + // There's no way to indicate an error to SQLite - we have to return a sorting decision. > > + // We arbitrarily choose "A > B" > > + return 1; > > I would suggest that even if the data is not deserializable, you still write a collation function that obeys the properties here: http://www.sqlite.org/c3ref/create_collation.html > Otherwise, "the behavior of SQLite is undefined". If the data coming in is bogus, then the behavior of the Indexed Database API is "undefined", so we're not losing anything by being undefined in SQLite terms as well. > I think it would be best if you put all un-deserializable things together, maybe before everything else. So you would always try to deserialize both things, even if the first one can't be deserialized. When comparing two un-deserializable things, just call memcmp. I think memcmp on blobs that are supposed to represent a complex object would be just as much of an undefined "lie" as this arbitrary decision. > You may also want to consider performing sqlite3_interrupte(): > http://sqlite.1065341.n5.nabble.com/Signaling-errors-from-collation-functions-td43419.html This is an interesting option that I wasn't aware of! Definitely outside the scope of this patch (as bogus data already means IDB is acting in an undefined manner) but an interesting improvement in the future to give better error reporting. Thanks for suggesting it!