Bug 96663 – IndexedDB: Remove "current transaction" concept from backing store

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

[Joshua Bell]

IndexedDB: Remove "current transaction" concept from backing store

[Joshua Bell]

Created attachment 163895 [details]
Patch

[Joshua Bell]

This seems like a good initial step.

As a follow-up, should we flip the API around so that most of the methods are on an IDBBackingStore::Transaction() (that has an internal IDBBackingStore pointer) rather than on IDBBackingStore itself? Or do that in this patch as well? (I vote for follow-up)

[WebKit Review Bot]

Comment on attachment 163895 [details]
Patch

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

[Joshua Bell]

Created attachment 163900 [details]
Patch

[David Grogan]

Why is Transaction& passed around instead of Transaction*?  The WK style guide calls for out parameters to be passed by reference but that doesn't seem to be what's happening.

Other than that, LGTM

[Joshua Bell]

(In reply to comment #5)
> Why is Transaction& passed around instead of Transaction*?  The WK style guide calls for out parameters to be passed by reference but that doesn't seem to be what's happening.

Bleah, that's just my personal style showing through. Must... conform... !

[Joshua Bell]

Created attachment 164411 [details]
Patch

[Joshua Bell]

References -> Pointers

Using an overloaded casting operator is still a bit weird. Better suggestions appreciated.

[David Grogan]

(In reply to comment #8)
> Using an overloaded casting operator is still a bit weird. Better suggestions appreciated.

It does seem a little weird to pass an IDB object to leveldb.  I guess the obvious alternative is to add getLevelDBTransaction() to IDBTransactionBackendImpl. I'd say the wordiness of that outweighs any downsides of overloading the casting operator.

[Alec Flett]

I made a similar comment this morning :) I wonder if it would alleviate the need for prpTransaction-fu that you're doing in some places too, since you're not dereferencing through an '=' ?

[Joshua Bell]

Created attachment 164471 [details]
Patch

[Joshua Bell]

(In reply to comment #9)
> (In reply to comment #8)
> > Using an overloaded casting operator is still a bit weird. Better suggestions appreciated.
> 
> It does seem a little weird to pass an IDB object to leveldb.  I guess the obvious alternative is to add getLevelDBTransaction() to IDBTransactionBackendImpl. I'd say the wordiness of that outweighs any downsides of overloading the casting operator.

Agreed, updated.

(In reply to comment #10)
> I made a similar comment this morning :) I wonder if it would alleviate the need for prpTransaction-fu that you're doing in some places too, since you're not dereferencing through an '=' ?

That too.

Also addressed the FIXME. We want to keep IDBBackingStore::begin() and create the LevelDBTransaction there, as that's the point we'd theoretically create the snapshot.

[Joshua Bell]

alec, david - one more look before I send this off to tony?

[Alec Flett]

Comment on attachment 164471 [details]
Patch


LGTM, with one nit - ironic because this is reason #92 I hate the ChangeLogs :)

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

> Source/WebCore/ChangeLog:89
> +        (WebCore::IDBTransactionBackendImpl::operator IDBBackingStore::Transaction&):

You don't have 'operator' in the patch anymore

[Joshua Bell]

Created attachment 164594 [details]
Patch

[Joshua Bell]

tony@ - another one, not urgent - r?

[WebKit Review Bot]

Comment on attachment 164594 [details]
Patch

Rejecting attachment 164594 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
/Modules/indexeddb/IDBLevelDBBackingStore.h
patching file Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp
patching file Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.h
patching file Source/WebCore/Modules/indexeddb/IDBTransactionBackendImpl.h
patching file Source/WebKit/chromium/tests/IDBFakeBackingStore.h

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Tony Chang']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue

Full output: http://queues.webkit.org/results/13902651

[Joshua Bell]

Created attachment 164793 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 164793 [details]
Patch for landing

Clearing flags on attachment: 164793

Committed r129066: <http://trac.webkit.org/changeset/129066>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.