Bug 92883

Summary:

IndexedDB: Make leveldb store integer versions and migrate old schemas

Product:

WebKit

Reporter:

David Grogan <dgrogan>

Component:

New Bugs

Assignee:

David Grogan <dgrogan>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, alecflett, jsbell, ojan, tony, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

92558

Bug Blocks:

92897

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch for landing	none

David Grogan

Reported 2012-08-01 10:34:39 PDT

IndexedDB: Make leveldb store integer versions and migrate old schemas

Attachments
Patch (7.72 KB, patch) 2012-08-01 10:53 PDT, David Grogan	no flags	Details Formatted Diff Diff
Patch (7.80 KB, patch) 2012-08-01 13:07 PDT, David Grogan	no flags	Details Formatted Diff Diff
Patch (7.95 KB, patch) 2012-08-01 15:59 PDT, David Grogan	no flags	Details Formatted Diff Diff
Patch (7.66 KB, patch) 2012-08-02 16:51 PDT, David Grogan	no flags	Details Formatted Diff Diff
Patch (7.69 KB, patch) 2012-08-02 18:04 PDT, David Grogan	no flags	Details Formatted Diff Diff
Patch for landing (7.85 KB, patch) 2012-08-06 22:20 PDT, David Grogan	no flags	Details Formatted Diff Diff
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.

David Grogan

Comment 1 2012-08-01 10:53:45 PDT

Created attachment 155842 [details] Patch

David Grogan

Comment 2 2012-08-01 10:55:05 PDT

Josh, Alec, could you look at this?

Joshua Bell

Comment 3 2012-08-01 11:38:28 PDT

Comment on attachment 155842 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=155842&action=review LGTM > Source/WebCore/ChangeLog:25 > + could let script call open("db", 0), get an upgradeneeded event, If version is 0 we should throw TypeError, so that should be prevented. > Source/WebCore/Modules/indexeddb/IDBMetadata.h:46 > + NoIntVersion = -1, Given that you can't call open(name, 0) I wonder if we could collapse these so NoIntVersion = 0 and skip the conversion in the metadata functions. I haven't looked at the other patches though to see if that's actually possible.

David Grogan

Comment 4 2012-08-01 11:42:28 PDT

(In reply to comment #3) > (From update of attachment 155842 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=155842&action=review > > LGTM > > > Source/WebCore/ChangeLog:25 > > + could let script call open("db", 0), get an upgradeneeded event, > > If version is 0 we should throw TypeError, so that should be prevented. Oh nice, I wish I'd noticed that earlier. > > Source/WebCore/Modules/indexeddb/IDBMetadata.h:46 > > + NoIntVersion = -1, > > Given that you can't call open(name, 0) I wonder if we could collapse these so NoIntVersion = 0 and skip the conversion in the metadata functions. I haven't looked at the other patches though to see if that's actually possible. I believe that that's possible, yes. I've suspected such but passing 0 to open was the only thing that made me uneasy. But if that's not allowed, I think they are collapsible. I'm adding a FIXME.

Joshua Bell

Comment 5 2012-08-01 11:46:46 PDT

(In reply to comment #3) > Given that you can't call open(name, 0) I wonder if we could collapse these so NoIntVersion = 0 and skip the conversion in the metadata functions. I haven't looked at the other patches though to see if that's actually possible. The other place 0 shows up in the spec is on an aborted versionchange transaction for a brand new database. My gut tells me that'll work out, but I haven't gone through the migration rules. Agreed, FIXME for now.

David Grogan

Comment 6 2012-08-01 13:07:31 PDT

Created attachment 155866 [details] Patch

David Grogan

Comment 7 2012-08-01 15:59:28 PDT

Created attachment 155912 [details] Patch

David Grogan

Comment 8 2012-08-02 16:51:53 PDT

Created attachment 156205 [details] Patch

David Grogan

Comment 9 2012-08-02 18:04:30 PDT

Created attachment 156223 [details] Patch

David Grogan

Comment 10 2012-08-02 18:11:33 PDT

abarth@, could you review this? It's only been updated to ToT since jsbell@ LGTM'd it, no real changes.

Adam Barth

Comment 11 2012-08-02 18:15:52 PDT

> It's only been updated to ToT since jsbell@ LGTM'd it, no real changes. If that's the case, you can land it with jsbell's review. You can fill out the "Reviewed by" line with his name (exactly as it appears in other ChangeLog entries).

David Grogan

Comment 12 2012-08-02 18:21:35 PDT

I can do that even though he's not a reviewer? (Did he become a reviewer in the past 24 hours or something?) (In reply to comment #11) > > It's only been updated to ToT since jsbell@ LGTM'd it, no real changes. > > If that's the case, you can land it with jsbell's review. You can fill out the "Reviewed by" line with his name (exactly as it appears in other ChangeLog entries).

Adam Barth

Comment 13 2012-08-02 18:26:45 PDT

jsbell is not a reviewer! What has the world come to.

Adam Barth

Comment 14 2012-08-02 18:28:58 PDT

Comment on attachment 156223 [details] Patch Hum... I don't really understand this patch, and it looks like some delicate migration code.

Adam Barth

Comment 15 2012-08-02 18:30:25 PDT

It looks like Tony and Ojan have been reviewing these sorts of patches recently. Maybe they have better context than I do?

Tony Chang

Comment 16 2012-08-05 22:18:22 PDT

Comment on attachment 156223 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=156223&action=review (In reply to comment #15) > It looks like Tony and Ojan have been reviewing these sorts of patches recently. Maybe they have better context than I do? Barely, but I trust jsbell. > Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:173 > + ASSERT(ok); > + if (!ok) > + return false; Nit: It think it's more common to put ASSERT_NOT_REACHED() before "return false;" > Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:327 > + ASSERT_WITH_MESSAGE(intVersion >= 0, "intVersion was %"PRId64, intVersion); Does PRId64 work with VS2008? Does it work with the gcc arm compilers? You might want to just static_cast to an int.

Tony Chang

Comment 17 2012-08-05 22:21:17 PDT

(In reply to comment #16) > > Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:327 > > + ASSERT_WITH_MESSAGE(intVersion >= 0, "intVersion was %"PRId64, intVersion); > > Does PRId64 work with VS2008? Does it work with the gcc arm compilers? You might want to just static_cast to an int. I see on bug 92558 that PRId64 doesn't work on VS2008, but it sounds like you decided to cast to long long.

David Grogan

Comment 18 2012-08-06 17:21:27 PDT

Comment on attachment 156223 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=156223&action=review >> Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:173 >> + return false; > > Nit: It think it's more common to put ASSERT_NOT_REACHED() before "return false;" Fixed throughout. >> Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:327 >> + ASSERT_WITH_MESSAGE(intVersion >= 0, "intVersion was %"PRId64, intVersion); > > Does PRId64 work with VS2008? Does it work with the gcc arm compilers? You might want to just static_cast to an int. Thanks for noticing this, I'd have been embarrassed if I broke the tree twice with PRId64. Casted to long long.

David Grogan

Comment 19 2012-08-06 22:20:14 PDT

Created attachment 156864 [details] Patch for landing

WebKit Review Bot

Comment 20 2012-08-06 23:55:10 PDT

Comment on attachment 156864 [details] Patch for landing Clearing flags on attachment: 156864 Committed r124858: <http://trac.webkit.org/changeset/124858>

WebKit Review Bot

Comment 21 2012-08-06 23:55:15 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.