Bug 99636 – IndexedDB: Destroy leveldb directory if unknown schema is detected

• Component: New Bugs
• Status: RESOLVED FIXED

[David Grogan]

IndexedDB: Destroy leveldb directory if unknown schema is detected

[David Grogan]

Created attachment 169255 [details]
Patch

[David Grogan]

Created attachment 169257 [details]
Patch

[David Grogan]

jsbell/alecflett, could you take a look at this?

The webcore patch is small and simple; the test in chromium is huge and messy.

[Joshua Bell]

Comment on attachment 169257 [details]
Patch

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

overall lgtm, w/ nits

> Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:149
> +static const int64_t latestSchemaVersion = 1;

static qualifier not necessary for consts.

> Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:150
> +static bool isSchemaOnDiskUnknown(LevelDBDatabase* db)

Is "OnDisk" necessary? We'll only use it in the on-disk path, but I don't think it is needed in the function name.

Also, I think readability would be improved if the semantics were flipped, i.e. true = known, false = unknown. So maybe isSchemaKnown() ?

[David Grogan]

Comment on attachment 169257 [details]
Patch

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

>> Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:149
>> +static const int64_t latestSchemaVersion = 1;
> 
> static qualifier not necessary for consts.

Removed.

>> Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:150
>> +static bool isSchemaOnDiskUnknown(LevelDBDatabase* db)
> 
> Is "OnDisk" necessary? We'll only use it in the on-disk path, but I don't think it is needed in the function name.
> 
> Also, I think readability would be improved if the semantics were flipped, i.e. true = known, false = unknown. So maybe isSchemaKnown() ?

Changed.

[David Grogan]

Created attachment 169260 [details]
Patch

[David Grogan]

Tony, could you review this?

[Tony Chang]

Comment on attachment 169260 [details]
Patch

Is it possible to write a test in webkit_unit_tests for this?

[David Grogan]

(In reply to comment #8)
> (From update of attachment 169260 [details])
> Is it possible to write a test in webkit_unit_tests for this?

It would be possible, but even more work than the content_shell test.

[WebKit Review Bot]

Comment on attachment 169260 [details]
Patch

Clearing flags on attachment: 169260

Committed r131672: <http://trac.webkit.org/changeset/131672>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.