Bug 99636 - IndexedDB: Destroy leveldb directory if unknown schema is detected
Summary: IndexedDB: Destroy leveldb directory if unknown schema is detected
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: David Grogan
URL:
Keywords:
Depends on:
Blocks: 100692
  Show dependency tree / graph
 
Reported: 2012-10-17 13:55 PDT by David Grogan
Modified: 2012-10-29 12:19 PDT (History)
4 users (show)

See Also:

Attachments
Patch (2.97 KB, patch)
2012-10-17 13:59 PDT, David Grogan 		no flags Details | Formatted Diff | Diff
Patch (3.12 KB, patch)
2012-10-17 14:02 PDT, David Grogan 		no flags Details | Formatted Diff | Diff
Patch (3.09 KB, patch)
2012-10-17 14:25 PDT, David Grogan 		no flags Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description David Grogan 2012-10-17 13:55:59 PDT 
IndexedDB: Destroy leveldb directory if unknown schema is detected
Comment 1 David Grogan 2012-10-17 13:59:31 PDT 
Created attachment 169255 [details]
Patch
Comment 2 David Grogan 2012-10-17 14:02:32 PDT 
Created attachment 169257 [details]
Patch
Comment 3 David Grogan 2012-10-17 14:04:13 PDT 
jsbell/alecflett, could you take a look at this?

The webcore patch is small and simple; the test in chromium is huge and messy.
Comment 4 Joshua Bell 2012-10-17 14:14:39 PDT 
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() ?
Comment 5 David Grogan 2012-10-17 14:23:44 PDT 
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.
Comment 6 David Grogan 2012-10-17 14:25:46 PDT 
Created attachment 169260 [details]
Patch
Comment 7 David Grogan 2012-10-17 14:27:04 PDT 
Tony, could you review this?
Comment 8 Tony Chang 2012-10-17 14:38:09 PDT 
Comment on attachment 169260 [details]
Patch

Is it possible to write a test in webkit_unit_tests for this?
Comment 9 David Grogan 2012-10-17 16:22:35 PDT 
(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.
Comment 10 WebKit Review Bot 2012-10-17 17:02:29 PDT 
Comment on attachment 169260 [details]
Patch

Clearing flags on attachment: 169260

Committed r131672: <http://trac.webkit.org/changeset/131672>
Comment 11 WebKit Review Bot 2012-10-17 17:02:34 PDT 
All reviewed patches have been landed.  Closing bug.