Bug 107311 – IndexedDB: Switch to new createTransaction call

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alec Flett]

IndexedDB: Switch to new createTransaction call

[Alec Flett]

Created attachment 183525 [details]
Patch

[Alec Flett]

jsbell / dgrogan - have a look? This won't pass tests until the chromium side lands.

[Joshua Bell]

Comment on attachment 183525 [details]
Patch

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

overall lgtm

> Source/WebCore/ChangeLog:10
> +        transaction objects. This allows an asynchronous call in Chromium

Replace "Chromium" with "multiprocess ports".

> Source/WebCore/ChangeLog:15
> +        No new tests (OOPS!).

Replace the OOPS! (or it won't commit).

> Source/WebKit/chromium/ChangeLog:6
> +        Reviewed by NOBODY (OOPS!).

Ditto.

> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:667
> +        LOG_ERROR("Not committing aborted transaction %ld", transactionId);

Shouldn't LOG_ERROR here, since this is expected behavior. and the LOG doesn't tell us or a developer/user anything actionable.

I think the comment/code in abort() (below) is a better approach.

> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:1161
>  // FIXME: Remove this method in https://bugs.webkit.org/show_bug.cgi?id=103923.

Can't this method be removed now, just leaving the stub in WebKit/chromium/public?

> Source/WebCore/Modules/indexeddb/IDBDatabaseCallbacksImpl.cpp:76
> +    ASSERT(!(transactionId >> 32));

Do these ASSERTs belong in WebCore are they just enforcing how Chromium uses the IDs? (i.e. maybe move them into WebKit/chromium/src)

> Source/WebCore/Modules/indexeddb/IDBTransaction.h:151
>      // FIXME: Remove references to the backend when the backend is fully flattened: https://bugs.webkit.org/show_bug.cgi?id=99774

Remove this comment?

[Alec Flett]

Created attachment 183543 [details]
Patch

[Alec Flett]

Comment on attachment 183543 [details]
Patch

tony@ - r? 

jsbell: I have a bunch of code to remove and I was going to get it all in a WebKit API-oriented patch, so I'll take care of removing the old createTransaction then.

[Alec Flett]

[trying that again] tony@ - r?

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[WebKit Review Bot]

Comment on attachment 183543 [details]
Patch

Attachment 183543 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15938833

[Peter Beverloo (cr-android ews)]

Comment on attachment 183543 [details]
Patch

Attachment 183543 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/15937919

[Alec Flett]

Created attachment 183561 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 183561 [details]
Patch for landing

Clearing flags on attachment: 183561

Committed r140219: <http://trac.webkit.org/changeset/140219>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.