Bug 97834 - IndexedDB: promote objectstore/index backend ids to the frontend
Summary: IndexedDB: promote objectstore/index backend ids to the frontend
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alec Flett
URL:
Keywords:
Depends on:
Blocks: 98085
  Show dependency treegraph
 
Reported: 2012-09-27 17:24 PDT by Alec Flett
Modified: 2012-10-04 15:38 PDT (History)
10 users (show)

See Also:


Attachments
Patch (60.85 KB, patch)
2012-09-27 17:24 PDT, Alec Flett
no flags Details | Formatted Diff | Diff
Patch (63.39 KB, patch)
2012-10-03 15:51 PDT, Alec Flett
no flags Details | Formatted Diff | Diff
Patch (63.39 KB, patch)
2012-10-03 16:54 PDT, Alec Flett
no flags Details | Formatted Diff | Diff
Patch (63.70 KB, patch)
2012-10-03 17:39 PDT, Alec Flett
no flags Details | Formatted Diff | Diff
Patch (69.49 KB, patch)
2012-10-04 12:09 PDT, Alec Flett
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alec Flett 2012-09-27 17:24:10 PDT
IndexedDB: promote objectstore/index backend ids to the frontend
Comment 1 Alec Flett 2012-09-27 17:24:34 PDT
Created attachment 166102 [details]
Patch
Comment 2 Alec Flett 2012-09-27 17:25:50 PDT
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?
Comment 3 WebKit Review Bot 2012-09-27 17:27:18 PDT
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 4 WebKit Review Bot 2012-09-27 17:49:00 PDT
Comment on attachment 166102 [details]
Patch

Attachment 166102 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14034992
Comment 5 Peter Beverloo (cr-android ews) 2012-09-27 18:50:20 PDT
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 6 Joshua Bell 2012-09-28 14:44:45 PDT
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.
Comment 7 Alec Flett 2012-10-03 15:51:10 PDT
Created attachment 166977 [details]
Patch
Comment 8 Alec Flett 2012-10-03 16:08:25 PDT
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 9 WebKit Review Bot 2012-10-03 16:23:03 PDT
Comment on attachment 166977 [details]
Patch

Attachment 166977 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14132817
Comment 10 Alec Flett 2012-10-03 16:54:53 PDT
Created attachment 166998 [details]
Patch
Comment 11 Joshua Bell 2012-10-03 17:04:48 PDT
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 12 WebKit Review Bot 2012-10-03 17:32:15 PDT
Comment on attachment 166998 [details]
Patch

Attachment 166998 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14129854
Comment 13 Alec Flett 2012-10-03 17:39:07 PDT
Created attachment 167007 [details]
Patch
Comment 14 WebKit Review Bot 2012-10-03 18:07:39 PDT
Comment on attachment 167007 [details]
Patch

Attachment 167007 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14126995
Comment 15 Peter Beverloo (cr-android ews) 2012-10-03 19:09:42 PDT
Comment on attachment 167007 [details]
Patch

Attachment 167007 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14136815
Comment 16 Alec Flett 2012-10-04 12:09:07 PDT
Created attachment 167154 [details]
Patch
Comment 17 Alec Flett 2012-10-04 12:10:32 PDT
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 18 Tony Chang 2012-10-04 13:02:45 PDT
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 19 Alec Flett 2012-10-04 14:03:50 PDT
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 20 WebKit Review Bot 2012-10-04 14:43:01 PDT
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 21 Alec Flett 2012-10-04 15:02:57 PDT
Comment on attachment 167154 [details]
Patch

that seems to be bogus - there's no LayoutTests change
Comment 22 WebKit Review Bot 2012-10-04 15:38:10 PDT
Comment on attachment 167154 [details]
Patch

Clearing flags on attachment: 167154

Committed r130428: <http://trac.webkit.org/changeset/130428>
Comment 23 WebKit Review Bot 2012-10-04 15:38:15 PDT
All reviewed patches have been landed.  Closing bug.