Bug 150033 – Modern IDB: Start version change transaction for connections to new database

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Brady Eidson]

Modern IDB: Start version change transaction for connections to new database

Can't do anything with the transaction yet - That'll be next.

[Brady Eidson]

Created attachment 262897 [details]
Patch v1

[Alex Christensen]

Comment on attachment 262897 [details]
Patch v1

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

Looks good.  Nothing jumps out as wrong.  A few small comments.

> LayoutTests/storage/indexeddb/modern/opendatabase-request-event-expected.txt:3
> +ALERT: upgradeneeded: old version - 0 new version - 1

Ideas for future tests:
Upgrade to a different number, like 42
Upgrade to INT64_MAX
Upgrade to INT64_MAX - 1
Upgrade to a smaller positive number
Upgrade to a negative number and 0
Upgrade to a string or an object.
Upgrade to a non-integer number

> LayoutTests/storage/indexeddb/modern/opendatabase-request-event.html:17
> +    window.indexedDB.open("TestDatabase", 0);

Why does this make a Type error?

> Source/WebCore/Modules/indexeddb/client/IDBDatabaseImpl.cpp:133
> +    return WTF::move(transaction);

I don't think WTF::move is necessary here.

> Source/WebCore/Modules/indexeddb/client/IDBRequestImpl.h:80
> +    RefPtr<IDBAny> m_result;

Why do you have WebCore::IDBAny elsewhere in this same class but not here?

> Source/WebCore/Modules/indexeddb/server/MemoryIDBBackingStore.cpp:31
> +#include "Logging.h"

This isn't used.

> Source/WebCore/Modules/indexeddb/server/MemoryIDBBackingStore.h:45
> -    
> +

Touching a file for only whitespace?  I don't think this is necessary.

> Source/WebCore/Modules/indexeddb/shared/IDBResourceIdentifier.cpp:78
> +    return IDBResourceIdentifier(0, 0);

Would return { }; work here?

> Source/WebCore/Modules/indexeddb/shared/IDBTransactionInfo.cpp:53
> +    return WTF::move(result);

These explicit moves might also not be needed.

[Brady Eidson]

(In reply to comment #2)
> Comment on attachment 262897 [details]
> Patch v1
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=262897&action=review
> 
> Looks good.  Nothing jumps out as wrong.  A few small comments.
> 
> > LayoutTests/storage/indexeddb/modern/opendatabase-request-event-expected.txt:3
> > +ALERT: upgradeneeded: old version - 0 new version - 1
> 
> Ideas for future tests:
> Upgrade to a different number, like 42
> Upgrade to INT64_MAX
> Upgrade to INT64_MAX - 1
> Upgrade to a smaller positive number
> Upgrade to a negative number and 0
> Upgrade to a string or an object.
> Upgrade to a non-integer number

These *are* interesting, but require an already-established database to upgrade from, which isn't yet complete - next patch!

> 
> > LayoutTests/storage/indexeddb/modern/opendatabase-request-event.html:17
> > +    window.indexedDB.open("TestDatabase", 0);
> 
> Why does this make a Type error?

The spec demands it.

> 
> > Source/WebCore/Modules/indexeddb/client/IDBRequestImpl.h:80
> > +    RefPtr<IDBAny> m_result;
> 
> Why do you have WebCore::IDBAny elsewhere in this same class but not here?

Because there's the abstract WebCore::IDBAny, and then there's the IDBClient::IDBAny that this member is.

That divide is to support living side-by-side with Legacy IDB, and the ugliness will go away when we drop legacy IDB and fold the abstract classes into the concrete.

> 
> > Source/WebCore/Modules/indexeddb/server/MemoryIDBBackingStore.cpp:31
> > +#include "Logging.h"
> 
> This isn't used.

Indeed

> 
> > Source/WebCore/Modules/indexeddb/server/MemoryIDBBackingStore.h:45
> > -    
> > +
> 
> Touching a file for only whitespace?  I don't think this is necessary.

Indeed.

[Brady Eidson]

With blind attempt to handle the broken efl-wk2 compiler:
https://trac.webkit.org/changeset/190884

[Brady Eidson]

Followup landed in https://trac.webkit.org/changeset/190899