Bug 227815 - Implement IDBTransaction.commit()
Summary: Implement IDBTransaction.commit()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sihui Liu
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-07-08 14:19 PDT by Chris Dumez
Modified: 2021-07-19 16:10 PDT (History)
11 users (show)

See Also:


Attachments
Patch (56.44 KB, patch)
2021-07-16 00:03 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch (55.77 KB, patch)
2021-07-16 09:53 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch (57.14 KB, patch)
2021-07-16 12:00 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch (57.37 KB, patch)
2021-07-19 12:26 PDT, Sihui Liu
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (57.68 KB, patch)
2021-07-19 12:29 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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].