IndexedDB: Make leveldb store integer versions and migrate old schemas
Created attachment 155842 [details] Patch
Josh, Alec, could you look at this?
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.
(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.
(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.
Created attachment 155866 [details] Patch
Created attachment 155912 [details] Patch
Created attachment 156205 [details] Patch
Created attachment 156223 [details] Patch
abarth@, could you review this? It's only been updated to ToT since jsbell@ LGTM'd it, no real changes.
> 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).
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).
jsbell is not a reviewer! What has the world come to.
Comment on attachment 156223 [details] Patch Hum... I don't really understand this patch, and it looks like some delicate migration code.
It looks like Tony and Ojan have been reviewing these sorts of patches recently. Maybe they have better context than I do?
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.
(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.
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.
Created attachment 156864 [details] Patch for landing
Comment on attachment 156864 [details] Patch for landing Clearing flags on attachment: 156864 Committed r124858: <http://trac.webkit.org/changeset/124858>
All reviewed patches have been landed. Closing bug.