Bug 227815 – Implement IDBTransaction.commit()

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

[Chris Dumez]

Missing support for IDBTransaction.commit():
- https://www.w3.org/TR/IndexedDB/#dom-idbtransaction-commit

Both Gecko and Blink support it so this is a compatibility risk.

This is causing some WPT tests to fail in WebKit only.

[Chris Dumez]

If someone more knowledgeable with IDB could implement this, that'd be great. If no-one implements it, I'll probably look into it eventually but it will likely take me some time.

[Radar WebKit Bug Importer]

<rdar://problem/80651270>

[Sihui Liu]

Created attachment 433661 [details]
Patch

[Sihui Liu]

Created attachment 433681 [details]
Patch

[Sihui Liu]

Created attachment 433689 [details]
Patch

[Chris Dumez]

Comment on attachment 433689 [details]
Patch

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

> Source/WebCore/Modules/indexeddb/IDBRequest.cpp:299
> +    m_dispatchingEvent = &event;

We should rename the data member now that it is no longer a boolean, maybe m_eventBeingDispatched.

> Source/WebCore/Modules/indexeddb/IDBTransaction.cpp:487
> +    for (auto request : m_openRequests) {

auto& to avoid ref counting churn.

Could also be written as (if you prefer):
auto pendingRequestCount = std::count_if(m_openRequests.begin(), m_openRequests.end(), [](auto& request) { return !request->isDone(); });

> Source/WebCore/Modules/indexeddb/IDBTransaction.cpp:492
> +    scheduleOperation(IDBClient::TransactionOperationImpl::create(*this, nullptr, [protectedThis = makeRef(*this), pendingRequestCount] (auto& operation) {

makeRef() is deprecated. `protectedThis = Ref { *this }` is shorter with C++17.

> LayoutTests/imported/w3c/ChangeLog:11
> +        * web-platform-tests/IndexedDB/idb-explicit-commit.any-expected.txt:

The change log should probably explain why the test is timing out and why we think that's OK.

[Sihui Liu]

Created attachment 433810 [details]
Patch

[Sihui Liu]

Created attachment 433811 [details]
Patch

[Alex Christensen]

Comment on attachment 433811 [details]
Patch

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

> LayoutTests/imported/w3c/web-platform-tests/IndexedDB/idb-explicit-commit.any-expected.txt:13
> +TIMEOUT Transactions with same scope should stay in program order, even if one calls commit. Test timed out

Is there a good explanation why this is happening, or do we know what it would take to fix it?

[Chris Dumez]

Comment on attachment 433811 [details]
Patch

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

> LayoutTests/imported/w3c/ChangeLog:11
> +        * web-platform-tests/IndexedDB/idb-explicit-commit.any-expected.txt: Some test is timed out as our backend does

Alex: I asked Sihui to add an explanation about this in the ChangeLog.

[Sihui Liu]

Comment on attachment 433811 [details]
Patch

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

>> LayoutTests/imported/w3c/web-platform-tests/IndexedDB/idb-explicit-commit.any-expected.txt:13
>> +TIMEOUT Transactions with same scope should stay in program order, even if one calls commit. Test timed out
> 
> Is there a good explanation why this is happening, or do we know what it would take to fix it?

Yes, the test expects a later transaction to finish and fire events to end the first transactions. We don't support running multiple simultaneous transactions so the later transaction will be blocked.

We would have passed the next two NOTRUN tests, but the test is timed out here.

[Brady Eidson]

Comment on attachment 433811 [details]
Patch

Seems fine.

[Brady Eidson]

(In reply to Brady Eidson from comment #12)
> Comment on attachment 433811 [details]
> Patch
> 
> Seems fine.

(With other previous feedback)

[EWS]

Committed r280053 (239788@main): <https://commits.webkit.org/239788@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 433811 [details].