Bug 100426 – IndexedDB: switch frontend to use int64_t-based references

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Alec Flett]

This is the followup to bug 100425, which cleans up the frontend code to use int64_t-based objectStore/index references, and removes backend support for String-based references.

[Alec Flett]

From the patch in bug 100425, jsbell sez:
> Make sure to include a test with 100426 that tests put-record/delete-index/create-index-with-same-name-different-keypath

[Alec Flett]

Created attachment 172134 [details]
Patch

[Joshua Bell]

Comment on attachment 172134 [details]
Patch

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

Initial pass lgtm

> Source/WebCore/Modules/indexeddb/IDBMetadata.h:81
>          for (IndexMap::const_iterator it = indexes.begin(); it != indexes.end(); ++it) {

Maybe add a FIXME: Add a HashMap of name->id if this is ever a performance concern.

(Realistically, no-one is going to call these particular methods in a performance-critical loop.)

> Source/WebCore/inspector/Inspector.json:1302
> +                    { "name": "objectStoreId", "type": "number", "description": "Object store name." },

Update description. Also, "number" or "integer"?

> Source/WebCore/inspector/Inspector.json:1303
> +                    { "name": "indexId", "type": "number", "description": "Index name, empty string for object store data requests." },

Update description.

> Source/WebCore/inspector/InspectorIndexedDBAgent.h:63
> +    virtual void requestData(ErrorString*, const String& frameId, const String& databaseName, double objectStoreId, double indexId, int skipCount, int pageSize, const RefPtr<InspectorObject>* keyRange, PassRefPtr<RequestDataCallback>);

I'm guessing "number" above maps to "double" here. I guess the question is which is worse: ids > 2^53 getting corrupted due to precision loss, or ids > 2^31 being truncated.

I'd still lean towards "integer" and make these ints, and perhaps there a place to wedge in an early return if the id is > 2^31? Or at least a FIXME?

[Alec Flett]

There's more code removal coming, (some of the scaffolding from the previous patch) and the inspector tests are still broken.  But thanks for the first pass.

I think at least with doubles, we should be able to test if the value is > 2^53, rather than integers which will arrive pre-truncated.

I'll poke around and see if there's a place to return early.

[Alec Flett]

Created attachment 172448 [details]
Patch

[Alec Flett]

ok, this patch is ready for jsbell/dgrogan to take a look.

[David Grogan]

Comment on attachment 172448 [details]
Patch

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

LGTM

> Source/WebCore/Modules/indexeddb/IDBDatabase.cpp:360
> +int64_t IDBDatabase::findObjectStoreId(const String& name) const

This moved from IDBMetaData so that IDBTransaction could use it too?

> Source/WebCore/Modules/indexeddb/IDBDatabase.h:106
> +

nit blank line

> Source/WebCore/inspector/Inspector.json:-1299
> -                    { "name": "objectStoreName", "type": "string", "description": "Object store name." },

What does this affect?  What view will now see an id instead of a name?

[Alec Flett]

Created attachment 173082 [details]
Patch

[Alec Flett]

ok, final version ready to go.

tony@ - r?

95% code removal
5% minor API code migration..

[Alec Flett]

Comment on attachment 172448 [details]
Patch

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

>> Source/WebCore/Modules/indexeddb/IDBDatabase.cpp:360
>> +int64_t IDBDatabase::findObjectStoreId(const String& name) const
> 
> This moved from IDBMetaData so that IDBTransaction could use it too?

more that I put it on IDBMetadata temporarily for use during the transition, but now only IDBDatabase uses it - I wanted to bring IDBMetadata back to being pure structs after the refactor was over.

>> Source/WebCore/inspector/Inspector.json:-1299
>> -                    { "name": "objectStoreName", "type": "string", "description": "Object store name." },
> 
> What does this affect?  What view will now see an id instead of a name?

I'm nuking the inspector stuff since vsevik switched the inspector over in a better way

[WebKit Review Bot]

Comment on attachment 173082 [details]
Patch

Clearing flags on attachment: 173082

Committed r133984: <http://trac.webkit.org/changeset/133984>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by bug 101684

[Alec Flett]

Created attachment 173198 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 173198 [details]
Patch for landing

Clearing flags on attachment: 173198

Committed r134010: <http://trac.webkit.org/changeset/134010>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[noel gordon]

Broke the window build, it seems.

18>------ Build started: Project: webkit_unit_tests, Configuration: Release Win32 ------
18>  IDBAbortOnCorruptTest.cpp
18>  IDBDatabaseBackendTest.cpp
18>  IDBRequestTest.cpp
18>E:\google-windows-1\chromium-win-release\build\Source\WTF\wtf/PassRefPtr.h(53): error C2027: use of undefined type 'WebCore::DOMStringList'
18>          e:\google-windows-1\chromium-win-release\build\source\webcore\modules\indexeddb\IDBCallbacks.h(43) : see declaration of 'WebCore::DOMStringList'
18>          E:\google-windows-1\chromium-win-release\build\Source\WTF\wtf/PassRefPtr.h(68) : see reference to function template instantiation 'void WTF::derefIfNotNull<T>(T *)' being compiled
18>          with
18>          [
18>              T=WebCore::DOMStringList
18>          ]
18>          E:\google-windows-1\chromium-win-release\build\Source\WTF\wtf/PassRefPtr.h(68) : while compiling class template member function 'WTF::PassRefPtr<T>::~PassRefPtr(void)'
18>          with
18>          [
18>              T=WebCore::DOMStringList
18>          ]
18>          tests\IDBAbortOnCorruptTest.cpp(55) : see reference to class template instantiation 'WTF::PassRefPtr<T>' being compiled
18>          with
18>          [
18>              T=WebCore::DOMStringList
18>          ]
18>E:\google-windows-1\chromium-win-release\build\Source\WTF\wtf/PassRefPtr.h(53): error C2227: left of '->deref' must point to class/struct/union/generic type
16>  TestWebKitAPI.vcxproj -> ..\..\..\Source\WebKit\chromium\build\Release\\TestWebKitAPI.exe
17>  DumpRenderTree.vcxproj -> ..\..\..\Source\WebKit\chromium\build\Release\\DumpRenderTree.exe
========== Build: 17 succeeded, 1 failed, 144 up-to-date, 0 skipped ==========

[WebKit Review Bot]

Re-opened since this is blocked by bug 101716

[Alec Flett]

Created attachment 173337 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 173337 [details]
Patch for landing

Clearing flags on attachment: 173337

Committed r134095: <http://trac.webkit.org/changeset/134095>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.