Bug 97834 – IndexedDB: promote objectstore/index backend ids to the frontend

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alec Flett]

IndexedDB: promote objectstore/index backend ids to the frontend

[Alec Flett]

Created attachment 166102 [details]
Patch

[Alec Flett]

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?

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[WebKit Review Bot]

Comment on attachment 166102 [details]
Patch

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

[Peter Beverloo (cr-android ews)]

Comment on attachment 166102 [details]
Patch

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

[Joshua Bell]

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.

[Alec Flett]

Created attachment 166977 [details]
Patch

[Alec Flett]

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.

[WebKit Review Bot]

Comment on attachment 166977 [details]
Patch

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

[Alec Flett]

Created attachment 166998 [details]
Patch

[Joshua Bell]

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?

[WebKit Review Bot]

Comment on attachment 166998 [details]
Patch

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

[Alec Flett]

Created attachment 167007 [details]
Patch

[WebKit Review Bot]

Comment on attachment 167007 [details]
Patch

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

[Peter Beverloo (cr-android ews)]

Comment on attachment 167007 [details]
Patch

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

[Alec Flett]

Created attachment 167154 [details]
Patch

[Alec Flett]

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?

[Tony Chang]

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?

[Alec Flett]

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.

[WebKit Review Bot]

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

[Alec Flett]

Comment on attachment 167154 [details]
Patch

that seems to be bogus - there's no LayoutTests change

[WebKit Review Bot]

Comment on attachment 167154 [details]
Patch

Clearing flags on attachment: 167154

Committed r130428: <http://trac.webkit.org/changeset/130428>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.