Bug 206196

Created attachment 387585 [details]
Patch

<rdar://problem/53596307>

Created attachment 387694 [details]
Patch

Comment on attachment 387694 [details]
Patch

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

> Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.h:116
> +    Optional<bool> ensureValidObjectStoreInfoTable();

This seems like a confusing return type? I can't tell from reading this function declaration what the return value would indicate,

(In reply to Sam Weinig from comment #4)
> Comment on attachment 387694 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=387694&action=review
> 
> > Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.h:116
> > +    Optional<bool> ensureValidObjectStoreInfoTable();
> 
> This seems like a confusing return type? I can't tell from reading this
> function declaration what the return value would indicate,

ensureValidObjectStoreInfoTable return nullopt if the operation fails; the boolean indicates whether there is a schema change.

I will update the name.

Created attachment 388078 [details]
Patch

Comment on attachment 388078 [details]
Patch

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

Comment on a small coding style issue, not the overall patch/technique, which seems excellent!

> Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:200
> +    static NeverDestroyed<WTF::String> indexRecordsRecordIndexSchemaString("CREATE INDEX IndexRecordsRecordIndex ON IndexRecords (objectStoreID, objectStoreRecordID)");

Please use _s here to use ASCIILiteral to avoid copying the string’s data, and point to it instead. Also unclear why storing this in a global is a helpful optimization. Paying even this small permanent memory cost may not be worthwhile if there is other memory allocation/deallocation going on.

> Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:238
> +static const String v1ObjectStoreInfoSchema(const String& tableName)

This should not take a const String&. Instead take an ASCIILiteral so we don’t create/destroy a String for no reason. makeString can take all kinds of types besides String, including ASCIILiteral and const char*. But also, why compute these at runtime? They could be computed at compile time if we use macros. Finally, I am really unsure that caching these in globals is a useful thing to do. Spending all the memory all the time, but are these operations so fast that we can’t take the time to create/destroy a String?

All of this applies to the existing code too; just a mistake being repeated.

> Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:255
> +static const String v2ObjectStoreInfoSchema(const String& tableName)

Same comment applies here.

Created attachment 388332 [details]
Patch

Comment on attachment 388332 [details]
Patch

I kind of cruised over the upgrade schema stuff since that is old hat boiler plate at this point.

lgtm

(In reply to Brady Eidson from comment #9)
> Comment on attachment 388332 [details]
> Patch
> 
> I kind of cruised over the upgrade schema stuff since that is old hat boiler
> plate at this point.
> 
> lgtm

Thanks for the review!

Comment on attachment 388332 [details]
Patch

Clearing flags on attachment: 388332

Committed r255318: <https://trac.webkit.org/changeset/255318>

All reviewed patches have been landed.  Closing bug.