Bug 100055 – IndexedDB: refactor backend to use IDB*Metadata

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

[Alec Flett]

IndexedDB: refactor backend to use IDB*Metadata

[Alec Flett]

Created attachment 170031 [details]
Patch

[Alec Flett]

jsbell/dgrogan - further refactors, in case you want to take a look. I have another one built on top of this which moves the backend to be even more int64_t-oriented

tony@ - r?

[Joshua Bell]

Comment on attachment 170031 [details]
Patch

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

> Source/WebCore/Modules/indexeddb/IDBBackingStore.h:60
> +    virtual void getObjectStores(int64_t databaseId, Vector<IDBObjectStoreMetadata>&) = 0;

Maybe change signature to return Vector<metadata> now?

> Source/WebCore/Modules/indexeddb/IDBBackingStore.h:86
> +    virtual void getIndexes(int64_t databaseId, int64_t objectStoreId, Vector<IDBIndexMetadata>&) = 0;

Ditto?

> Source/WebCore/Modules/indexeddb/IDBIndexBackendImpl.h:57
> +    void setId(int64_t id) { m_metadata.id = id; }

I don't think setId is called anywhere - remove?

> Source/WebCore/Modules/indexeddb/IDBIndexBackendImpl.h:58
> +    bool hasValidId() const { return m_metadata.id != InvalidId; };

... which begs the question of whether or not this is still meaningful.

[Alec Flett]

Created attachment 170192 [details]
Patch

Addresses review comments

[Alec Flett]

tony@  - r?

[Joshua Bell]

Comment on attachment 170192 [details]
Patch

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

> Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:-476
> -            return;

This behavior change seems like it should be part of a separate patch.

[Alec Flett]

Technically the behavior is not changing here - previous we did an early-return which meant possibly returning partial results. Now we break early, which means we don't add the partially-complete metadata object to the array, but we still return the array of previous partial results.

So the behavior, while probably needing an improvement, does not change.

[Joshua Bell]

(In reply to comment #7)
> So the behavior, while probably needing an improvement, does not change.

Cool, thanks for the clarification.

[Tony Chang]

Comment on attachment 170192 [details]
Patch

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

> Source/WebCore/Modules/indexeddb/IDBBackingStore.h:53
> +    virtual Vector<String> getDatabaseNames() = 0;

Would it be possible to still pass in the vector by reference?  That would save having to copy the vector when returning.  Same thing for getObjectStores and getIndexes.

[Alec Flett]

tony@ - see http://stackoverflow.com/questions/3721217/returning-a-c-stdvector-without-a-copy

Basically any compiler made in the last 5 or so years (gcc 3.1+, VS2005+) won't copy.

[Tony Chang]

(In reply to comment #10)
> tony@ - see http://stackoverflow.com/questions/3721217/returning-a-c-stdvector-without-a-copy
> 
> Basically any compiler made in the last 5 or so years (gcc 3.1+, VS2005+) won't copy.

Is this true for gcc-arm too?

[Alec Flett]

pretty sure it's a frontend optimization, not backend, so it shouldn't affect the port involved... not sure how to answer definitively though?

[Tony Chang]

Comment on attachment 170192 [details]
Patch

After discussion with our resident compiler expert, this LGTM.

[WebKit Review Bot]

Comment on attachment 170192 [details]
Patch

Clearing flags on attachment: 170192

Committed r132258: <http://trac.webkit.org/changeset/132258>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.