Bug 92883 – IndexedDB: Make leveldb store integer versions and migrate old schemas

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[David Grogan]

IndexedDB: Make leveldb store integer versions and migrate old schemas

[David Grogan]

Created attachment 155842 [details]
Patch

[David Grogan]

Josh, Alec, could you look at this?

[Joshua Bell]

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]

(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]

(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]

Created attachment 155866 [details]
Patch

[David Grogan]

Created attachment 155912 [details]
Patch

[David Grogan]

Created attachment 156205 [details]
Patch

[David Grogan]

Created attachment 156223 [details]
Patch

[David Grogan]

abarth@, could you review this?

It's only been updated to ToT since jsbell@ LGTM'd it, no real changes.

[Adam Barth]

> 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]

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]

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

[Adam Barth]

Comment on attachment 156223 [details]
Patch

Hum...  I don't really understand this patch, and it looks like some delicate migration code.

[Adam Barth]

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 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]

(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 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]

Created attachment 156864 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 156864 [details]
Patch for landing

Clearing flags on attachment: 156864

Committed r124858: <http://trac.webkit.org/changeset/124858>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.