Bug 227815

Summary: Implement IDBTransaction.commit()
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: WebCore Misc.Assignee: Sihui Liu <sihui_liu>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, alecflett, beidson, esprehn+autocc, ews-watchlist, ggaren, japhet, jsbell, kondapallykalyan, sihui_liu, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
ews-feeder: commit-queue-
Patch none

Description Chris Dumez 2021-07-08 14:19:46 PDT
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.
Comment 1 Chris Dumez 2021-07-08 14:20:49 PDT
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.
Comment 2 Radar WebKit Bug Importer 2021-07-15 14:20:25 PDT
<rdar://problem/80651270>
Comment 3 Sihui Liu 2021-07-16 00:03:30 PDT
Created attachment 433661 [details]
Patch
Comment 4 Sihui Liu 2021-07-16 09:53:00 PDT
Created attachment 433681 [details]
Patch
Comment 5 Sihui Liu 2021-07-16 12:00:11 PDT
Created attachment 433689 [details]
Patch
Comment 6 Chris Dumez 2021-07-19 09:30:36 PDT
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.
Comment 7 Sihui Liu 2021-07-19 12:26:35 PDT
Created attachment 433810 [details]
Patch
Comment 8 Sihui Liu 2021-07-19 12:29:17 PDT
Created attachment 433811 [details]
Patch
Comment 9 Alex Christensen 2021-07-19 12:57:17 PDT
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?
Comment 10 Chris Dumez 2021-07-19 12:58:26 PDT
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.
Comment 11 Sihui Liu 2021-07-19 13:49:53 PDT
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.
Comment 12 Brady Eidson 2021-07-19 13:55:21 PDT
Comment on attachment 433811 [details]
Patch

Seems fine.
Comment 13 Brady Eidson 2021-07-19 13:55:42 PDT
(In reply to Brady Eidson from comment #12)
> Comment on attachment 433811 [details]
> Patch
> 
> Seems fine.

(With other previous feedback)
Comment 14 EWS 2021-07-19 16:09:58 PDT
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].