Bug 89607 – IndexedDB: refactor index-writing to be more self-contained

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

[Alec Flett]

IndexedDB: refactor index-writing to be more self-contained

[Alec Flett]

Created attachment 148668 [details]
Patch

[Alec Flett]

jsbell@ / dgrogan@ - this is purely a refactor, no new changes are introduced. This is also stacked on top of bug 86123, so must wait until that lands.

The gist of the refactor is:

- Broaden the scope of the existing PopulateIndexCallback to be a more general purpose utility class, renaming it IndexWriter
- split the gathering (loadIndexKeysForValue) of indexKeys and the writing of index keys (writeIndexKeys), so they can happen at different times.
- in putInternal, Call loadIndexKeysForValue rather than the first loop which previously created a list of indexKeys, and call writeIndexKeys rather than the second loop, which previously duplicated the same logic in order to write them.

Ultimately this consolidates 3 duplicate sections of code into a single one, and allows you to gather the index keys for a given value without mutating the database in any way. I think eventually (after further refactoring) loadIndexKeysForValue will be called on the frontend, and writeIndexKeys will be called from the backend, and we'll essentially be passing a whole structure of (primaryKey, (index -> indexKeys)) from the frontend to the backend.

[Alec Flett]

actually you can hold off a bit looking at this - I just realized if I break out the 'callback' stuff, then I can implement IndexWriter without even relying on the backend interface instance until write-time.

[Alec Flett]

Created attachment 148673 [details]
Patch

[Alec Flett]

Created attachment 148674 [details]
Patch

[Alec Flett]

Created attachment 148854 [details]
Patch

[Alec Flett]

this is ready for review - it's not going to pass the bots until the other bug lands, because it won't apply cleanly.

[David Grogan]

Comment on attachment 148854 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=148854&action=review

> Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:237
> +                    *errorMessage = String::format("Unable to add key to index '%s': the key does not satisfy its uniqueness requirements.",

Yay for better error messages!  Maybe we can add an IDBKey.toString() method and even include the key.

[Alec Flett]

Created attachment 149060 [details]
Patch

[Joshua Bell]

Comment on attachment 149060 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=149060&action=review

> Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:231
> +        if (!indexKey->isValid())

Can the logic here be reordered to match the substeps in the spec (5.1 "Object Store Storage Operation", step 7), to make it easier to see that it conforms?

> Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:275
> +        if (indexKey && m_index->multiEntry() && indexKey->type() == IDBKey::ArrayType)

Recommend assigning indexKey to a real RefPtr<> before using, although as written this looks okay.

[Alec Flett]

Created attachment 149100 [details]
Patch

[Alec Flett]

Comment on attachment 149100 [details]
Patch

tony@ - r? cq?

no logic is changing here, this is a pure refactor

[Tony Chang]

Comment on attachment 149100 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=149100&action=review

> Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:222
> +    IndexWriter(PassRefPtr<IDBIndexBackendImpl> index)

Nit: explicit

> Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:223
> +            : m_index(index)

Nit: Too much indent here (should be 8 spaces total).

> Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:229
> +    bool loadIndexKeysForValue(SerializedScriptValue* objectValue,
> +                               const IDBKey* primaryKey = 0,
> +                               String* errorMessage = 0)

Nit: I think it's rare in WebKit style to wrap function declarations. If you do want to wrap it, only indent one level (8 spaces total).

> Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:249
> +            m_indexKeys.append(indexKey);
> +        } else {

Nit: You could early return true here and remove the else block.

> Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:269
> +    bool writeIndexKeys(const IDBBackingStore::ObjectStoreRecordIdentifier* recordIdentifier,
> +                        IDBBackingStore& backingStore, int64_t databaseId, int64_t objectStoreId,
> +                        String* errorMessage = 0)

Nit: Weird indent like above.

> Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:271
> +        for (size_t i = 0; i < m_indexKeys.size(); i++) {

Nit: ++i

[Alec Flett]

Created attachment 149121 [details]
Patch

[Joshua Bell]

Created attachment 149123 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 149123 [details]
Patch for landing

Clearing flags on attachment: 149123

Committed r121066: <http://trac.webkit.org/changeset/121066>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.