[webkit-reviews] review granted: [Bug 195650] Layout tests imported/w3c/web-platform-tests/IndexedDB/*-exception-order.html are failing : [Attachment 364561] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has granted Sihui Liu <sihui_liu at apple.com>'s
request for review:
Bug 195650: Layout tests
imported/w3c/web-platform-tests/IndexedDB/*-exception-order.html are failing
https://bugs.webkit.org/show_bug.cgi?id=195650

Attachment 364561: Patch

https://bugs.webkit.org/attachment.cgi?id=364561&action=review




--- Comment #11 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 364561
  --> https://bugs.webkit.org/attachment.cgi?id=364561
Patch

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

> Source/WebCore/ChangeLog:8
> +	   Fix some exception orders in IDB.

Please add a URL to the spec.

> Source/WebCore/ChangeLog:11
> +	   (WebCore::IDBDatabase::createObjectStore):

Step 6 of https://www.w3.org/TR/IndexedDB-2/#dom-idbdatabase-createobjectstore

> Source/WebCore/ChangeLog:12
> +	   (WebCore::IDBDatabase::transaction):

Step 1 of https://www.w3.org/TR/IndexedDB-2/#dom-idbdatabase-transaction

> Source/WebCore/ChangeLog:15
> +	   (WebCore::IDBIndex::openCursor):

Step 5: https://www.w3.org/TR/IndexedDB-2/#dom-idbindex-opencursor

> Source/WebCore/ChangeLog:16
> +	   (WebCore::IDBIndex::doOpenKeyCursor):

Step 5: https://www.w3.org/TR/IndexedDB-2/#dom-idbindex-openkeycursor

> Source/WebCore/ChangeLog:18
> +	   (WebCore::IDBIndex::count):

Step 5: https://www.w3.org/TR/IndexedDB-2/#dom-idbindex-count

> Source/WebCore/ChangeLog:20
> +	   (WebCore::IDBIndex::get):

Step 5: https://www.w3.org/TR/IndexedDB-2/#dom-idbindex-get

> Source/WebCore/ChangeLog:22
> +	   (WebCore::IDBIndex::getKey):

Step 5: https://www.w3.org/TR/IndexedDB-2/#dom-idbindex-getkey

> Source/WebCore/ChangeLog:24
> +	   (WebCore::IDBIndex::doGetAll):

Step 5: https://www.w3.org/TR/IndexedDB-2/#dom-idbindex-getallkeys

> Source/WebCore/Modules/indexeddb/IDBDatabase.cpp:175
> +    if (m_versionChangeTransaction &&
!m_versionChangeTransaction->isFinishedOrFinishing())
> +	   return Exception { InvalidStateError, "Failed to execute
'transaction' on 'IDBDatabase': A version change transaction is running."_s };
> +

For consistency with the spec, we should probably check this before
m_closePending:
https://www.w3.org/TR/IndexedDB-2/#dom-idbdatabase-transaction

> Source/WebCore/Modules/indexeddb/IDBDatabase.cpp:183
>      if (objectStores.isEmpty())
>	   return Exception { InvalidAccessError, "Failed to execute
'transaction' on 'IDBDatabase': The storeNames parameter was empty."_s };

This check also probably needs to be after the NotFoundError check below per
step 4:
https://www.w3.org/TR/IndexedDB-2/#dom-idbdatabase-transaction

> Source/WebCore/Modules/indexeddb/IDBDatabase.cpp:199
> +    if (mode != IDBTransactionMode::Readonly && mode !=
IDBTransactionMode::Readwrite)

Step 6 of https://www.w3.org/TR/IndexedDB-2/#dom-idbdatabase-transaction

> Source/WebCore/Modules/indexeddb/IDBIndex.cpp:176
> +ExceptionOr<Ref<IDBRequest>> IDBIndex::openCursor(ExecState& execState,
IDBKeyRange* range, IDBCursorDirection direction)

We should make this function take RefPtr<>&& like other functions.

> Source/WebCore/Modules/indexeddb/IDBIndex.cpp:178
> +    return doOpenCursor(execState, direction, [range]() {

And do range = WTFMove(range).

> Source/WebCore/Modules/indexeddb/IDBIndex.cpp:188
> +	       return ExceptionOr<RefPtr<IDBKeyRange>>(Exception(DataError,
"Failed to execute 'openCursor' on 'IDBIndex': The parameter is not a valid
key."_s));

Step 5 of https://www.w3.org/TR/IndexedDB-2/#dom-idbobjectstore-opencursor

> Source/WebCore/Modules/indexeddb/IDBIndex.cpp:190
> +	   return ExceptionOr<RefPtr<IDBKeyRange>> {
makeRefPtr(onlyResult.releaseReturnValue().ptr()) };

You shouldn't have to manually construct RefPtr out of Ref like this. Just do
WTFMove(onlyResult.releaseReturnValue()).
Otherwise, we'd do a ref-churn.

> Source/WebCore/Modules/indexeddb/IDBIndex.cpp:216
> +    return doOpenKeyCursor(execState, direction, [range]() {

Ditto about making RefPtr earlier.

> Source/WebCore/Modules/indexeddb/IDBIndex.cpp:228
> +	   return ExceptionOr<RefPtr<IDBKeyRange>> {
makeRefPtr(onlyResult.releaseReturnValue().ptr()) };

Ditto about using WTF instead.

> Source/WebCore/Modules/indexeddb/IDBIndex.cpp:279
> -	   return Exception { DataError, "Failed to execute 'get' on
'IDBIndex': The parameter is not a valid key."_s };
> +	   return doGet(execState, Exception(DataError, "Failed to execute
'get' on 'IDBIndex': The parameter is not a valid key."_s));

Hm... it's a bit funky that we still call doGet just to check two more
conditions but okay.

> Source/WebCore/Modules/indexeddb/IDBIndex.cpp:377
> +	   return ExceptionOr<RefPtr<IDBKeyRange>> {
makeRefPtr(onlyResult.releaseReturnValue().ptr()) };

Ditto about avoiding ref-churn.

> Source/WebCore/Modules/indexeddb/IDBIndex.cpp:400
> +ExceptionOr<Ref<IDBRequest>> IDBIndex::getAllKeys(ExecState& execState,
RefPtr<IDBKeyRange> range, Optional<uint32_t> count)

Use RefPtr<>&&

> Source/WebCore/Modules/indexeddb/IDBIndex.cpp:414
> +	   return ExceptionOr<RefPtr<IDBKeyRange>> {
makeRefPtr(onlyResult.releaseReturnValue().ptr()) };

Ditto about avoiding ref-churn.

> Source/WebCore/Modules/indexeddb/IDBObjectStore.cpp:172
> +ExceptionOr<Ref<IDBRequest>> IDBObjectStore::openCursor(ExecState&
execState, RefPtr<IDBKeyRange> range, IDBCursorDirection direction)

Use RefPtr&&

> Source/WebCore/Modules/indexeddb/IDBObjectStore.cpp:186
> +	   return ExceptionOr<RefPtr<IDBKeyRange>> {
makeRefPtr(onlyResult.releaseReturnValue().ptr()) };

Fix this one also.

> Source/WebCore/Modules/indexeddb/IDBObjectStore.cpp:210
> +ExceptionOr<Ref<IDBRequest>> IDBObjectStore::openKeyCursor(ExecState&
execState, RefPtr<IDBKeyRange> range, IDBCursorDirection direction)

Ditto about RefPtr<>&&

> Source/WebCore/Modules/indexeddb/IDBObjectStore.cpp:224
> +	   return ExceptionOr<RefPtr<IDBKeyRange>> {
makeRefPtr(onlyResult.releaseReturnValue().ptr()) };

Fix this one also.

> Source/WebCore/Modules/indexeddb/IDBObjectStore.cpp:431
> +	   Ref<IDBKey> idbKey = scriptValueToIDBKey(*state, key);

Use auto?

> Source/WebCore/Modules/indexeddb/IDBObjectStore.cpp:434
> +	   return ExceptionOr<RefPtr<IDBKeyRange>> {
makeRefPtr(IDBKeyRange::create(WTFMove(idbKey)).ptr()) };

Hm.. you should just be able to just return
IDBKeyRange::create(WTFMove(idbKey)) or maybe you need to do
WTFMove(~::create(~))

> Source/WebCore/Modules/indexeddb/IDBObjectStore.cpp:638
> +	       return ExceptionOr<RefPtr<IDBKeyRange>>(Exception(DataError,
"Failed to execute 'getAll' on 'IDBObjectStore': The parameter is not a valid
key."_s));

Why not use { ~ }?