Bug 97834

Created attachment 166102 [details] Patch

jsbell/dgrogan - this is a WIP - this passes all the tests but I think I need to do some chromium plumbing before I can land this cleanly. Mind taking a look at the Modules/indexeddb side of things in the meantime to make sure this looks kosher?

Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

Comment on attachment 166102 [details] Patch Attachment 166102 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14034992

Comment on attachment 166102 [details] Patch Attachment 166102 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/14054127

Comment on attachment 166102 [details] Patch This looks pretty much as I expected. Nits and suggestions: View in context: https://bugs.webkit.org/attachment.cgi?id=166102&action=review > Source/WebCore/Modules/indexeddb/IDBBackingStore.h:52 > + virtual bool getIDBDatabaseMetaData(const String& name, String& foundStringVersion, int64_t& foundIntVersion, int64_t& foundId, int64_t& maxObjectStoreId) = 0; At some point it would make sense to have this populate an IDBDatatabaseMetadata object (but not this patch) > Source/WebCore/Modules/indexeddb/IDBDatabase.cpp:174 > + m_metadata.maxObjectStoreId++; Prefer pre-increment unless taking advantage of the special behavior of post-increment. > Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:208 > + if (!getInt(db, maxObjectStoreIdKey, maxObjectStoreId)) It looks like this is to match the previous behavior in getNewObjectStoreId (where this key may not be present). It might make the overall code cleaner to do a "schema change" and add this to setUpMetadata(). setUpMetadata would read as: const latestSchemaVersion = 2; if (schemaVersion < 1) { // current code that adds int versions for each database schemaVersion = 1; } if (schemaVersion < 2) { // new code to ensure MaxObjectStoreId is set for each database schemaVersion = 2; } ASSERT(schemaVersion == latestSchemaVersion); .. and maybe we factor out the "for each database" logic a bit. (In hindsight, we should have been more aggressive about doing this. We could also do this as a separate patch to land first.) > Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:321 > + maxObjectStoreId = getMaxObjectStoreId(m_db.get(), foundId); Ideally this would follow the same pattern as the other accessors, and just be getInt (See above.) > Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:531 > + int64_t maxObjectStoreId = getMaxObjectStoreId(transaction, maxObjectStoreIdKey); This is only used for the following ASSERT, correct? Wrap this plus the ASSERT in #if DEBUG to avoid unnecessary work in Release builds. > Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:532 > + ASSERT_WITH_MESSAGE(objectStoreId > maxObjectStoreId, "New database ids must be increasing."); Nit: "object store ids" > Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:939 > if (!getInt(transaction, maxIndexIdKey, maxIndexId)) Wrap this in an #if DEBUG guard, since it's only needed for the ASSERT. > Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:950 > + // FIXME: this is just a sanity check Still relevant? > Source/WebCore/Modules/indexeddb/IDBObjectStore.cpp:422 > + m_metadata.maxIndexId++; Prefer pre-increment unless taking advantage of the special behavior of post-increment. > Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:436 > + ASSERT_WITH_MESSAGE(!m_indexes.contains(name), "Indexes alread contain %s", name.utf8().data()); Typo: already > Source/WebKit/chromium/src/WebIDBDatabaseImpl.cpp:75 > + RefPtr<IDBObjectStoreBackendInterface> objectStore = m_databaseBackend->createObjectStore(-1, name, keyPath, autoIncrement, transaction.getIDBTransactionBackendInterface(), ec); I assume this is a work in progress, and the magic number will reference a constant for "make one up for me" during the transition period.

Created attachment 166977 [details] Patch

jsbell@ - finally we have the first part of this. The key here is that after this lands, the webkit side will support both explicit ids and "autogenerate" ids (i.e. -1) - when I land the chromium side, explicit ids will be used, and then then I will clean up the webkit side of things and remove all support for "autogenerate" ids.

Comment on attachment 166977 [details] Patch Attachment 166977 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14132817

Created attachment 166998 [details] Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=166977&action=review Overall LGTM (not the way I've split up these multi-part patches in the past, but should work) > Source/WebCore/Modules/indexeddb/IDBBackingStore.h:49 > + // FIXME: Remove these when chromium plumbing has landed: https://bugs.webkit.org/show_bug.cgi?id=98085 I'd change this (and similar comments) to: FIXME: Remove when switch to front-end ID management is complete: https://bugs.webkit.org/show_bug.cgi?id=98085 > Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:193 > + ASSERT_WITH_MESSAGE(id == AutogenerateObjectStoreId || id > m_maxObjectStoreId, "createObjectStore with %llu but needed something bigger than %lld", id, m_maxObjectStoreId); Ideally, this could ASSERT(id == AutogenerateObjectStoreId || id == m_maxObjectStoreId + 1), but from offline conversations there are some edge cases which prevent that. Maybe a bug/FIXME to track adding that assertion and/or test those edge cases explicitly? > Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:627 > + ASSERT_WITH_MESSAGE(maxIndexIds.size() == ids.size(), "Expected %lu maxIndexIds, but got %lu", ids.size(), maxIndexIds.size()); I don't think this merits a message. > Source/WebCore/Modules/indexeddb/IDBDatabaseBackendInterface.h:60 > + virtual PassRefPtr<IDBObjectStoreBackendInterface> createObjectStore(int64_t, const String& name, const IDBKeyPath&, bool autoIncrement, IDBTransactionBackendInterface*, ExceptionCode&) = 0; Name the argument, since it isn't obviously an ID given the type. > Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:545 > + if (objectStoreId == -1) Should this be IDBBackingStore::AutogenerateObjectStoreId ? > Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:964 > + if (indexId == -1) Should this be IDBBackingStore::AutogenerateIndexId? > Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:980 > + if (indexId == -1) Should this be IDBBackingStore::AutogenerateIndexId? > Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendInterface.h:62 > + static const int64_t AutogenerateIndexId = -1; Should this be IDBBackingStore::AutogenerateIndexId or are the uses separate?

Comment on attachment 166998 [details] Patch Attachment 166998 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14129854

Created attachment 167007 [details] Patch

Comment on attachment 167007 [details] Patch Attachment 167007 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14126995

Comment on attachment 167007 [details] Patch Attachment 167007 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/14136815

Created attachment 167154 [details] Patch

ok, finally a working patch - I had missed some of the chromium tests. tony@ - r? for the non-chromium stuff? abarth@ - r? for the chromium stuff?

Comment on attachment 167154 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=167154&action=review > Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendInterface.h:60 > + static const int64_t MinimumIndexId = 30; Why 30?

Comment on attachment 167154 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=167154&action=review >> Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendInterface.h:60 >> + static const int64_t MinimumIndexId = 30; > > Why 30? History - the first 7 or so slots in the database are reserved for specific metadata, and I guess the original author added padding in case they needed 23 or so more pieces of metadata.

Comment on attachment 167154 [details] Patch Rejecting attachment 167154 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1 ERROR: /mnt/git/webkit-commit-queue/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Full output: http://queues.webkit.org/results/14171406

Comment on attachment 167154 [details] Patch that seems to be bogus - there's no LayoutTests change

Comment on attachment 167154 [details] Patch Clearing flags on attachment: 167154 Committed r130428: <http://trac.webkit.org/changeset/130428>

All reviewed patches have been landed. Closing bug.